Re: [PATCH v2 net-next 4/5] net: dsa: Allow default tag protocol to be overridden from DT

[Vladimir Oltean <olteanv@gmail.com>]

On Thu, Apr 15, 2021 at 11:26:09AM +0200, Tobias Waldekranz wrote:
> Some combinations of tag protocols and Ethernet controllers are
> incompatible, and it is hard for the driver to keep track of these.
> 
> Therefore, allow the device tree author (typically the board vendor)
> to inform the driver of this fact by selecting an alternate protocol
> that is known to work.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  include/net/dsa.h |  5 +++
>  net/dsa/dsa2.c    | 95 ++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 83 insertions(+), 17 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 1259b0f40684..2b25fe1ad5b7 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -149,6 +149,11 @@ struct dsa_switch_tree {
>  	/* Tagging protocol operations */
>  	const struct dsa_device_ops *tag_ops;
>  
> +	/* Default tagging protocol preferred by the switches in this
> +	 * tree.
> +	 */
> +	enum dsa_tag_protocol default_proto;
> +
>  	/*
>  	 * Configuration data for the platform device that owns
>  	 * this dsa switch tree instance.
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index d7c22e3a1fbf..80dbf8b6bf8f 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -668,6 +668,35 @@ static const struct devlink_ops dsa_devlink_ops = {
>  	.sb_occ_tc_port_bind_get	= dsa_devlink_sb_occ_tc_port_bind_get,
>  };
>  
> +static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
> +{
> +	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
> +	struct dsa_switch_tree *dst = ds->dst;
> +	int port, err;
> +
> +	if (tag_ops->proto == dst->default_proto)
> +		return 0;
> +
> +	if (!ds->ops->change_tag_protocol) {
> +		dev_err(ds->dev, "Tag protocol cannot be modified\n");
> +		return -EINVAL;
> +	}

We validated this already here:

		if (ds->ops->change_tag_protocol) {
			tag_ops = dsa_find_tagger_by_name(user_protocol);
		} else {
			dev_err(ds->dev, "Tag protocol cannot be modified\n");
			return -EINVAL;
		}

> +
> +	for (port = 0; port < ds->num_ports; port++) {
> +		if (!dsa_is_cpu_port(ds, port))
> +			continue;
> +
> +		err = ds->ops->change_tag_protocol(ds, port, tag_ops->proto);
> +		if (err) {
> +			dev_err(ds->dev, "Tag protocol \"%s\" is not supported\n",
> +				tag_ops->name);

Maybe instead of saying "is not supported", you can say
"Changing the tag protocol to \"%s\" returned %pe", tag_ops->name, ERR_PTR(err)
which is a bit more informative.

> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int dsa_switch_setup(struct dsa_switch *ds)
>  {
>  	struct dsa_devlink_priv *dl_priv;
> @@ -718,6 +747,10 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>  	if (err < 0)
>  		goto unregister_notifier;
>  
> +	err = dsa_switch_setup_tag_protocol(ds);
> +	if (err)
> +		goto teardown;
> +
>  	devlink_params_publish(ds->devlink);
>  
>  	if (!ds->slave_mii_bus && ds->ops->phy_read) {
> @@ -1068,34 +1101,60 @@ static enum dsa_tag_protocol dsa_get_tag_protocol(struct dsa_port *dp,
>  	return ds->ops->get_tag_protocol(ds, dp->index, tag_protocol);
>  }
>  
> -static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master)
> +static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
> +			      const char *user_protocol)
>  {
>  	struct dsa_switch *ds = dp->ds;
>  	struct dsa_switch_tree *dst = ds->dst;
>  	const struct dsa_device_ops *tag_ops;
> -	enum dsa_tag_protocol tag_protocol;
> +	enum dsa_tag_protocol default_proto;
> +
> +	/* Find out which protocol the switch would prefer. */
> +	default_proto = dsa_get_tag_protocol(dp, master);
> +	if (dst->default_proto) {
> +		if (dst->default_proto != default_proto) {
> +			dev_err(ds->dev,
> +				"A DSA switch tree can have only one tagging protocol\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		dst->default_proto = default_proto;
> +	}
> +
> +	/* See if the user wants to override that preference. */
> +	if (user_protocol) {
> +		if (ds->ops->change_tag_protocol) {
> +			tag_ops = dsa_find_tagger_by_name(user_protocol);
> +		} else {
> +			dev_err(ds->dev, "Tag protocol cannot be modified\n");
> +			return -EINVAL;
> +		}

Your choice, but how about:

		if (!ds->ops->change_tag_protocol) {
			dev_err(ds->dev, "Tag protocol cannot be modified\n");
			return -EINVAL;
		}

		tag_ops = dsa_find_tagger_by_name(user_protocol);

> +	} else {
> +		tag_ops = dsa_tag_driver_get(default_proto);
> +	}
> +
> +	if (IS_ERR(tag_ops)) {
> +		if (PTR_ERR(tag_ops) == -ENOPROTOOPT)
> +			return -EPROBE_DEFER;
> +
> +		dev_warn(ds->dev, "No tagger for this switch\n");
> +		return PTR_ERR(tag_ops);
> +	}
>  
> -	tag_protocol = dsa_get_tag_protocol(dp, master);
>  	if (dst->tag_ops) {
> -		if (dst->tag_ops->proto != tag_protocol) {
> +		if (dst->tag_ops != tag_ops) {
>  			dev_err(ds->dev,
>  				"A DSA switch tree can have only one tagging protocol\n");
> +
> +			dsa_tag_driver_put(tag_ops);
>  			return -EINVAL;
>  		}
> +
>  		/* In the case of multiple CPU ports per switch, the tagging
> -		 * protocol is still reference-counted only per switch tree, so
> -		 * nothing to do here.
> +		 * protocol is still reference-counted only per switch tree.
>  		 */
> +		dsa_tag_driver_put(tag_ops);
>  	} else {
> -		tag_ops = dsa_tag_driver_get(tag_protocol);
> -		if (IS_ERR(tag_ops)) {
> -			if (PTR_ERR(tag_ops) == -ENOPROTOOPT)
> -				return -EPROBE_DEFER;
> -			dev_warn(ds->dev, "No tagger for this switch\n");
> -			dp->master = NULL;
> -			return PTR_ERR(tag_ops);
> -		}
> -
>  		dst->tag_ops = tag_ops;

So at the end of dsa_port_parse_cpu, we have a dst->tag_ops which is
temporarily out of sync with the driver. We call dsa_port_set_tag_protocol()
with the new tagging protocol _before_ we call ds->ops->change_tag_protocol.
But as opposed to dsa_switch_change_tag_proto(), if ds->ops->change_tag_protocol
fails from the probe path, we treat it as a catastrophic error. So at
the end there is no risk of having anything out of sync I believe.

Maybe you should write this down in a comment? The logic is pretty
convoluted and hard to follow.

>  	}
>  
> @@ -1117,12 +1176,14 @@ static int dsa_port_parse_of(struct dsa_port *dp, struct device_node *dn)
>  
>  	if (ethernet) {
>  		struct net_device *master;
> +		const char *user_protocol;
>  
>  		master = of_find_net_device_by_node(ethernet);
>  		if (!master)
>  			return -EPROBE_DEFER;
>  
> -		return dsa_port_parse_cpu(dp, master);
> +		user_protocol = of_get_property(dn, "dsa,tag-protocol", NULL);
> +		return dsa_port_parse_cpu(dp, master, user_protocol);
>  	}
>  
>  	if (link)
> @@ -1234,7 +1295,7 @@ static int dsa_port_parse(struct dsa_port *dp, const char *name,
>  
>  		dev_put(master);
>  
> -		return dsa_port_parse_cpu(dp, master);
> +		return dsa_port_parse_cpu(dp, master, NULL);
>  	}
>  
>  	if (!strcmp(name, "dsa"))
> -- 
> 2.25.1
>