* [dpdk-dev] [PATCH] net/i40e: fix wrong handle when enable interrupt @ 2017-02-09 20:02 Qi Zhang 2017-02-10 10:03 ` Ferruh Yigit 2017-03-21 12:09 ` [dpdk-dev] [PATCH v2] " Qi Zhang 0 siblings, 2 replies; 6+ messages in thread From: Qi Zhang @ 2017-02-09 20:02 UTC (permalink / raw) To: helin.zhang, jingjing.wu; +Cc: dev, cunming.liang, Qi Zhang In i40e_dev_interrupt_handler, when call rte_intr_enable, We should parse dev->intr_handle but not intr_handle. intr_handle is the copy of dev->intr_handle when it is registered, but parameter of dev->intr_handle is possible to be modifed later in i40e driver. Fixes: 2ce7a1ed09fc ("net/i40e: localize mapping of ethdev to PCI device") Signed-off-by: Qi Zhang <qi.z.zhang@intel.com> --- drivers/net/i40e/i40e_ethdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 4492bcc..1032d0f 100644 --- a/drivers/net/i40e/i40e_ethdev.c +++ b/drivers/net/i40e/i40e_ethdev.c @@ -5770,7 +5770,7 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev) * void */ static void -i40e_dev_interrupt_handler(struct rte_intr_handle *intr_handle, +i40e_dev_interrupt_handler(__rte_unused struct rte_intr_handle *intr_handle, void *param) { struct rte_eth_dev *dev = (struct rte_eth_dev *)param; @@ -5817,7 +5817,7 @@ i40e_dev_interrupt_handler(struct rte_intr_handle *intr_handle, done: /* Enable interrupt */ i40e_pf_enable_irq0(hw); - rte_intr_enable(intr_handle); + rte_intr_enable(dev->intr_handle); } static int -- 2.9.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] net/i40e: fix wrong handle when enable interrupt 2017-02-09 20:02 [dpdk-dev] [PATCH] net/i40e: fix wrong handle when enable interrupt Qi Zhang @ 2017-02-10 10:03 ` Ferruh Yigit 2017-03-21 12:09 ` [dpdk-dev] [PATCH v2] " Qi Zhang 1 sibling, 0 replies; 6+ messages in thread From: Ferruh Yigit @ 2017-02-10 10:03 UTC (permalink / raw) To: Qi Zhang, helin.zhang, jingjing.wu; +Cc: dev, cunming.liang On 2/9/2017 8:02 PM, Qi Zhang wrote: > In i40e_dev_interrupt_handler, when call rte_intr_enable, > We should parse dev->intr_handle but not intr_handle. > intr_handle is the copy of dev->intr_handle when > it is registered, but parameter of dev->intr_handle is > possible to be modifed later in i40e driver. If dev->intr_handle modified, shouldn't driver unregister old one and register new interrupt handle? Registering one handle but using another in handler function seems wrong. > > Fixes: 2ce7a1ed09fc ("net/i40e: localize mapping of ethdev to PCI device") > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com> <...> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH v2] net/i40e: fix wrong handle when enable interrupt 2017-02-09 20:02 [dpdk-dev] [PATCH] net/i40e: fix wrong handle when enable interrupt Qi Zhang 2017-02-10 10:03 ` Ferruh Yigit @ 2017-03-21 12:09 ` Qi Zhang 2017-03-21 11:34 ` Zhang, Qi Z 1 sibling, 1 reply; 6+ messages in thread From: Qi Zhang @ 2017-03-21 12:09 UTC (permalink / raw) To: jingjing.wu, helin.zhang; +Cc: dev, Qi Zhang, stable In i40e_dev_interrupt_handler, when call rte_intr_enable, the intr_handle is the copy when we registered. According to interrupt handle framework, if the requirement of intr_handle is changed, we need to unregister then register a new copy. This happens on i40e driver when bind to vfio-pci, the rte_intr_efd_enable function will modify the max_intr according the queue number, so a new copy of intr_handle need to be registered. Without this fix, we saw lw3fwd-power does not work due to wrong interrupt count in vfio_irq_set when set vfio interrupt. Fixes: 2ce7a1ed09fc ("net/i40e: localize mapping of ethdev to PCI device") Cc: stable@dpdk.org Signed-off-by: Qi Zhang <qi.z.zhang@intel.com> --- v2: - follow current design, when intr_handle is modified, unregister the old one and register the new one. - there should be a patch set to fix on other devices. drivers/net/i40e/i40e_ethdev.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 9c76baa..e7bbea5 100644 --- a/drivers/net/i40e/i40e_ethdev.c +++ b/drivers/net/i40e/i40e_ethdev.c @@ -1886,6 +1886,14 @@ i40e_dev_start(struct rte_eth_dev *dev) ret = rte_intr_efd_enable(intr_handle, intr_vector); if (ret) return ret; + /** + * intr_handle may be modified in rte_intr_efd_enable + * so unregster the old one and register the new one. + */ + rte_intr_callback_unregister(intr_handle, + i40e_dev_interrupt_handler, dev); + rte_intr_callback_register(intr_handle, + i40e_dev_interrupt_handler, dev); } if (rte_intr_dp_is_en(intr_handle) && !intr_handle->intr_vec) { -- 2.9.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: fix wrong handle when enable interrupt 2017-03-21 12:09 ` [dpdk-dev] [PATCH v2] " Qi Zhang @ 2017-03-21 11:34 ` Zhang, Qi Z 2017-03-30 12:26 ` Wu, Jingjing 0 siblings, 1 reply; 6+ messages in thread From: Zhang, Qi Z @ 2017-03-21 11:34 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev, Yigit, Ferruh Hi Thomas: > -----Original Message----- > From: Zhang, Qi Z > Sent: Tuesday, March 21, 2017 8:10 PM > To: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin > <helin.zhang@intel.com> > Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org > Subject: [PATCH v2] net/i40e: fix wrong handle when enable interrupt > > In i40e_dev_interrupt_handler, when call rte_intr_enable, the intr_handle is > the copy when we registered. > According to interrupt handle framework, if the requirement of intr_handle > is changed, we need to unregister then register a new copy. This happens on > i40e driver when bind to vfio-pci, the rte_intr_efd_enable function will > modify the max_intr according the queue number, so a new copy of > intr_handle need to be registered. > Without this fix, we saw lw3fwd-power does not work due to wrong > interrupt count in vfio_irq_set when set vfio interrupt. > > Fixes: 2ce7a1ed09fc ("net/i40e: localize mapping of ethdev to PCI device") > > Cc: stable@dpdk.org > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com> > --- > v2: > - follow current design, when intr_handle is modified, unregister > the old one and register the new one. > - there should be a patch set to fix on other devices. > > drivers/net/i40e/i40e_ethdev.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c > index 9c76baa..e7bbea5 100644 > --- a/drivers/net/i40e/i40e_ethdev.c > +++ b/drivers/net/i40e/i40e_ethdev.c > @@ -1886,6 +1886,14 @@ i40e_dev_start(struct rte_eth_dev *dev) > ret = rte_intr_efd_enable(intr_handle, intr_vector); > if (ret) > return ret; > + /** > + * intr_handle may be modified in rte_intr_efd_enable > + * so unregster the old one and register the new one. > + */ > + rte_intr_callback_unregister(intr_handle, > + i40e_dev_interrupt_handler, dev); > + rte_intr_callback_register(intr_handle, > + i40e_dev_interrupt_handler, dev); To me, this fix looks a little bit weird. May I know why we need to register an copy of intr_handle ? What do you think about my previous clean up patch. http://dpdk.org/dev/patchwork/patch/21529/ Thanks Qi. > } > > if (rte_intr_dp_is_en(intr_handle) && !intr_handle->intr_vec) { > -- > 2.9.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: fix wrong handle when enable interrupt 2017-03-21 11:34 ` Zhang, Qi Z @ 2017-03-30 12:26 ` Wu, Jingjing 2017-04-11 11:00 ` Ferruh Yigit 0 siblings, 1 reply; 6+ messages in thread From: Wu, Jingjing @ 2017-03-30 12:26 UTC (permalink / raw) To: Zhang, Qi Z, Thomas Monjalon; +Cc: dev, Yigit, Ferruh > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhang, Qi Z > Sent: Tuesday, March 21, 2017 7:34 PM > To: Thomas Monjalon <thomas.monjalon@6wind.com> > Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com> > Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix wrong handle when enable > interrupt > > Hi Thomas: > > > > -----Original Message----- > > From: Zhang, Qi Z > > Sent: Tuesday, March 21, 2017 8:10 PM > > To: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin > > <helin.zhang@intel.com> > > Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org > > Subject: [PATCH v2] net/i40e: fix wrong handle when enable interrupt > > > > In i40e_dev_interrupt_handler, when call rte_intr_enable, the > > intr_handle is the copy when we registered. > > According to interrupt handle framework, if the requirement of > > intr_handle is changed, we need to unregister then register a new > > copy. This happens on i40e driver when bind to vfio-pci, the > > rte_intr_efd_enable function will modify the max_intr according the > > queue number, so a new copy of intr_handle need to be registered. > > Without this fix, we saw lw3fwd-power does not work due to wrong > > interrupt count in vfio_irq_set when set vfio interrupt. > > > > Fixes: 2ce7a1ed09fc ("net/i40e: localize mapping of ethdev to PCI > > device") > > > > Cc: stable@dpdk.org > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com> > > --- > > v2: > > - follow current design, when intr_handle is modified, unregister > > the old one and register the new one. > > - there should be a patch set to fix on other devices. > > > > drivers/net/i40e/i40e_ethdev.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c > > b/drivers/net/i40e/i40e_ethdev.c index 9c76baa..e7bbea5 100644 > > --- a/drivers/net/i40e/i40e_ethdev.c > > +++ b/drivers/net/i40e/i40e_ethdev.c > > @@ -1886,6 +1886,14 @@ i40e_dev_start(struct rte_eth_dev *dev) > > ret = rte_intr_efd_enable(intr_handle, intr_vector); > > if (ret) > > return ret; > > + /** > > + * intr_handle may be modified in rte_intr_efd_enable > > + * so unregster the old one and register the new one. > > + */ > > + rte_intr_callback_unregister(intr_handle, > > + i40e_dev_interrupt_handler, dev); > > + rte_intr_callback_register(intr_handle, > > + i40e_dev_interrupt_handler, dev); > To me, this fix looks a little bit weird. > May I know why we need to register an copy of intr_handle ? > What do you think about my previous clean up patch. > http://dpdk.org/dev/patchwork/patch/21529/ > Thanks > Qi. It seems [dpdk-dev,v2,1/3] vfio: keep interrupt source read only http://www.dpdk.org/dev/patchwork/patch/21528/ is already be acked by Anatoly. I think we need to support that change, but not this workaround. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: fix wrong handle when enable interrupt 2017-03-30 12:26 ` Wu, Jingjing @ 2017-04-11 11:00 ` Ferruh Yigit 0 siblings, 0 replies; 6+ messages in thread From: Ferruh Yigit @ 2017-04-11 11:00 UTC (permalink / raw) To: Wu, Jingjing, Zhang, Qi Z, Thomas Monjalon; +Cc: dev On 3/30/2017 1:26 PM, Wu, Jingjing wrote: > > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhang, Qi Z >> Sent: Tuesday, March 21, 2017 7:34 PM >> To: Thomas Monjalon <thomas.monjalon@6wind.com> >> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com> >> Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix wrong handle when enable >> interrupt >> >> Hi Thomas: >> >> >>> -----Original Message----- >>> From: Zhang, Qi Z >>> Sent: Tuesday, March 21, 2017 8:10 PM >>> To: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin >>> <helin.zhang@intel.com> >>> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org >>> Subject: [PATCH v2] net/i40e: fix wrong handle when enable interrupt >>> >>> In i40e_dev_interrupt_handler, when call rte_intr_enable, the >>> intr_handle is the copy when we registered. >>> According to interrupt handle framework, if the requirement of >>> intr_handle is changed, we need to unregister then register a new >>> copy. This happens on i40e driver when bind to vfio-pci, the >>> rte_intr_efd_enable function will modify the max_intr according the >>> queue number, so a new copy of intr_handle need to be registered. >>> Without this fix, we saw lw3fwd-power does not work due to wrong >>> interrupt count in vfio_irq_set when set vfio interrupt. >>> >>> Fixes: 2ce7a1ed09fc ("net/i40e: localize mapping of ethdev to PCI >>> device") >>> >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com> >>> --- >>> v2: >>> - follow current design, when intr_handle is modified, unregister >>> the old one and register the new one. >>> - there should be a patch set to fix on other devices. >>> >>> drivers/net/i40e/i40e_ethdev.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/net/i40e/i40e_ethdev.c >>> b/drivers/net/i40e/i40e_ethdev.c index 9c76baa..e7bbea5 100644 >>> --- a/drivers/net/i40e/i40e_ethdev.c >>> +++ b/drivers/net/i40e/i40e_ethdev.c >>> @@ -1886,6 +1886,14 @@ i40e_dev_start(struct rte_eth_dev *dev) >>> ret = rte_intr_efd_enable(intr_handle, intr_vector); >>> if (ret) >>> return ret; >>> + /** >>> + * intr_handle may be modified in rte_intr_efd_enable >>> + * so unregster the old one and register the new one. >>> + */ >>> + rte_intr_callback_unregister(intr_handle, >>> + i40e_dev_interrupt_handler, dev); >>> + rte_intr_callback_register(intr_handle, >>> + i40e_dev_interrupt_handler, dev); >> To me, this fix looks a little bit weird. >> May I know why we need to register an copy of intr_handle ? >> What do you think about my previous clean up patch. >> http://dpdk.org/dev/patchwork/patch/21529/ >> Thanks >> Qi. > > It seems [dpdk-dev,v2,1/3] vfio: keep interrupt source read only > http://www.dpdk.org/dev/patchwork/patch/21528/ is already > be acked by Anatoly. > > I think we need to support that change, but not this workaround. > No more required after "clean up interrupt handle" patches, patch status updated to rejected. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-11 11:00 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-09 20:02 [dpdk-dev] [PATCH] net/i40e: fix wrong handle when enable interrupt Qi Zhang 2017-02-10 10:03 ` Ferruh Yigit 2017-03-21 12:09 ` [dpdk-dev] [PATCH v2] " Qi Zhang 2017-03-21 11:34 ` Zhang, Qi Z 2017-03-30 12:26 ` Wu, Jingjing 2017-04-11 11:00 ` Ferruh Yigit
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).