| Message ID | 20220301100321.951175-7-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: 21 this patch: 21 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 26 this patch: 26 |
| 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: 21 this patch: 21 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 44 lines checked |
| 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:17AM +0100, Tobias Waldekranz wrote: > Add the usual trampoline functionality from the generic DSA layer down > to the drivers for VLAN MSTI migrations. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > include/net/dsa.h | 3 +++ > net/dsa/dsa_priv.h | 1 + > net/dsa/port.c | 10 ++++++++++ > net/dsa/slave.c | 6 ++++++ > 4 files changed, 20 insertions(+) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index cfedcfb86350..cc8acb01bd9b 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -962,6 +962,9 @@ struct dsa_switch_ops { > struct netlink_ext_ack *extack); > int (*port_vlan_del)(struct dsa_switch *ds, int port, > const struct switchdev_obj_port_vlan *vlan); > + int (*vlan_msti_set)(struct dsa_switch *ds, > + const struct switchdev_attr *attr); I would rather pass the struct switchdev_vlan_attr and the orig_dev (bridge) as separate arguments here. Or even the struct dsa_bridge, for consistency to the API changes for database isolation. > + > /* > * Forwarding database > */ > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h > index 07c0ad52395a..87ec0697e92e 100644 > --- a/net/dsa/dsa_priv.h > +++ b/net/dsa/dsa_priv.h > @@ -217,6 +217,7 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering, > struct netlink_ext_ack *extack); > bool dsa_port_skip_vlan_configuration(struct dsa_port *dp); > int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock); > +int dsa_port_vlan_msti(struct dsa_port *dp, const struct switchdev_attr *attr); > int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu, > bool targeted_match); > int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr, > diff --git a/net/dsa/port.c b/net/dsa/port.c > index d9da425a17fb..5f45cb7d70ba 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -778,6 +778,16 @@ int dsa_port_bridge_flags(struct dsa_port *dp, > return 0; > } > > +int dsa_port_vlan_msti(struct dsa_port *dp, const struct switchdev_attr *attr) > +{ > + struct dsa_switch *ds = dp->ds; > + > + if (!ds->ops->vlan_msti_set) > + return -EOPNOTSUPP; > + > + return ds->ops->vlan_msti_set(ds, attr); I guess this doesn't need to be a cross-chip notifier event for all switches, because replication to all bridge ports is handled by switchdev_handle_port_attr_set(). Ok. But isn't it called too many times per switch? > +} > + > int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu, > bool targeted_match) > { > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 089616206b11..c6ffcd782b5a 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -314,6 +314,12 @@ static int dsa_slave_port_attr_set(struct net_device *dev, const void *ctx, > > ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, extack); > break; > + case SWITCHDEV_ATTR_ID_VLAN_MSTI: > + if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev)) > + return -EOPNOTSUPP; > + > + ret = dsa_port_vlan_msti(dp, attr); > + break; > default: > ret = -EOPNOTSUPP; > break; > -- > 2.25.1 >
On Fri, Mar 04, 2022 at 00:29, Vladimir Oltean <olteanv@gmail.com> wrote: > On Tue, Mar 01, 2022 at 11:03:17AM +0100, Tobias Waldekranz wrote: >> Add the usual trampoline functionality from the generic DSA layer down >> to the drivers for VLAN MSTI migrations. >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- >> include/net/dsa.h | 3 +++ >> net/dsa/dsa_priv.h | 1 + >> net/dsa/port.c | 10 ++++++++++ >> net/dsa/slave.c | 6 ++++++ >> 4 files changed, 20 insertions(+) >> >> diff --git a/include/net/dsa.h b/include/net/dsa.h >> index cfedcfb86350..cc8acb01bd9b 100644 >> --- a/include/net/dsa.h >> +++ b/include/net/dsa.h >> @@ -962,6 +962,9 @@ struct dsa_switch_ops { >> struct netlink_ext_ack *extack); >> int (*port_vlan_del)(struct dsa_switch *ds, int port, >> const struct switchdev_obj_port_vlan *vlan); >> + int (*vlan_msti_set)(struct dsa_switch *ds, >> + const struct switchdev_attr *attr); > > I would rather pass the struct switchdev_vlan_attr and the orig_dev > (bridge) as separate arguments here. Or even the struct dsa_bridge, for > consistency to the API changes for database isolation. Fair point. I'll change. >> + >> /* >> * Forwarding database >> */ >> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h >> index 07c0ad52395a..87ec0697e92e 100644 >> --- a/net/dsa/dsa_priv.h >> +++ b/net/dsa/dsa_priv.h >> @@ -217,6 +217,7 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering, >> struct netlink_ext_ack *extack); >> bool dsa_port_skip_vlan_configuration(struct dsa_port *dp); >> int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock); >> +int dsa_port_vlan_msti(struct dsa_port *dp, const struct switchdev_attr *attr); >> int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu, >> bool targeted_match); >> int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr, >> diff --git a/net/dsa/port.c b/net/dsa/port.c >> index d9da425a17fb..5f45cb7d70ba 100644 >> --- a/net/dsa/port.c >> +++ b/net/dsa/port.c >> @@ -778,6 +778,16 @@ int dsa_port_bridge_flags(struct dsa_port *dp, >> return 0; >> } >> >> +int dsa_port_vlan_msti(struct dsa_port *dp, const struct switchdev_attr *attr) >> +{ >> + struct dsa_switch *ds = dp->ds; >> + >> + if (!ds->ops->vlan_msti_set) >> + return -EOPNOTSUPP; >> + >> + return ds->ops->vlan_msti_set(ds, attr); > > I guess this doesn't need to be a cross-chip notifier event for all > switches, because replication to all bridge ports is handled by > switchdev_handle_port_attr_set(). Ok. But isn't it called too many times > per switch? It is certainly called more times than necessary. But I'm not aware of any way to limit it. Just as with other bridge-global settings like ageing timeout, the bridge will just replicate the event to each port, not knowing whether some of them belong to the same underlying ASIC or not. We could leverage hwdoms in the bridge to figure that out, but then: - Drivers that do not implement forward offloading would miss out on this optimization. Unfortunate but not a big deal. - Since DSA presents multi-chip trees as a single switchdev, the DSA layer would have to replicate the event out to each device. Doable, but feels like a series of its own. >> +} >> + >> int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu, >> bool targeted_match) >> { >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c >> index 089616206b11..c6ffcd782b5a 100644 >> --- a/net/dsa/slave.c >> +++ b/net/dsa/slave.c >> @@ -314,6 +314,12 @@ static int dsa_slave_port_attr_set(struct net_device *dev, const void *ctx, >> >> ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, extack); >> break; >> + case SWITCHDEV_ATTR_ID_VLAN_MSTI: >> + if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev)) >> + return -EOPNOTSUPP; >> + >> + ret = dsa_port_vlan_msti(dp, attr); >> + break; >> default: >> ret = -EOPNOTSUPP; >> break; >> -- >> 2.25.1 >>
On Wed, Mar 09, 2022 at 04:47:02PM +0100, Tobias Waldekranz wrote: > >> +int dsa_port_vlan_msti(struct dsa_port *dp, const struct switchdev_attr *attr) > >> +{ > >> + struct dsa_switch *ds = dp->ds; > >> + > >> + if (!ds->ops->vlan_msti_set) > >> + return -EOPNOTSUPP; > >> + > >> + return ds->ops->vlan_msti_set(ds, attr); > > > > I guess this doesn't need to be a cross-chip notifier event for all > > switches, because replication to all bridge ports is handled by > > switchdev_handle_port_attr_set(). Ok. But isn't it called too many times > > per switch? > > It is certainly called more times than necessary. But I'm not aware of > any way to limit it. Just as with other bridge-global settings like > ageing timeout, the bridge will just replicate the event to each port, > not knowing whether some of them belong to the same underlying ASIC or > not. > > We could leverage hwdoms in the bridge to figure that out, but then: Hmm, uncalled for. Also, not sure how it helps (it just plain doesn't work, as you've pointed out below yourself). > > - Drivers that do not implement forward offloading would miss out on > this optimization. Unfortunate but not a big deal. > - Since DSA presents multi-chip trees as a single switchdev, the DSA > layer would have to replicate the event out to each device. Doable, > but feels like a series of its own. I've mentally walked through the alternatives and I don't see a practical alternative than letting the driver cut out the duplicate calls. Maybe it's worth raising awareness by adding a comment above the dsa_switch_ops :: vlan_msti_set definition that drivers should be prepared to handle such calls. Case in point, in mv88e6xxx_vlan_msti_set() you could avoid some useless MDIO transactions (a call to mv88e6xxx_vtu_loadpurge) with a simple "if (vlan.sid != new_sid)" check. Basically just go through a refcount bump followed by an immediate drop.
diff --git a/include/net/dsa.h b/include/net/dsa.h index cfedcfb86350..cc8acb01bd9b 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -962,6 +962,9 @@ struct dsa_switch_ops { struct netlink_ext_ack *extack); int (*port_vlan_del)(struct dsa_switch *ds, int port, const struct switchdev_obj_port_vlan *vlan); + int (*vlan_msti_set)(struct dsa_switch *ds, + const struct switchdev_attr *attr); + /* * Forwarding database */ diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 07c0ad52395a..87ec0697e92e 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -217,6 +217,7 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering, struct netlink_ext_ack *extack); bool dsa_port_skip_vlan_configuration(struct dsa_port *dp); int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock); +int dsa_port_vlan_msti(struct dsa_port *dp, const struct switchdev_attr *attr); int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu, bool targeted_match); int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr, diff --git a/net/dsa/port.c b/net/dsa/port.c index d9da425a17fb..5f45cb7d70ba 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -778,6 +778,16 @@ int dsa_port_bridge_flags(struct dsa_port *dp, return 0; } +int dsa_port_vlan_msti(struct dsa_port *dp, const struct switchdev_attr *attr) +{ + struct dsa_switch *ds = dp->ds; + + if (!ds->ops->vlan_msti_set) + return -EOPNOTSUPP; + + return ds->ops->vlan_msti_set(ds, attr); +} + int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu, bool targeted_match) { diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 089616206b11..c6ffcd782b5a 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -314,6 +314,12 @@ static int dsa_slave_port_attr_set(struct net_device *dev, const void *ctx, ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, extack); break; + case SWITCHDEV_ATTR_ID_VLAN_MSTI: + if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev)) + return -EOPNOTSUPP; + + ret = dsa_port_vlan_msti(dp, attr); + break; default: ret = -EOPNOTSUPP; break;
Add the usual trampoline functionality from the generic DSA layer down to the drivers for VLAN MSTI migrations. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- include/net/dsa.h | 3 +++ net/dsa/dsa_priv.h | 1 + net/dsa/port.c | 10 ++++++++++ net/dsa/slave.c | 6 ++++++ 4 files changed, 20 insertions(+)