| Message ID | 20210326105648.2492411-3-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: Allow default tag protocol to be overridden from DT | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
Hi Tobias, On Fri, Mar 26, 2021 at 11:56:47AM +0100, Tobias Waldekranz wrote: > } else { > - dst->tag_ops = dsa_tag_driver_get(tag_protocol); > - if (IS_ERR(dst->tag_ops)) { > - if (PTR_ERR(dst->tag_ops) == -ENOPROTOOPT) > - return -EPROBE_DEFER; > - dev_warn(ds->dev, "No tagger for this switch\n"); > - dp->master = NULL; > - return PTR_ERR(dst->tag_ops); > - } > + dst->tag_ops = tag_ops; > } This will conflict with George's bug fix for 'net', am I right? https://patchwork.kernel.org/project/netdevbpf/patch/20210322202650.45776-1-george.mccollister@gmail.com/ Would you mind resending after David merges 'net' into 'net-next'? This process usually looks like commit d489ded1a369 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net"). However, during this kernel development cycle, I have seen no merge of 'net' into 'net-next' since commit 05a59d79793d ("Merge git://git.kernel.org:/pub/scm/linux/kernel/git/netdev/net"), but that comes directly from Linus Torvalds' v5.12-rc2. Nonetheless, at some point (and sooner rather than later, I think), David or Jakub should merge the two trees. I would prefer to do it this way because the merge is going to be a bit messy otherwise, and I might want to cherry-pick these patches to some trees and it would be nice if the history was linear. Thanks!
On Fri, Mar 26, 2021 at 14:57, Vladimir Oltean <olteanv@gmail.com> wrote: > Hi Tobias, > > On Fri, Mar 26, 2021 at 11:56:47AM +0100, Tobias Waldekranz wrote: >> } else { >> - dst->tag_ops = dsa_tag_driver_get(tag_protocol); >> - if (IS_ERR(dst->tag_ops)) { >> - if (PTR_ERR(dst->tag_ops) == -ENOPROTOOPT) >> - return -EPROBE_DEFER; >> - dev_warn(ds->dev, "No tagger for this switch\n"); >> - dp->master = NULL; >> - return PTR_ERR(dst->tag_ops); >> - } >> + dst->tag_ops = tag_ops; >> } > > This will conflict with George's bug fix for 'net', am I right? > https://patchwork.kernel.org/project/netdevbpf/patch/20210322202650.45776-1-george.mccollister@gmail.com/ Yes; this version also fixes George's problem I think, as we do not assign dst->tag_ops until we know it is good, but it will not merge cleanly. > Would you mind resending after David merges 'net' into 'net-next'? Sure thing. Should I then call that v2 or a resend of v1? The patches will not be identical, so v2 I guess? > This process usually looks like commit d489ded1a369 ("Merge > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net"). However, > during this kernel development cycle, I have seen no merge of 'net' into > 'net-next' since commit 05a59d79793d ("Merge > git://git.kernel.org:/pub/scm/linux/kernel/git/netdev/net"), but that > comes directly from Linus Torvalds' v5.12-rc2. > > Nonetheless, at some point (and sooner rather than later, I think), > David or Jakub should merge the two trees. I would prefer to do it this > way because the merge is going to be a bit messy otherwise, and I might > want to cherry-pick these patches to some trees and it would be nice if > the history was linear. > > Thanks!
> +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; > + } > + > + for (port = 0; port < ds->num_ports; port++) { > + if (!(dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))) > + continue; dsa_is_dsa_port() is interesting. Do we care about the tagging protocol on DSA ports? We never see that traffic? > + > + 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); > + return err; > + } > + } > + > + return 0; > +} > + > -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; > - enum dsa_tag_protocol tag_protocol; > + const struct dsa_device_ops *tag_ops; > + 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 protovol\n"); > + return -EINVAL; > + } > + } else { > + dst->default_proto = default_proto; > + } > + > + /* See if the user wants to override that preference. */ > + if (user_protocol && ds->ops->change_tag_protocol) { > + tag_ops = dsa_find_tagger_by_name(user_protocol); > + } else { > + if (user_protocol) > + dev_warn(ds->dev, > + "Tag protocol cannot be modified, using default\n"); I would probably error out here. I don't think it is a good idea to ignore what DT says. We also potentially have forward compatibility problems. Somebody cut/pastes a DT fragment including an invalid override. But the driver does not support it, so it just gives this warning and keeps going. Sometime in the future, change support is added, it then becomes a real error, and the driver stops probing. Andrew
On Sun, Mar 28, 2021 at 05:52:43PM +0200, Andrew Lunn wrote: > > +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; > > + } > > + > > + for (port = 0; port < ds->num_ports; port++) { > > + if (!(dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))) > > + continue; > > dsa_is_dsa_port() is interesting. Do we care about the tagging > protocol on DSA ports? We never see that traffic? I believe this comes from me (see dsa_switch_tag_proto_match). I did not take into consideration at that time the fact that Marvell switches can translate between DSA and EDSA. So I assumed that every switch in the tree needs a notification about the tagging protocol, not just the top-most one.
On Mon, Mar 29, 2021 at 12:53:09AM +0300, Vladimir Oltean wrote: > On Sun, Mar 28, 2021 at 05:52:43PM +0200, Andrew Lunn wrote: > > > +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; > > > + } > > > + > > > + for (port = 0; port < ds->num_ports; port++) { > > > + if (!(dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))) > > > + continue; > > > > dsa_is_dsa_port() is interesting. Do we care about the tagging > > protocol on DSA ports? We never see that traffic? > > I believe this comes from me (see dsa_switch_tag_proto_match). I did not > take into consideration at that time the fact that Marvell switches can > translate between DSA and EDSA. So I assumed that every switch in the > tree needs a notification about the tagging protocol, not just the > top-most one. Hi Vladimir static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port) { if (dsa_is_dsa_port(chip->ds, port)) return mv88e6xxx_set_port_mode_dsa(chip, port); So DSA ports, the ports connecting two switches together, are hard coded to use DSA. if (dsa_is_user_port(chip->ds, port)) return mv88e6xxx_set_port_mode_normal(chip, port); /* Setup CPU port mode depending on its supported tag format */ if (chip->info->tag_protocol == DSA_TAG_PROTO_DSA) return mv88e6xxx_set_port_mode_dsa(chip, port); if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA) return mv88e6xxx_set_port_mode_edsa(chip, port); CPU ports can be configured to DSA or EDSA. The switches seem happy to translate between DSA and EDSA as needed. Andrew
On Mon, Mar 29, 2021 at 12:04:39AM +0200, Andrew Lunn wrote: > On Mon, Mar 29, 2021 at 12:53:09AM +0300, Vladimir Oltean wrote: > > On Sun, Mar 28, 2021 at 05:52:43PM +0200, Andrew Lunn wrote: > > > > +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; > > > > + } > > > > + > > > > + for (port = 0; port < ds->num_ports; port++) { > > > > + if (!(dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))) > > > > + continue; > > > > > > dsa_is_dsa_port() is interesting. Do we care about the tagging > > > protocol on DSA ports? We never see that traffic? > > > > I believe this comes from me (see dsa_switch_tag_proto_match). I did not > > take into consideration at that time the fact that Marvell switches can > > translate between DSA and EDSA. So I assumed that every switch in the > > tree needs a notification about the tagging protocol, not just the > > top-most one. > > Hi Vladimir > > static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port) > { > if (dsa_is_dsa_port(chip->ds, port)) > return mv88e6xxx_set_port_mode_dsa(chip, port); > > So DSA ports, the ports connecting two switches together, are hard > coded to use DSA. > > if (dsa_is_user_port(chip->ds, port)) > return mv88e6xxx_set_port_mode_normal(chip, port); > > /* Setup CPU port mode depending on its supported tag format */ > if (chip->info->tag_protocol == DSA_TAG_PROTO_DSA) > return mv88e6xxx_set_port_mode_dsa(chip, port); > > if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA) > return mv88e6xxx_set_port_mode_edsa(chip, port); > > CPU ports can be configured to DSA or EDSA. > > The switches seem happy to translate between DSA and EDSA as needed. I agree, and this is a particular feature of Marvell, not something that can be said in general. However, I have nothing against deleting the dsa_is_dsa_port checks from dsa_switch_tag_proto_match and from Tobias' patch, since nobody needs them at the moment.
On Fri, Mar 26, 2021 at 02:57:20PM +0200, Vladimir Oltean wrote: > Hi Tobias, > > On Fri, Mar 26, 2021 at 11:56:47AM +0100, Tobias Waldekranz wrote: > > } else { > > - dst->tag_ops = dsa_tag_driver_get(tag_protocol); > > - if (IS_ERR(dst->tag_ops)) { > > - if (PTR_ERR(dst->tag_ops) == -ENOPROTOOPT) > > - return -EPROBE_DEFER; > > - dev_warn(ds->dev, "No tagger for this switch\n"); > > - dp->master = NULL; > > - return PTR_ERR(dst->tag_ops); > > - } > > + dst->tag_ops = tag_ops; > > } > > This will conflict with George's bug fix for 'net', am I right? > https://patchwork.kernel.org/project/netdevbpf/patch/20210322202650.45776-1-george.mccollister@gmail.com/ > > Would you mind resending after David merges 'net' into 'net-next'? > > This process usually looks like commit d489ded1a369 ("Merge > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net"). However, > during this kernel development cycle, I have seen no merge of 'net' into > 'net-next' since commit 05a59d79793d ("Merge > git://git.kernel.org:/pub/scm/linux/kernel/git/netdev/net"), but that > comes directly from Linus Torvalds' v5.12-rc2. > > Nonetheless, at some point (and sooner rather than later, I think), > David or Jakub should merge the two trees. I would prefer to do it this > way because the merge is going to be a bit messy otherwise, and I might > want to cherry-pick these patches to some trees and it would be nice if > the history was linear. > > Thanks! Tobias, I think you can safely resend now, I see George's change is in net-next: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/dsa/dsa2.c#n1084
diff --git a/include/net/dsa.h b/include/net/dsa.h index 57b2c49f72f4..2385ba317888 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 4d4956ed303b..52afe70f75af 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; + } + + for (port = 0; port < ds->num_ports; port++) { + if (!(dsa_is_dsa_port(ds, port) || 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); + 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) { @@ -1062,32 +1095,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; - enum dsa_tag_protocol tag_protocol; + const struct dsa_device_ops *tag_ops; + 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 protovol\n"); + return -EINVAL; + } + } else { + dst->default_proto = default_proto; + } + + /* See if the user wants to override that preference. */ + if (user_protocol && ds->ops->change_tag_protocol) { + tag_ops = dsa_find_tagger_by_name(user_protocol); + } else { + if (user_protocol) + dev_warn(ds->dev, + "Tag protocol cannot be modified, using default\n"); + + 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 { - dst->tag_ops = dsa_tag_driver_get(tag_protocol); - if (IS_ERR(dst->tag_ops)) { - if (PTR_ERR(dst->tag_ops) == -ENOPROTOOPT) - return -EPROBE_DEFER; - dev_warn(ds->dev, "No tagger for this switch\n"); - dp->master = NULL; - return PTR_ERR(dst->tag_ops); - } + dst->tag_ops = tag_ops; } dp->master = master; @@ -1108,12 +1169,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) @@ -1225,7 +1288,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"))
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@waldekranz.com> --- This deviates from the advise given by Andrew in that we store the switches' default protocol on the tree rather than the override value. I chose this because you want to make sure that the tagger (default or overwritten) is available at probe time, and since that means we actually load it, it made more sense to keep it loaded. Then we just check during setup if we are running a different tagger than what the driver would expect. Somewhat related: since we know the default now, users could easily revert to it via sysfs: `echo >/sys/class/eth0/dsa/tagging`. Not sure if that would be useful. include/net/dsa.h | 5 +++ net/dsa/dsa2.c | 95 +++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 84 insertions(+), 16 deletions(-)