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

login
register
mail settings
Submitter Neil Horman
Date Nov. 11, 2008, 2 p.m.
Message ID <20081111140055.GA30481@hmsreliant.think-freely.org>
Download mbox | patch
Permalink /patch/8130/
State Superseded
Delegated to: David Miller
Headers show

Comments

Neil Horman - Nov. 11, 2008, 2 p.m.
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(-)
Andy Gospodarek - Nov. 11, 2008, 2:43 p.m.
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>


--
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
Michael Chan - Nov. 11, 2008, 3:12 p.m.
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

--
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 - Nov. 11, 2008, 4:46 p.m.
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
>
Michael Chan - Nov. 11, 2008, 5:09 p.m.
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.


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

Patch

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