diff mbox

gianfar: fix bug caused by 87c288c6e9aa31720b72e2bc2d665e24e1653c3e

Message ID 1311674593.17190.7.camel@DENEC1DT0191
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Sebastian =?ISO-8859-1?Q?P=F6hn?= July 26, 2011, 10:03 a.m. UTC 
commit 87c288c6e9aa31720b72e2bc2d665e24e1653c3e "gianfar: do vlan cleanup" has two issues:
# permutation of rx and tx flags
# enabling vlan tag insertion by default (this leads to unusable connections on some configurations)

If VLAN insertion is requested (via ethtool) it will be set at an other point ...

Signed-off-by: Sebastian Poehn <sebastian.poehn@belden.com>
---

 drivers/net/gianfar.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)



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

Comments

Jiri Pirko July 26, 2011, 10:46 a.m. UTC | #1 
Tue, Jul 26, 2011 at 12:03:13PM CEST, sebastian.belden@googlemail.com wrote:
>commit 87c288c6e9aa31720b72e2bc2d665e24e1653c3e "gianfar: do vlan cleanup" has two issues:
># permutation of rx and tx flags
># enabling vlan tag insertion by default (this leads to unusable connections on some configurations)

How so? What's causing that?

>
>If VLAN insertion is requested (via ethtool) it will be set at an other point ...
>
>Signed-off-by: Sebastian Poehn <sebastian.poehn@belden.com>
>---
>
> drivers/net/gianfar.c |    6 +-----
> 1 files changed, 1 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
>index 835cd25..2659daa 100644
>--- a/drivers/net/gianfar.c
>+++ b/drivers/net/gianfar.c
>@@ -388,12 +388,8 @@ static void gfar_init_mac(struct net_device *ndev)
> 	if (priv->hwts_rx_en)
> 		rctrl |= RCTRL_PRSDEP_INIT | RCTRL_TS_ENABLE;
> 
>-	/* keep vlan related bits if it's enabled */
>-	if (ndev->features & NETIF_F_HW_VLAN_TX)
>-		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
>-
> 	if (ndev->features & NETIF_F_HW_VLAN_RX)
>-		tctrl |= TCTRL_VLINS;
>+		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
> 
> 	/* Init rctrl based on our settings */
> 	gfar_write(&regs->rctrl, rctrl);
>
>

If you really need that to be done, you should remove NETIF_F_HW_VLAN_TX
from features (not hw_features) (never add it).

--
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
Sebastian =?ISO-8859-1?Q?P=F6hn?= July 26, 2011, 11:13 a.m. UTC | #2 
Am Dienstag, den 26.07.2011, 12:46 +0200 schrieb Jiri Pirko:
> Tue, Jul 26, 2011 at 12:03:13PM CEST, sebastian.belden@googlemail.com wrote:
> >commit 87c288c6e9aa31720b72e2bc2d665e24e1653c3e "gianfar: do vlan cleanup" has two issues:
> ># permutation of rx and tx flags
> ># enabling vlan tag insertion by default (this leads to unusable connections on some configurations)
> 
> How so? What's causing that?
If you enable the VLINS bit of txctrl and do not alter the vlan tag
configuration of the NIC, every packet will get a all zero vlan tag
(0x8100 0000). If you run a network without vlan awareness all packets
will be ignored.

