Re: [PATCH v5 net-next 07/10] net: dsa: allow changing the tag protocol via the "tagging" device attribute

[Florian Fainelli <f.fainelli@gmail.com>]

On 1/20/2021 6:36 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Currently DSA exposes the following sysfs:
> $ cat /sys/class/net/eno2/dsa/tagging
> ocelot
> 
> which is a read-only device attribute, introduced in the kernel as
> commit 98cdb4807123 ("net: dsa: Expose tagging protocol to user-space"),
> and used by libpcap since its commit 993db3800d7d ("Add support for DSA
> link-layer types").
> 
> It would be nice if we could extend this device attribute by making it
> writable:
> $ echo ocelot-8021q > /sys/class/net/eno2/dsa/tagging
> 
> This is useful with DSA switches that can make use of more than one
> tagging protocol. It may be useful in dsa_loop in the future too, to
> perform offline testing of various taggers, or for changing between dsa
> and edsa on Marvell switches, if that is desirable.
> 
> In terms of implementation, drivers can now move their tagging protocol
> configuration outside of .setup/.teardown, and into .set_tag_protocol
> and .del_tag_protocol. The calling order is:
> 
> .setup -> [.set_tag_protocol -> .del_tag_protocol]+ -> .teardown
> 
> There was one more contract between the DSA framework and drivers, which
> is that if a CPU port needs to account for the tagger overhead in its
> MTU configuration, it must do that privately. Which means it needs the
> information about what tagger it uses before we call its MTU
> configuration function. That promise is still held.
> 
> Writing to the tagging sysfs will first tear down the tagging protocol
> for all switches in the tree attached to that DSA master, then will
> attempt setup with the new tagger.
> 
> Writing will fail quickly with -EOPNOTSUPP for drivers that don't
> support .set_tag_protocol, since that is checked during the deletion
> phase. It is assumed that all switches within the same DSA tree use the
> same driver, and therefore either all have .set_tag_protocol implemented,
> or none do.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

We talked about it over IRC and I like the approach you have taken, few
comments below:

[snip]

> +static int dsa_switch_tag_proto_del(struct dsa_switch *ds,
> +				    struct dsa_notifier_tag_proto_info *info)
> +{
> +	int err = 0, port;
> +
> +	for (port = 0; port < ds->num_ports; port++) {
> +		if (!dsa_switch_tag_proto_match(ds, port, info))
> +			continue;
> +
> +		/* Check early if we can replace it, so we don't delete it
> +		 * for nothing and leave the switch dangling.
> +		 */
> +		if (!ds->ops->set_tag_protocol) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}

This can be moved out of the loop.

> +
> +		/* The delete method is optional, just the setter
> +		 * is mandatory
> +		 */
> +		if (ds->ops->del_tag_protocol)
> +			ds->ops->del_tag_protocol(ds, port,
> +						  info->tag_ops->proto);
> +	}
> +
> +	return err;
> +}
> +
> +static int dsa_switch_tag_proto_set(struct dsa_switch *ds,
> +				    struct dsa_notifier_tag_proto_info *info)
> +{
> +	bool proto_changed = false;
> +	int port, err;
> +
> +	for (port = 0; port < ds->num_ports; port++) {
> +		struct dsa_port *cpu_dp = dsa_to_port(ds, port);
> +
> +		if (!dsa_switch_tag_proto_match(ds, port, info))
> +			continue;
> +
> +		err = ds->ops->set_tag_protocol(ds, cpu_dp->index,
> +						info->tag_ops->proto);
> +		if (err)
> +			return err;

Don't you need to test for ds->ops->set_tag_protocol to be implemented
before calling it? Similar comment to earlier, can we do an early check
for the operation being supported outside of the loop?
-- 
Florian