* [PATCH net-next 0/3] net: dsa: Allow default tag protocol to be overridden from DT @ 2021-03-26 10:56 Tobias Waldekranz 2021-03-26 10:56 ` [PATCH net-next 1/3] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol Tobias Waldekranz ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Tobias Waldekranz @ 2021-03-26 10:56 UTC (permalink / raw) To: davem, kuba Cc: andrew, vivien.didelot, f.fainelli, olteanv, netdev, robh+dt, devicetree This is logically the v2 of this patch: https://lore.kernel.org/netdev/20210323102326.3677940-1-tobias@waldekranz.com/ In addition to the mv88e6xxx support to dynamically change the protocol, it is now possible to override the protocol from the device tree. This means that when a board vendor finds an incompatibility, they can specify a working protocol in the DT, and users will not have to worry about it. Some background information: In a system using an NXP T1023 SoC connected to a 6390X switch, we noticed that TO_CPU frames where not reaching the CPU. This only happened on hardware port 8. Looking at the DSA master interface (dpaa-ethernet) we could see that an Rx error counter was bumped at the same rate. The logs indicated a parser error. It just so happens that a TO_CPU coming in on device 0, port 8, will result in the first two bytes of the DSA tag being one of: 00 40 00 44 00 46 My guess was that since these values looked like 802.3 length fields, the controller's parser would signal an error if the frame length did not match what was in the header. This was later confirmed using two different workarounds provided by Vladimir. Unfortunately these either bypass or ignore the hardware parser and thus robs working combinations of the ability to do RSS and other nifty things. It was therefore decided to go with the option of a DT override. Tobias Waldekranz (3): net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol net: dsa: Allow default tag protocol to be overridden from DT dt-bindings: net: dsa: Document dsa,tag-protocol property .../devicetree/bindings/net/dsa/dsa.yaml | 7 ++ drivers/net/dsa/mv88e6xxx/chip.c | 41 +++++++- drivers/net/dsa/mv88e6xxx/chip.h | 3 + include/net/dsa.h | 5 + net/dsa/dsa2.c | 95 +++++++++++++++---- 5 files changed, 132 insertions(+), 19 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net-next 1/3] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol 2021-03-26 10:56 [PATCH net-next 0/3] net: dsa: Allow default tag protocol to be overridden from DT Tobias Waldekranz @ 2021-03-26 10:56 ` Tobias Waldekranz 2021-03-28 15:24 ` Andrew Lunn 2021-03-26 10:56 ` [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT Tobias Waldekranz 2021-03-26 10:56 ` [PATCH net-next 3/3] dt-bindings: net: dsa: Document dsa,tag-protocol property Tobias Waldekranz 2 siblings, 1 reply; 18+ messages in thread From: Tobias Waldekranz @ 2021-03-26 10:56 UTC (permalink / raw) To: davem, kuba Cc: andrew, vivien.didelot, f.fainelli, olteanv, netdev, robh+dt, devicetree All devices are capable of using regular DSA tags. Support for Ethertyped DSA tags sort into three categories: 1. No support. Older chips fall into this category. 2. Full support. Datasheet explicitly supports configuring the CPU port to receive FORWARDs with a DSA tag. 3. Undocumented support. Datasheet lists the configuration from category 2 as "reserved for future use", but does empirically behave like a category 2 device. Because there are ethernet controllers that do not handle regular DSA tags in all cases, it is sometimes preferable to rely on the undocumented behavior, as the alternative is a very crippled system. But, in those cases, make sure to log the fact that an undocumented feature has been enabled. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 41 +++++++++++++++++++++++++++++--- drivers/net/dsa/mv88e6xxx/chip.h | 3 +++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 95f07fcd4f85..e7ec883d5f6b 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -2531,10 +2531,10 @@ static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int 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) + if (chip->tag_protocol == DSA_TAG_PROTO_DSA) return mv88e6xxx_set_port_mode_dsa(chip, port); - if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA) + if (chip->tag_protocol == DSA_TAG_PROTO_EDSA) return mv88e6xxx_set_port_mode_edsa(chip, port); return -EINVAL; @@ -5564,7 +5564,39 @@ static enum dsa_tag_protocol mv88e6xxx_get_tag_protocol(struct dsa_switch *ds, { struct mv88e6xxx_chip *chip = ds->priv; - return chip->info->tag_protocol; + return chip->tag_protocol; +} + +static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds, int port, + enum dsa_tag_protocol proto) +{ + struct mv88e6xxx_chip *chip = ds->priv; + enum dsa_tag_protocol old_protocol; + int err; + + switch (proto) { + case DSA_TAG_PROTO_EDSA: + if (chip->info->tag_protocol != DSA_TAG_PROTO_EDSA) + dev_warn(chip->dev, "Relying on undocumented EDSA tagging behavior\n"); + + break; + case DSA_TAG_PROTO_DSA: + break; + default: + return -EPROTONOSUPPORT; + } + + old_protocol = chip->tag_protocol; + chip->tag_protocol = proto; + + mv88e6xxx_reg_lock(chip); + err = mv88e6xxx_setup_port_mode(chip, port); + mv88e6xxx_reg_unlock(chip); + + if (err) + chip->tag_protocol = old_protocol; + + return err; } static int mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port, @@ -6029,6 +6061,7 @@ static int mv88e6xxx_crosschip_lag_leave(struct dsa_switch *ds, int sw_index, static const struct dsa_switch_ops mv88e6xxx_switch_ops = { .get_tag_protocol = mv88e6xxx_get_tag_protocol, + .change_tag_protocol = mv88e6xxx_change_tag_protocol, .setup = mv88e6xxx_setup, .teardown = mv88e6xxx_teardown, .phylink_validate = mv88e6xxx_validate, @@ -6209,6 +6242,8 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev) if (err) goto out; + chip->tag_protocol = chip->info->tag_protocol; + mv88e6xxx_phy_init(chip); if (chip->info->ops->get_eeprom) { diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index bce6e0dc8535..96b775f3fda2 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -261,6 +261,9 @@ struct mv88e6xxx_region_priv { struct mv88e6xxx_chip { const struct mv88e6xxx_info *info; + /* Currently configured tagging protocol */ + enum dsa_tag_protocol tag_protocol; + /* The dsa_switch this private structure is related to */ struct dsa_switch *ds; -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 1/3] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol 2021-03-26 10:56 ` [PATCH net-next 1/3] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol Tobias Waldekranz @ 2021-03-28 15:24 ` Andrew Lunn 2021-04-06 9:07 ` Tobias Waldekranz 0 siblings, 1 reply; 18+ messages in thread From: Andrew Lunn @ 2021-03-28 15:24 UTC (permalink / raw) To: Tobias Waldekranz Cc: davem, kuba, vivien.didelot, f.fainelli, olteanv, netdev, robh+dt, devicetree On Fri, Mar 26, 2021 at 11:56:46AM +0100, Tobias Waldekranz wrote: > All devices are capable of using regular DSA tags. Support for > Ethertyped DSA tags sort into three categories: > > 1. No support. Older chips fall into this category. > > 2. Full support. Datasheet explicitly supports configuring the CPU > port to receive FORWARDs with a DSA tag. > > 3. Undocumented support. Datasheet lists the configuration from > category 2 as "reserved for future use", but does empirically > behave like a category 2 device. > +static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds, int port, > + enum dsa_tag_protocol proto) > +{ > + struct mv88e6xxx_chip *chip = ds->priv; > + enum dsa_tag_protocol old_protocol; > + int err; > + > + switch (proto) { > + case DSA_TAG_PROTO_EDSA: > + if (chip->info->tag_protocol != DSA_TAG_PROTO_EDSA) > + dev_warn(chip->dev, "Relying on undocumented EDSA tagging behavior\n"); > + > + break; > + case DSA_TAG_PROTO_DSA: > + break; > + default: > + return -EPROTONOSUPPORT; > + } You are handling cases 2 and 3 here, but not 1. Which makes it a bit of a foot cannon for older devices. Now that we have chip->tag_protocol, maybe we should change chip->info->tag_protocol to mean supported protocols? BIT(0) DSA BIT(1) EDSA BIT(2) Undocumented EDSA Andrew ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 1/3] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol 2021-03-28 15:24 ` Andrew Lunn @ 2021-04-06 9:07 ` Tobias Waldekranz 2021-04-06 13:30 ` Andrew Lunn 0 siblings, 1 reply; 18+ messages in thread From: Tobias Waldekranz @ 2021-04-06 9:07 UTC (permalink / raw) To: Andrew Lunn Cc: davem, kuba, vivien.didelot, f.fainelli, olteanv, netdev, robh+dt, devicetree On Sun, Mar 28, 2021 at 17:24, Andrew Lunn <andrew@lunn.ch> wrote: > On Fri, Mar 26, 2021 at 11:56:46AM +0100, Tobias Waldekranz wrote: >> All devices are capable of using regular DSA tags. Support for >> Ethertyped DSA tags sort into three categories: >> >> 1. No support. Older chips fall into this category. >> >> 2. Full support. Datasheet explicitly supports configuring the CPU >> port to receive FORWARDs with a DSA tag. >> >> 3. Undocumented support. Datasheet lists the configuration from >> category 2 as "reserved for future use", but does empirically >> behave like a category 2 device. > >> +static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds, int port, >> + enum dsa_tag_protocol proto) >> +{ >> + struct mv88e6xxx_chip *chip = ds->priv; >> + enum dsa_tag_protocol old_protocol; >> + int err; >> + >> + switch (proto) { >> + case DSA_TAG_PROTO_EDSA: >> + if (chip->info->tag_protocol != DSA_TAG_PROTO_EDSA) >> + dev_warn(chip->dev, "Relying on undocumented EDSA tagging behavior\n"); >> + >> + break; >> + case DSA_TAG_PROTO_DSA: >> + break; >> + default: >> + return -EPROTONOSUPPORT; >> + } > > You are handling cases 2 and 3 here, but not 1. Which makes it a bit > of a foot cannon for older devices. > > Now that we have chip->tag_protocol, maybe we should change > chip->info->tag_protocol to mean supported protocols? > > BIT(0) DSA > BIT(1) EDSA > BIT(2) Undocumented EDSA Since DSA is supported on all devices, perhaps we should just have: enum mv88e6xxx_edsa_support { MV88E6XXX_EDSA_UNSUPPORTED, MV88E6XXX_EDSA_UNDOCUMENTED, MV88E6XXX_EDSA_SUPPORTED, }; ? Do we also want to default to DSA on all devices unless there is a DT-property saying something else? Using EDSA does not really give you anything over bare tags anymore. You have fixed the tcpdump-issue, and the tagger drivers have been unified so there should be no risk of any regressions there either. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 1/3] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol 2021-04-06 9:07 ` Tobias Waldekranz @ 2021-04-06 13:30 ` Andrew Lunn 0 siblings, 0 replies; 18+ messages in thread From: Andrew Lunn @ 2021-04-06 13:30 UTC (permalink / raw) To: Tobias Waldekranz Cc: davem, kuba, vivien.didelot, f.fainelli, olteanv, netdev, robh+dt, devicetree > Since DSA is supported on all devices, perhaps we should just have: > > enum mv88e6xxx_edsa_support { > MV88E6XXX_EDSA_UNSUPPORTED, > MV88E6XXX_EDSA_UNDOCUMENTED, > MV88E6XXX_EDSA_SUPPORTED, > }; Yes, that is O.K. > Do we also want to default to DSA on all devices unless there is a > DT-property saying something else? Using EDSA does not really give you > anything over bare tags anymore. You have fixed the tcpdump-issue, and > the tagger drivers have been unified so there should be no risk of any > regressions there either. The regressions with be exactly what you are trying to fix here. A MAC which does not understand the DSA tag and does the wrong thing, where as currently it is using EDSA and working. So i would keep things as they are by default. Andrew ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT 2021-03-26 10:56 [PATCH net-next 0/3] net: dsa: Allow default tag protocol to be overridden from DT Tobias Waldekranz 2021-03-26 10:56 ` [PATCH net-next 1/3] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol Tobias Waldekranz @ 2021-03-26 10:56 ` Tobias Waldekranz 2021-03-26 12:57 ` Vladimir Oltean 2021-03-28 15:52 ` Andrew Lunn 2021-03-26 10:56 ` [PATCH net-next 3/3] dt-bindings: net: dsa: Document dsa,tag-protocol property Tobias Waldekranz 2 siblings, 2 replies; 18+ messages in thread From: Tobias Waldekranz @ 2021-03-26 10:56 UTC (permalink / raw) To: davem, kuba Cc: andrew, vivien.didelot, f.fainelli, olteanv, netdev, robh+dt, devicetree 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(-) 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")) -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT 2021-03-26 10:56 ` [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT Tobias Waldekranz @ 2021-03-26 12:57 ` Vladimir Oltean 2021-03-26 13:33 ` Tobias Waldekranz 2021-03-31 13:20 ` Vladimir Oltean 2021-03-28 15:52 ` Andrew Lunn 1 sibling, 2 replies; 18+ messages in thread From: Vladimir Oltean @ 2021-03-26 12:57 UTC (permalink / raw) To: Tobias Waldekranz Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, netdev, robh+dt, devicetree 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! ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT 2021-03-26 12:57 ` Vladimir Oltean @ 2021-03-26 13:33 ` Tobias Waldekranz 2021-03-31 13:20 ` Vladimir Oltean 1 sibling, 0 replies; 18+ messages in thread From: Tobias Waldekranz @ 2021-03-26 13:33 UTC (permalink / raw) To: Vladimir Oltean Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, netdev, robh+dt, devicetree 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! ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT 2021-03-26 12:57 ` Vladimir Oltean 2021-03-26 13:33 ` Tobias Waldekranz @ 2021-03-31 13:20 ` Vladimir Oltean 1 sibling, 0 replies; 18+ messages in thread From: Vladimir Oltean @ 2021-03-31 13:20 UTC (permalink / raw) To: Tobias Waldekranz Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, netdev, robh+dt, devicetree 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT 2021-03-26 10:56 ` [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT Tobias Waldekranz 2021-03-26 12:57 ` Vladimir Oltean @ 2021-03-28 15:52 ` Andrew Lunn 2021-03-28 21:53 ` Vladimir Oltean 1 sibling, 1 reply; 18+ messages in thread From: Andrew Lunn @ 2021-03-28 15:52 UTC (permalink / raw) To: Tobias Waldekranz Cc: davem, kuba, vivien.didelot, f.fainelli, olteanv, netdev, robh+dt, devicetree > +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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT 2021-03-28 15:52 ` Andrew Lunn @ 2021-03-28 21:53 ` Vladimir Oltean 2021-03-28 22:04 ` Andrew Lunn 0 siblings, 1 reply; 18+ messages in thread From: Vladimir Oltean @ 2021-03-28 21:53 UTC (permalink / raw) To: Andrew Lunn Cc: Tobias Waldekranz, davem, kuba, vivien.didelot, f.fainelli, netdev, robh+dt, devicetree 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT 2021-03-28 21:53 ` Vladimir Oltean @ 2021-03-28 22:04 ` Andrew Lunn 2021-03-29 7:15 ` Vladimir Oltean 0 siblings, 1 reply; 18+ messages in thread From: Andrew Lunn @ 2021-03-28 22:04 UTC (permalink / raw) To: Vladimir Oltean Cc: Tobias Waldekranz, davem, kuba, vivien.didelot, f.fainelli, netdev, robh+dt, devicetree 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT 2021-03-28 22:04 ` Andrew Lunn @ 2021-03-29 7:15 ` Vladimir Oltean 0 siblings, 0 replies; 18+ messages in thread From: Vladimir Oltean @ 2021-03-29 7:15 UTC (permalink / raw) To: Andrew Lunn Cc: Tobias Waldekranz, davem, kuba, vivien.didelot, f.fainelli, netdev, robh+dt, devicetree 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net-next 3/3] dt-bindings: net: dsa: Document dsa,tag-protocol property 2021-03-26 10:56 [PATCH net-next 0/3] net: dsa: Allow default tag protocol to be overridden from DT Tobias Waldekranz 2021-03-26 10:56 ` [PATCH net-next 1/3] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol Tobias Waldekranz 2021-03-26 10:56 ` [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT Tobias Waldekranz @ 2021-03-26 10:56 ` Tobias Waldekranz 2021-03-27 18:13 ` Rob Herring 2 siblings, 1 reply; 18+ messages in thread From: Tobias Waldekranz @ 2021-03-26 10:56 UTC (permalink / raw) To: davem, kuba Cc: andrew, vivien.didelot, f.fainelli, olteanv, netdev, robh+dt, devicetree The 'dsa,tag-protocol' is used to force a switch tree to use a particular tag protocol, typically because the Ethernet controller that it is connected to is not compatible with the default one. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- Documentation/devicetree/bindings/net/dsa/dsa.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml index 8a3494db4d8d..5dcfab049aa2 100644 --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml @@ -70,6 +70,13 @@ patternProperties: device is what the switch port is connected to $ref: /schemas/types.yaml#/definitions/phandle + dsa,tag-protocol: + description: + Instead of the default, the switch will use this tag protocol if + possible. Useful when a device supports multiple protcols and + the default is incompatible with the Ethernet device. + $ref: /schemas/types.yaml#/definitions/string + phy-handle: true phy-mode: true -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 3/3] dt-bindings: net: dsa: Document dsa,tag-protocol property 2021-03-26 10:56 ` [PATCH net-next 3/3] dt-bindings: net: dsa: Document dsa,tag-protocol property Tobias Waldekranz @ 2021-03-27 18:13 ` Rob Herring 2021-04-06 9:52 ` Tobias Waldekranz 0 siblings, 1 reply; 18+ messages in thread From: Rob Herring @ 2021-03-27 18:13 UTC (permalink / raw) To: Tobias Waldekranz Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, olteanv, netdev, devicetree On Fri, Mar 26, 2021 at 11:56:48AM +0100, Tobias Waldekranz wrote: > The 'dsa,tag-protocol' is used to force a switch tree to use a > particular tag protocol, typically because the Ethernet controller > that it is connected to is not compatible with the default one. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > Documentation/devicetree/bindings/net/dsa/dsa.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml > index 8a3494db4d8d..5dcfab049aa2 100644 > --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml > @@ -70,6 +70,13 @@ patternProperties: > device is what the switch port is connected to > $ref: /schemas/types.yaml#/definitions/phandle > > + dsa,tag-protocol: 'dsa' is not a vendor. > + description: > + Instead of the default, the switch will use this tag protocol if > + possible. Useful when a device supports multiple protcols and > + the default is incompatible with the Ethernet device. > + $ref: /schemas/types.yaml#/definitions/string You need to define the possible strings. > + > phy-handle: true > > phy-mode: true > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 3/3] dt-bindings: net: dsa: Document dsa,tag-protocol property 2021-03-27 18:13 ` Rob Herring @ 2021-04-06 9:52 ` Tobias Waldekranz 2021-04-06 13:30 ` Andrew Lunn 0 siblings, 1 reply; 18+ messages in thread From: Tobias Waldekranz @ 2021-04-06 9:52 UTC (permalink / raw) To: Rob Herring Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, olteanv, netdev, devicetree On Sat, Mar 27, 2021 at 12:13, Rob Herring <robh@kernel.org> wrote: > On Fri, Mar 26, 2021 at 11:56:48AM +0100, Tobias Waldekranz wrote: >> The 'dsa,tag-protocol' is used to force a switch tree to use a >> particular tag protocol, typically because the Ethernet controller >> that it is connected to is not compatible with the default one. >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- >> Documentation/devicetree/bindings/net/dsa/dsa.yaml | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml >> index 8a3494db4d8d..5dcfab049aa2 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml >> @@ -70,6 +70,13 @@ patternProperties: >> device is what the switch port is connected to >> $ref: /schemas/types.yaml#/definitions/phandle >> >> + dsa,tag-protocol: > > 'dsa' is not a vendor. It is not. The property is intended to be consumed by the vendor-independent driver. So should it be linux,tag-protocol? Just tag-protocol? >> + description: >> + Instead of the default, the switch will use this tag protocol if >> + possible. Useful when a device supports multiple protcols and >> + the default is incompatible with the Ethernet device. >> + $ref: /schemas/types.yaml#/definitions/string > > You need to define the possible strings. Alright. Andrew, Vladimir: I will just list dsa and edsa for now. If it is needed on other devices, people can add them to the list after they have tested their drivers. Fair? >> + >> phy-handle: true >> >> phy-mode: true >> -- >> 2.25.1 >> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 3/3] dt-bindings: net: dsa: Document dsa,tag-protocol property 2021-04-06 9:52 ` Tobias Waldekranz @ 2021-04-06 13:30 ` Andrew Lunn 2021-04-07 23:34 ` Vladimir Oltean 0 siblings, 1 reply; 18+ messages in thread From: Andrew Lunn @ 2021-04-06 13:30 UTC (permalink / raw) To: Tobias Waldekranz Cc: Rob Herring, davem, kuba, vivien.didelot, f.fainelli, olteanv, netdev, devicetree > Andrew, Vladimir: I will just list dsa and edsa for now. If it is needed > on other devices, people can add them to the list after they have tested > their drivers. Fair? O.K. Andrew ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 3/3] dt-bindings: net: dsa: Document dsa,tag-protocol property 2021-04-06 13:30 ` Andrew Lunn @ 2021-04-07 23:34 ` Vladimir Oltean 0 siblings, 0 replies; 18+ messages in thread From: Vladimir Oltean @ 2021-04-07 23:34 UTC (permalink / raw) To: Andrew Lunn Cc: Tobias Waldekranz, Rob Herring, davem, kuba, vivien.didelot, f.fainelli, netdev, devicetree On Tue, Apr 06, 2021 at 03:30:46PM +0200, Andrew Lunn wrote: > > Andrew, Vladimir: I will just list dsa and edsa for now. If it is needed > > on other devices, people can add them to the list after they have tested > > their drivers. Fair? > > O.K. Same here. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-04-07 23:34 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-26 10:56 [PATCH net-next 0/3] net: dsa: Allow default tag protocol to be overridden from DT Tobias Waldekranz 2021-03-26 10:56 ` [PATCH net-next 1/3] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol Tobias Waldekranz 2021-03-28 15:24 ` Andrew Lunn 2021-04-06 9:07 ` Tobias Waldekranz 2021-04-06 13:30 ` Andrew Lunn 2021-03-26 10:56 ` [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT Tobias Waldekranz 2021-03-26 12:57 ` Vladimir Oltean 2021-03-26 13:33 ` Tobias Waldekranz 2021-03-31 13:20 ` Vladimir Oltean 2021-03-28 15:52 ` Andrew Lunn 2021-03-28 21:53 ` Vladimir Oltean 2021-03-28 22:04 ` Andrew Lunn 2021-03-29 7:15 ` Vladimir Oltean 2021-03-26 10:56 ` [PATCH net-next 3/3] dt-bindings: net: dsa: Document dsa,tag-protocol property Tobias Waldekranz 2021-03-27 18:13 ` Rob Herring 2021-04-06 9:52 ` Tobias Waldekranz 2021-04-06 13:30 ` Andrew Lunn 2021-04-07 23:34 ` Vladimir Oltean
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).