Re: [PATCH v2 net-next 06/10] net: dsa: Pass VLAN MSTI migration notifications to driver

[Tobias Waldekranz]

On Fri, Mar 04, 2022 at 00:29, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
> On Tue, Mar 01, 2022 at 11:03:17AM +0100, Tobias Waldekranz wrote:
>> Add the usual trampoline functionality from the generic DSA layer down
>> to the drivers for VLAN MSTI migrations.
>> 
>> Signed-off-by: Tobias Waldekranz <tobias@xxxxxxxxxxxxxx>
>> ---
>>  include/net/dsa.h  |  3 +++
>>  net/dsa/dsa_priv.h |  1 +
>>  net/dsa/port.c     | 10 ++++++++++
>>  net/dsa/slave.c    |  6 ++++++
>>  4 files changed, 20 insertions(+)
>> 
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index cfedcfb86350..cc8acb01bd9b 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -962,6 +962,9 @@ struct dsa_switch_ops {
>>  				 struct netlink_ext_ack *extack);
>>  	int	(*port_vlan_del)(struct dsa_switch *ds, int port,
>>  				 const struct switchdev_obj_port_vlan *vlan);
>> +	int	(*vlan_msti_set)(struct dsa_switch *ds,
>> +				 const struct switchdev_attr *attr);
>
> I would rather pass the struct switchdev_vlan_attr and the orig_dev
> (bridge) as separate arguments here. Or even the struct dsa_bridge, for
> consistency to the API changes for database isolation.

Fair point. I'll change.

>> +
>>  	/*
>>  	 * Forwarding database
>>  	 */
>> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
>> index 07c0ad52395a..87ec0697e92e 100644
>> --- a/net/dsa/dsa_priv.h
>> +++ b/net/dsa/dsa_priv.h
>> @@ -217,6 +217,7 @@ 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_vlan_msti(struct dsa_port *dp, const struct switchdev_attr *attr);
>>  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 d9da425a17fb..5f45cb7d70ba 100644
>> --- a/net/dsa/port.c
>> +++ b/net/dsa/port.c
>> @@ -778,6 +778,16 @@ int dsa_port_bridge_flags(struct dsa_port *dp,
>>  	return 0;
>>  }
>>  
>> +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:

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

>> +}
>> +
>>  int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu,
>>  			bool targeted_match)
>>  {
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 089616206b11..c6ffcd782b5a 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -314,6 +314,12 @@ static int dsa_slave_port_attr_set(struct net_device *dev, const void *ctx,
>>  
>>  		ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, extack);
>>  		break;
>> +	case SWITCHDEV_ATTR_ID_VLAN_MSTI:
>> +		if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev))
>> +			return -EOPNOTSUPP;
>> +
>> +		ret = dsa_port_vlan_msti(dp, attr);
>> +		break;
>>  	default:
>>  		ret = -EOPNOTSUPP;
>>  		break;
>> -- 
>> 2.25.1
>>