| Message ID | 20220314095231.3486931-10-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: 0 this patch: 1 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 0 this patch: 2 |
| 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: 0 this patch: 1 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 52 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Mar 14, 2022 at 10:52:26AM +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 | 20 ++++++++++++++++++++ > net/dsa/slave.c | 6 ++++++ > 3 files changed, 28 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..1a17a0efa2fa 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -240,6 +240,10 @@ static int dsa_port_switchdev_sync_attrs(struct dsa_port *dp, > if (err && err != -EOPNOTSUPP) > return err; > > + err = dsa_port_mst_enable(dp, br_mst_enabled(br), extack); > + if (err && err != -EOPNOTSUPP) > + return err; Sadly this will break down because we don't have unwinding on error in place (sorry). We'd end up with an unoffloaded bridge port with partially synced bridge port attributes. Could you please add a patch previous to this one that handles this, and unoffloads those on error? > + > return 0; > } > > @@ -735,6 +739,22 @@ 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) > +{ > + struct dsa_switch *ds = dp->ds; > + > + if (!on) > + return 0; > + > + if (!dsa_port_can_configure_learning(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 a61a7c54af20..333f5702ea4f 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 Mon, Mar 14, 2022 at 10:52:26AM +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 | 20 ++++++++++++++++++++ > net/dsa/slave.c | 6 ++++++ > 3 files changed, 28 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..1a17a0efa2fa 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -240,6 +240,10 @@ static int dsa_port_switchdev_sync_attrs(struct dsa_port *dp, > if (err && err != -EOPNOTSUPP) > return err; > > + err = dsa_port_mst_enable(dp, br_mst_enabled(br), extack); > + if (err && err != -EOPNOTSUPP) > + return err; The "err && err != -EOPNOTSUPP" scheme is in place for compatibility with drivers that don't all support the bridge port attributes, but used to work prior to dsa_port_switchdev_sync_attrs()'s existence. I don't think this scheme is necessary for dsa_port_mst_enable(), you can just return -EOPNOTSUPP and remove the special handling for this code. > + > return 0; > } > > @@ -735,6 +739,22 @@ 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) > +{ > + struct dsa_switch *ds = dp->ds; > + > + if (!on) > + return 0; > + > + if (!dsa_port_can_configure_learning(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 a61a7c54af20..333f5702ea4f 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 Mon, Mar 14, 2022 at 06:56:49PM +0200, Vladimir Oltean wrote: > > diff --git a/net/dsa/port.c b/net/dsa/port.c > > index 58291df14cdb..1a17a0efa2fa 100644 > > --- a/net/dsa/port.c > > +++ b/net/dsa/port.c > > @@ -240,6 +240,10 @@ static int dsa_port_switchdev_sync_attrs(struct dsa_port *dp, > > if (err && err != -EOPNOTSUPP) > > return err; > > > > + err = dsa_port_mst_enable(dp, br_mst_enabled(br), extack); > > + if (err && err != -EOPNOTSUPP) > > + return err; > > Sadly this will break down because we don't have unwinding on error in > place (sorry). We'd end up with an unoffloaded bridge port with > partially synced bridge port attributes. Could you please add a patch > previous to this one that handles this, and unoffloads those on error? Actually I would rather rename the entire dsa_port_mst_enable() function to dsa_port_mst_validate() and move it to the beginning of dsa_port_bridge_join(). This simplifies the unwinding that needs to take place quite a bit.
On Mon, Mar 14, 2022 at 19:55, Vladimir Oltean <olteanv@gmail.com> wrote: > On Mon, Mar 14, 2022 at 06:56:49PM +0200, Vladimir Oltean wrote: >> > diff --git a/net/dsa/port.c b/net/dsa/port.c >> > index 58291df14cdb..1a17a0efa2fa 100644 >> > --- a/net/dsa/port.c >> > +++ b/net/dsa/port.c >> > @@ -240,6 +240,10 @@ static int dsa_port_switchdev_sync_attrs(struct dsa_port *dp, >> > if (err && err != -EOPNOTSUPP) >> > return err; >> > >> > + err = dsa_port_mst_enable(dp, br_mst_enabled(br), extack); >> > + if (err && err != -EOPNOTSUPP) >> > + return err; >> >> Sadly this will break down because we don't have unwinding on error in >> place (sorry). We'd end up with an unoffloaded bridge port with >> partially synced bridge port attributes. Could you please add a patch >> previous to this one that handles this, and unoffloads those on error? > > Actually I would rather rename the entire dsa_port_mst_enable() function > to dsa_port_mst_validate() and move it to the beginning of dsa_port_bridge_join(). > This simplifies the unwinding that needs to take place quite a bit. Well you still need to unwind vlan filtering if setting the ageing time fails, which is the most complicated one, right? Still, I agree that _validate is a better name, and then _bridge_join seems like a more reasonable placement. Should the unwinding patch still be part of this series then? While we're here, I actually made this a hard error in both scenarios (but forgot to update the log - will do that in v4, depending on what we decide here). There's a dilemma: - When reacting to the attribute event, i.e. changing the mode on a member we're apart of, we _can't_ return -EOPNOTSUPP as it will be ignored, which is why dsa_port_mst_validate (nee _enable) returns -EINVAL. - When joining a bridge, we _must_ return -EOPNOTSUPP to trigger the software fallback. Having something like this in dsa_port_bridge_join... err = dsa_port_mst_validate(dp); if (err == -EINVAL) return -EOPNOTSUPP; else if (err) return err; ...works I suppose, but feels somewhat awkwark. Any better ideas?
On Mon, Mar 14, 2022 at 09:01:12PM +0100, Tobias Waldekranz wrote: > On Mon, Mar 14, 2022 at 19:55, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Mon, Mar 14, 2022 at 06:56:49PM +0200, Vladimir Oltean wrote: > >> > diff --git a/net/dsa/port.c b/net/dsa/port.c > >> > index 58291df14cdb..1a17a0efa2fa 100644 > >> > --- a/net/dsa/port.c > >> > +++ b/net/dsa/port.c > >> > @@ -240,6 +240,10 @@ static int dsa_port_switchdev_sync_attrs(struct dsa_port *dp, > >> > if (err && err != -EOPNOTSUPP) > >> > return err; > >> > > >> > + err = dsa_port_mst_enable(dp, br_mst_enabled(br), extack); > >> > + if (err && err != -EOPNOTSUPP) > >> > + return err; > >> > >> Sadly this will break down because we don't have unwinding on error in > >> place (sorry). We'd end up with an unoffloaded bridge port with > >> partially synced bridge port attributes. Could you please add a patch > >> previous to this one that handles this, and unoffloads those on error? > > > > Actually I would rather rename the entire dsa_port_mst_enable() function > > to dsa_port_mst_validate() and move it to the beginning of dsa_port_bridge_join(). > > This simplifies the unwinding that needs to take place quite a bit. > > Well you still need to unwind vlan filtering if setting the ageing time > fails, which is the most complicated one, right? Yes, but we can leave that for another day :) ...ergo > Should the unwinding patch still be part of this series then? no. > Still, I agree that _validate is a better name, and then _bridge_join > seems like a more reasonable placement. > > While we're here, I actually made this a hard error in both scenarios > (but forgot to update the log - will do that in v4, depending on what we > decide here). There's a dilemma: > > - When reacting to the attribute event, i.e. changing the mode on a > member we're apart of, we _can't_ return -EOPNOTSUPP as it will be > ignored, which is why dsa_port_mst_validate (nee _enable) returns > -EINVAL. > > - When joining a bridge, we _must_ return -EOPNOTSUPP to trigger the > software fallback. > > Having something like this in dsa_port_bridge_join... > > err = dsa_port_mst_validate(dp); > if (err == -EINVAL) > return -EOPNOTSUPP; > else if (err) > return err; > > ...works I suppose, but feels somewhat awkwark. Any better ideas? What you can do is follow the model of dsa_switch_supports_uc_filtering(), and create a dsa_switch_supports_mst() which is called inside an "if br_mst_enabled(br)" check, and returns bool. When false, you could return -EINVAL or -EOPNOTSUPP, as appropriate. This is mostly fine, except for the pesky dsa_port_can_configure_learning(dp) check :) So while you could name it dsa_port_supports_mst() and pass it a dsa_port, the problem is that you can't put the implementation of this new dsa_port_supports_mst() next to dsa_switch_supports_uc_filtering() where it would be nice to sit for symmetry, because the latter is static inline and we're missing the definition of dsa_port_can_configure_learning(). So.. the second best thing is to keep dsa_port_supports_mst() in the same place where dsa_port_mst_enable() currently is. What do you think?
On Mon, Mar 14, 2022 at 22:20, Vladimir Oltean <olteanv@gmail.com> wrote: > On Mon, Mar 14, 2022 at 09:01:12PM +0100, Tobias Waldekranz wrote: >> On Mon, Mar 14, 2022 at 19:55, Vladimir Oltean <olteanv@gmail.com> wrote: >> > On Mon, Mar 14, 2022 at 06:56:49PM +0200, Vladimir Oltean wrote: >> >> > diff --git a/net/dsa/port.c b/net/dsa/port.c >> >> > index 58291df14cdb..1a17a0efa2fa 100644 >> >> > --- a/net/dsa/port.c >> >> > +++ b/net/dsa/port.c >> >> > @@ -240,6 +240,10 @@ static int dsa_port_switchdev_sync_attrs(struct dsa_port *dp, >> >> > if (err && err != -EOPNOTSUPP) >> >> > return err; >> >> > >> >> > + err = dsa_port_mst_enable(dp, br_mst_enabled(br), extack); >> >> > + if (err && err != -EOPNOTSUPP) >> >> > + return err; >> >> >> >> Sadly this will break down because we don't have unwinding on error in >> >> place (sorry). We'd end up with an unoffloaded bridge port with >> >> partially synced bridge port attributes. Could you please add a patch >> >> previous to this one that handles this, and unoffloads those on error? >> > >> > Actually I would rather rename the entire dsa_port_mst_enable() function >> > to dsa_port_mst_validate() and move it to the beginning of dsa_port_bridge_join(). >> > This simplifies the unwinding that needs to take place quite a bit. >> >> Well you still need to unwind vlan filtering if setting the ageing time >> fails, which is the most complicated one, right? > > Yes, but we can leave that for another day :) > > ...ergo > >> Should the unwinding patch still be part of this series then? > > no. Agreed >> Still, I agree that _validate is a better name, and then _bridge_join >> seems like a more reasonable placement. >> >> While we're here, I actually made this a hard error in both scenarios >> (but forgot to update the log - will do that in v4, depending on what we >> decide here). There's a dilemma: >> >> - When reacting to the attribute event, i.e. changing the mode on a >> member we're apart of, we _can't_ return -EOPNOTSUPP as it will be >> ignored, which is why dsa_port_mst_validate (nee _enable) returns >> -EINVAL. >> >> - When joining a bridge, we _must_ return -EOPNOTSUPP to trigger the >> software fallback. >> >> Having something like this in dsa_port_bridge_join... >> >> err = dsa_port_mst_validate(dp); >> if (err == -EINVAL) >> return -EOPNOTSUPP; >> else if (err) >> return err; >> >> ...works I suppose, but feels somewhat awkwark. Any better ideas? > > What you can do is follow the model of dsa_switch_supports_uc_filtering(), > and create a dsa_switch_supports_mst() which is called inside an > "if br_mst_enabled(br)" check, and returns bool. When false, you could > return -EINVAL or -EOPNOTSUPP, as appropriate. > > This is mostly fine, except for the pesky dsa_port_can_configure_learning(dp) > check :) So while you could name it dsa_port_supports_mst() and pass it > a dsa_port, the problem is that you can't put the implementation of this > new dsa_port_supports_mst() next to dsa_switch_supports_uc_filtering() > where it would be nice to sit for symmetry, because the latter is static > inline and we're missing the definition of dsa_port_can_configure_learning(). > So.. the second best thing is to keep dsa_port_supports_mst() in the > same place where dsa_port_mst_enable() currently is. > > What do you think? I think that would mostly work. It would have to be positioned higher up in the file though, so that it can be called from _bridge_join. Unless we add a forward for it of course, but that seems to break with existing conventions.
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..1a17a0efa2fa 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -240,6 +240,10 @@ static int dsa_port_switchdev_sync_attrs(struct dsa_port *dp, if (err && err != -EOPNOTSUPP) return err; + err = dsa_port_mst_enable(dp, br_mst_enabled(br), extack); + if (err && err != -EOPNOTSUPP) + return err; + return 0; } @@ -735,6 +739,22 @@ 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) +{ + struct dsa_switch *ds = dp->ds; + + if (!on) + return 0; + + if (!dsa_port_can_configure_learning(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 a61a7c54af20..333f5702ea4f 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 | 20 ++++++++++++++++++++ net/dsa/slave.c | 6 ++++++ 3 files changed, 28 insertions(+)