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

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? 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?