[PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[Neil Horman]

Hey-
	I noted recently that bnx2 had a few problems with its poll_controller
method:

1) It passes a net_device structure to bnx2_interrupt, but bnx2_interrupt
expects a bnx2_napi structure.

2) bnx2_interrupt never checks all its rx queues, so theres no guarantee that
we'll see incomming frames.

This patch should fix both those problems

Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 bnx2.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 430d430..9e1f0e3 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -7203,10 +7203,12 @@ bnx2_change_mtu(struct net_device *dev, int new_mtu)
 static void
 poll_bnx2(struct net_device *dev)
 {
+	int i;
 	struct bnx2 *bp = netdev_priv(dev);
 
 	disable_irq(bp->pdev->irq);
-	bnx2_interrupt(bp->pdev->irq, dev);
+	for (i = 0; i < bp->irq_nvecs; i++)
+		bnx2_interrupt(bp->pdev->irq, &bp->bnx2_napi[i]);
 	enable_irq(bp->pdev->irq);
 }
 #endif
-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/

Re: [PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[Andy Gospodarek]

On Tue, Nov 11, 2008 at 09:00:55AM -0500, Neil Horman wrote:
> Hey-
> 	I noted recently that bnx2 had a few problems with its poll_controller
> method:
> 
> 1) It passes a net_device structure to bnx2_interrupt, but bnx2_interrupt
> expects a bnx2_napi structure.
> 
> 2) bnx2_interrupt never checks all its rx queues, so theres no guarantee that
> we'll see incomming frames.
> 
> This patch should fix both those problems
> 
> Regards
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
>  bnx2.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 430d430..9e1f0e3 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -7203,10 +7203,12 @@ bnx2_change_mtu(struct net_device *dev, int new_mtu)
>  static void
>  poll_bnx2(struct net_device *dev)
>  {
> +	int i;
>  	struct bnx2 *bp = netdev_priv(dev);
>  
>  	disable_irq(bp->pdev->irq);
> -	bnx2_interrupt(bp->pdev->irq, dev);
> +	for (i = 0; i < bp->irq_nvecs; i++)
> +		bnx2_interrupt(bp->pdev->irq, &bp->bnx2_napi[i]);
>  	enable_irq(bp->pdev->irq);
>  }
>  #endif

I suspect bnx2 isn't the only one that needs to address #2 above due to
ever-increasing usage of MSI-X and multiqueue rx.

Acked-by: Andy Gospodarek <andy@greyhouse.net>

Re: [PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[Michael Chan]

Neil Horman wrote:

> Hey-
>       I noted recently that bnx2 had a few problems with its
> poll_controller
> method:
>
> 1) It passes a net_device structure to bnx2_interrupt, but
> bnx2_interrupt
> expects a bnx2_napi structure.
>
> 2) bnx2_interrupt never checks all its rx queues, so theres
> no guarantee that
> we'll see incomming frames.
>
> This patch should fix both those problems
>
> Regards
> Neil
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>
>
>  bnx2.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
>
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 430d430..9e1f0e3 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -7203,10 +7203,12 @@ bnx2_change_mtu(struct net_device
> *dev, int new_mtu)
>  static void
>  poll_bnx2(struct net_device *dev)
>  {
> +     int i;
>       struct bnx2 *bp = netdev_priv(dev);
>
>       disable_irq(bp->pdev->irq);
> -     bnx2_interrupt(bp->pdev->irq, dev);
> +     for (i = 0; i < bp->irq_nvecs; i++)
> +             bnx2_interrupt(bp->pdev->irq, &bp->bnx2_napi[i]);

Neil, thanks for the patch.

We should probably pass bp->irq_tbl[i].vector as 1st parameter
for correctness, even though it doesn't really matter in this
case since it is not used by bnx2_interrupt().

>       enable_irq(bp->pdev->irq);
>  }
>  #endif

