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 <mmann...@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.
