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

[Vladimir Oltean <olteanv@gmail.com>]

Hi Florian,

On Wed, Jan 20, 2021 at 07:53:41PM -0800, Florian Fainelli wrote:
> > +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.

Thanks a lot for reviewing.
Yes, you are right, this can be moved out. It is a left-over from using
dsa_broadcast in the previous version. It is not the only left-over: the
info->tree_index is now redundant because we are notifying within a
single DSA switch tree.

> > +
> > +		/* 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?

My assumption was that I had already added the check in tag_proto_del.
There are no code paths that call one but not the other. I can add the
check here too.

There's one more thing I would change: the dsa_switch_tag_proto_match.
Right now I am matching only on the CPU port, but if I take a look at
the mv88e6xxx driver:

static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port)
{
	if (dsa_is_dsa_port(chip->ds, port))
		return mv88e6xxx_set_port_mode_dsa(chip, port);

	if (dsa_is_user_port(chip->ds, port))
		return mv88e6xxx_set_port_mode_normal(chip, port);

	/* Setup CPU port mode depending on its supported tag format */
	if (chip->info->tag_protocol == DSA_TAG_PROTO_DSA)
		return mv88e6xxx_set_port_mode_dsa(chip, port);

	if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA)
		return mv88e6xxx_set_port_mode_edsa(chip, port);

	return -EINVAL;
}

DSA links call the same function as CPU ports configured for
DSA_TAG_PROTO_DSA. So to cater to Marvell switches too, and to ease a
potential conversion to this API, I could add dsa_is_dsa_port to the
matching function too.