On Mon, Mar 14, 2022 at 09:01:12PM +0100, Tobias Waldekranz wrote:

On Mon, Mar 14, 2022 at 19:55, Vladimir Oltean [off-list ref] wrote:

quoted

On Mon, Mar 14, 2022 at 06:56:49PM +0200, Vladimir Oltean wrote:

quoted

quoted

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?