| Message ID | 20190413012822.30931-11-olteanv@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | NXP SJA1105 DSA driver | expand |
On Sat, Apr 13, 2019 at 04:28:08AM +0300, Vladimir Oltean wrote: > When ports are standalone (after they left the bridge), they should have > no VLAN filtering semantics (they should pass all traffic to the CPU). > Currently this is not true for switchdev drivers, because the bridge > "forgets" to unset that. > > Normally one would think that doing this at the bridge layer would be a > better idea, i.e. call br_vlan_filter_toggle() from br_del_if(), similar > to how nbp_vlan_init() is called from br_add_if(). > > However what complicates that approach, and makes this one preferable, > is the fact that for the bridge core, vlan_filtering is a per-bridge > setting, whereas for switchdev/DSA it is per-port. Also there are > switches where the setting is per the entire device, and unsetting > vlan_filtering one by one, for each leaving port, would not be possible > from the bridge core without a certain level of awareness. So do this in > DSA and let drivers be unaware of it. > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 12/04/2019 18:28, Vladimir Oltean wrote: > When ports are standalone (after they left the bridge), they should have > no VLAN filtering semantics (they should pass all traffic to the CPU). The reset of the commit message is fine but this particular sentence sort of conflicts with the fact that we set NETIF_F_VLAN_RX_CTAG after my recent changes in order to support standalone ports + bridge ports w/ VLAN filtering on. What this feature flag means is that ingress VLAN filtering is active/supported. On that particular topic this also means that for things like tcpdump to keep working on standalone ports while we have bridged ports w/ VLAN filtering turned on, we might have to re-configure how the switch performs ingress VLAN filtering checking when we have a standalone port in promiscuous mode. On BCM53xx we can achieve that by having the CPU port be part of all VLANs (there is a shorthand register for that purpose) and doing ingress VID checking + copy violated frames to CPU port. Food for thought... > Currently this is not true for switchdev drivers, because the bridge > "forgets" to unset that. > > Normally one would think that doing this at the bridge layer would be a > better idea, i.e. call br_vlan_filter_toggle() from br_del_if(), similar > to how nbp_vlan_init() is called from br_add_if(). > > However what complicates that approach, and makes this one preferable, > is the fact that for the bridge core, vlan_filtering is a per-bridge > setting, whereas for switchdev/DSA it is per-port. Also there are > switches where the setting is per the entire device, and unsetting > vlan_filtering one by one, for each leaving port, would not be possible > from the bridge core without a certain level of awareness. So do this in > DSA and let drivers be unaware of it. > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff --git a/net/dsa/switch.c b/net/dsa/switch.c index 03b8d8928651..7d8cd9bc0ecc 100644 --- a/net/dsa/switch.c +++ b/net/dsa/switch.c @@ -72,6 +72,9 @@ static int dsa_switch_bridge_join(struct dsa_switch *ds, static int dsa_switch_bridge_leave(struct dsa_switch *ds, struct dsa_notifier_bridge_info *info) { + bool unset_vlan_filtering = br_vlan_enabled(info->br); + int err, i; + if (ds->index == info->sw_index && ds->ops->port_bridge_leave) ds->ops->port_bridge_leave(ds, info->port, info->br); @@ -79,6 +82,31 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds, ds->ops->crosschip_bridge_leave(ds, info->sw_index, info->port, info->br); + /* If the bridge was vlan_filtering, the bridge core doesn't trigger an + * event for changing vlan_filtering setting upon slave ports leaving + * it. That is a good thing, because that lets us handle it and also + * handle the case where the switch's vlan_filtering setting is global + * (not per port). When that happens, the correct moment to trigger the + * vlan_filtering callback is only when the last port left this bridge. + */ + if (unset_vlan_filtering && ds->vlan_filtering_is_global) { + for (i = 0; i < ds->num_ports; i++) { + if (i == info->port) + continue; + if (dsa_to_port(ds, i)->bridge_dev == info->br) { + unset_vlan_filtering = false; + break; + } + } + } + if (unset_vlan_filtering) { + struct switchdev_trans trans = {0}; + + err = dsa_port_vlan_filtering(&ds->ports[info->port], + false, &trans); + if (err && err != EOPNOTSUPP) + return err; + } return 0; }
When ports are standalone (after they left the bridge), they should have no VLAN filtering semantics (they should pass all traffic to the CPU). Currently this is not true for switchdev drivers, because the bridge "forgets" to unset that. Normally one would think that doing this at the bridge layer would be a better idea, i.e. call br_vlan_filter_toggle() from br_del_if(), similar to how nbp_vlan_init() is called from br_add_if(). However what complicates that approach, and makes this one preferable, is the fact that for the bridge core, vlan_filtering is a per-bridge setting, whereas for switchdev/DSA it is per-port. Also there are switches where the setting is per the entire device, and unsetting vlan_filtering one by one, for each leaving port, would not be possible from the bridge core without a certain level of awareness. So do this in DSA and let drivers be unaware of it. Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- Changes in v3: Patch is new. net/dsa/switch.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)