[dpdk-dev,v2] net/i40e: fix wrong handle when enable interrupt
Checks
Commit Message
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(+)
Comments
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
> -----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.
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.
@@ -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) {