* [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is enabled
@ 2017-03-28 6:46 Jianfeng Tan
2017-04-06 5:19 ` Yuanhan Liu
0 siblings, 1 reply; 5+ messages in thread
From: Jianfeng Tan @ 2017-03-28 6:46 UTC (permalink / raw)
To: zhiyong.yang; +Cc: Jianfeng Tan, stable, Stephen Hemminger
We should not always check isr to know if link status is changed.
Here is how driver should handle interrupt quoted from virtio spec,
If MSI-X capability is disabled:
– Read the ISR Status field, which will reset it to zero.
– If the lower bit is set: look through the used rings of all
virtqueues for the device, to see if any progress has been made
by the device which requires servicing.
– If the second lower bit is set: re-examine the configuration
space to see what changed.
If MSI-X capability is enabled:
– Look through the used rings of all virtqueues mapped to that
MSI-X vector for the device, to see if any progress has been
made by the device which requires servicing.
– If the MSI-X vector is equal to config_msix_vector, re-examine
the configuration space to see what changed
Fixes: 8d6d7e5cb3b1 ("virtio: support link state interrupt")
CC: stable@dpdk.org
CC: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
drivers/net/virtio/virtio_ethdev.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 3cf4102..cb30d11 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -36,6 +36,7 @@
#include <stdio.h>
#include <errno.h>
#include <unistd.h>
+#include <stdbool.h>
#include <rte_ethdev.h>
#include <rte_memcpy.h>
@@ -1197,20 +1198,23 @@ virtio_interrupt_handler(struct rte_intr_handle *handle,
struct rte_eth_dev *dev = param;
struct virtio_hw *hw = dev->data->dev_private;
uint8_t isr;
+ bool check_config;
- /* Read interrupt status which clears interrupt */
- isr = vtpci_isr(hw);
- PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
+ /* isr is used only when msix is not enabled */
+ if (!hw->modern && !hw->use_msix) {
+ isr = vtpci_isr(hw); /* Read to clears interrupt */
+ PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
+ check_config = (isr & VIRTIO_PCI_ISR_CONFIG) ? true : false;
+ } else {
+ check_config = true;
+ }
if (rte_intr_enable(handle) < 0)
PMD_DRV_LOG(ERR, "interrupt enable failed");
- if (isr & VIRTIO_PCI_ISR_CONFIG) {
- if (virtio_dev_link_update(dev, 0) == 0)
- _rte_eth_dev_callback_process(dev,
- RTE_ETH_EVENT_INTR_LSC, NULL);
- }
-
+ if (check_config && virtio_dev_link_update(dev, 0) == 0)
+ _rte_eth_dev_callback_process(dev,
+ RTE_ETH_EVENT_INTR_LSC, NULL);
}
static void
--
2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is enabled
2017-03-28 6:46 [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is enabled Jianfeng Tan
@ 2017-04-06 5:19 ` Yuanhan Liu
2017-04-06 5:30 ` Tan, Jianfeng
0 siblings, 1 reply; 5+ messages in thread
From: Yuanhan Liu @ 2017-04-06 5:19 UTC (permalink / raw)
To: Jianfeng Tan; +Cc: zhiyong.yang, stable, Stephen Hemminger
On Tue, Mar 28, 2017 at 06:46:51AM +0000, Jianfeng Tan wrote:
> We should not always check isr to know if link status is changed.
Did this patch fix a real issue?
> Here is how driver should handle interrupt quoted from virtio spec,
>
> If MSI-X capability is disabled:
> – Read the ISR Status field, which will reset it to zero.
> – If the lower bit is set: look through the used rings of all
> virtqueues for the device, to see if any progress has been made
> by the device which requires servicing.
> – If the second lower bit is set: re-examine the configuration
> space to see what changed.
> If MSI-X capability is enabled:
> – Look through the used rings of all virtqueues mapped to that
> MSI-X vector for the device, to see if any progress has been
> made by the device which requires servicing.
> – If the MSI-X vector is equal to config_msix_vector, re-examine
> the configuration space to see what changed
>
> Fixes: 8d6d7e5cb3b1 ("virtio: support link state interrupt")
> CC: stable@dpdk.org
>
> CC: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 3cf4102..cb30d11 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -36,6 +36,7 @@
> #include <stdio.h>
> #include <errno.h>
> #include <unistd.h>
> +#include <stdbool.h>
>
> #include <rte_ethdev.h>
> #include <rte_memcpy.h>
> @@ -1197,20 +1198,23 @@ virtio_interrupt_handler(struct rte_intr_handle *handle,
> struct rte_eth_dev *dev = param;
> struct virtio_hw *hw = dev->data->dev_private;
> uint8_t isr;
> + bool check_config;
>
> - /* Read interrupt status which clears interrupt */
> - isr = vtpci_isr(hw);
> - PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> + /* isr is used only when msix is not enabled */
> + if (!hw->modern && !hw->use_msix) {
The comment doesn't quite match the code: the hw->modern part is missing.
> + isr = vtpci_isr(hw); /* Read to clears interrupt */
> + PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> + check_config = (isr & VIRTIO_PCI_ISR_CONFIG) ? true : false;
The "? :" is unnecessary.
--yliu
> + } else {
> + check_config = true;
> + }
>
> if (rte_intr_enable(handle) < 0)
> PMD_DRV_LOG(ERR, "interrupt enable failed");
>
> - if (isr & VIRTIO_PCI_ISR_CONFIG) {
> - if (virtio_dev_link_update(dev, 0) == 0)
> - _rte_eth_dev_callback_process(dev,
> - RTE_ETH_EVENT_INTR_LSC, NULL);
> - }
> -
> + if (check_config && virtio_dev_link_update(dev, 0) == 0)
> + _rte_eth_dev_callback_process(dev,
> + RTE_ETH_EVENT_INTR_LSC, NULL);
> }
>
> static void
> --
> 2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is enabled
2017-04-06 5:19 ` Yuanhan Liu
@ 2017-04-06 5:30 ` Tan, Jianfeng
2017-04-06 5:36 ` Yuanhan Liu
0 siblings, 1 reply; 5+ messages in thread
From: Tan, Jianfeng @ 2017-04-06 5:30 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: Yang, Zhiyong, stable, Stephen Hemminger
Sorry, please just NACK this patch. (I'll do that in patchwork)
I was sharing this half-baked patch with Zhiyong and forget to remove "CC".
Zhiyong and I are still working on this bug. Will send it out later.
Thanks,
Jianfeng
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Thursday, April 6, 2017 1:19 PM
> To: Tan, Jianfeng
> Cc: Yang, Zhiyong; stable@dpdk.org; Stephen Hemminger
> Subject: Re: [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is enabled
>
> On Tue, Mar 28, 2017 at 06:46:51AM +0000, Jianfeng Tan wrote:
> > We should not always check isr to know if link status is changed.
>
> Did this patch fix a real issue?
>
> > Here is how driver should handle interrupt quoted from virtio spec,
> >
> > If MSI-X capability is disabled:
> > – Read the ISR Status field, which will reset it to zero.
> > – If the lower bit is set: look through the used rings of all
> > virtqueues for the device, to see if any progress has been made
> > by the device which requires servicing.
> > – If the second lower bit is set: re-examine the configuration
> > space to see what changed.
> > If MSI-X capability is enabled:
> > – Look through the used rings of all virtqueues mapped to that
> > MSI-X vector for the device, to see if any progress has been
> > made by the device which requires servicing.
> > – If the MSI-X vector is equal to config_msix_vector, re-examine
> > the configuration space to see what changed
> >
> > Fixes: 8d6d7e5cb3b1 ("virtio: support link state interrupt")
> > CC: stable@dpdk.org
> >
> > CC: Stephen Hemminger <stephen@networkplumber.org>
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > ---
> > drivers/net/virtio/virtio_ethdev.c | 22 +++++++++++++---------
> > 1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> > index 3cf4102..cb30d11 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -36,6 +36,7 @@
> > #include <stdio.h>
> > #include <errno.h>
> > #include <unistd.h>
> > +#include <stdbool.h>
> >
> > #include <rte_ethdev.h>
> > #include <rte_memcpy.h>
> > @@ -1197,20 +1198,23 @@ virtio_interrupt_handler(struct rte_intr_handle
> *handle,
> > struct rte_eth_dev *dev = param;
> > struct virtio_hw *hw = dev->data->dev_private;
> > uint8_t isr;
> > + bool check_config;
> >
> > - /* Read interrupt status which clears interrupt */
> > - isr = vtpci_isr(hw);
> > - PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> > + /* isr is used only when msix is not enabled */
> > + if (!hw->modern && !hw->use_msix) {
>
> The comment doesn't quite match the code: the hw->modern part is missing.
>
> > + isr = vtpci_isr(hw); /* Read to clears interrupt */
> > + PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> > + check_config = (isr & VIRTIO_PCI_ISR_CONFIG) ? true : false;
>
> The "? :" is unnecessary.
>
> --yliu
>
> > + } else {
> > + check_config = true;
> > + }
> >
> > if (rte_intr_enable(handle) < 0)
> > PMD_DRV_LOG(ERR, "interrupt enable failed");
> >
> > - if (isr & VIRTIO_PCI_ISR_CONFIG) {
> > - if (virtio_dev_link_update(dev, 0) == 0)
> > - _rte_eth_dev_callback_process(dev,
> > -
> RTE_ETH_EVENT_INTR_LSC, NULL);
> > - }
> > -
> > + if (check_config && virtio_dev_link_update(dev, 0) == 0)
> > + _rte_eth_dev_callback_process(dev,
> > + RTE_ETH_EVENT_INTR_LSC, NULL);
> > }
> >
> > static void
> > --
> > 2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is enabled
2017-04-06 5:30 ` Tan, Jianfeng
@ 2017-04-06 5:36 ` Yuanhan Liu
2017-04-06 5:40 ` Tan, Jianfeng
0 siblings, 1 reply; 5+ messages in thread
From: Yuanhan Liu @ 2017-04-06 5:36 UTC (permalink / raw)
To: Tan, Jianfeng; +Cc: Yang, Zhiyong, stable, Stephen Hemminger
On Thu, Apr 06, 2017 at 05:30:10AM +0000, Tan, Jianfeng wrote:
> Sorry, please just NACK this patch. (I'll do that in patchwork)
>
> I was sharing this half-baked patch with Zhiyong and forget to remove "CC".
> Zhiyong and I are still working on this bug. Will send it out later.
Oh, I didn't notice that this patch is not cc'ed to dev ml. Then you
don't need update patchwork then (patchwork doesn't tract patches
from stable list).
--yliu
>
> Thanks,
> Jianfeng
>
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Thursday, April 6, 2017 1:19 PM
> > To: Tan, Jianfeng
> > Cc: Yang, Zhiyong; stable@dpdk.org; Stephen Hemminger
> > Subject: Re: [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is enabled
> >
> > On Tue, Mar 28, 2017 at 06:46:51AM +0000, Jianfeng Tan wrote:
> > > We should not always check isr to know if link status is changed.
> >
> > Did this patch fix a real issue?
> >
> > > Here is how driver should handle interrupt quoted from virtio spec,
> > >
> > > If MSI-X capability is disabled:
> > > – Read the ISR Status field, which will reset it to zero.
> > > – If the lower bit is set: look through the used rings of all
> > > virtqueues for the device, to see if any progress has been made
> > > by the device which requires servicing.
> > > – If the second lower bit is set: re-examine the configuration
> > > space to see what changed.
> > > If MSI-X capability is enabled:
> > > – Look through the used rings of all virtqueues mapped to that
> > > MSI-X vector for the device, to see if any progress has been
> > > made by the device which requires servicing.
> > > – If the MSI-X vector is equal to config_msix_vector, re-examine
> > > the configuration space to see what changed
> > >
> > > Fixes: 8d6d7e5cb3b1 ("virtio: support link state interrupt")
> > > CC: stable@dpdk.org
> > >
> > > CC: Stephen Hemminger <stephen@networkplumber.org>
> > > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > > ---
> > > drivers/net/virtio/virtio_ethdev.c | 22 +++++++++++++---------
> > > 1 file changed, 13 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > b/drivers/net/virtio/virtio_ethdev.c
> > > index 3cf4102..cb30d11 100644
> > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > @@ -36,6 +36,7 @@
> > > #include <stdio.h>
> > > #include <errno.h>
> > > #include <unistd.h>
> > > +#include <stdbool.h>
> > >
> > > #include <rte_ethdev.h>
> > > #include <rte_memcpy.h>
> > > @@ -1197,20 +1198,23 @@ virtio_interrupt_handler(struct rte_intr_handle
> > *handle,
> > > struct rte_eth_dev *dev = param;
> > > struct virtio_hw *hw = dev->data->dev_private;
> > > uint8_t isr;
> > > + bool check_config;
> > >
> > > - /* Read interrupt status which clears interrupt */
> > > - isr = vtpci_isr(hw);
> > > - PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> > > + /* isr is used only when msix is not enabled */
> > > + if (!hw->modern && !hw->use_msix) {
> >
> > The comment doesn't quite match the code: the hw->modern part is missing.
> >
> > > + isr = vtpci_isr(hw); /* Read to clears interrupt */
> > > + PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> > > + check_config = (isr & VIRTIO_PCI_ISR_CONFIG) ? true : false;
> >
> > The "? :" is unnecessary.
> >
> > --yliu
> >
> > > + } else {
> > > + check_config = true;
> > > + }
> > >
> > > if (rte_intr_enable(handle) < 0)
> > > PMD_DRV_LOG(ERR, "interrupt enable failed");
> > >
> > > - if (isr & VIRTIO_PCI_ISR_CONFIG) {
> > > - if (virtio_dev_link_update(dev, 0) == 0)
> > > - _rte_eth_dev_callback_process(dev,
> > > -
> > RTE_ETH_EVENT_INTR_LSC, NULL);
> > > - }
> > > -
> > > + if (check_config && virtio_dev_link_update(dev, 0) == 0)
> > > + _rte_eth_dev_callback_process(dev,
> > > + RTE_ETH_EVENT_INTR_LSC, NULL);
> > > }
> > >
> > > static void
> > > --
> > > 2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is enabled
2017-04-06 5:36 ` Yuanhan Liu
@ 2017-04-06 5:40 ` Tan, Jianfeng
0 siblings, 0 replies; 5+ messages in thread
From: Tan, Jianfeng @ 2017-04-06 5:40 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: Yang, Zhiyong, stable, Stephen Hemminger
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Thursday, April 6, 2017 1:36 PM
> To: Tan, Jianfeng
> Cc: Yang, Zhiyong; stable@dpdk.org; Stephen Hemminger
> Subject: Re: [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is enabled
>
> On Thu, Apr 06, 2017 at 05:30:10AM +0000, Tan, Jianfeng wrote:
> > Sorry, please just NACK this patch. (I'll do that in patchwork)
> >
> > I was sharing this half-baked patch with Zhiyong and forget to remove "CC".
> > Zhiyong and I are still working on this bug. Will send it out later.
>
> Oh, I didn't notice that this patch is not cc'ed to dev ml. Then you
> don't need update patchwork then (patchwork doesn't tract patches
> from stable list).
No wonder I did not find it. :-)
>
> --yliu
> >
> > Thanks,
> > Jianfeng
> >
> > > -----Original Message-----
> > > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > > Sent: Thursday, April 6, 2017 1:19 PM
> > > To: Tan, Jianfeng
> > > Cc: Yang, Zhiyong; stable@dpdk.org; Stephen Hemminger
> > > Subject: Re: [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is
> enabled
> > >
> > > On Tue, Mar 28, 2017 at 06:46:51AM +0000, Jianfeng Tan wrote:
> > > > We should not always check isr to know if link status is changed.
> > >
> > > Did this patch fix a real issue?
> > >
> > > > Here is how driver should handle interrupt quoted from virtio spec,
> > > >
> > > > If MSI-X capability is disabled:
> > > > – Read the ISR Status field, which will reset it to zero.
> > > > – If the lower bit is set: look through the used rings of all
> > > > virtqueues for the device, to see if any progress has been made
> > > > by the device which requires servicing.
> > > > – If the second lower bit is set: re-examine the configuration
> > > > space to see what changed.
> > > > If MSI-X capability is enabled:
> > > > – Look through the used rings of all virtqueues mapped to that
> > > > MSI-X vector for the device, to see if any progress has been
> > > > made by the device which requires servicing.
> > > > – If the MSI-X vector is equal to config_msix_vector, re-examine
> > > > the configuration space to see what changed
> > > >
> > > > Fixes: 8d6d7e5cb3b1 ("virtio: support link state interrupt")
> > > > CC: stable@dpdk.org
> > > >
> > > > CC: Stephen Hemminger <stephen@networkplumber.org>
> > > > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > > > ---
> > > > drivers/net/virtio/virtio_ethdev.c | 22 +++++++++++++---------
> > > > 1 file changed, 13 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > > b/drivers/net/virtio/virtio_ethdev.c
> > > > index 3cf4102..cb30d11 100644
> > > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > > @@ -36,6 +36,7 @@
> > > > #include <stdio.h>
> > > > #include <errno.h>
> > > > #include <unistd.h>
> > > > +#include <stdbool.h>
> > > >
> > > > #include <rte_ethdev.h>
> > > > #include <rte_memcpy.h>
> > > > @@ -1197,20 +1198,23 @@ virtio_interrupt_handler(struct
> rte_intr_handle
> > > *handle,
> > > > struct rte_eth_dev *dev = param;
> > > > struct virtio_hw *hw = dev->data->dev_private;
> > > > uint8_t isr;
> > > > + bool check_config;
> > > >
> > > > - /* Read interrupt status which clears interrupt */
> > > > - isr = vtpci_isr(hw);
> > > > - PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> > > > + /* isr is used only when msix is not enabled */
> > > > + if (!hw->modern && !hw->use_msix) {
> > >
> > > The comment doesn't quite match the code: the hw->modern part is
> missing.
> > >
> > > > + isr = vtpci_isr(hw); /* Read to clears interrupt */
> > > > + PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> > > > + check_config = (isr & VIRTIO_PCI_ISR_CONFIG) ? true : false;
> > >
> > > The "? :" is unnecessary.
> > >
> > > --yliu
> > >
> > > > + } else {
> > > > + check_config = true;
> > > > + }
> > > >
> > > > if (rte_intr_enable(handle) < 0)
> > > > PMD_DRV_LOG(ERR, "interrupt enable failed");
> > > >
> > > > - if (isr & VIRTIO_PCI_ISR_CONFIG) {
> > > > - if (virtio_dev_link_update(dev, 0) == 0)
> > > > - _rte_eth_dev_callback_process(dev,
> > > > -
> > > RTE_ETH_EVENT_INTR_LSC, NULL);
> > > > - }
> > > > -
> > > > + if (check_config && virtio_dev_link_update(dev, 0) == 0)
> > > > + _rte_eth_dev_callback_process(dev,
> > > > + RTE_ETH_EVENT_INTR_LSC, NULL);
> > > > }
> > > >
> > > > static void
> > > > --
> > > > 2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-06 5:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-28 6:46 [dpdk-stable] [PATCH] net/virtio: fix read isr as MSI is enabled Jianfeng Tan
2017-04-06 5:19 ` Yuanhan Liu
2017-04-06 5:30 ` Tan, Jianfeng
2017-04-06 5:36 ` Yuanhan Liu
2017-04-06 5:40 ` Tan, Jianfeng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).