: Fix VLAN unregistration

SKY2 In review Patrick McHardy 3 participants 9 messages May 27, 2007 – Jun 1, 2007

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Patrick McHardy

author

2007-05-27 18:44 UTC #0 lore raw

Fix sky2 disabling VLAN completely when the first vid is unregistered.

For some reason the VLAN code insists on the driver providing a vlan_rx_kill_vid function even if only NETIF_F_HW_VLAN_RX and not NETIF_F_HW_VLAN_FILTER is set, so this patch keeps an empty function. This seems to be a bug though, vlan_rx_add_vid is only required with NETIF_F_HW_VLAN_FILTER. Ben?

[SKY2]: Fix VLAN unregistration

sky2 disables VLAN completely when the first VID is unregistered. It should instead disable VLAN when the group is unregistered by calling sky2_vlan_rx_register with grp = NULL.

Signed-off-by Patrick McHardy <[email protected]>

---
commit 62d7f463e185a0b65915110d25d8c4c732e67c0a
tree dc6efd2d80b927bb45e0b33b33a7b5174d1bf05a
parent add81ec9b23f1fbe973093d5999a3d70e9d4c48b
author Patrick McHardy <[email protected]> Sun, 27 May 2007 20:39:09 +0200 committer Patrick McHardy <[email protected]> Sun, 27 May 2007 20:39:09 +0200

 drivers/net/sky2.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

drivers/net/sky2.c +12 ~~-13~~

