[net-next,4/4] bridge: update vlan dev state when port added to or deleted from vlan

If vlan bridge binding is enabled, then the link state of a vlan device
that is an upper device of the bridge should track the state of bridge
ports that are members of that vlan. So if a bridge port becomes or
stops being a member of a vlan, then update the link state of the
vlan device if necessary.

Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>
---
 net/bridge/br_vlan.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

On 02/04/2019 18:35, Mike Manning wrote:
> If vlan bridge binding is enabled, then the link state of a vlan device
> that is an upper device of the bridge should track the state of bridge
> ports that are members of that vlan. So if a bridge port becomes or
> stops being a member of a vlan, then update the link state of the
> vlan device if necessary.
> 
> Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>
> ---
>  net/bridge/br_vlan.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 642373231386..7c11607cf1f4 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -7,6 +7,9 @@
>  #include "br_private.h"
>  #include "br_private_tunnel.h"
>  
> +static void nbp_vlan_set_vlan_dev_state(struct net_bridge_port *p,
> +					struct net_bridge *br, u16 vid);
> +
>  static inline int br_vlan_cmp(struct rhashtable_compare_arg *arg,
>  			      const void *ptr)
>  {
> @@ -294,6 +297,9 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
>  
>  	__vlan_add_list(v);
>  	__vlan_add_flags(v, flags);
> +
> +	if (p)
> +		nbp_vlan_set_vlan_dev_state(p, br, v->vid);

since you need this as a last action after vlan_add and acts only on ports
why not move it to nbp_vlan_add() ?

>  out:
>  	return err;
>  
> @@ -358,6 +364,8 @@ static int __vlan_del(struct net_bridge_vlan *v)
>  		rhashtable_remove_fast(&vg->vlan_hash, &v->vnode,
>  				       br_vlan_rht_params);
>  		__vlan_del_list(v);
> +		if (p)
> +			nbp_vlan_set_vlan_dev_state(p, p->br, v->vid);

p is guaranteed to be set here, but also there's an if (p) earlier in the function.
this can probably also move to nbp_vlan_delete if the other set_state moves to 
nbp_vlan_add

>  		call_rcu(&v->rcu, nbp_vlan_rcu_free);
>  	}
>  
> @@ -1357,6 +1365,21 @@ static void br_vlan_set_vlan_dev_state(struct net_bridge *br,
>  	}
>  }
>  
> +static void nbp_vlan_set_vlan_dev_state(struct net_bridge_port *p,
> +					struct net_bridge *br, u16 vid)
> +{
> +	struct net_device *vlan_dev;
> +
> +	if (!br->vlan_bridge_binding)
> +		return;
> +
> +	vlan_dev = br_vlan_get_upper_bind_vlan_dev(br->dev, vid);
> +	if (vlan_dev) {
> +		br_vlan_set_vlan_dev_state(br, vlan_dev);
> +		dev_put(vlan_dev);

I think this is running under rtnl.

> +	}
> +}
> +
>  static void br_vlan_set_all_vlan_dev_state(struct net_bridge_port *p,
>  					   struct net_bridge *br)
>  {
>

On 02/04/2019 21:10, Nikolay Aleksandrov wrote:
> On 02/04/2019 18:35, Mike Manning wrote:
>> If vlan bridge binding is enabled, then the link state of a vlan device
>> that is an upper device of the bridge should track the state of bridge
>> ports that are members of that vlan. So if a bridge port becomes or
>> stops being a member of a vlan, then update the link state of the
>> vlan device if necessary.
>>
>> Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>
>> ---
>>  net/bridge/br_vlan.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 642373231386..7c11607cf1f4 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -7,6 +7,9 @@
>>  #include "br_private.h"
>>  #include "br_private_tunnel.h"
>>  
>> +static void nbp_vlan_set_vlan_dev_state(struct net_bridge_port *p,
>> +					struct net_bridge *br, u16 vid);
>> +
>>  static inline int br_vlan_cmp(struct rhashtable_compare_arg *arg,
>>  			      const void *ptr)
>>  {
>> @@ -294,6 +297,9 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
>>  
>>  	__vlan_add_list(v);
>>  	__vlan_add_flags(v, flags);
>> +
>> +	if (p)
>> +		nbp_vlan_set_vlan_dev_state(p, br, v->vid);
> since you need this as a last action after vlan_add and acts only on ports
> why not move it to nbp_vlan_add() ?
The call is in __vlan_add() rather than nbp_vlan_add() only for reasons
of consistency with the delete case, please see below.
>
>>  out:
>>  	return err;
>>  
>> @@ -358,6 +364,8 @@ static int __vlan_del(struct net_bridge_vlan *v)
>>  		rhashtable_remove_fast(&vg->vlan_hash, &v->vnode,
>>  				       br_vlan_rht_params);
>>  		__vlan_del_list(v);
>> +		if (p)
>> +			nbp_vlan_set_vlan_dev_state(p, p->br, v->vid);
> p is guaranteed to be set here, but also there's an if (p) earlier in the function.
> this can probably also move to nbp_vlan_delete if the other set_state moves to 
> nbp_vlan_add
>
Agreed re p being set here, I was just safeguarding against future
changes where this might not be true, but I will get rid of this.

The call is made here so as to be after the removal of the node from the
rhashtable.

The check is also needed as a result of calling nbp_vlan_flush(), hence
making the call here rather than in nbp_vlan_delete().

>>  		call_rcu(&v->rcu, nbp_vlan_rcu_free);
>>  	}
>>  
>> @@ -1357,6 +1365,21 @@ static void br_vlan_set_vlan_dev_state(struct net_bridge *br,
>>  	}
>>  }
>>  
>> +static void nbp_vlan_set_vlan_dev_state(struct net_bridge_port *p,
>> +					struct net_bridge *br, u16 vid)
>> +{
>> +	struct net_device *vlan_dev;
>> +
>> +	if (!br->vlan_bridge_binding)
>> +		return;
>> +
>> +	vlan_dev = br_vlan_get_upper_bind_vlan_dev(br->dev, vid);
>> +	if (vlan_dev) {
>> +		br_vlan_set_vlan_dev_state(br, vlan_dev);
>> +		dev_put(vlan_dev);
> I think this is running under rtnl.
Agreed, I will get rid of this and the other instance of this use.