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

Hi Florian,

On Wed, Jan 20, 2021 at 07:53:41PM -0800, Florian Fainelli wrote:

quoted

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

quoted

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