In my configuration gianfar system <-> 3c59x system the 3com system
discards all packets received with the vlan tag.
> 
> >
> >If VLAN insertion is requested (via ethtool) it will be set at an other point ...
> >
> >Signed-off-by: Sebastian Poehn <sebastian.poehn@belden.com>
> >---
> >
> > drivers/net/gianfar.c |    6 +-----
> > 1 files changed, 1 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> >index 835cd25..2659daa 100644
> >--- a/drivers/net/gianfar.c
> >+++ b/drivers/net/gianfar.c
> >@@ -388,12 +388,8 @@ static void gfar_init_mac(struct net_device *ndev)
> > 	if (priv->hwts_rx_en)
> > 		rctrl |= RCTRL_PRSDEP_INIT | RCTRL_TS_ENABLE;
> > 
> >-	/* keep vlan related bits if it's enabled */
> >-	if (ndev->features & NETIF_F_HW_VLAN_TX)
> >-		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
> >-
> > 	if (ndev->features & NETIF_F_HW_VLAN_RX)
> >-		tctrl |= TCTRL_VLINS;
> >+		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
> > 
> > 	/* Init rctrl based on our settings */
> > 	gfar_write(&regs->rctrl, rctrl);
> >
> >
> 
> If you really need that to be done, you should remove NETIF_F_HW_VLAN_TX
> from features (not hw_features) (never add it).
> 
The only thing I did is to let the vlan insertion disabled by default.
If someone wants it, it may be enabled via ethtool.


--
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
Jiri Pirko July 26, 2011, 12:04 p.m. UTC | #3 
Tue, Jul 26, 2011 at 01:13:41PM CEST, sebastian.belden@googlemail.com wrote:
>Am Dienstag, den 26.07.2011, 12:46 +0200 schrieb Jiri Pirko:
>> Tue, Jul 26, 2011 at 12:03:13PM CEST, sebastian.belden@googlemail.com wrote:
>> >commit 87c288c6e9aa31720b72e2bc2d665e24e1653c3e "gianfar: do vlan cleanup" has two issues:
>> ># permutation of rx and tx flags
>> ># enabling vlan tag insertion by default (this leads to unusable connections on some configurations)
>> 
>> How so? What's causing that?
>If you enable the VLINS bit of txctrl and do not alter the vlan tag
>configuration of the NIC, every packet will get a all zero vlan tag
>(0x8100 0000). If you run a network without vlan awareness all packets
>will be ignored.


Interesting hw... I would guess that if gfar_tx_vlan() is not called, no
tag would be put there...

>
>In my configuration gianfar system <-> 3c59x system the 3com system
>discards all packets received with the vlan tag.
>> 
>> >
>> >If VLAN insertion is requested (via ethtool) it will be set at an other point ...
>> >
>> >Signed-off-by: Sebastian Poehn <sebastian.poehn@belden.com>
>> >---
>> >
>> > drivers/net/gianfar.c |    6 +-----
>> > 1 files changed, 1 insertions(+), 5 deletions(-)
>> >
>> >diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
>> >index 835cd25..2659daa 100644
>> >--- a/drivers/net/gianfar.c
>> >+++ b/drivers/net/gianfar.c
>> >@@ -388,12 +388,8 @@ static void gfar_init_mac(struct net_device *ndev)
>> > 	if (priv->hwts_rx_en)
>> > 		rctrl |= RCTRL_PRSDEP_INIT | RCTRL_TS_ENABLE;
>> > 
>> >-	/* keep vlan related bits if it's enabled */
>> >-	if (ndev->features & NETIF_F_HW_VLAN_TX)
>> >-		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
>> >-
>> > 	if (ndev->features & NETIF_F_HW_VLAN_RX)
>> >-		tctrl |= TCTRL_VLINS;
>> >+		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
>> > 
>> > 	/* Init rctrl based on our settings */
>> > 	gfar_write(&regs->rctrl, rctrl);
>> >
>> >
>> 
>> If you really need that to be done, you should remove NETIF_F_HW_VLAN_TX
>> from features (not hw_features) (never add it).
>> 
>The only thing I did is to let the vlan insertion disabled by default.
>If someone wants it, it may be enabled via ethtool.
>
>
--
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
Sebastian =?ISO-8859-1?Q?P=F6hn?= July 26, 2011, 12:21 p.m. UTC | #4 
Am Dienstag, den 26.07.2011, 14:04 +0200 schrieb Jiri Pirko:
> Tue, Jul 26, 2011 at 01:13:41PM CEST, sebastian.belden@googlemail.com wrote:
> >Am Dienstag, den 26.07.2011, 12:46 +0200 schrieb Jiri Pirko:
> >> Tue, Jul 26, 2011 at 12:03:13PM CEST, sebastian.belden@googlemail.com wrote:
> >> >commit 87c288c6e9aa31720b72e2bc2d665e24e1653c3e "gianfar: do vlan cleanup" has two issues:
> >> ># permutation of rx and tx flags
> >> ># enabling vlan tag insertion by default (this leads to unusable connections on some configurations)
> >> 
> >> How so? What's causing that?
> >If you enable the VLINS bit of txctrl and do not alter the vlan tag
> >configuration of the NIC, every packet will get a all zero vlan tag
> >(0x8100 0000). If you run a network without vlan awareness all packets
> >will be ignored.
> 
> 
> Interesting hw... I would guess that if gfar_tx_vlan() is not called, no
> tag would be put there...
The Freescale documentation says in detail:

if(FCB has valid VLAN field)
	send packet with this one
else
	send packet with default vlan tag

default vlan tag = dfvlan register = 0x8100 0000 by default
> 
> >
> >In my configuration gianfar system <-> 3c59x system the 3com system
> >discards all packets received with the vlan tag.
> >> 
> >> >
> >> >If VLAN insertion is requested (via ethtool) it will be set at an other point ...
> >> >
> >> >Signed-off-by: Sebastian Poehn <sebastian.poehn@belden.com>
> >> >---
> >> >
> >> > drivers/net/gianfar.c |    6 +-----
> >> > 1 files changed, 1 insertions(+), 5 deletions(-)
> >> >
> >> >diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> >> >index 835cd25..2659daa 100644
> >> >--- a/drivers/net/gianfar.c
> >> >+++ b/drivers/net/gianfar.c
> >> >@@ -388,12 +388,8 @@ static void gfar_init_mac(struct net_device *ndev)
> >> > 	if (priv->hwts_rx_en)
> >> > 		rctrl |= RCTRL_PRSDEP_INIT | RCTRL_TS_ENABLE;
> >> > 
> >> >-	/* keep vlan related bits if it's enabled */
> >> >-	if (ndev->features & NETIF_F_HW_VLAN_TX)
> >> >-		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
> >> >-
> >> > 	if (ndev->features & NETIF_F_HW_VLAN_RX)
> >> >-		tctrl |= TCTRL_VLINS;
> >> >+		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
> >> > 
> >> > 	/* Init rctrl based on our settings */
> >> > 	gfar_write(&regs->rctrl, rctrl);
> >> >
> >> >
> >> 
> >> If you really need that to be done, you should remove NETIF_F_HW_VLAN_TX
> >> from features (not hw_features) (never add it).
> >> 
> >The only thing I did is to let the vlan insertion disabled by default.
> >If someone wants it, it may be enabled via ethtool.
> >
> >


--
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
David Miller July 28, 2011, 5:43 a.m. UTC | #5 
From: "Sebastian Pöhn" <sebastian.belden@googlemail.com>
Date: Tue, 26 Jul 2011 12:03:13 +0200

> commit 87c288c6e9aa31720b72e2bc2d665e24e1653c3e "gianfar: do vlan cleanup" has two issues:
> # permutation of rx and tx flags
> # enabling vlan tag insertion by default (this leads to unusable connections on some configurations)
> 
> If VLAN insertion is requested (via ethtool) it will be set at an other point ...
> 
> Signed-off-by: Sebastian Poehn <sebastian.poehn@belden.com>

Applied.
--
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 mbox

Patch

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 835cd25..2659daa 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -388,12 +388,8 @@  static void gfar_init_mac(struct net_device *ndev)
 	if (priv->hwts_rx_en)
 		rctrl |= RCTRL_PRSDEP_INIT | RCTRL_TS_ENABLE;
 
-	/* keep vlan related bits if it's enabled */
-	if (ndev->features & NETIF_F_HW_VLAN_TX)
-		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
-
 	if (ndev->features & NETIF_F_HW_VLAN_RX)
-		tctrl |= TCTRL_VLINS;
+		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
 
 	/* Init rctrl based on our settings */
 	gfar_write(&regs->rctrl, rctrl);