Re: [PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[Neil Horman]

On Tue, Nov 11, 2008 at 07:12:13AM -0800, Michael Chan wrote:
> Neil Horman wrote:
> 
> > Hey-
> >       I noted recently that bnx2 had a few problems with its
> > poll_controller
> > method:
> >
> > 1) It passes a net_device structure to bnx2_interrupt, but
> > bnx2_interrupt
> > expects a bnx2_napi structure.
> >
> > 2) bnx2_interrupt never checks all its rx queues, so theres
> > no guarantee that
> > we'll see incomming frames.
> >
> > This patch should fix both those problems
> >
> > Regards
> > Neil
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> >
> >
> >  bnx2.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> >
> > diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> > index 430d430..9e1f0e3 100644
> > --- a/drivers/net/bnx2.c
> > +++ b/drivers/net/bnx2.c
> > @@ -7203,10 +7203,12 @@ bnx2_change_mtu(struct net_device
> > *dev, int new_mtu)
> >  static void
> >  poll_bnx2(struct net_device *dev)
> >  {
> > +     int i;
> >       struct bnx2 *bp = netdev_priv(dev);
> >
> >       disable_irq(bp->pdev->irq);
> > -     bnx2_interrupt(bp->pdev->irq, dev);
> > +     for (i = 0; i < bp->irq_nvecs; i++)
> > +             bnx2_interrupt(bp->pdev->irq, &bp->bnx2_napi[i]);
> 
> Neil, thanks for the patch.
> 
> We should probably pass bp->irq_tbl[i].vector as 1st parameter
> for correctness, even though it doesn't really matter in this
> case since it is not used by bnx2_interrupt().
> 
I thought we overwrote pdev->irq with the value from irq_tbl when we registered
for msi interrupts?

Neil

> >       enable_irq(bp->pdev->irq);
> >  }
> >  #endif
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/

