| Message ID | 20081111140055.GA30481@hmsreliant.think-freely.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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 >
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
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
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(-)