| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210415165420.pb5mgxyzhezqnvh5@skbuf>
Date: Thu, 15 Apr 2021 19:54:20 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
vivien.didelot@...il.com, f.fainelli@...il.com,
netdev@...r.kernel.org, robh+dt@...nel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v2 net-next 4/5] net: dsa: Allow default tag protocol to
be overridden from DT
On Thu, Apr 15, 2021 at 11:26:09AM +0200, Tobias Waldekranz wrote:
> Some combinations of tag protocols and Ethernet controllers are
> incompatible, and it is hard for the driver to keep track of these.
>
> Therefore, allow the device tree author (typically the board vendor)
> to inform the driver of this fact by selecting an alternate protocol
> that is known to work.
>
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> ---
> include/net/dsa.h | 5 +++
> net/dsa/dsa2.c | 95 ++++++++++++++++++++++++++++++++++++++---------
> 2 files changed, 83 insertions(+), 17 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 1259b0f40684..2b25fe1ad5b7 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -149,6 +149,11 @@ struct dsa_switch_tree {
> /* Tagging protocol operations */
> const struct dsa_device_ops *tag_ops;
>
> + /* Default tagging protocol preferred by the switches in this
> + * tree.
> + */
> + enum dsa_tag_protocol default_proto;
> +
> /*
> * Configuration data for the platform device that owns
> * this dsa switch tree instance.
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index d7c22e3a1fbf..80dbf8b6bf8f 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -668,6 +668,35 @@ static const struct devlink_ops dsa_devlink_ops = {
> .sb_occ_tc_port_bind_get = dsa_devlink_sb_occ_tc_port_bind_get,
> };
>
> +static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
> +{
> + const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
> + struct dsa_switch_tree *dst = ds->dst;
> + int port, err;
> +
> + if (tag_ops->proto == dst->default_proto)
> + return 0;
> +
> + if (!ds->ops->change_tag_protocol) {
> + dev_err(ds->dev, "Tag protocol cannot be modified\n");
> + return -EINVAL;
> + }
We validated this already here:
if (ds->ops->change_tag_protocol) {
tag_ops = dsa_find_tagger_by_name(user_protocol);
} else {
dev_err(ds->dev, "Tag protocol cannot be modified\n");
return -EINVAL;
}
> +
> + for (port = 0; port < ds->num_ports; port++) {
> + if (!dsa_is_cpu_port(ds, port))
> + continue;
> +
> + err = ds->ops->change_tag_protocol(ds, port, tag_ops->proto);
> + if (err) {
> + dev_err(ds->dev, "Tag protocol \"%s\" is not supported\n",
> + tag_ops->name);
Maybe instead of saying "is not supported", you can say
"Changing the tag protocol to \"%s\" returned %pe", tag_ops->name, ERR_PTR(err)
which is a bit more informative.
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int dsa_switch_setup(struct dsa_switch *ds)
> {
> struct dsa_devlink_priv *dl_priv;
> @@ -718,6 +747,10 @@ static int dsa_switch_setup(struct dsa_switch *ds)
> if (err < 0)
> goto unregister_notifier;
>
> + err = dsa_switch_setup_tag_protocol(ds);
> + if (err)
> + goto teardown;
> +
> devlink_params_publish(ds->devlink);
>
> if (!ds->slave_mii_bus && ds->ops->phy_read) {
> @@ -1068,34 +1101,60 @@ static enum dsa_tag_protocol dsa_get_tag_protocol(struct dsa_port *dp,
> return ds->ops->get_tag_protocol(ds, dp->index, tag_protocol);
> }
>
> -static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master)
> +static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
> + const char *user_protocol)
> {
> struct dsa_switch *ds = dp->ds;
> struct dsa_switch_tree *dst = ds->dst;
> const struct dsa_device_ops *tag_ops;
> - enum dsa_tag_protocol tag_protocol;
> + enum dsa_tag_protocol default_proto;
> +
> + /* Find out which protocol the switch would prefer. */
> + default_proto = dsa_get_tag_protocol(dp, master);
> + if (dst->default_proto) {
> + if (dst->default_proto != default_proto) {
> + dev_err(ds->dev,
> + "A DSA switch tree can have only one tagging protocol\n");
> + return -EINVAL;
> + }
> + } else {
> + dst->default_proto = default_proto;
> + }
> +
> + /* See if the user wants to override that preference. */
> + if (user_protocol) {
> + if (ds->ops->change_tag_protocol) {
> + tag_ops = dsa_find_tagger_by_name(user_protocol);
> + } else {
> + dev_err(ds->dev, "Tag protocol cannot be modified\n");
> + return -EINVAL;
> + }
Your choice, but how about:
if (!ds->ops->change_tag_protocol) {
dev_err(ds->dev, "Tag protocol cannot be modified\n");
return -EINVAL;
}
tag_ops = dsa_find_tagger_by_name(user_protocol);
> + } else {
> + tag_ops = dsa_tag_driver_get(default_proto);
> + }
> +
> + if (IS_ERR(tag_ops)) {
> + if (PTR_ERR(tag_ops) == -ENOPROTOOPT)
> + return -EPROBE_DEFER;
> +
> + dev_warn(ds->dev, "No tagger for this switch\n");
> + return PTR_ERR(tag_ops);
> + }
>
> - tag_protocol = dsa_get_tag_protocol(dp, master);
> if (dst->tag_ops) {
> - if (dst->tag_ops->proto != tag_protocol) {
> + if (dst->tag_ops != tag_ops) {
> dev_err(ds->dev,
> "A DSA switch tree can have only one tagging protocol\n");
> +
> + dsa_tag_driver_put(tag_ops);
> return -EINVAL;
> }
> +
> /* In the case of multiple CPU ports per switch, the tagging
> - * protocol is still reference-counted only per switch tree, so
> - * nothing to do here.
> + * protocol is still reference-counted only per switch tree.
> */
> + dsa_tag_driver_put(tag_ops);
> } else {
> - tag_ops = dsa_tag_driver_get(tag_protocol);
> - if (IS_ERR(tag_ops)) {
> - if (PTR_ERR(tag_ops) == -ENOPROTOOPT)
> - return -EPROBE_DEFER;
> - dev_warn(ds->dev, "No tagger for this switch\n");
> - dp->master = NULL;
> - return PTR_ERR(tag_ops);
> - }
> -
> dst->tag_ops = tag_ops;
So at the end of dsa_port_parse_cpu, we have a dst->tag_ops which is
temporarily out of sync with the driver. We call dsa_port_set_tag_protocol()
with the new tagging protocol _before_ we call ds->ops->change_tag_protocol.
But as opposed to dsa_switch_change_tag_proto(), if ds->ops->change_tag_protocol
fails from the probe path, we treat it as a catastrophic error. So at
the end there is no risk of having anything out of sync I believe.
Maybe you should write this down in a comment? The logic is pretty
convoluted and hard to follow.
> }
>
> @@ -1117,12 +1176,14 @@ static int dsa_port_parse_of(struct dsa_port *dp, struct device_node *dn)
>
> if (ethernet) {
> struct net_device *master;
> + const char *user_protocol;
>
> master = of_find_net_device_by_node(ethernet);
> if (!master)
> return -EPROBE_DEFER;
>
> - return dsa_port_parse_cpu(dp, master);
> + user_protocol = of_get_property(dn, "dsa,tag-protocol", NULL);
> + return dsa_port_parse_cpu(dp, master, user_protocol);
> }
>
> if (link)
> @@ -1234,7 +1295,7 @@ static int dsa_port_parse(struct dsa_port *dp, const char *name,
>
> dev_put(master);
>
> - return dsa_port_parse_cpu(dp, master);
> + return dsa_port_parse_cpu(dp, master, NULL);
> }
>
> if (!strcmp(name, "dsa"))
> --
> 2.25.1
>
Powered by blists - more mailing lists