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

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