On 1/20/2021 6:36 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.olt...@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.olt...@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
