From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-gx0-f211.google.com (mail-gx0-f211.google.com [209.85.217.211]) by bilbo.ozlabs.org (Postfix) with ESMTP id 93FA0B7B3E for ; Tue, 25 Aug 2009 09:32:21 +1000 (EST) Received: by gxk7 with SMTP id 7so3607978gxk.8 for ; Mon, 24 Aug 2009 16:32:18 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <200908241810.54239.to-fleischer@t-online.de> References: <200908241810.54239.to-fleischer@t-online.de> Date: Mon, 24 Aug 2009 18:32:18 -0500 Message-ID: <2acbd3e40908241632j7e9da3f7o7e11aa672838b9a5@mail.gmail.com> Subject: Re: gianfar.c: Unwanted VLAN tagging on TX frames From: Andy Fleming To: Torsten Fleischer Content-Type: multipart/alternative; boundary=000e0cd5c764f48ab90471eba05f Cc: linuxppc-dev@lists.ozlabs.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , --000e0cd5c764f48ab90471eba05f Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit On Mon, Aug 24, 2009 at 11:10 AM, Torsten Fleischer < to-fleischer@t-online.de> wrote: > Hello everyone, > > I have the Freescale's MPC8313erdb eval board and run the latest stable > linux > kernel version (linux-2.6.30.5). > > After creating a VLAN device (e.g. eth0.2) a VLAN tag is also inserted into > frames that don't relate to a VLAN device. This is the case for frames that > are directly sent through a standard ethernet interface (e.g. eth0). > > When creating a VLAN device the gianfar driver enables the hardware > supported > VLAN tagging on TX frames. This is done by setting the VLINS bit of the > TCTRL > register inside the function gianfar_vlan_rx_register(). > The problem is that every outgoing frame will be tagged. For frames from > an > interface like eth0 the VLN bit of the FCB isn't set. Therefore the eTSEC > uses > the content of the Default VLAN control word register (DFVLAN) to tag the > frame. As long as this register will not be modified after a reset of the > controller the outgoing frames will be tagged with VID = 0 (priority tagged > frames). > > The following patch solves this problem. > > diff -uprN linux-2.6.30.5_orig//drivers/net/gianfar.c > linux-2.6.30.5/drivers/net/gianfar.c > --- linux-2.6.30.5_orig//drivers/net/gianfar.c 2009-08-16 > 23:19:38.000000000 +0200 > +++ linux-2.6.30.5/drivers/net/gianfar.c 2009-08-22 > 10:38:28.000000000 +0200 > @@ -1309,6 +1309,7 @@ static int gfar_start_xmit(struct sk_buf > u32 bufaddr; > unsigned long flags; > unsigned int nr_frags, length; > + u32 tempval; > > base = priv->tx_bd_base; > > @@ -1385,13 +1386,30 @@ static int gfar_start_xmit(struct sk_buf > gfar_tx_checksum(skb, fcb); > } > > - if (priv->vlgrp && vlan_tx_tag_present(skb)) { > - if (unlikely(NULL == fcb)) { > - fcb = gfar_add_fcb(skb); > - lstatus |= BD_LFLAG(TXBD_TOE); > - } > + if (priv->vlgrp) { > + if (vlan_tx_tag_present(skb)) { > + if (unlikely(NULL == fcb)) { > + fcb = gfar_add_fcb(skb); > + lstatus |= BD_LFLAG(TXBD_TOE); > + } > + > + /* Enable VLAN tag insertion for frames from VLAN > devices */ > + tempval = gfar_read(&priv->regs->tctrl); > + if ( !(tempval & TCTRL_VLINS) ) { > + tempval |= TCTRL_VLINS; > + gfar_write(&priv->regs->tctrl, tempval); > + } > > - gfar_tx_vlan(skb, fcb); > + gfar_tx_vlan(skb, fcb); > + } > + else { > + /* Disable VLAN tag insertion for frames that are > not from a VLAN device */ > + tempval = gfar_read(&priv->regs->tctrl); > + if ( tempval & TCTRL_VLINS ) { > + tempval &= ~TCTRL_VLINS; > + gfar_write(&priv->regs->tctrl, tempval); > + } > + } > } Hmmm....how have you tested this? This looks like it has a bad race condition. The TCTRL register applies to all packets, which means if you send a packet with VLAN tags, followed by one without, or visa versa, there's a reasonable chance that the second packet's VLAN tags (or lack thereof) will take precedence. Without speaking for the company, I suspect that this is just how the eTSEC works with VLAN -- all, or nothing. Andy --000e0cd5c764f48ab90471eba05f Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

