diff mbox series

net/qede: remove interrupt reconfigure in handler

Message ID 1561469937-16077-1-git-send-email-david.marchand@redhat.com (mailing list archive)
State Rejected, archived
Delegated to: Ferruh Yigit
Headers
Series net/qede: remove interrupt reconfigure in handler |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS

Commit Message

David Marchand June 25, 2019, 1:38 p.m. UTC 
  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

Rasesh Mody June 25, 2019, 10:50 p.m. UTC | #1 
>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
David Marchand June 26, 2019, 7:37 a.m. UTC | #2 
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.
Shahed Shaikh July 1, 2019, 9:15 a.m. UTC | #3 
>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
David Marchand July 1, 2019, 9:24 a.m. UTC | #4 
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.
diff mbox series

Patch

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