Re: [RFC] vlan handling of up/down

[Ben Greear <greearb@candelatech.com>]

Stephen Hemminger wrote:
> Untested, but here is the basic idea of how I think up/down should be
> handled.  Basically, rather than changing the flags directly the VLAN code
> should call dev_open/dev_close.  The notifier end's up recursively calling
> but this is okay.
> 
> 
> --- vlan.orig/net/8021q/vlan.c
> +++ vlan/net/8021q/vlan.c
> @@ -427,7 +427,7 @@ static struct net_device *register_vlan_
>  	/* The real device must be up and operating in order to
>  	 * assosciate a VLAN device with it.
>  	 */
> -	if (!(real_dev->flags & IFF_UP))
> +	if (!netif_running(real_dev))
>  		goto out_unlock;

I can't remember why I thought this check was a good idea..but is there
any reason to keep it in?  I mean, why not allow attaching VLANs to a
'down' device?

>  	if (__find_vlan_dev(real_dev, VLAN_ID) != NULL) {
> @@ -476,10 +476,7 @@ static struct net_device *register_vlan_
>  	printk(VLAN_DBG "Allocated new name -:%s:-\n", new_dev->name);
>  #endif
>  	/* IFF_BROADCAST|IFF_MULTICAST; ??? */
> -	new_dev->flags = real_dev->flags;
> -	new_dev->flags &= ~IFF_UP;
> -
> -	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);
> +	new_dev->flags = real_dev->flags & ~IFF_UP;
>  
>  	/* need 4 bytes for extra VLAN header info,
>  	 * hope the underlying device can handle it.
> @@ -566,6 +563,9 @@ static struct net_device *register_vlan_
>  	if (real_dev->features & NETIF_F_HW_VLAN_FILTER)
>  		real_dev->vlan_rx_add_vid(real_dev, VLAN_ID);
>  
> +	/* Real device is up so bring up the vlan */
> +	dev_open(new_dev);
> +
>  	rtnl_unlock();

Assuming we take out the 'is-up' check above, this would need
a check added here (if real-dev is up, then bring up vlan, else leave it down.)

> @@ -624,11 +624,7 @@ static int vlan_device_event(struct noti
>  			if (!vlandev)
>  				continue;
>  
> -			flgs = vlandev->flags;
> -			if (!(flgs & IFF_UP))
> -				continue;
> -
> -			dev_change_flags(vlandev, flgs & ~IFF_UP);
> +			dev_close(vlandev);
>  		}
>  		break;
>  
> @@ -638,12 +634,8 @@ static int vlan_device_event(struct noti
>  			vlandev = grp->vlan_devices[i];
>  			if (!vlandev)
>  				continue;
> -				
> -			flgs = vlandev->flags;
> -			if (flgs & IFF_UP)
> -				continue;
>  
> -			dev_change_flags(vlandev, flgs | IFF_UP);
> +			dev_open(vlandev);
>  		}
>  		break;

Although it may be too late to change this (don't want to change
user-visible behaviour), I don't really like that bouncing the
real-dev could cause the VLANs to come up, even if previously
user-space had forced them down.

Perhaps a piece of state in the vlan dev could be added to remember
the 'desired state', and then only bring back up interfaces that are
desired-up when the real-dev comes back online....

Ben

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