[SKY2]: Fix VLAN unregistration

[SKY2]: Fix VLAN unregistration

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]

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?


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2001 bytes --]

[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 <kaber@trash.net>

---
commit 62d7f463e185a0b65915110d25d8c4c732e67c0a
tree dc6efd2d80b927bb45e0b33b33a7b5174d1bf05a
parent add81ec9b23f1fbe973093d5999a3d70e9d4c48b
author Patrick McHardy <kaber@trash.net> Sun, 27 May 2007 20:39:09 +0200
committer Patrick McHardy <kaber@trash.net> Sun, 27 May 2007 20:39:09 +0200

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

diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index 832fd69..c973def 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -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
 

Re: [SKY2]: Fix VLAN unregistration

[Ben Greear]

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

-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: [SKY2]: Fix VLAN unregistration

[Patrick McHardy]

Ben Greear wrote:
> Patrick McHardy 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 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.

Re: [SKY2]: Fix VLAN unregistration

[Stephen Hemminger]

On Sun, 27 May 2007 20:44:04 +0200
Patrick McHardy <kaber@trash.net> 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.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH]: sky2: Fix VLAN unregistration

[Stephen Hemminger]

On Sun, 27 May 2007 20:44:04 +0200
Patrick McHardy <kaber@trash.net> 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 <shemminger@linux-foundation.org>

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

--- a/drivers/net/sky2.c	2007-05-31 10:00:34.000000000 -0700
+++ b/drivers/net/sky2.c	2007-05-31 10:15:53.000000000 -0700
@@ -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
 

Re: [PATCH]: sky2: Fix VLAN unregistration

[Patrick McHardy]

Stephen Hemminger wrote:
> On Sun, 27 May 2007 20:44:04 +0200
> Patrick McHardy <kaber@trash.net> 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.


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.

Re: [PATCH]: sky2: Fix VLAN unregistration

[Stephen Hemminger]

On Thu, 31 May 2007 19:37:07 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Stephen Hemminger wrote:
> > On Sun, 27 May 2007 20:44:04 +0200
> > Patrick McHardy <kaber@trash.net> 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.
> 
> 
> 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, ...

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH]: sky2: Fix VLAN unregistration

[Patrick McHardy]

Stephen Hemminger wrote:
> On Thu, 31 May 2007 19:37:07 +0200
> Patrick McHardy <kaber@trash.net> wrote:
> 
> 
>>Stephen Hemminger wrote:
>>
>>>On Sun, 27 May 2007 20:44:04 +0200
>>>Patrick McHardy <kaber@trash.net> 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.
>>
>>
>>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.

Re: [PATCH]: sky2: Fix VLAN unregistration

[Stephen Hemminger]

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