Re: [PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[Michael Chan]

On Tue, 2008-11-11 at 08:46 -0800, Neil Horman wrote:
> On Tue, Nov 11, 2008 at 07:12:13AM -0800, Michael Chan wrote:
> > Neil Horman wrote:
> > > diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> > > index 430d430..9e1f0e3 100644
> > > --- a/drivers/net/bnx2.c
> > > +++ b/drivers/net/bnx2.c
> > > @@ -7203,10 +7203,12 @@ bnx2_change_mtu(struct net_device
> > > *dev, int new_mtu)
> > >  static void
> > >  poll_bnx2(struct net_device *dev)
> > >  {
> > > +     int i;
> > >       struct bnx2 *bp = netdev_priv(dev);
> > >
> > >       disable_irq(bp->pdev->irq);
> > > -     bnx2_interrupt(bp->pdev->irq, dev);
> > > +     for (i = 0; i < bp->irq_nvecs; i++)
> > > +             bnx2_interrupt(bp->pdev->irq, &bp->bnx2_napi[i]);
> > 
> > Neil, thanks for the patch.
> > 
> > We should probably pass bp->irq_tbl[i].vector as 1st parameter
> > for correctness, even though it doesn't really matter in this
> > case since it is not used by bnx2_interrupt().
> > 
> I thought we overwrote pdev->irq with the value from irq_tbl when we registered
> for msi interrupts?
> 

In MSI and INTA mode, we store pdev->irq into irq_tbl[0].vector.  In
MSI-X mode, we'll have multiple vectors in the irq_tbl entries and
pdev->irq is no longer relevant.  So to be correct when we have multiple
vectors, the irq_tbl values should be used.

Thanks.

Re: [PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[Neil Horman]

On Tue, Nov 11, 2008 at 09:09:10AM -0800, Michael Chan wrote:
> 
> On Tue, 2008-11-11 at 08:46 -0800, Neil Horman wrote:
> > On Tue, Nov 11, 2008 at 07:12:13AM -0800, Michael Chan wrote:
> > > Neil Horman wrote:
> > > > diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> > > > index 430d430..9e1f0e3 100644
> > > > --- a/drivers/net/bnx2.c
> > > > +++ b/drivers/net/bnx2.c
> > > > @@ -7203,10 +7203,12 @@ bnx2_change_mtu(struct net_device
> > > > *dev, int new_mtu)
> > > >  static void
> > > >  poll_bnx2(struct net_device *dev)
> > > >  {
> > > > +     int i;
> > > >       struct bnx2 *bp = netdev_priv(dev);
> > > >
> > > >       disable_irq(bp->pdev->irq);
> > > > -     bnx2_interrupt(bp->pdev->irq, dev);
> > > > +     for (i = 0; i < bp->irq_nvecs; i++)
> > > > +             bnx2_interrupt(bp->pdev->irq, &bp->bnx2_napi[i]);
> > > 
> > > Neil, thanks for the patch.
> > > 
> > > We should probably pass bp->irq_tbl[i].vector as 1st parameter
> > > for correctness, even though it doesn't really matter in this
> > > case since it is not used by bnx2_interrupt().
> > > 
> > I thought we overwrote pdev->irq with the value from irq_tbl when we registered
> > for msi interrupts?
> > 
> 
> In MSI and INTA mode, we store pdev->irq into irq_tbl[0].vector.  In
> MSI-X mode, we'll have multiple vectors in the irq_tbl entries and
> pdev->irq is no longer relevant.  So to be correct when we have multiple
> vectors, the irq_tbl values should be used.
> 
> Thanks.
> 


Copy that.  Here you go, followon patch to change how we pass the irq vector to
bnx2_interrupt.  Doesn't do anything super-usefull, but good for the sake of
correctness

Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 bnx2.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 9e1f0e3..cad8b7a 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -7208,7 +7208,7 @@ poll_bnx2(struct net_device *dev)
 
 	disable_irq(bp->pdev->irq);
 	for (i = 0; i < bp->irq_nvecs; i++)
-		bnx2_interrupt(bp->pdev->irq, &bp->bnx2_napi[i]);
+		bnx2_interrupt(bp->irq_tbl[i].vector, &bp->bnx2_napi[i]);
 	enable_irq(bp->pdev->irq);
 }
 #endif
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/

Re: [PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[Michael Chan]

On Tue, 2008-11-11 at 09:37 -0800, Neil Horman wrote:
> Copy that.  Here you go, followon patch to change how we pass the irq vector to
> bnx2_interrupt.  Doesn't do anything super-usefull, but good for the sake of
> correctness

Sorry, I missed something earlier.  After looking at this more closely,
we should also move disable_irq() into the loop and call it with the
same vector values from irq_tbl.

Thanks.
> 
> Regards
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
>  bnx2.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 9e1f0e3..cad8b7a 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -7208,7 +7208,7 @@ poll_bnx2(struct net_device *dev)
>  
>         disable_irq(bp->pdev->irq);
>         for (i = 0; i < bp->irq_nvecs; i++)
> -               bnx2_interrupt(bp->pdev->irq, &bp->bnx2_napi[i]);
> +               bnx2_interrupt(bp->irq_tbl[i].vector, &bp->bnx2_napi[i]);
>         enable_irq(bp->pdev->irq);
>  }
>  #endif

Re: [PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[Neil Horman]

On Tue, Nov 11, 2008 at 09:59:24AM -0800, Michael Chan wrote:
> 
> On Tue, 2008-11-11 at 09:37 -0800, Neil Horman wrote:
> > Copy that.  Here you go, followon patch to change how we pass the irq vector to
> > bnx2_interrupt.  Doesn't do anything super-usefull, but good for the sake of
> > correctness
> 
> Sorry, I missed something earlier.  After looking at this more closely,
> we should also move disable_irq() into the loop and call it with the
> same vector values from irq_tbl.
> 

Oops your right, new comprehensive patch, replacing the previous 2.

Fix bnx2 so that netpoll works properly.  Specifically:

1) Fix parameters to bnx2_interrupt to be a struct bnx2_napi rather than a
struct net_device

2) Fix poll_controller method to check every queue in the rx case so frames
aren't missed

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 bnx2.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)


diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 430d430..d07e3f1 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -7204,10 +7204,13 @@ static void
 poll_bnx2(struct net_device *dev)
 {
 	struct bnx2 *bp = netdev_priv(dev);
+	int i;
 
-	disable_irq(bp->pdev->irq);
-	bnx2_interrupt(bp->pdev->irq, dev);
-	enable_irq(bp->pdev->irq);
+	for (i = 0; i < bp->irq_nvecs; i++) {
+		disable_irq(bp->irq_tbl[i].vector);
+		bnx2_interrupt(bp->irq_tbl[i].vector, &bp->bnx2_napi[i]);
+		enable_irq(bp->irq_tbl[i].vector);
+	}
 }
 #endif
 
-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/

Re: [PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 11 Nov 2008 15:58:03 -0500

> Fix bnx2 so that netpoll works properly.  Specifically:
> 
> 1) Fix parameters to bnx2_interrupt to be a struct bnx2_napi rather than a
> struct net_device
> 
> 2) Fix poll_controller method to check every queue in the rx case so frames
> aren't missed
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

This looks good, applied, thanks Neil!

Re: [PATCH] bnx2: fix poll_controller method so that proper structures are passed and all rx queues are checked

[Neil Horman]

On Wed, Nov 12, 2008 at 04:23:59PM -0800, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Tue, 11 Nov 2008 15:58:03 -0500
> 
> > Fix bnx2 so that netpoll works properly.  Specifically:
> > 
> > 1) Fix parameters to bnx2_interrupt to be a struct bnx2_napi rather than a
> > struct net_device
> > 
> > 2) Fix poll_controller method to check every queue in the rx case so frames
> > aren't missed
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> This looks good, applied, thanks Neil!
> 
Thank you Dave!
Neil