On Tue, Mar 08, 2022 at 09:01:04AM +0100, Tobias Waldekranz wrote:
> On Thu, Mar 03, 2022 at 22:59, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
> > On Tue, Mar 01, 2022 at 11:03:15AM +0100, Tobias Waldekranz wrote:
> >> 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@xxxxxxxxxxxxxx>
> >> ---
> >> 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.
>
> > 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.
> >> +};
> >> +
> >> 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.
>
> >> + .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.
