On Tue, Mar 08, 2022 at 19:17, Vladimir Oltean [off-list ref] wrote:

On Tue, Mar 08, 2022 at 09:01:04AM +0100, Tobias Waldekranz wrote:

quoted

On Thu, Mar 03, 2022 at 22:59, Vladimir Oltean [off-list ref] wrote:

quoted

On Tue, Mar 01, 2022 at 11:03:15AM +0100, Tobias Waldekranz wrote:

quoted

Whenever a VLAN moves to a new MSTI, send a switchdev notification so
that switchdevs can...

...either refuse the migration if the hardware does not support
offloading of MST...

..or track a bridge's VID to MSTI mapping when offloading is
supported.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 include/net/switchdev.h   | 10 +++++++
 net/bridge/br_mst.c       | 15 +++++++++++
 net/bridge/br_switchdev.c | 57 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 3e424d40fae3..39e57aa5005a 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h

@@ -28,6 +28,7 @@ enum switchdev_attr_id {
 	SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
 	SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
 	SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
+	SWITCHDEV_ATTR_ID_VLAN_MSTI,
 };
 
 struct switchdev_brport_flags {

@@ -35,6 +36,14 @@ struct switchdev_brport_flags {
 	unsigned long mask;
 };
 
+struct switchdev_vlan_attr {
+	u16 vid;
+
+	union {
+		u16 msti;
+	};

Do you see other VLAN attributes that would be added in the future, such
as to justify making this a single-element union from the get-go?

I could imagine being able to control things like multicast snooping on
a per-VLAN basis. Being able to act as a multicast router in one VLAN
but not another.

quoted

Anyway if that is the case, we're lacking an id for the attribute type,
so we'd end up needing to change drivers when a second union element
appears. Otherwise they'd all expect an u16 msti.

My idea was that `enum switchdev_attr_id` would hold all of that
information. In this example SWITCHDEV_ATTR_ID_VLAN_MSTI, denotes both
that `vlan_attr` is the valid member of `u` and that `msti` is the valid
member of `vlan_attr`. If we add SWITCHDEV_ATTR_ID_VLAN_SNOOPING, that
would point to both `vlan_attr` and a new `bool snooping` in the union.

Do you think we should just have a SWITCHDEV_ATTR_ID_VLAN_ATTR for all
per-VLAN attributes and then have a separate union?

It's the first nested union that I see, and a bit confusing.

I think it would be better if we had a

struct switchdev_vlan_attr_msti {
	u16 vid;
	u16 msti;
};

and different structures for other, future VLAN attributes. Basically
keep a 1:1 mapping between an attribute id and a union.

Yeah, I like the simplicity of that. Changing.

quoted

quoted

quoted

+};
+
 struct switchdev_attr {
 	struct net_device *orig_dev;
 	enum switchdev_attr_id id;

@@ -50,6 +59,7 @@ struct switchdev_attr {
 		u16 vlan_protocol;			/* BRIDGE_VLAN_PROTOCOL */
 		bool mc_disabled;			/* MC_DISABLED */
 		u8 mrp_port_role;			/* MRP_PORT_ROLE */
+		struct switchdev_vlan_attr vlan_attr;	/* VLAN_* */
 	} u;
 };
 

diff --git a/net/bridge/br_mst.c b/net/bridge/br_mst.c
index 8dea8e7257fd..aba603675165 100644
--- a/net/bridge/br_mst.c
+++ b/net/bridge/br_mst.c

@@ -7,6 +7,7 @@
  */
 
 #include <linux/kernel.h>
+#include <net/switchdev.h>
 
 #include "br_private.h"
 

@@ -65,9 +66,23 @@ static void br_mst_vlan_sync_state(struct net_bridge_vlan *pv, u16 msti)
 
 int br_mst_vlan_set_msti(struct net_bridge_vlan *mv, u16 msti)
 {
+	struct switchdev_attr attr = {
+		.id = SWITCHDEV_ATTR_ID_VLAN_MSTI,
+		.flags = SWITCHDEV_F_DEFER,

Is the bridge spinlock held (atomic context), or otherwise why is
SWITCHDEV_F_DEFER needed here?

Nope, just copypasta. In fact, it shouldn't be needed when setting the
state either, as you can only change the state via a netlink message. I
will remove it.

quoted

quoted

+		.orig_dev = mv->br->dev,
+		.u.vlan_attr = {
+			.vid = mv->vid,
+			.msti = msti,
+		},
+	};
 	struct net_bridge_vlan_group *vg;
 	struct net_bridge_vlan *pv;
 	struct net_bridge_port *p;
+	int err;
+
+	err = switchdev_port_attr_set(mv->br->dev, &attr, NULL);

Treating a "VLAN attribute" as a "port attribute of the bridge" is
pushing the taxonomy just a little, but I don't have a better suggestion.

Isn't there prior art here? I thought things like VLAN filtering already
worked like this?

Hmm, I can think of VLAN filtering as being an attribute of the bridge
device, but 'which MSTI does VLAN X belong to' is an attribute of the
VLAN (in itself a switchdev object, i.e. something countable).

If the prior art would apply as straightforward as you say, then we'd be
replaying the VLAN MSTIs together with the other port attributes - in
"pull" mode, in dsa_port_switchdev_sync_attrs(), rather than in "push"
mode with the rest of the objects - in nbp_switchdev_sync_objs().
But we're not doing that.

To prove that there is a difference between VLAN filtering as a port
property of the bridge device, and VLAN MSTIs (or other per-VLAN global
bridge options), consider this.
You create a bridge, add 10 VLANs on br0, enable VLAN filtering, then
delete the 10 VLANs and re-create them. The bridge is still VLAN
filtering.
So VLAN filtering is a property of the bridge.

Next you create a bridge, add 10 VLANs on br0, run your new command:
'bridge vlan global set dev br0 vid <VID> msti <MSTI>'
then delete the 10 VLANs and create them back.
Their MSTI is 0, not what was set via the bridge vlan global options...
Because the MSTI is a property of the VLANs, not of the bridge.

A real port attribute wouldn't behave like that.

At least this is what I understand from your patch set, I haven't run it;
sorry if I'm mistaken about something, but I can't find a clearer way to
express what I find strange.

Anyway, I'll stop uselessly commenting here - I can understand the
practical reasons why you wouldn't want to bother expanding the taxonomy
to describe this for what it really is - an "object attribute" of sorts -
because a port attribute for the bridge device has the call path you
need already laid out, including replication towards all bridge ports.

I yield, I yield! :)