All of lore.kernel.org
 help / color / mirror / Atom feed
* [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 ` [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 related	[flat|nested] 6+ messages in thread
* Re: [PATCH] net/i40e: fix wrong handle when enable interrupt
  2017-02-09 20:02 [PATCH] net/i40e: fix wrong handle when enable interrupt Qi Zhang
@ 2017-02-10 10:03 ` Ferruh Yigit
  2017-03-21 12:09 ` [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
* Re: [PATCH v2] net/i40e: fix wrong handle when enable interrupt
  2017-03-21 12:09 ` [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
* [PATCH v2] net/i40e: fix wrong handle when enable interrupt
  2017-02-09 20:02 [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 related	[flat|nested] 6+ messages in thread
* Re: [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: [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 [PATCH] net/i40e: fix wrong handle when enable interrupt Qi Zhang
2017-02-10 10:03 ` Ferruh Yigit
2017-03-21 12:09 ` [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.