Patchwork [net-next,1/2] tg3: Fix NETIF_F_LOOPBACK error

login
register
mail settings
Submitter Matt Carlson
Date May 20, 2011, 1:11 a.m.
Message ID <1305853864-2135-2-git-send-email-mcarlson@broadcom.com>
Download mbox | patch
Permalink /patch/96515/
State Superseded
Delegated to: David Miller
Headers show

Comments

Matt Carlson - May 20, 2011, 1:11 a.m.
Mahesh Bandewar noticed that the features cleanup in commit
0da0606f493c5cdab74bdcc96b12f4305ad94085, entitled
"tg3: Consolidate all netdev feature assignments", mistakenly sets
NETIF_F_LOOPBACK by default.  This patch corrects the error.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
---
 drivers/net/tg3.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Mahesh Bandewar - May 20, 2011, 1:15 a.m.
On Thu, May 19, 2011 at 6:11 PM, Matt Carlson <mcarlson@broadcom.com> wrote:
> Mahesh Bandewar noticed that the features cleanup in commit
> 0da0606f493c5cdab74bdcc96b12f4305ad94085, entitled
> "tg3: Consolidate all netdev feature assignments", mistakenly sets
> NETIF_F_LOOPBACK by default.  This patch corrects the error.
>
> Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> ---
>  drivers/net/tg3.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 012ce70..0b78c5d 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -15080,6 +15080,8 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
>                        features |= NETIF_F_TSO_ECN;
>        }
>
> +       dev->features |= features;
> +
>        /*
>         * Add loopback capability only for a subset of devices that support
>         * MAC-LOOPBACK. Eventually this need to be enhanced to allow INT-PHY
> @@ -15090,7 +15092,6 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
>                /* Add the loopback capability */
>                features |= NETIF_F_LOOPBACK;
>
> -       dev->features |= features;
>        dev->hw_features |= features;
>        dev->vlan_features |= features;
I think this line should go up too. Otherwise newly created vlan
device(s) will have spurious loopback bit set.
>
> --
> 1.7.3.4
>
>
> --
> 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
>
--
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
Matt Carlson - May 20, 2011, 1:59 a.m.
On Thu, May 19, 2011 at 06:15:18PM -0700, Mahesh Bandewar wrote:
> On Thu, May 19, 2011 at 6:11 PM, Matt Carlson <mcarlson@broadcom.com> wrote:
> > Mahesh Bandewar noticed that the features cleanup in commit
> > 0da0606f493c5cdab74bdcc96b12f4305ad94085, entitled
> > "tg3: Consolidate all netdev feature assignments", mistakenly sets
> > NETIF_F_LOOPBACK by default. ?This patch corrects the error.
> >
> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > ---
> > ?drivers/net/tg3.c | ? ?3 ++-
> > ?1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> > index 012ce70..0b78c5d 100644
> > --- a/drivers/net/tg3.c
> > +++ b/drivers/net/tg3.c
> > @@ -15080,6 +15080,8 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
> > ? ? ? ? ? ? ? ? ? ? ? ?features |= NETIF_F_TSO_ECN;
> > ? ? ? ?}
> >
> > + ? ? ? dev->features |= features;
> > +
> > ? ? ? ?/*
> > ? ? ? ? * Add loopback capability only for a subset of devices that support
> > ? ? ? ? * MAC-LOOPBACK. Eventually this need to be enhanced to allow INT-PHY
> > @@ -15090,7 +15092,6 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
> > ? ? ? ? ? ? ? ?/* Add the loopback capability */
> > ? ? ? ? ? ? ? ?features |= NETIF_F_LOOPBACK;
> >
> > - ? ? ? dev->features |= features;
> > ? ? ? ?dev->hw_features |= features;
> > ? ? ? ?dev->vlan_features |= features;
> I think this line should go up too. Otherwise newly created vlan
> device(s) will have spurious loopback bit set.

Yes.  You are right.  I thought vlan_features functioned like
hw_features.

