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 <tob...@waldekranz.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
> 

Reply via email to