[patch netdev-2.6 0/2] r8169: vlan hwaccel fixes

After taking a little time to implement vlan hwaccl features for the
r8169 driver, I discovered that someone had already beaten me to it! :-)

Anyway, at least the experience put me in an ideal position to review
the changes that had already been made.  I found a couple of places that
need some work.

Patch 1:

The return value of rtl8169_tx_vlan_tag() is not being
endian-swapped to little endian.  The hardware registers are little
endian, even though the vlan tag value in this register (16-bits only)
is big endian -- confusing!  Anyway, I'll be posting a follow-up patch
to correct this.

Patch 2:

The RxVlan bit in the CPlusCmd register is being turned-on in
rtl8169_vlan_rx_register() without regard to the value passed-in for
grp.  rtl8169_vlan_rx_register() is called w/ a non-NULL grp value when
the first vlan interface is created, then w/ a NULL grp value when the
last vlan interface is removed.

Similarly, the RxVlan bit in the CPlusCmd register is being turned-off
in rtl8169_vlan_rx_kill_vid() without regard to anything.  This function
is called when vlan interfaces are removed, but there may still be other
vlan interfaces still associated with the physical interface.

The net effect of the status quo is that vlan hwaccel is enabled after
the first vlan interfaces is created, UNTIL a vlan interface is removed.
After a single vlan interface is removed, no vlan hwaccel will occur (on
receive) until another vlan interface is created.  The second follow-up
patch I post will correct this by turning-on the RxVlan bit when
rtl8169_vlan_rx_register() is called w/ a non-NULL grp value and
turning-off the RxVlan bit when it is called w/ a NULL grp value.
Manipulation of the RxVlan bit will be removed from
rtl8169_vlan_rx_kill_vid().

Patches to follow...

Thanks,

John
-- 
John W. Linville
linville@tuxdriver.com

Endian-swap return of rtl8169_tx_vlan_tag() in rtl8169_start_xmit()

Signed-off-by: John W. Linville <redacted>
---

 drivers/net/r8169.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

--- netdev-2.6/drivers/net/r8169.c.netdev	2004-10-21 14:48:38.753676111 -0400
+++ netdev-2.6/drivers/net/r8169.c	2004-10-21 14:49:04.874988962 -0400

@@ -1906,7 +1906,7 @@ static int rtl8169_start_xmit(struct sk_
 
 	tp->tx_skb[entry].len = len;
 	txd->addr = cpu_to_le64(mapping);
-	txd->opts2 = rtl8169_tx_vlan_tag(tp, skb);
+	txd->opts2 = cpu_to_le32(rtl8169_tx_vlan_tag(tp, skb));
 
 	wmb();
 

Fix manipulation of RxVlan bit in rtl8169_vlan_rx_register(), and
remove it from rtl8169_vlan_rx_kill_vid().

Signed-off-by: John W. Linville <redacted>
---

 drivers/net/r8169.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

--- netdev-2.6/drivers/net/r8169.c.netdev	2004-10-21 14:38:16.000000000 -0400
+++ netdev-2.6/drivers/net/r8169.c	2004-10-21 14:44:22.968783027 -0400

@@ -705,8 +705,10 @@ static void rtl8169_vlan_rx_register(str
 	unsigned long flags;
 
 	spin_lock_irqsave(&tp->lock, flags);
-	tp->vlgrp = grp;
-	tp->cp_cmd |= RxVlan;
+	if ((tp->vlgrp = grp))
+		tp->cp_cmd |= RxVlan;
+	else
+		tp->cp_cmd &= ~RxVlan;
 	RTL_W16(CPlusCmd, tp->cp_cmd);
 	RTL_R16(CPlusCmd);
 	spin_unlock_irqrestore(&tp->lock, flags);

@@ -715,13 +717,9 @@ static void rtl8169_vlan_rx_register(str
 static void rtl8169_vlan_rx_kill_vid(struct net_device *dev, unsigned short vid)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
-	void __iomem *ioaddr = tp->mmio_addr;
 	unsigned long flags;
 
 	spin_lock_irqsave(&tp->lock, flags);
-	tp->cp_cmd &= ~RxVlan;
-	RTL_W16(CPlusCmd, tp->cp_cmd);
-	RTL_R16(CPlusCmd);
 	if (tp->vlgrp)
 		tp->vlgrp->vlan_devices[vid] = NULL;
 	spin_unlock_irqrestore(&tp->lock, flags);

John W. Linville [off-list ref] :
[...]

Patch 1:

The return value of rtl8169_tx_vlan_tag() is not being
endian-swapped to little endian.  The hardware registers are little
endian, even though the vlan tag value in this register (16-bits only)
is big endian -- confusing!  Anyway, I'll be posting a follow-up patch
to correct this.

Oops.

Patch 2:

[nice explanation]

Any objection against me replacing the actual comment of patch #2 (i.e.
"why" instead of "how") and splitting the "if ((tp->>vlgrp = grp))" over
two lines ?

--
Ueimor

Simplify tricky if() expression in rtl8169_vlan_rx_register().

Signed-off-by: John W. Linville <redacted>
---
You're probably right -- the "if ((tp->vlgrp = grp))" line is probably
a little TOO clever... :-)

 drivers/net/r8169.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

--- ./drivers/net/r8169.c.orig	2004-10-22 21:44:06.050154952 -0400
+++ ./drivers/net/r8169.c	2004-10-22 21:44:26.228087440 -0400

@@ -703,7 +703,8 @@ static void rtl8169_vlan_rx_register(str
 	unsigned long flags;
 
 	spin_lock_irqsave(&tp->lock, flags);
-	if ((tp->vlgrp = grp))
+	tp->vlgrp = grp;
+	if (tp->vlgrp)
 		tp->cp_cmd |= RxVlan;
 	else
 		tp->cp_cmd &= ~RxVlan;

On Fri, Oct 22, 2004 at 10:28:51PM +0200, Francois Romieu wrote:

John W. Linville [off-list ref] :

quoted

Patch 2:

[nice explanation]

Any objection against me replacing the actual comment of patch #2 (i.e.
"why" instead of "how") and splitting the "if ((tp->>vlgrp = grp))" over
two lines ?

Not quite sure which comment you mean, but I'm sure that's fine.
I posted a third patch to fix-up that tricky if() -- you're right,
it is a little TOO clever... :-)

John
-- 
John W. Linville
linville@tuxdriver.com