[dpdk-dev] [PATCH] net/qede: remove interrupt reconfigure in handler

[dpdk-dev] [PATCH] net/qede: remove interrupt reconfigure in handler

[David Marchand]

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(-)

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

Re: [dpdk-dev] [EXT] [PATCH] net/qede: remove interrupt reconfigure in handler

[Rasesh Mody]

>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

Re: [dpdk-dev] [EXT] [PATCH] net/qede: remove interrupt reconfigure in handler

[David Marchand]

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.


-- 
David Marchand

Re: [dpdk-dev] [EXT] [PATCH] net/qede: remove interrupt reconfigure in handler

[Shahed Shaikh]

>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

Re: [dpdk-dev] [dpdk-stable] [EXT] [PATCH] net/qede: remove interrupt reconfigure in handler

[David Marchand]

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.


-- 
David Marchand