| Message ID | 20220301100321.951175-5-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bridge: Multiple Spanning Trees | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | fail | Errors and warnings before: 59 this patch: 54 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 28 this patch: 28 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 70 this patch: 68 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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@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? 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. > +}; > + > 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? > + .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. > + if (err && err != -EOPNOTSUPP) > + return err; > > mv->msti = msti; > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > index 6f6a70121a5e..160d7659f88a 100644 > --- a/net/bridge/br_switchdev.c > +++ b/net/bridge/br_switchdev.c > @@ -428,6 +428,57 @@ static int br_switchdev_vlan_replay(struct net_device *br_dev, > return 0; > } > > +static int br_switchdev_mst_replay(struct net_device *br_dev, > + const void *ctx, bool adding, > + struct notifier_block *nb, > + struct netlink_ext_ack *extack) "bool adding" is unused, and replaying the VLAN to MSTI associations before deleting them makes little sense anyway. I understand the appeal of symmetry, so maybe put an if (adding) { err = br_switchdev_vlan_attr_replay(...); if (err && err != -EOPNOTSUPP) return err; } at the end of br_switchdev_vlan_replay()? > +{ > + struct switchdev_notifier_port_attr_info attr_info = { > + .info = { > + .dev = br_dev, > + .extack = extack, > + .ctx = ctx, > + }, > + }; > + struct net_bridge *br = netdev_priv(br_dev); > + struct net_bridge_vlan_group *vg; > + struct net_bridge_vlan *v; > + int err; > + > + ASSERT_RTNL(); > + > + if (!nb) > + return 0; > + > + if (!netif_is_bridge_master(br_dev)) > + return -EINVAL; > + > + vg = br_vlan_group(br); > + > + list_for_each_entry(v, &vg->vlan_list, vlist) { > + struct switchdev_attr attr = { > + .id = SWITCHDEV_ATTR_ID_VLAN_MSTI, > + .flags = SWITCHDEV_F_DEFER, I don't think SWITCHDEV_F_DEFER has any effect on a replay. > + .orig_dev = br_dev, > + .u.vlan_attr = { > + .vid = v->vid, > + .msti = v->msti, > + } > + }; > + > + if (!v->msti) > + continue; > + > + attr_info.attr = &attr; > + err = nb->notifier_call(nb, SWITCHDEV_PORT_ATTR_SET, &attr_info); > + err = notifier_to_errno(err); > + if (err) > + return err; > + } > + > + return 0; > +} > + > #ifdef CONFIG_BRIDGE_IGMP_SNOOPING > struct br_switchdev_mdb_complete_info { > struct net_bridge_port *port; > @@ -695,6 +746,10 @@ static int nbp_switchdev_sync_objs(struct net_bridge_port *p, const void *ctx, > if (err && err != -EOPNOTSUPP) > return err; > > + err = br_switchdev_mst_replay(br_dev, ctx, true, blocking_nb, extack); > + if (err && err != -EOPNOTSUPP) > + return err; > + > err = br_switchdev_mdb_replay(br_dev, dev, ctx, true, blocking_nb, > extack); > if (err && err != -EOPNOTSUPP) > @@ -719,6 +774,8 @@ static void nbp_switchdev_unsync_objs(struct net_bridge_port *p, > > br_switchdev_mdb_replay(br_dev, dev, ctx, false, blocking_nb, NULL); > > + br_switchdev_mst_replay(br_dev, ctx, false, blocking_nb, NULL); > + > br_switchdev_vlan_replay(br_dev, ctx, false, blocking_nb, NULL); > } > > -- > 2.25.1 >
On Thu, Mar 03, 2022 at 22:59, Vladimir Oltean <olteanv@gmail.com> 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@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. > 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? >> +}; >> + >> 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? >> + if (err && err != -EOPNOTSUPP) >> + return err; >> >> mv->msti = msti; >> >> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c >> index 6f6a70121a5e..160d7659f88a 100644 >> --- a/net/bridge/br_switchdev.c >> +++ b/net/bridge/br_switchdev.c >> @@ -428,6 +428,57 @@ static int br_switchdev_vlan_replay(struct net_device *br_dev, >> return 0; >> } >> >> +static int br_switchdev_mst_replay(struct net_device *br_dev, >> + const void *ctx, bool adding, >> + struct notifier_block *nb, >> + struct netlink_ext_ack *extack) > > "bool adding" is unused, and replaying the VLAN to MSTI associations > before deleting them makes little sense anyway. > > I understand the appeal of symmetry, so maybe put an > > if (adding) { > err = br_switchdev_vlan_attr_replay(...); > if (err && err != -EOPNOTSUPP) > return err; > } > > at the end of br_switchdev_vlan_replay()? Yeah, that is better. Will change. >> +{ >> + struct switchdev_notifier_port_attr_info attr_info = { >> + .info = { >> + .dev = br_dev, >> + .extack = extack, >> + .ctx = ctx, >> + }, >> + }; >> + struct net_bridge *br = netdev_priv(br_dev); >> + struct net_bridge_vlan_group *vg; >> + struct net_bridge_vlan *v; >> + int err; >> + >> + ASSERT_RTNL(); >> + >> + if (!nb) >> + return 0; >> + >> + if (!netif_is_bridge_master(br_dev)) >> + return -EINVAL; >> + >> + vg = br_vlan_group(br); >> + >> + list_for_each_entry(v, &vg->vlan_list, vlist) { >> + struct switchdev_attr attr = { >> + .id = SWITCHDEV_ATTR_ID_VLAN_MSTI, >> + .flags = SWITCHDEV_F_DEFER, > > I don't think SWITCHDEV_F_DEFER has any effect on a replay. Right, will fix. >> + .orig_dev = br_dev, >> + .u.vlan_attr = { >> + .vid = v->vid, >> + .msti = v->msti, >> + } >> + }; >> + >> + if (!v->msti) >> + continue; >> + >> + attr_info.attr = &attr; >> + err = nb->notifier_call(nb, SWITCHDEV_PORT_ATTR_SET, &attr_info); >> + err = notifier_to_errno(err); >> + if (err) >> + return err; >> + } >> + >> + return 0; >> +} >> + >> #ifdef CONFIG_BRIDGE_IGMP_SNOOPING >> struct br_switchdev_mdb_complete_info { >> struct net_bridge_port *port; >> @@ -695,6 +746,10 @@ static int nbp_switchdev_sync_objs(struct net_bridge_port *p, const void *ctx, >> if (err && err != -EOPNOTSUPP) >> return err; >> >> + err = br_switchdev_mst_replay(br_dev, ctx, true, blocking_nb, extack); >> + if (err && err != -EOPNOTSUPP) >> + return err; >> + >> err = br_switchdev_mdb_replay(br_dev, dev, ctx, true, blocking_nb, >> extack); >> if (err && err != -EOPNOTSUPP) >> @@ -719,6 +774,8 @@ static void nbp_switchdev_unsync_objs(struct net_bridge_port *p, >> >> br_switchdev_mdb_replay(br_dev, dev, ctx, false, blocking_nb, NULL); >> >> + br_switchdev_mst_replay(br_dev, ctx, false, blocking_nb, NULL); >> + >> br_switchdev_vlan_replay(br_dev, ctx, false, blocking_nb, NULL); >> } >> >> -- >> 2.25.1 >>
On Tue, Mar 08, 2022 at 09:01:04AM +0100, Tobias Waldekranz wrote: > On Thu, Mar 03, 2022 at 22:59, Vladimir Oltean <olteanv@gmail.com> 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@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. > > > 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.
On Tue, Mar 08, 2022 at 19:17, Vladimir Oltean <olteanv@gmail.com> wrote: > On Tue, Mar 08, 2022 at 09:01:04AM +0100, Tobias Waldekranz wrote: >> On Thu, Mar 03, 2022 at 22:59, Vladimir Oltean <olteanv@gmail.com> 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@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. >> >> > 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. >> >> +}; >> >> + >> >> 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. I yield, I yield! :)
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; + }; +}; + 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, + .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); + if (err && err != -EOPNOTSUPP) + return err; mv->msti = msti; diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index 6f6a70121a5e..160d7659f88a 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -428,6 +428,57 @@ static int br_switchdev_vlan_replay(struct net_device *br_dev, return 0; } +static int br_switchdev_mst_replay(struct net_device *br_dev, + const void *ctx, bool adding, + struct notifier_block *nb, + struct netlink_ext_ack *extack) +{ + struct switchdev_notifier_port_attr_info attr_info = { + .info = { + .dev = br_dev, + .extack = extack, + .ctx = ctx, + }, + }; + struct net_bridge *br = netdev_priv(br_dev); + struct net_bridge_vlan_group *vg; + struct net_bridge_vlan *v; + int err; + + ASSERT_RTNL(); + + if (!nb) + return 0; + + if (!netif_is_bridge_master(br_dev)) + return -EINVAL; + + vg = br_vlan_group(br); + + list_for_each_entry(v, &vg->vlan_list, vlist) { + struct switchdev_attr attr = { + .id = SWITCHDEV_ATTR_ID_VLAN_MSTI, + .flags = SWITCHDEV_F_DEFER, + .orig_dev = br_dev, + .u.vlan_attr = { + .vid = v->vid, + .msti = v->msti, + } + }; + + if (!v->msti) + continue; + + attr_info.attr = &attr; + err = nb->notifier_call(nb, SWITCHDEV_PORT_ATTR_SET, &attr_info); + err = notifier_to_errno(err); + if (err) + return err; + } + + return 0; +} + #ifdef CONFIG_BRIDGE_IGMP_SNOOPING struct br_switchdev_mdb_complete_info { struct net_bridge_port *port; @@ -695,6 +746,10 @@ static int nbp_switchdev_sync_objs(struct net_bridge_port *p, const void *ctx, if (err && err != -EOPNOTSUPP) return err; + err = br_switchdev_mst_replay(br_dev, ctx, true, blocking_nb, extack); + if (err && err != -EOPNOTSUPP) + return err; + err = br_switchdev_mdb_replay(br_dev, dev, ctx, true, blocking_nb, extack); if (err && err != -EOPNOTSUPP) @@ -719,6 +774,8 @@ static void nbp_switchdev_unsync_objs(struct net_bridge_port *p, br_switchdev_mdb_replay(br_dev, dev, ctx, false, blocking_nb, NULL); + br_switchdev_mst_replay(br_dev, ctx, false, blocking_nb, NULL); + br_switchdev_vlan_replay(br_dev, ctx, false, blocking_nb, NULL); }
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(+)