net/qede: remove interrupt reconfigure in handler
Checks
Commit Message
rte_intr_enable/rte_intr_disable configure the interrupt context on the
kernel side (either uio or vfio).
In VFIO case, calling it from the interrupt handlers triggers an
unneeded interrupt handlers reconfiguration.
During this reconfiguration window, the device can trigger interrupts
which are left unserviced.
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1654824
Fixes: 245aec289338 ("net/qede: fix legacy interrupt mode")
Fixes: 2ea6f76aff40 ("qede: add core driver")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
drivers/net/qede/qede_ethdev.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
Comments
>From: David Marchand <david.marchand@redhat.com>
>Sent: Tuesday, June 25, 2019 6:39 AM
>
>----------------------------------------------------------------------
>rte_intr_enable/rte_intr_disable configure the interrupt context on the
>kernel side (either uio or vfio).
>In VFIO case, calling it from the interrupt handlers triggers an unneeded
>interrupt handlers reconfiguration.
>During this reconfiguration window, the device can trigger interrupts which
>are left unserviced.
>
>Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1654824
>Fixes: 245aec289338 ("net/qede: fix legacy interrupt mode")
>Fixes: 2ea6f76aff40 ("qede: add core driver")
>Cc: stable@dpdk.org
>
>Signed-off-by: David Marchand <david.marchand@redhat.com>
>---
Change looks good, thanks.
Acked-by: Rasesh Mody <rmody@marvell.com>
> drivers/net/qede/qede_ethdev.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
>diff --git a/drivers/net/qede/qede_ethdev.c
>b/drivers/net/qede/qede_ethdev.c index 82363e6..807016a 100644
>--- a/drivers/net/qede/qede_ethdev.c
>+++ b/drivers/net/qede/qede_ethdev.c
>@@ -245,12 +245,8 @@ static void qede_interrupt_action(struct ecore_hwfn
>*p_hwfn)
>
> /* Check if our device actually raised an interrupt */
> status =
>ecore_int_igu_read_sisr_reg(ECORE_LEADING_HWFN(edev));
>- if (status & 0x1) {
>+ if (status & 0x1)
> qede_interrupt_action(ECORE_LEADING_HWFN(edev));
>-
>- if (rte_intr_enable(eth_dev->intr_handle))
>- DP_ERR(edev, "rte_intr_enable failed\n");
>- }
> }
>
> static void
>@@ -261,8 +257,6 @@ static void qede_interrupt_action(struct ecore_hwfn
>*p_hwfn)
> struct ecore_dev *edev = &qdev->edev;
>
> qede_interrupt_action(ECORE_LEADING_HWFN(edev));
>- if (rte_intr_enable(eth_dev->intr_handle))
>- DP_ERR(edev, "rte_intr_enable failed\n");
> }
>
> static void
>--
>1.8.3.1
On Wed, Jun 26, 2019 at 12:50 AM Rasesh Mody <rmody@marvell.com> wrote:
> >From: David Marchand <david.marchand@redhat.com>
> >Sent: Tuesday, June 25, 2019 6:39 AM
> >
> >----------------------------------------------------------------------
> >rte_intr_enable/rte_intr_disable configure the interrupt context on the
> >kernel side (either uio or vfio).
> >In VFIO case, calling it from the interrupt handlers triggers an unneeded
> >interrupt handlers reconfiguration.
> >During this reconfiguration window, the device can trigger interrupts
> which
> >are left unserviced.
> >
> >Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1654824
> >Fixes: 245aec289338 ("net/qede: fix legacy interrupt mode")
> >Fixes: 2ea6f76aff40 ("qede: add core driver")
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: David Marchand <david.marchand@redhat.com>
> >---
>
> Change looks good, thanks.
>
> Acked-by: Rasesh Mody <rmody@marvell.com>
>
Something still bothers me about the meaning of rte_intr_enable()...
Let me write a mail to a little more people :-)
For now, let's put this patch on hold.
>From: David Marchand <david.marchand@redhat.com>
>Sent: Wednesday, June 26, 2019 1:07 PM
>To: Rasesh Mody <rmody@marvell.com>
>Cc: dev@dpdk.org; stable@dpdk.org; Shahed Shaikh <shshaikh@marvell.com>
>Subject: Re: [EXT] [PATCH] net/qede: remove interrupt reconfigure in handler
>On Wed, Jun 26, 2019 at 12:50 AM Rasesh Mody <mailto:rmody@marvell.com> wrote:
>>From: David Marchand <mailto:david.marchand@redhat.com>
>>Sent: Tuesday, June 25, 2019 6:39 AM
>>
>>----------------------------------------------------------------------
>>rte_intr_enable/rte_intr_disable configure the interrupt context on the
>>kernel side (either uio or vfio).
>>In VFIO case, calling it from the interrupt handlers triggers an unneeded
>>interrupt handlers reconfiguration.
>>During this reconfiguration window, the device can trigger interrupts which
>>are left unserviced.
>>
>>Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1654824
>>Fixes: 245aec289338 ("net/qede: fix legacy interrupt mode")
>>Fixes: 2ea6f76aff40 ("qede: add core driver")
>>Cc: mailto:stable@dpdk.org
>>
>>Signed-off-by: David Marchand <mailto:david.marchand@redhat.com>
>>---
>Change looks good, thanks.
>Acked-by: Rasesh Mody <mailto:rmody@marvell.com>
>Something still bothers me about the meaning of rte_intr_enable()...
>Let me write a mail to a little more people :-)
>For now, let's put this patch on hold.
Another question I have is, is it required to re-enable interrupt by PMD at the end of interrupt handling by calling rte_intr_enable()?
Does DPDK core / vfio/uio module take care of re-enabling the interrupt after the interrupt is handled?
Thanks,
Shahed
On Mon, Jul 1, 2019 at 11:15 AM Shahed Shaikh <shshaikh@marvell.com> wrote:
>
>
> >From: David Marchand <david.marchand@redhat.com>
> >Sent: Wednesday, June 26, 2019 1:07 PM
> >To: Rasesh Mody <rmody@marvell.com>
> >Cc: dev@dpdk.org; stable@dpdk.org; Shahed Shaikh <shshaikh@marvell.com>
> >Subject: Re: [EXT] [PATCH] net/qede: remove interrupt reconfigure in
> handler
>
> >On Wed, Jun 26, 2019 at 12:50 AM Rasesh Mody <mailto:rmody@marvell.com>
> wrote:
> >>From: David Marchand <mailto:david.marchand@redhat.com>
> >>Sent: Tuesday, June 25, 2019 6:39 AM
> >>
> >>----------------------------------------------------------------------
> >>rte_intr_enable/rte_intr_disable configure the interrupt context on the
> >>kernel side (either uio or vfio).
> >>In VFIO case, calling it from the interrupt handlers triggers an unneeded
> >>interrupt handlers reconfiguration.
> >>During this reconfiguration window, the device can trigger interrupts
> which
> >>are left unserviced.
> >>
> >>Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1654824
> >>Fixes: 245aec289338 ("net/qede: fix legacy interrupt mode")
> >>Fixes: 2ea6f76aff40 ("qede: add core driver")
> >>Cc: mailto:stable@dpdk.org
> >>
> >>Signed-off-by: David Marchand <mailto:david.marchand@redhat.com>
> >>---
> >Change looks good, thanks.
> >Acked-by: Rasesh Mody <mailto:rmody@marvell.com>
>
> >Something still bothers me about the meaning of rte_intr_enable()...
> >Let me write a mail to a little more people :-)
>
> >For now, let's put this patch on hold.
>
> Another question I have is, is it required to re-enable interrupt by PMD
> at the end of interrupt handling by calling rte_intr_enable()?
> Does DPDK core / vfio/uio module take care of re-enabling the interrupt
> after the interrupt is handled?
>
This is exactly why I wanted to put this on hold.
This patch is just wrong, auto NAK for me :-).
I am currently reading the VFIO api and how UIO behaves to try and come
with a common fix on the DPDK infrastructure side.
@@ -245,12 +245,8 @@ static void qede_interrupt_action(struct ecore_hwfn *p_hwfn)
/* Check if our device actually raised an interrupt */
status = ecore_int_igu_read_sisr_reg(ECORE_LEADING_HWFN(edev));
- if (status & 0x1) {
+ if (status & 0x1)
qede_interrupt_action(ECORE_LEADING_HWFN(edev));
-
- if (rte_intr_enable(eth_dev->intr_handle))
- DP_ERR(edev, "rte_intr_enable failed\n");
- }
}
static void
@@ -261,8 +257,6 @@ static void qede_interrupt_action(struct ecore_hwfn *p_hwfn)
struct ecore_dev *edev = &qdev->edev;
qede_interrupt_action(ECORE_LEADING_HWFN(edev));
- if (rte_intr_enable(eth_dev->intr_handle))
- DP_ERR(edev, "rte_intr_enable failed\n");
}
static void