Re: [PATCH v7 net-next 08/11] net: dsa: allow changing the tag protocol via the "tagging" device attribute
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-01-28 01:32:40
On Tue, 26 Jan 2021 00:03:30 +0200 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>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>quoted hunk ↗ jump to hunk
+const struct dsa_device_ops *dsa_find_tagger_by_name(const char *buf) +{ + const struct dsa_device_ops *ops = NULL; + struct dsa_tag_driver *dsa_tag_driver; + + mutex_lock(&dsa_tag_drivers_lock); + list_for_each_entry(dsa_tag_driver, &dsa_tag_drivers_list, list) { + const struct dsa_device_ops *tmp = dsa_tag_driver->ops; + + if (!sysfs_streq(buf, tmp->name)) + continue; + + ops = tmp; + break; + } + mutex_unlock(&dsa_tag_drivers_lock);
What's protecting from the tag driver unloading at this very moment? Some nit picks below if you need to respin.
quoted hunk ↗ jump to hunk
+ return ops; +} +
quoted hunk ↗ jump to hunk
+/* Since the dsa/tagging sysfs device attribute is per master, the assumption + * is that all DSA switches within a tree share the same tagger, otherwise + * they would have formed disjoint trees (different "dsa,member" values). + */ +int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst, + struct net_device *master, + const struct dsa_device_ops *tag_ops, + const struct dsa_device_ops *old_tag_ops) +{ + struct dsa_notifier_tag_proto_info info; + struct dsa_port *dp; + int err = -EBUSY; + + if (!rtnl_trylock()) + return restart_syscall(); + + /* At the moment we don't allow changing the tag protocol under + * traffic. The rtnl_mutex also happens to serialize concurrent + * attempts to change the tagging protocol. If we ever lift the IFF_UP + * restriction, there needs to be another mutex which serializes this. + */ + if (master->flags & IFF_UP) + goto out_unlock; + + list_for_each_entry(dp, &dst->ports, list) { + if (!dsa_is_user_port(dp->ds, dp->index)) + continue; + + if (dp->slave->flags & IFF_UP) + goto out_unlock; + } + + info.tag_ops = old_tag_ops; + err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_DEL, &info); + if (err) + goto out_unlock; + + info.tag_ops = tag_ops; + err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_SET, &info);
Not sure I should bother you about this or not, but it looks like Ocelot does allocations on SET, so there is a chance of not being able to return to the previous config, leaving things broken. There's quite a few examples where we use REPLACE instead of set, so that a careful driver can prep its resources before it kills the previous config. Although that's not perfect either because we'd rather have as much of that logic in the core as possible. What are your thoughts?
quoted hunk ↗ jump to hunk
+ if (err) + goto out_unwind_tagger; + + dst->tag_ops = tag_ops; + + rtnl_unlock(); + + return 0; + +out_unwind_tagger: + info.tag_ops = old_tag_ops; + dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_SET, &info); +out_unlock: + rtnl_unlock(); + return err; +}
quoted hunk ↗ jump to hunk
+static bool dsa_switch_tag_proto_match(struct dsa_switch *ds, int port, + struct dsa_notifier_tag_proto_info *info) +{ + if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)) + return true; + + return false;
return dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ?
quoted hunk ↗ jump to hunk
+} + +static int dsa_switch_tag_proto_del(struct dsa_switch *ds, + struct dsa_notifier_tag_proto_info *info) +{ + int port; + + /* 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) + return -EOPNOTSUPP; + + /* The delete method is optional, just the setter is mandatory */ + if (!ds->ops->del_tag_protocol) + return 0; + + ASSERT_RTNL(); + + for (port = 0; port < ds->num_ports; port++) { + if (dsa_switch_tag_proto_match(ds, port, info)) { + ds->ops->del_tag_protocol(ds, port, + info->tag_ops->proto);
invert condition, save indentation
quoted hunk ↗ jump to hunk
+ } + } + + return 0; +}