On Wed, Mar 09, 2022 at 04:47:02PM +0100, Tobias Waldekranz wrote:

quoted

quoted

+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.