--
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
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= - May 20, 2011, 6:57 a.m.
2011/5/20 Matt Carlson <mcarlson@broadcom.com>:
> On Thu, May 19, 2011 at 06:15:18PM -0700, Mahesh Bandewar wrote:
>> On Thu, May 19, 2011 at 6:11 PM, Matt Carlson <mcarlson@broadcom.com> wrote:
>> > Mahesh Bandewar noticed that the features cleanup in commit
>> > 0da0606f493c5cdab74bdcc96b12f4305ad94085, entitled
>> > "tg3: Consolidate all netdev feature assignments", mistakenly sets
>> > NETIF_F_LOOPBACK by default. ?This patch corrects the error.
>> >
>> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
>> > ---
>> > ?drivers/net/tg3.c | ? ?3 ++-
>> > ?1 files changed, 2 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
>> > index 012ce70..0b78c5d 100644
>> > --- a/drivers/net/tg3.c
>> > +++ b/drivers/net/tg3.c
>> > @@ -15080,6 +15080,8 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
>> > ? ? ? ? ? ? ? ? ? ? ? ?features |= NETIF_F_TSO_ECN;
>> > ? ? ? ?}
>> >
>> > + ? ? ? dev->features |= features;
>> > +
>> > ? ? ? ?/*
>> > ? ? ? ? * Add loopback capability only for a subset of devices that support
>> > ? ? ? ? * MAC-LOOPBACK. Eventually this need to be enhanced to allow INT-PHY
>> > @@ -15090,7 +15092,6 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
>> > ? ? ? ? ? ? ? ?/* Add the loopback capability */
>> > ? ? ? ? ? ? ? ?features |= NETIF_F_LOOPBACK;
>> >
>> > - ? ? ? dev->features |= features;
>> > ? ? ? ?dev->hw_features |= features;
>> > ? ? ? ?dev->vlan_features |= features;
>> I think this line should go up too. Otherwise newly created vlan
>> device(s) will have spurious loopback bit set.
> Yes.  You are right.  I thought vlan_features functioned like
> hw_features.

Probably NETIF_F_LOOPBACK should be forcibly set on VLAN devices when
underlying device has it enabled. Just a quick thought for discussion.

Best Regards,
Michał Mirosław
--
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
Mahesh Bandewar - May 20, 2011, 5:56 p.m.
On Thu, May 19, 2011 at 11:57 PM, Michał Mirosław <mirqus@gmail.com> wrote:
> 2011/5/20 Matt Carlson <mcarlson@broadcom.com>:
>> On Thu, May 19, 2011 at 06:15:18PM -0700, Mahesh Bandewar wrote:
>>> On Thu, May 19, 2011 at 6:11 PM, Matt Carlson <mcarlson@broadcom.com> wrote:
>>> > Mahesh Bandewar noticed that the features cleanup in commit
>>> > 0da0606f493c5cdab74bdcc96b12f4305ad94085, entitled
>>> > "tg3: Consolidate all netdev feature assignments", mistakenly sets
>>> > NETIF_F_LOOPBACK by default. ?This patch corrects the error.
>>> >
>>> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
>>> > ---
>>> > ?drivers/net/tg3.c | ? ?3 ++-
>>> > ?1 files changed, 2 insertions(+), 1 deletions(-)
>>> >
>>> > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
>>> > index 012ce70..0b78c5d 100644
>>> > --- a/drivers/net/tg3.c
>>> > +++ b/drivers/net/tg3.c
>>> > @@ -15080,6 +15080,8 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
>>> > ? ? ? ? ? ? ? ? ? ? ? ?features |= NETIF_F_TSO_ECN;
>>> > ? ? ? ?}
>>> >
>>> > + ? ? ? dev->features |= features;
>>> > +
>>> > ? ? ? ?/*
>>> > ? ? ? ? * Add loopback capability only for a subset of devices that support
>>> > ? ? ? ? * MAC-LOOPBACK. Eventually this need to be enhanced to allow INT-PHY
>>> > @@ -15090,7 +15092,6 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
>>> > ? ? ? ? ? ? ? ?/* Add the loopback capability */
>>> > ? ? ? ? ? ? ? ?features |= NETIF_F_LOOPBACK;
>>> >
>>> > - ? ? ? dev->features |= features;
>>> > ? ? ? ?dev->hw_features |= features;
>>> > ? ? ? ?dev->vlan_features |= features;
>>> I think this line should go up too. Otherwise newly created vlan
>>> device(s) will have spurious loopback bit set.
>> Yes.  You are right.  I thought vlan_features functioned like
>> hw_features.
>
> Probably NETIF_F_LOOPBACK should be forcibly set on VLAN devices when
> underlying device has it enabled. Just a quick thought for discussion.
>
I think that's a good idea for the sake of correctness and should be
done when we enable / disable loopback.

--mahesh..

> Best Regards,
> Michał Mirosław
>
--
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/tg3.c b/drivers/net/tg3.c
index 012ce70..0b78c5d 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -15080,6 +15080,8 @@  static int __devinit tg3_init_one(struct pci_dev *pdev,
 			features |= NETIF_F_TSO_ECN;
 	}
 
+	dev->features |= features;
+
 	/*
 	 * Add loopback capability only for a subset of devices that support
 	 * MAC-LOOPBACK. Eventually this need to be enhanced to allow INT-PHY
@@ -15090,7 +15092,6 @@  static int __devinit tg3_init_one(struct pci_dev *pdev,
 		/* Add the loopback capability */
 		features |= NETIF_F_LOOPBACK;
 
-	dev->features |= features;
 	dev->hw_features |= features;
 	dev->vlan_features |= features;