[dpdk-dev] [PATCH] net/i40e: fix wrong handle when enable interrupt

[dpdk-dev] [PATCH] net/i40e: fix wrong handle when enable interrupt

[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

Re: [dpdk-dev] [PATCH] net/i40e: fix wrong handle when enable interrupt

[Ferruh Yigit]

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>

<...>

[dpdk-dev] [PATCH v2] net/i40e: fix wrong handle when enable interrupt

[Qi Zhang]

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

Re: [dpdk-dev] [PATCH v2] net/i40e: fix wrong handle when enable interrupt

[Zhang, Qi Z]

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

Re: [dpdk-dev] [PATCH v2] net/i40e: fix wrong handle when enable interrupt

[Wu, Jingjing]

> -----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.

Re: [dpdk-dev] [PATCH v2] net/i40e: fix wrong handle when enable interrupt

[Ferruh Yigit]

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.