Florian Fainelli
2018-May-24 17:20 UTC
[Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs
Hi Petr, On 05/24/2018 08:09 AM, Petr Machata wrote:> In commit 946a11e7408e ("mlxsw: spectrum_span: Allow bridge for gretap > mirror"), mlxsw got support for offloading mirror-to-gretap such that > the underlay packet path involves a bridge. In that case, the offload is > also influenced by PVID setting of said bridge. However, changes to VLAN > configuration of the bridge itself do not generate switchdev > notifications, so there's no mechanism to prod mlxsw to update the > offload when these settings change. > > In this patchset, the problem is resolved by distributing the switchdev > notification SWITCHDEV_OBJ_ID_PORT_VLAN also for configuration changes > on bridge VLANs. Since stacked devices distribute the notification to > lower devices, such event eventually reaches the driver, which can > determine whether it's a bridge or port VLAN by inspecting orig_dev. > > To keep things consistent, the newly-distributed notifications observe > the same protocol as the existing ones: dual prepare/commit, with > -EOPNOTSUPP indicating lack of support, even though there's currently > nothing to prepare for and nothing to support. Correspondingly, all > switchdev drivers have been updated to return -EOPNOTSUPP for bridge > VLAN notifications.You seem to have approached the bridge changes a little differently from this series: https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.html Both have the same intent that by targeting the bridge device itself, you can propagate that through switchdev to the switch drivers, and in turn create configurations where for instance, you have: - CPU/management port present in specific VLANs that is a subset or superset of the VLANs configured on front-panel ports - CPU/management port tagged/untagged in specific VLANs which can be a different setting from the front-panel ports One problem we have in DSA at the moment is that we always add the CPU port to the VLANs configured to the front-panel port but we do this with the same attributes as the front panel ports! For instance, if you add Port 0 to VLAN1 untagged, the the CPU port also gets added to that VLAN1, also untagged. As long as there is just one VLAN untagged, this is not much of a problem. Now do this with another VLAN or another port, and the CPU can no longer differentiate the traffic from which VLAN it is coming from, no bueno. I had specifically changed b53 to always add the CPU port as tagged, because that would always allow for differentiating traffic, but I would rather have the capability to configure that at the bridge layer, which you series seem to allow. For the record, here is what the first commit in the series intended to let an user do: The following happens now (assuming bridge master device is already created): bridge vlan add vid 2 dev port0 pvid untagged -> port0 (e.g: switch port 0) gets programmed -> CPU port gets programmed bridge vlan add vid 2 dev br0 self -> CPU port gets programmed bridge vlan add vid 2 dev port0 -> port0 (switch port 0) gets programmed Are these use cases possible with your series? It seems to me like it is if we drop the netif_is_bridge_master() checks and resolve orig_dev as being a hint for the CPU/management port. Thanks for reading me :)> > In patch #1, the code to send notifications for adding and deleting is > factored out into two named functions. > > In patches #2-#5, respectively for mlxsw, rocker, DSA and DPAA2 ethsw, > the new notifications (which are not enabled yet) are ignored to > maintain the current behavior. > > In patch #6, the notification is actually enabled. > > In patch #7, mlxsw is changed to update offloads of mirror-to-gre also > for bridge-related notifications. > > Petr Machata (7): > net: bridge: Extract boilerplate around switchdev_port_obj_*() > mlxsw: spectrum_switchdev: Ignore bridge VLAN events > rocker: rocker_main: Ignore bridge VLAN events > dsa: port: Ignore bridge VLAN events > staging: fsl-dpaa2: ethsw: Ignore bridge VLAN events > net: bridge: Notify about bridge VLANs > mlxsw: spectrum_switchdev: Schedule respin during trans prepare > > .../ethernet/mellanox/mlxsw/spectrum_switchdev.c | 8 ++- > drivers/net/ethernet/rocker/rocker_main.c | 6 +++ > drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 6 +++ > net/bridge/br_vlan.c | 58 ++++++++++++++-------- > net/dsa/port.c | 6 +++ > 5 files changed, 62 insertions(+), 22 deletions(-) >-- Florian
Florian Fainelli
2018-May-25 03:40 UTC
[Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs
On 05/24/2018 10:20 AM, Florian Fainelli wrote:> Hi Petr, > > On 05/24/2018 08:09 AM, Petr Machata wrote: >> In commit 946a11e7408e ("mlxsw: spectrum_span: Allow bridge for gretap >> mirror"), mlxsw got support for offloading mirror-to-gretap such that >> the underlay packet path involves a bridge. In that case, the offload is >> also influenced by PVID setting of said bridge. However, changes to VLAN >> configuration of the bridge itself do not generate switchdev >> notifications, so there's no mechanism to prod mlxsw to update the >> offload when these settings change. >> >> In this patchset, the problem is resolved by distributing the switchdev >> notification SWITCHDEV_OBJ_ID_PORT_VLAN also for configuration changes >> on bridge VLANs. Since stacked devices distribute the notification to >> lower devices, such event eventually reaches the driver, which can >> determine whether it's a bridge or port VLAN by inspecting orig_dev. >> >> To keep things consistent, the newly-distributed notifications observe >> the same protocol as the existing ones: dual prepare/commit, with >> -EOPNOTSUPP indicating lack of support, even though there's currently >> nothing to prepare for and nothing to support. Correspondingly, all >> switchdev drivers have been updated to return -EOPNOTSUPP for bridge >> VLAN notifications. > > You seem to have approached the bridge changes a little differently from > this series: > > https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.html > > Both have the same intent that by targeting the bridge device itself, > you can propagate that through switchdev to the switch drivers, and in > turn create configurations where for instance, you have: > > - CPU/management port present in specific VLANs that is a subset or > superset of the VLANs configured on front-panel ports > - CPU/management port tagged/untagged in specific VLANs which can be a > different setting from the front-panel ports > > One problem we have in DSA at the moment is that we always add the CPU > port to the VLANs configured to the front-panel port but we do this with > the same attributes as the front panel ports! For instance, if you add > Port 0 to VLAN1 untagged, the the CPU port also gets added to that > VLAN1, also untagged. As long as there is just one VLAN untagged, this > is not much of a problem. Now do this with another VLAN or another port, > and the CPU can no longer differentiate the traffic from which VLAN it > is coming from, no bueno. > > I had specifically changed b53 to always add the CPU port as tagged, > because that would always allow for differentiating traffic, but I would > rather have the capability to configure that at the bridge layer, which > you series seem to allow. > > For the record, here is what the first commit in the series intended to > let an user do: > > The following happens now (assuming bridge master device is already > created): > > bridge vlan add vid 2 dev port0 pvid untagged > -> port0 (e.g: switch port 0) gets programmed > -> CPU port gets programmed > bridge vlan add vid 2 dev br0 self > -> CPU port gets programmed > bridge vlan add vid 2 dev port0 > -> port0 (switch port 0) gets programmed > > Are these use cases possible with your series? It seems to me like it is > if we drop the netif_is_bridge_master() checks and resolve orig_dev as > being a hint for the CPU/management port.So I changed your code a little bit in net/dsa/port.c to verify what would happen and so far, this looks good except that I am seeing more programming events than I am expecting. The change I did is the following: diff --git a/net/dsa/port.c b/net/dsa/port.c index ed0595459df1..37385e491117 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -253,7 +253,7 @@ int dsa_port_vlan_add(struct dsa_port *dp, }; if (netif_is_bridge_master(vlan->obj.orig_dev)) - return -EOPNOTSUPP; + info.port = dp->cpu_dp->index; if (br_vlan_enabled(dp->bridge_dev)) return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info); @@ -271,7 +271,7 @@ int dsa_port_vlan_del(struct dsa_port *dp, }; if (netif_is_bridge_master(vlan->obj.orig_dev)) - return -EOPNOTSUPP; + info.port = dp->cpu_dp->index; if (br_vlan_enabled(dp->bridge_dev)) return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info); And the commands above result in the following: root at net-vm:~# bridge vlan add vid 2 dev lan1 pvid untagged [ 478.065728] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0 vlan: 2, untagged [ 478.066440] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5 vlan: 2, untagged [ 478.067890] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5 vlan: 2, tagged [ 478.068486] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5 vlan: 2, tagged root at net-vm:~# bridge vlan add vid 2 dev br0 self [ 507.931313] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5 vlan: 2, tagged [ 507.931826] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5 vlan: 2, tagged root at net-vm:~# bridge vlan add vid 2 dev lan1 [ 518.955814] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0 vlan: 2, tagged [ 518.956454] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5 vlan: 2, tagged root at net-vm:~# So commands #1 and #2 get too many events targeting the CPU port, if we remove the following hunk: diff --git a/net/dsa/switch.c b/net/dsa/switch.c index b93511726069..60ecc87bf6c0 100644 --- a/net/dsa/switch.c +++ b/net/dsa/switch.c @@ -212,7 +212,7 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds, if (ds->index == info->sw_index) set_bit(info->port, members); for (port = 0; port < ds->num_ports; port++) - if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)) + if (dsa_is_dsa_port(ds, port)) set_bit(port, members); if (switchdev_trans_ph_prepare(trans)) Then we get the expected number of events for the ports: root at net-vm:~# bridge vlan add vid 2 dev lan1 pvid untagged [ 111.906710] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0 vlan: 2, untagged, pvid: 1 [ 111.908988] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5 vlan: 2, tagged, pvid: 0 root at net-vm:~# bridge vlan add vid 2 dev br0 self [ 121.829272] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5 vlan: 2, tagged, pvid: 0 root at net-vm:~# bridge vlan add vid 2 dev lan1 [ 133.224113] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0 vlan: 2, tagged, pvid: 0 Andrew, Vivien, if the following hunks get applied are we possibly breaking mv88e6xxx? This is the use case that is really missing IMHO at the moment in DSA: we cannot control the VLAN membership and attributes of the CPU port(s), so either we make it always tagged in every VLAN (not great), or we introduce the ability to target the CPU port which is what Petr's patches + mine do. Thanks a second time for reading me :) -- Florian
Petr Machata
2018-May-25 10:09 UTC
[Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs
Florian Fainelli <f.fainelli at gmail.com> writes:> You seem to have approached the bridge changes a little differently from > this series: > > https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.htmlIt pretty much extends the patchset to also send the notifications for the CPU port. I missed this e-mail yesterday and now I see you already found out for yourself how it behaves.> Both have the same intent that by targeting the bridge device itself, > you can propagate that through switchdev to the switch drivers, and in > turn create configurations where for instance, you have: > > - CPU/management port present in specific VLANs that is a subset or > superset of the VLANs configured on front-panel ports > - CPU/management port tagged/untagged in specific VLANs which can be a > different setting from the front-panel ports > > One problem we have in DSA at the moment is that we always add the CPU > port to the VLANs configured to the front-panel port but we do this with > the same attributes as the front panel ports! For instance, if you add > Port 0 to VLAN1 untagged, the the CPU port also gets added to that > VLAN1, also untagged. As long as there is just one VLAN untagged, this > is not much of a problem. Now do this with another VLAN or another port, > and the CPU can no longer differentiate the traffic from which VLAN it > is coming from, no bueno.Yep, with this patchset you should be able to use the CPU port notifications to configure things exactly.> bridge vlan add vid 2 dev port0 pvid untagged > -> port0 (e.g: switch port 0) gets programmed > -> CPU port gets programmed > bridge vlan add vid 2 dev br0 self > -> CPU port gets programmed > bridge vlan add vid 2 dev port0 > -> port0 (switch port 0) gets programmed > > Are these use cases possible with your series? It seems to me like it is > if we drop the netif_is_bridge_master() checks and resolve orig_dev as > being a hint for the CPU/management port.Yeah, that's how it behaves. If you accept the events where netif_is_bridge_master(orig_dev), you can tell the CPU port-related events from the rest by BRIDGE_VLAN_INFO_BRENTRY. Thanks, Petr