index 832fd69..c973def 100644
@@ -1050,8 +1050,17 @@ static void sky2_vlan_rx_register(struct net_device *dev, struct vlan_group *grp
 
 	netif_tx_lock_bh(dev);
 
-	sky2_write32(hw, SK_REG(port, RX_GMF_CTRL_T), RX_VLAN_STRIP_ON);
-	sky2_write32(hw, SK_REG(port, TX_GMF_CTRL_T), TX_VLAN_TAG_ON);
+	if (grp) {
+		sky2_write32(hw, SK_REG(port, RX_GMF_CTRL_T),
+			     RX_VLAN_STRIP_ON);
+		sky2_write32(hw, SK_REG(port, TX_GMF_CTRL_T),
+			     TX_VLAN_TAG_ON);
+	} else {
+		sky2_write32(hw, SK_REG(port, RX_GMF_CTRL_T),
+			     RX_VLAN_STRIP_OFF);
+		sky2_write32(hw, SK_REG(port, TX_GMF_CTRL_T),
+			     TX_VLAN_TAG_OFF);
+	}
 	sky2->vlgrp = grp;
 
 	netif_tx_unlock_bh(dev);

@@ -1059,17 +1068,7 @@ static void sky2_vlan_rx_register(struct net_device *dev, struct vlan_group *grp
 
 static void sky2_vlan_rx_kill_vid(struct net_device *dev, unsigned short vid)
 {
-	struct sky2_port *sky2 = netdev_priv(dev);
-	struct sky2_hw *hw = sky2->hw;
-	u16 port = sky2->port;
-
-	netif_tx_lock_bh(dev);
-
-	sky2_write32(hw, SK_REG(port, RX_GMF_CTRL_T), RX_VLAN_STRIP_OFF);
-	sky2_write32(hw, SK_REG(port, TX_GMF_CTRL_T), TX_VLAN_TAG_OFF);
-	vlan_group_set_device(sky2->vlgrp, vid, NULL);
-
-	netif_tx_unlock_bh(dev);
+	return;
 }
 #endif

Ben Greear

2007-05-28 21:42 UTC #1 lore raw

Quoted 8 lines · Patrick McHardy

Fix sky2 disabling VLAN completely when the first vid is unregistered.
For some reason the VLAN code insists on the driver providing a vlan_rx_kill_vid function even if only NETIF_F_HW_VLAN_RX and not NETIF_F_HW_VLAN_FILTER is set, so this patch keeps an empty function. This seems to be a bug though, vlan_rx_add_vid is only required with NETIF_F_HW_VLAN_FILTER. Ben?

I believe DaveM did most of the vlan hw-accel work. It would be easy enough to check for a null function before calling the vlan_rx_kill_vid function and relax the checks based on the flags, but there may be valid reasons to keep this as is. Perhaps DaveM will comment...

Thanks,
Ben

Signature · 1 line

Ben Greear <[email protected]> Candela Technologies Inc  http://www.candelatech.com

Patrick McHardy

author

2007-05-29 10:57 UTC #2 lore raw

Quoted 11 lines · Ben Greear

Quoted 8 lines · Patrick McHardy
Fix sky2 disabling VLAN completely when the first vid is unregistered.
For some reason the VLAN code insists on the driver providing a vlan_rx_kill_vid function even if only NETIF_F_HW_VLAN_RX and not NETIF_F_HW_VLAN_FILTER is set, so this patch keeps an empty function. This seems to be a bug though, vlan_rx_add_vid is only required with NETIF_F_HW_VLAN_FILTER. Ben?

Quoted 5 lines

I believe DaveM did most of the vlan hw-accel work. It would be easy enough to check
for a null function before calling the vlan_rx_kill_vid function and relax the checks
based on the flags, but there may be valid reasons to keep this as is.

Most drivers are using vlan_rx_kill_vid to remove a vid from the group under their local local lock, which seems unnecessary since the VLAN code calls synchronize_net before freeing it and also does the removal itself. A few others have empty functions and 8139cp seems to have the same bug as sky2. In all cases it doesn't seem to be needed for drivers without NETIF_F_HW_VLAN_FILTER.

Stephen Hemminger

2007-05-29 17:24 UTC #3 lore raw

In reply to Patrick McHardy

Quoted 8 lines · On Sun, 27 May 2007 20:44:04 +0200 Patrick McHardy <[email protected]> wrote:

Fix sky2 disabling VLAN completely when the first vid is unregistered.
For some reason the VLAN code insists on the driver providing a vlan_rx_kill_vid function even if only NETIF_F_HW_VLAN_RX and not NETIF_F_HW_VLAN_FILTER is set, so this patch keeps an empty function. This seems to be a bug though, vlan_rx_add_vid is only required with NETIF_F_HW_VLAN_FILTER. Ben?

I put it in the next sky2 patch set. Still waiting for Jeff to take the last one.

Signature · 1 line

Stephen Hemminger <[email protected]>

Stephen Hemminger

2007-05-31 17:31 UTC #4 lore raw

PATCH 1/1 : sky2: Fix VLAN unregistration

In reply to Patrick McHardy

Quoted 8 lines · On Sun, 27 May 2007 20:44:04 +0200 Patrick McHardy <[email protected]> wrote:

Fix sky2 disabling VLAN completely when the first vid is unregistered.
For some reason the VLAN code insists on the driver providing a vlan_rx_kill_vid function even if only NETIF_F_HW_VLAN_RX and not NETIF_F_HW_VLAN_FILTER is set, so this patch keeps an empty function. This seems to be a bug though, vlan_rx_add_vid is only required with NETIF_F_HW_VLAN_FILTER. Ben?

Reading other drivers, it looks like a better patch would be to handle mode change in both set and kill.

Signed-off-by Stephen Hemminger <[email protected]>

 drivers/net/sky2.c |   32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

drivers/net/sky2.c +24 -8

@@ -1042,34 +1042,50 @@ static int sky2_ioctl(struct net_device 
 }
 
 #ifdef SKY2_VLAN_TAG_USED
-static void sky2_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
+static void sky2_set_vlan_mode(struct sky2_port *sky2)
 {
-	struct sky2_port *sky2 = netdev_priv(dev);
 	struct sky2_hw *hw = sky2->hw;
 	u16 port = sky2->port;
 
+	if (sky2->vlgrp) {
+		sky2_write32(hw, SK_REG(port, RX_GMF_CTRL_T),
+			     RX_VLAN_STRIP_ON);
+		sky2_write32(hw, SK_REG(port, TX_GMF_CTRL_T),
+			     TX_VLAN_TAG_ON);
+	} else {
+		sky2_write32(hw, SK_REG(port, RX_GMF_CTRL_T),
+			     RX_VLAN_STRIP_OFF);
+		sky2_write32(hw, SK_REG(port, TX_GMF_CTRL_T),
+			     TX_VLAN_TAG_OFF);
+	}
+}
+
+static void sky2_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
+{
+	struct sky2_port *sky2 = netdev_priv(dev);
+
 	netif_tx_lock_bh(dev);
+	netif_poll_disable(sky2->hw->dev[0]);
 
-	sky2_write32(hw, SK_REG(port, RX_GMF_CTRL_T), RX_VLAN_STRIP_ON);
-	sky2_write32(hw, SK_REG(port, TX_GMF_CTRL_T), TX_VLAN_TAG_ON);
 	sky2->vlgrp = grp;
+	sky2_set_vlan_mode(sky2);
 
 	netif_tx_unlock_bh(dev);
+	netif_poll_enable(sky2->hw->dev[0]);
 }
 
 static void sky2_vlan_rx_kill_vid(struct net_device *dev, unsigned short vid)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
-	struct sky2_hw *hw = sky2->hw;
-	u16 port = sky2->port;
 
 	netif_tx_lock_bh(dev);
+	netif_poll_disable(sky2->hw->dev[0]);
 
-	sky2_write32(hw, SK_REG(port, RX_GMF_CTRL_T), RX_VLAN_STRIP_OFF);
-	sky2_write32(hw, SK_REG(port, TX_GMF_CTRL_T), TX_VLAN_TAG_OFF);
 	vlan_group_set_device(sky2->vlgrp, vid, NULL);
+	sky2_set_vlan_mode(sky2);
 
 	netif_tx_unlock_bh(dev);
+	netif_poll_enable(sky2->hw->dev[0]);
 }
 #endif

Patrick McHardy

author

2007-05-31 17:38 UTC #5 lore raw

Re: [PATCH]: sky2: Fix VLAN unregistration

Quoted 14 lines · Stephen Hemminger

Quoted 8 lines · On Sun, 27 May 2007 20:44:04 +0200 Patrick McHardy <[email protected]> wrote:
Fix sky2 disabling VLAN completely when the first vid is unregistered.
For some reason the VLAN code insists on the driver providing a vlan_rx_kill_vid function even if only NETIF_F_HW_VLAN_RX and not NETIF_F_HW_VLAN_FILTER is set, so this patch keeps an empty function. This seems to be a bug though, vlan_rx_add_vid is only required with NETIF_F_HW_VLAN_FILTER. Ben?

Quoted 2 lines

Reading other drivers, it looks like a better patch would be to handle mode change in both set and kill.

The VLAN code tracks registered VIDs and will always call vlan_rx_register(NULL) when the last VID is unregistered, so this is not necessary.

Stephen Hemminger

2007-05-31 18:32 UTC #6 lore raw

Re: [PATCH]: sky2: Fix VLAN unregistration

Quoted 15 lines · On Thu, 31 May 2007 19:37:07 +0200 Patrick McHardy <[email protected]> wrote:

Quoted 14 lines · Stephen Hemminger
Quoted 8 lines · On Sun, 27 May 2007 20:44:04 +0200 Patrick McHardy <[email protected]> wrote:
Fix sky2 disabling VLAN completely when the first vid is unregistered.
For some reason the VLAN code insists on the driver providing a vlan_rx_kill_vid function even if only NETIF_F_HW_VLAN_RX and not NETIF_F_HW_VLAN_FILTER is set, so this patch keeps an empty function. This seems to be a bug though, vlan_rx_add_vid is only required with NETIF_F_HW_VLAN_FILTER. Ben?

Quoted 8 lines

Quoted 2 lines
Reading other drivers, it looks like a better patch would be to handle mode change in both set and kill.
The VLAN code tracks registered VIDs and will always call vlan_rx_register(NULL) when the last VID is unregistered, so this is not necessary.

Then should others be fixed...

	tg3, bnx2, r8169, gianfar, s2io, 8139cp, acenic, ...

Signature · 1 line

Stephen Hemminger <[email protected]>

Patrick McHardy

author

2007-06-01 15:17 UTC #7 lore raw

Re: [PATCH]: sky2: Fix VLAN unregistration

Quoted 25 lines · Stephen Hemminger

Quoted 21 lines · On Thu, 31 May 2007 19:37:07 +0200 Patrick McHardy <[email protected]> wrote:
Quoted 17 lines · Stephen Hemminger
Quoted 8 lines · On Sun, 27 May 2007 20:44:04 +0200 Patrick McHardy <[email protected]> wrote:
Fix sky2 disabling VLAN completely when the first vid is unregistered.
For some reason the VLAN code insists on the driver providing a vlan_rx_kill_vid function even if only NETIF_F_HW_VLAN_RX and not NETIF_F_HW_VLAN_FILTER is set, so this patch keeps an empty function. This seems to be a bug though, vlan_rx_add_vid is only required with NETIF_F_HW_VLAN_FILTER. Ben?
Reading other drivers, it looks like a better patch would be to handle mode change in both set and kill.

Quoted 8 lines

Quoted 4 lines
The VLAN code tracks registered VIDs and will always call vlan_rx_register(NULL) when the last VID is unregistered, so this is not necessary.
Then should others be fixed...
	tg3, bnx2, r8169, gianfar, s2io, 8139cp, acenic, ...

tg3 and r8169 both look fine, they only unregister a single VID within kill_vid and rely on VLAN to call rx_register(NULL) when the group is unregistered (haven't looked at the others).

As I wrote in my previous mails, they do have a part that looks unnecessary

        tg3_full_lock(tp, 0);
        vlan_group_set_device(tp->vlgrp, vid, NULL);

        tg3_full_unlock(tp);

since the vid is also removed by the VLAN code itself and locking doesn't seem to be necessary because the VLAN hwaccel rx helper deals with removal races itself.

Stephen Hemminger

2007-06-01 16:27 UTC #8 lore raw

Re: [PATCH]: sky2: Fix VLAN unregistration

I am working on a multi-driver cleanup patch right now...