On Mon, Aug 24, 2009 at 11:10 AM, Torste= n Fleischer <to-fleischer@t-online.de> wrote:
Hello everyone,

I have the Freescale's MPC8313erdb eval board and run the latest stable= linux
kernel version (linux-2.6.30.5).

After creating a VLAN device (e.g. eth0.2) a VLAN tag is also inserted into=
frames that don't relate to a VLAN device. This is the case for frames = that
are directly sent through a standard ethernet interface (e.g. eth0).

When creating a VLAN device the gianfar driver enables the =A0hardware supp= orted
VLAN tagging on TX frames. This is done by setting the VLINS bit of the TCT= RL
register inside the function gianfar_vlan_rx_register().
The problem is that every outgoing frame will be tagged. =A0For frames from= an
interface like eth0 the VLN bit of the FCB isn't set. Therefore the eTS= EC uses
the content of the Default VLAN control word register (DFVLAN) to tag the frame. As long as this register will not be modified after a reset of the controller the outgoing frames will be tagged with VID =3D 0 (priority tagg= ed
frames).

The following patch solves this problem.

diff -uprN linux-2.6.30.5_orig//drivers/net/gianfar.c linux-2.6.30.5/driver= s/net/gianfar.c
--- linux-2.6.30.5_orig//drivers/net/gianfar.c =A02009-08-16 23:19:38.00000= 0000 +0200
+++ linux-2.6.30.5/drivers/net/gianfar.c =A0 =A0 =A0 =A02009-08-22 10:38:28= .000000000 +0200
@@ -1309,6 +1309,7 @@ static int gfar_start_xmit(struct sk_buf
=A0 =A0 =A0 =A0u32 bufaddr;
=A0 =A0 =A0 =A0unsigned long flags;
=A0 =A0 =A0 =A0unsigned int nr_frags, length;
+ =A0 =A0 =A0 u32 tempval;

=A0 =A0 =A0 =A0base =3D priv->tx_bd_base;

@@ -1385,13 +1386,30 @@ static int gfar_start_xmit(struct sk_buf
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gfar_tx_checksum(skb, fcb);
=A0 =A0 =A0 =A0}

- =A0 =A0 =A0 if (priv->vlgrp && vlan_tx_tag_present(skb)) {
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (unlikely(NULL =3D=3D fcb)) {
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fcb =3D gfar_add_fcb(skb); - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 lstatus |=3D BD_LFLAG(TXBD_TO= E);
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
+ =A0 =A0 =A0 if (priv->vlgrp) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (vlan_tx_tag_present(skb)) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (unlikely(NULL =3D=3D fcb)= ) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fcb =3D gfar_= add_fcb(skb);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 lstatus |=3D = BD_LFLAG(TXBD_TOE);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
+
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Enable VLAN tag insertion = for frames from VLAN devices */
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 tempval =3D gfar_read(&pr= iv->regs->tctrl);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ( !(tempval & TCTRL_VL= INS) ) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 tempval |=3D = TCTRL_VLINS;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gfar_write(&a= mp;priv->regs->tctrl, tempval);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 gfar_tx_vlan(skb, fcb);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gfar_tx_vlan(skb, fcb);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 else {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Disable VLAN tag insertion= for frames that are not from a VLAN device */
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 tempval =3D gfar_read(&pr= iv->regs->tctrl);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ( tempval & TCTRL_VLIN= S ) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 tempval &= =3D ~TCTRL_VLINS;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gfar_write(&a= mp;priv->regs->tctrl, tempval);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
=A0 =A0 =A0 =A0}


Hmmm....ho= w have you tested this? =A0This looks like it has a bad race condition. =A0= The TCTRL register applies to all packets, which means if you send a packet= with VLAN tags, followed by one without, or visa versa, there's a reas= onable chance that the second packet's VLAN tags (or lack thereof) will= take precedence.

Without speaking for the company, I suspect that this i= s just how the eTSEC works with VLAN -- all, or nothing.

Andy=A0

--000e0cd5c764f48ab90471eba05f--