| Message ID | 20220315002543.190587-11-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 | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| 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 | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 60 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Mar 15, 2022 at 01:25:38AM +0100, Tobias Waldekranz wrote: > When joining a bridge where MST is enabled, we validate that the > proper offloading support is in place, otherwise we fallback to > software bridging. > > When then mode is changed on a bridge in which we are members, we > refuse the change if offloading is not supported. > > At the moment we only check for configurable learning, but this will > be further restricted as we support more MST related switchdev events. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > net/dsa/dsa_priv.h | 2 ++ > net/dsa/port.c | 22 ++++++++++++++++++++++ > net/dsa/slave.c | 6 ++++++ > 3 files changed, 30 insertions(+) > > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h > index f20bdd8ea0a8..2aba420696ef 100644 > --- a/net/dsa/dsa_priv.h > +++ b/net/dsa/dsa_priv.h > @@ -234,6 +234,8 @@ 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_mst_enable(struct dsa_port *dp, bool on, > + struct netlink_ext_ack *extack); > 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 58291df14cdb..02214033cec0 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -321,6 +321,11 @@ static void dsa_port_bridge_destroy(struct dsa_port *dp, > kfree(bridge); > } > > +static bool dsa_port_supports_mst(struct dsa_port *dp) > +{ > + return dsa_port_can_configure_learning(dp); > +} > + > int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br, > struct netlink_ext_ack *extack) > { > @@ -334,6 +339,9 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br, > struct net_device *brport_dev; > int err; > > + if (br_mst_enabled(br) && !dsa_port_supports_mst(dp)) > + return -EOPNOTSUPP; > + > /* Here the interface is already bridged. Reflect the current > * configuration so that drivers can program their chips accordingly. > */ > @@ -735,6 +743,20 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock) > return 0; > } > > +int dsa_port_mst_enable(struct dsa_port *dp, bool on, > + struct netlink_ext_ack *extack) > +{ > + if (!on) > + return 0; > + > + if (!dsa_port_supports_mst(dp)) { > + NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST"); > + return -EINVAL; > + } > + > + return 0; > +} Experimenting a bit... maybe this looks tidier? We make the "if" condition have the same basic structure as the previous "if (br_mst_enabled(br) && !dsa_port_supports_mst(dp))", albeit transformed using De Morgan's rules. { if (!on || dsa_port_supports_mst(dp)) return 0; NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST"); return -EINVAL; } > + > int dsa_port_pre_bridge_flags(const struct dsa_port *dp, > struct switchdev_brport_flags flags, > struct netlink_ext_ack *extack) > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 647adee97f7f..879d18cc99cb 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -463,6 +463,12 @@ static int dsa_slave_port_attr_set(struct net_device *dev, const void *ctx, > > ret = dsa_port_ageing_time(dp, attr->u.ageing_time); > break; > + case SWITCHDEV_ATTR_ID_BRIDGE_MST: > + if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev)) > + return -EOPNOTSUPP; > + > + ret = dsa_port_mst_enable(dp, attr->u.mst, extack); > + break; > case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS: > if (!dsa_port_offloads_bridge_port(dp, attr->orig_dev)) > return -EOPNOTSUPP; > -- > 2.25.1 >
On Tue, Mar 15, 2022 at 19:11, Vladimir Oltean <olteanv@gmail.com> wrote: > On Tue, Mar 15, 2022 at 01:25:38AM +0100, Tobias Waldekranz wrote: >> When joining a bridge where MST is enabled, we validate that the >> proper offloading support is in place, otherwise we fallback to >> software bridging. >> >> When then mode is changed on a bridge in which we are members, we >> refuse the change if offloading is not supported. >> >> At the moment we only check for configurable learning, but this will >> be further restricted as we support more MST related switchdev events. >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- >> net/dsa/dsa_priv.h | 2 ++ >> net/dsa/port.c | 22 ++++++++++++++++++++++ >> net/dsa/slave.c | 6 ++++++ >> 3 files changed, 30 insertions(+) >> >> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h >> index f20bdd8ea0a8..2aba420696ef 100644 >> --- a/net/dsa/dsa_priv.h >> +++ b/net/dsa/dsa_priv.h >> @@ -234,6 +234,8 @@ 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_mst_enable(struct dsa_port *dp, bool on, >> + struct netlink_ext_ack *extack); >> 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 58291df14cdb..02214033cec0 100644 >> --- a/net/dsa/port.c >> +++ b/net/dsa/port.c >> @@ -321,6 +321,11 @@ static void dsa_port_bridge_destroy(struct dsa_port *dp, >> kfree(bridge); >> } >> >> +static bool dsa_port_supports_mst(struct dsa_port *dp) >> +{ >> + return dsa_port_can_configure_learning(dp); >> +} >> + >> int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br, >> struct netlink_ext_ack *extack) >> { >> @@ -334,6 +339,9 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br, >> struct net_device *brport_dev; >> int err; >> >> + if (br_mst_enabled(br) && !dsa_port_supports_mst(dp)) >> + return -EOPNOTSUPP; >> + >> /* Here the interface is already bridged. Reflect the current >> * configuration so that drivers can program their chips accordingly. >> */ >> @@ -735,6 +743,20 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock) >> return 0; >> } >> >> +int dsa_port_mst_enable(struct dsa_port *dp, bool on, >> + struct netlink_ext_ack *extack) >> +{ >> + if (!on) >> + return 0; >> + >> + if (!dsa_port_supports_mst(dp)) { >> + NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} > > Experimenting a bit... maybe this looks tidier? We make the "if" condition > have the same basic structure as the previous "if (br_mst_enabled(br) && > !dsa_port_supports_mst(dp))", albeit transformed using De Morgan's rules. > > { > if (!on || dsa_port_supports_mst(dp)) > return 0; > > NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST"); > return -EINVAL; > } I initially had it like this. It looks tidier, yes - but to me the intent is less obvious when reading it. How about: { if (on && !dsa_port_supports_mst(dp)) { NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST"); return -EINVAL; } return 0; } >> + >> int dsa_port_pre_bridge_flags(const struct dsa_port *dp, >> struct switchdev_brport_flags flags, >> struct netlink_ext_ack *extack) >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c >> index 647adee97f7f..879d18cc99cb 100644 >> --- a/net/dsa/slave.c >> +++ b/net/dsa/slave.c >> @@ -463,6 +463,12 @@ static int dsa_slave_port_attr_set(struct net_device *dev, const void *ctx, >> >> ret = dsa_port_ageing_time(dp, attr->u.ageing_time); >> break; >> + case SWITCHDEV_ATTR_ID_BRIDGE_MST: >> + if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev)) >> + return -EOPNOTSUPP; >> + >> + ret = dsa_port_mst_enable(dp, attr->u.mst, extack); >> + break; >> case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS: >> if (!dsa_port_offloads_bridge_port(dp, attr->orig_dev)) >> return -EOPNOTSUPP; >> -- >> 2.25.1 >>
On Wed, Mar 16, 2022 at 10:15:18AM +0100, Tobias Waldekranz wrote: > >> +int dsa_port_mst_enable(struct dsa_port *dp, bool on, > >> + struct netlink_ext_ack *extack) > >> +{ > >> + if (!on) > >> + return 0; > >> + > >> + if (!dsa_port_supports_mst(dp)) { > >> + NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST"); > >> + return -EINVAL; > >> + } > >> + > >> + return 0; > >> +} > > > > Experimenting a bit... maybe this looks tidier? We make the "if" condition > > have the same basic structure as the previous "if (br_mst_enabled(br) && > > !dsa_port_supports_mst(dp))", albeit transformed using De Morgan's rules. > > > > { > > if (!on || dsa_port_supports_mst(dp)) > > return 0; > > > > NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST"); > > return -EINVAL; > > } > > I initially had it like this. It looks tidier, yes - but to me the > intent is less obvious when reading it. How about: > > { > if (on && !dsa_port_supports_mst(dp)) { > NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST"); > return -EINVAL; > } > > return 0; > } Yes, let's go with this.
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index f20bdd8ea0a8..2aba420696ef 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -234,6 +234,8 @@ 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_mst_enable(struct dsa_port *dp, bool on, + struct netlink_ext_ack *extack); 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 58291df14cdb..02214033cec0 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -321,6 +321,11 @@ static void dsa_port_bridge_destroy(struct dsa_port *dp, kfree(bridge); } +static bool dsa_port_supports_mst(struct dsa_port *dp) +{ + return dsa_port_can_configure_learning(dp); +} + int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br, struct netlink_ext_ack *extack) { @@ -334,6 +339,9 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br, struct net_device *brport_dev; int err; + if (br_mst_enabled(br) && !dsa_port_supports_mst(dp)) + return -EOPNOTSUPP; + /* Here the interface is already bridged. Reflect the current * configuration so that drivers can program their chips accordingly. */ @@ -735,6 +743,20 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock) return 0; } +int dsa_port_mst_enable(struct dsa_port *dp, bool on, + struct netlink_ext_ack *extack) +{ + if (!on) + return 0; + + if (!dsa_port_supports_mst(dp)) { + NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST"); + return -EINVAL; + } + + return 0; +} + int dsa_port_pre_bridge_flags(const struct dsa_port *dp, struct switchdev_brport_flags flags, struct netlink_ext_ack *extack) diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 647adee97f7f..879d18cc99cb 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -463,6 +463,12 @@ static int dsa_slave_port_attr_set(struct net_device *dev, const void *ctx, ret = dsa_port_ageing_time(dp, attr->u.ageing_time); break; + case SWITCHDEV_ATTR_ID_BRIDGE_MST: + if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev)) + return -EOPNOTSUPP; + + ret = dsa_port_mst_enable(dp, attr->u.mst, extack); + break; case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS: if (!dsa_port_offloads_bridge_port(dp, attr->orig_dev)) return -EOPNOTSUPP;
When joining a bridge where MST is enabled, we validate that the proper offloading support is in place, otherwise we fallback to software bridging. When then mode is changed on a bridge in which we are members, we refuse the change if offloading is not supported. At the moment we only check for configurable learning, but this will be further restricted as we support more MST related switchdev events. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- net/dsa/dsa_priv.h | 2 ++ net/dsa/port.c | 22 ++++++++++++++++++++++ net/dsa/slave.c | 6 ++++++ 3 files changed, 30 insertions(+)