DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/qede: remove interrupt reconfigure in handler
@ 2019-06-25 13:38 David Marchand
  2019-06-25 22:50 ` [dpdk-dev] [EXT] " Rasesh Mody
  0 siblings, 1 reply; 5+ messages in thread
From: David Marchand @ 2019-06-25 13:38 UTC (permalink / raw)
  To: dev; +Cc: stable, Rasesh Mody, Shahed Shaikh

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH] net/qede: remove interrupt reconfigure in handler
  2019-06-25 13:38 [dpdk-dev] [PATCH] net/qede: remove interrupt reconfigure in handler David Marchand
@ 2019-06-25 22:50 ` Rasesh Mody
  2019-06-26  7:37   ` David Marchand
  0 siblings, 1 reply; 5+ messages in thread
From: Rasesh Mody @ 2019-06-25 22:50 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: stable, Shahed Shaikh

>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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH] net/qede: remove interrupt reconfigure in handler
  2019-06-25 22:50 ` [dpdk-dev] [EXT] " Rasesh Mody
@ 2019-06-26  7:37   ` David Marchand
  2019-07-01  9:15     ` Shahed Shaikh
  0 siblings, 1 reply; 5+ messages in thread
From: David Marchand @ 2019-06-26  7:37 UTC (permalink / raw)
  To: Rasesh Mody; +Cc: dev, stable, Shahed Shaikh

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH] net/qede: remove interrupt reconfigure in handler
  2019-06-26  7:37   ` David Marchand
@ 2019-07-01  9:15     ` Shahed Shaikh
  2019-07-01  9:24       ` [dpdk-dev] [dpdk-stable] " David Marchand
  0 siblings, 1 reply; 5+ messages in thread
From: Shahed Shaikh @ 2019-07-01  9:15 UTC (permalink / raw)
  To: David Marchand, Rasesh Mody; +Cc: dev, stable



>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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [EXT] [PATCH] net/qede: remove interrupt reconfigure in handler
  2019-07-01  9:15     ` Shahed Shaikh
@ 2019-07-01  9:24       ` David Marchand
  0 siblings, 0 replies; 5+ messages in thread
From: David Marchand @ 2019-07-01  9:24 UTC (permalink / raw)
  To: Shahed Shaikh; +Cc: Rasesh Mody, dev, stable, Burakov, Anatoly

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-07-01  9:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-25 13:38 [dpdk-dev] [PATCH] net/qede: remove interrupt reconfigure in handler David Marchand
2019-06-25 22:50 ` [dpdk-dev] [EXT] " Rasesh Mody
2019-06-26  7:37   ` David Marchand
2019-07-01  9:15     ` Shahed Shaikh
2019-07-01  9:24       ` [dpdk-dev] [dpdk-stable] " David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).