| Message ID | 518b8b8b-0151-1053-3798-6009044ed53a@solarflare.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | John Linville |
| Headers | show |
| Series | [ethtool] ethtool: support combinations of FEC modes | expand |
On Wed, Sep 05, 2018 at 06:54:57PM +0100, Edward Cree wrote: > Of the three drivers that currently support FEC configuration, two (sfc > and cxgb4[vf]) accept configurations with more than one bit set in the > feccmd.fec bitmask. (The precise semantics of these combinations vary.) > Thus, this patch adds the ability to specify such combinations through a > comma-separated list of FEC modes in the 'encoding' argument on the > command line. > > Also adds --set-fec tests to test-cmdline.c, and corrects the man page > (the encoding argument is not optional) while updating it. > > Signed-off-by: Edward Cree <ecree@solarflare.com> > --- > I've CCed the maintainers of the other drivers (cxgb4, nfp) that support > --set-fec, in case they have opinions on this. > I'm not totally happy with the man page changebar; it might be clearer > just to leave the comma-less version in the syntax synopsis and only > mention the commas in the running-text. LGTM -- queued for next release...thanks! John > ethtool.8.in | 11 ++++++++--- > ethtool.c | 50 +++++++++++++++++++++++++++++++++++++++----------- > test-cmdline.c | 9 +++++++++ > 3 files changed, 56 insertions(+), 14 deletions(-) > > diff --git a/ethtool.8.in b/ethtool.8.in > index c8a902a..414eaa1 100644 > --- a/ethtool.8.in > +++ b/ethtool.8.in > @@ -389,7 +389,8 @@ ethtool \- query or control network driver and hardware settings > .HP > .B ethtool \-\-set\-fec > .I devname > -.B4 encoding auto off rs baser > +.B encoding > +.BR auto | off | rs | baser [ , ...] > . > .\" Adjust lines (i.e. full justification) and hyphenate. > .ad > @@ -1119,8 +1120,12 @@ current FEC mode, the driver or firmware must take the link down > administratively and report the problem in the system logs for users to correct. > .RS 4 > .TP > -.A4 encoding auto off rs baser > -Sets the FEC encoding for the device. > +.BR encoding\ auto | off | rs | baser [ , ...] > + > +Sets the FEC encoding for the device. Combinations of options are specified as > +e.g. > +.B auto,rs > +; the semantics of such combinations vary between drivers. > .TS > nokeep; > lB l. > diff --git a/ethtool.c b/ethtool.c > index e8b7703..9997930 100644 > --- a/ethtool.c > +++ b/ethtool.c > @@ -4967,20 +4967,48 @@ static int do_set_phy_tunable(struct cmd_context *ctx) > > static int fecmode_str_to_type(const char *str) > { > + if (!strcasecmp(str, "auto")) > + return ETHTOOL_FEC_AUTO; > + if (!strcasecmp(str, "off")) > + return ETHTOOL_FEC_OFF; > + if (!strcasecmp(str, "rs")) > + return ETHTOOL_FEC_RS; > + if (!strcasecmp(str, "baser")) > + return ETHTOOL_FEC_BASER; > + > + return 0; > +} > + > +/* Takes a comma-separated list of FEC modes, returns the bitwise OR of their > + * corresponding ETHTOOL_FEC_* constants. > + * Accepts repetitions (e.g. 'auto,auto') and trailing comma (e.g. 'off,'). > + */ > +static int parse_fecmode(const char *str) > +{ > int fecmode = 0; > + char buf[6]; > > if (!str) > - return fecmode; > - > - if (!strcasecmp(str, "auto")) > - fecmode |= ETHTOOL_FEC_AUTO; > - else if (!strcasecmp(str, "off")) > - fecmode |= ETHTOOL_FEC_OFF; > - else if (!strcasecmp(str, "rs")) > - fecmode |= ETHTOOL_FEC_RS; > - else if (!strcasecmp(str, "baser")) > - fecmode |= ETHTOOL_FEC_BASER; > + return 0; > + while (*str) { > + size_t next; > + int mode; > > + next = strcspn(str, ","); > + if (next >= 6) /* Bad mode, longest name is 5 chars */ > + return 0; > + /* Copy into temp buffer and nul-terminate */ > + memcpy(buf, str, next); > + buf[next] = 0; > + mode = fecmode_str_to_type(buf); > + if (!mode) /* Bad mode encountered */ > + return 0; > + fecmode |= mode; > + str += next; > + /* Skip over ',' (but not nul) */ > + if (*str) > + str++; > + } > return fecmode; > } > > @@ -5028,7 +5056,7 @@ static int do_sfec(struct cmd_context *ctx) > if (!fecmode_str) > exit_bad_args(); > > - fecmode = fecmode_str_to_type(fecmode_str); > + fecmode = parse_fecmode(fecmode_str); > if (!fecmode) > exit_bad_args(); > > diff --git a/test-cmdline.c b/test-cmdline.c > index a94edea..9c51cca 100644 > --- a/test-cmdline.c > +++ b/test-cmdline.c > @@ -266,6 +266,15 @@ static struct test_case { > { 0, "--set-eee devname tx-timer 42 advertise 0x4321" }, > { 1, "--set-eee devname tx-timer foo" }, > { 1, "--set-eee devname advertise foo" }, > + { 1, "--set-fec devname" }, > + { 0, "--set-fec devname encoding auto" }, > + { 0, "--set-fec devname encoding off," }, > + { 0, "--set-fec devname encoding baser,rs" }, > + { 0, "--set-fec devname encoding auto,auto," }, > + { 1, "--set-fec devname encoding foo" }, > + { 1, "--set-fec devname encoding auto,foo" }, > + { 1, "--set-fec devname encoding auto,," }, > + { 1, "--set-fec devname auto" }, > /* can't test --set-priv-flags yet */ > { 0, "-h" }, > { 0, "--help" }, >
On Wed, Sep 05, 2018 at 06:54:57PM +0100, Edward Cree wrote: > Of the three drivers that currently support FEC configuration, two (sfc > and cxgb4[vf]) accept configurations with more than one bit set in the > feccmd.fec bitmask. (The precise semantics of these combinations vary.) > Thus, this patch adds the ability to specify such combinations through a > comma-separated list of FEC modes in the 'encoding' argument on the > command line. > > Also adds --set-fec tests to test-cmdline.c, and corrects the man page > (the encoding argument is not optional) while updating it. > > Signed-off-by: Edward Cree <ecree@solarflare.com> ... > +/* Takes a comma-separated list of FEC modes, returns the bitwise OR of their > + * corresponding ETHTOOL_FEC_* constants. > + * Accepts repetitions (e.g. 'auto,auto') and trailing comma (e.g. 'off,'). > + */ > +static int parse_fecmode(const char *str) > +{ > int fecmode = 0; > + char buf[6]; > > if (!str) > - return fecmode; > - > - if (!strcasecmp(str, "auto")) > - fecmode |= ETHTOOL_FEC_AUTO; > - else if (!strcasecmp(str, "off")) > - fecmode |= ETHTOOL_FEC_OFF; > - else if (!strcasecmp(str, "rs")) > - fecmode |= ETHTOOL_FEC_RS; > - else if (!strcasecmp(str, "baser")) > - fecmode |= ETHTOOL_FEC_BASER; > + return 0; > + while (*str) { > + size_t next; > + int mode; > > + next = strcspn(str, ","); > + if (next >= 6) /* Bad mode, longest name is 5 chars */ > + return 0; > + /* Copy into temp buffer and nul-terminate */ > + memcpy(buf, str, next); > + buf[next] = 0; > + mode = fecmode_str_to_type(buf); > + if (!mode) /* Bad mode encountered */ > + return 0; > + fecmode |= mode; > + str += next; > + /* Skip over ',' (but not nul) */ > + if (*str) > + str++; > + } > return fecmode; > } > I'm sorry I didn't notice this before the patch was accepted but as it's not in a release yet, maybe it's still not too late. Could I suggest to make the syntax consistent with other options? I mean rather than a comma separated list to use either ethtool --set-fec <dev> encoding enc1 enc2 ... (as we have for --reset) or ethtool --set-fec <dev> encoding enc1 on|off enc2 on|off ... (as we have e.g. for msglvl, -K or --set-eee)? Second option seems more appropriate to me but it would require special handling of the case when there is only one argument after "encoding" to maintain backward compatibility with already released versions. One loosely related question: how sure can we be that we are never going to need more than 32 bits for FEC encodings? Is it something completely hypothetical or is it something that could happen in the future? Michal Kubecek
> One loosely related question: how sure can we be that we are never going > to need more than 32 bits for FEC encodings? Is it something completely > hypothetical or is it something that could happen in the future? > Hi Michal Hopefully we have moved to a netlink socket by that time :-) I recently found that EEE still uses a u32 for advertise link modes. We should fix that in the netlink API. Andrew
On 19/09/18 15:41, Michal Kubecek wrote: > I'm sorry I didn't notice this before the patch was accepted but as it's > not in a release yet, maybe it's still not too late. > > Could I suggest to make the syntax consistent with other options? I didn't realise ethtool had any patterns to be consistent with ;) > I mean rather than a comma separated list to use either > > ethtool --set-fec <dev> encoding enc1 enc2 ... but yes this looks fine to me, as long as we're reasonably confident that we won't want to add new parameters (that might require determining whether enc2 is an encoding or a parameter name) in the future, because while the parsing wouldn't be impossible it might get ugly. I'll rustle up an RFC patch. -Ed
On Wed, Sep 19, 2018 at 05:33:38PM +0200, Andrew Lunn wrote: > > One loosely related question: how sure can we be that we are never going > > to need more than 32 bits for FEC encodings? Is it something completely > > hypothetical or is it something that could happen in the future? > > > Hi Michal > > Hopefully we have moved to a netlink socket by that time :-) Actually, that's why I'm asking. :-) > I recently found that EEE still uses a u32 for advertise link modes. > We should fix that in the netlink API. For EEE it felt obvious that arbitrary length bitmap is the way to go so I used it (it's still u32 in ethtool_ops but that's easier to change when needed). For FEC I wasn't sure if it wouldn't be an overkill. Michal Kubecek
On Wed, Sep 19, 2018 at 04:38:27PM +0100, Edward Cree wrote: > On 19/09/18 15:41, Michal Kubecek wrote: > > I'm sorry I didn't notice this before the patch was accepted but as it's > > not in a release yet, maybe it's still not too late. > > > > Could I suggest to make the syntax consistent with other options? > I didn't realise ethtool had any patterns to be consistent with ;) Way too many, I must say. :-) That is why I wasn't happy about adding another. > > I mean rather than a comma separated list to use either > > > > ethtool --set-fec <dev> encoding enc1 enc2 ... > but yes this looks fine to me, as long as we're reasonably confident that > we won't want to add new parameters (that might require determining > whether enc2 is an encoding or a parameter name) in the future, because > while the parsing wouldn't be impossible it might get ugly. This problem already exists for "-s ... msglvl". In the parser for the netlink series I introduced an "end of list" marker (tentatively "--") for this purpose, perhaps that could be a way. > I'll rustle up an RFC patch. Thank you. Michal Kubecek
> --- a/ethtool.c > +++ b/ethtool.c > @@ -4967,20 +4967,48 @@ static int do_set_phy_tunable(struct cmd_context > *ctx) > static int fecmode_str_to_type(const char *str) > { > + if (!strcasecmp(str, "auto")) > + return ETHTOOL_FEC_AUTO; > + if (!strcasecmp(str, "off")) > + return ETHTOOL_FEC_OFF; > + if (!strcasecmp(str, "rs")) > + return ETHTOOL_FEC_RS; > + if (!strcasecmp(str, "baser")) > + return ETHTOOL_FEC_BASER; > + > + return 0; > +} I was won > + > +/* Takes a comma-separated list of FEC modes, returns the bitwise OR of > their > + * corresponding ETHTOOL_FEC_* constants. > + * Accepts repetitions (e.g. 'auto,auto') and trailing comma (e.g. 'off,'). > + */ > +static int parse_fecmode(const char *str) > +{ > int fecmode = 0; > + char buf[6]; > if (!str) > - return fecmode; > - > - if (!strcasecmp(str, "auto")) > - fecmode |= ETHTOOL_FEC_AUTO; > - else if (!strcasecmp(str, "off")) > - fecmode |= ETHTOOL_FEC_OFF; > - else if (!strcasecmp(str, "rs")) > - fecmode |= ETHTOOL_FEC_RS; > - else if (!strcasecmp(str, "baser")) > - fecmode |= ETHTOOL_FEC_BASER; > + return 0; > + while (*str) { > + size_t next; > + int mode; > + next = strcspn(str, ","); > + if (next >= 6) /* Bad mode, longest name is 5 chars */ > + return 0; > + /* Copy into temp buffer and nul-terminate */ > + memcpy(buf, str, next); > + buf[next] = 0; > + mode = fecmode_str_to_type(buf); > + if (!mode) /* Bad mode encountered */ > + return 0; > + fecmode |= mode; > + str += next; > + /* Skip over ',' (but not nul) */ > + if (*str) > + str++; > + } > return fecmode; I would like to apologize for my late response. I find the ability to set off, auto and specific FEC mode in the same command confusing. Here are some examples 1. What is the expected result of 'off' & other FEC mode such as 'RS'? -'off'? -'RS'? -automatic selection {'off','RS'}? w/o setting of auto? 2. What is the expected result of 'off', 'RS' and 'auto'? - automatic selection from the set of {'RS','off'} - if that is the case, what is the different from 'off' and 'RS' with out 'auto'? - allowing the device to use all three modes - automatic selection {auto, rs, off}. what is the meaning of auto of auto? I think that we shall have some mutual configuration limitation : 1. if 'auto' was set, any other configuration from within the set {'off', 'RS', 'base-r'} will imply the set of configuration to be selected by auto mode i.e. 'auto', 'RS' and 'off' configuration will result with automatic selection between {'off', 'RS'} 2. if 'auto' was not set, only one configuration from within {'off', 'RS', 'base-r'} can be set (and from that, 'off' cannot be set with other configuration) Thanks Ariel Almog Mellanox technologies
On 26/09/18 09:47, Ariel Almog wrote: > I was won Truncated sentence? ("... wondering"?) > I find the ability to set off, auto and specific FEC mode in the same > command confusing. I didn't try to define semantics here since each driver currently does something slightly different. Probably the configuration space that's meaningful is different for each piece of hardware anyway. > Here are some examples > > 1. What is the expected result of 'off' & other FEC mode such as 'RS'? > -'off'? > -'RS'? > -automatic selection {'off','RS'}? w/o setting of auto? In sfc, 'off' overrides everything else. The meaning (again, in sfc) of a combination of 'auto' and a specific mode (e.g. 'rs') is "prefer the specified mode, but fall back to autoneg if it's not supported". The combination {'rs', 'baser'} (with or without 'auto') means "use the strongest FEC supported", i.e. it will attempt to negotiate FEC even if the cable & link partner don't request it (e.g. a short cable). For us, those semantics make sense (our HW has a notion of 'supported' and 'requested' bits for each FEC type for each of local-device, cable and link-partner, and uses the strongest FEC mode that's supported by everyone and requested by anyone); but if something else is a better fit for your hardware I wouldn't worry too much about the inconsistency — people using this functionality will hopefully have read the hardware's user manual... -Ed
> For us, those semantics make sense (our HW has a notion of 'supported' > and 'requested' bits for each FEC type for each of local-device, cable > and link-partner, and uses the strongest FEC mode that's supported by > everyone and requested by anyone); but if something else is a better fit > for your hardware I wouldn't worry too much about the inconsistency — > people using this functionality will hopefully have read the hardware's > user manual... I wonder how true that will be in 5 years time, about reading the manual? SFP sockets are starting to appear in consumer devices. There are some Marvell SoC reference boards with SFP and SFP+. Broadcom also have some boards with SFP. With time, SFP will move out of the data centre and comms rack and into more everyday systems. In such context, reading the manual becomes less likely. It would be nice to avoid a future inconsistent mess caused be this sentiment now. Andrew
On 28/09/18 16:39, Andrew Lunn wrote: > I wonder how true that will be in 5 years time, about reading the > manual? SFP sockets are starting to appear in consumer devices. There > are some Marvell SoC reference boards with SFP and SFP+. Broadcom also > have some boards with SFP. With time, SFP will move out of the data > centre and comms rack and into more everyday systems. In such context, > reading the manual becomes less likely. It would be nice to avoid a > future inconsistent mess caused be this sentiment now. > > Andrew I see where you're coming from, but if people start needing to manually configure FEC on their consumer devices, possibly we have bigger problems. Ethtool FEC control is for those situations where autoneg, autodetect, autoconfigure etc. don't work (e.g. owing to out-of-spec switches, or just a user wanting to disable FEC to save a few hundred nanos). I would hope that FEC won't show up in consumer gear until these kinds of problems have settled down somewhat. Perhaps we can add something to the man page saying that not only can the semantics vary from NIC to NIC, but that the semantics for a given NIC might change in the future? Then if in five years' time we know what the Right Thing™ is, we can move everyone over to that (with appropriately *loud* release-notes). I think the alternative, of finding a set of semantics that fits everyone's hardware and covers everyone's requirements, is likely to be difficult (and probably require changing the ethtool API). -Ed
> I see where you're coming from, but if people start needing to manually > configure FEC on their consumer devices, possibly we have bigger > problems. Yes, i agree with that. For the consumer market, SFPs needs to grow up and start doing full and reliable auto-neg, just like copper Ethernet. However, there is often an intermediate step after the really niche market like TOR routers. Industrial applications start using this stuff. There are a lot of planes flying today using SFPs for the inflight entertainment systems. Fibre weights less than copper. It is a somewhat specialist market, so you probably can still force them to read the hardware manual, but i think they would prefer not to. And i'm sure they are not the only industrial users. There are likely to be more industrial users than TOR users. In general, it is hard to know which APIs are going to remain Unix Wizard level, and which are going to be used by mere mortals. So ideally, we want consistency everywhere. > I think the alternative, of finding a set of semantics that fits > everyone's hardware and covers everyone's requirements, is likely to > be difficult (and probably require changing the ethtool API). Now is a good time to change the API, since we are moving to a netlink socket. Which is why these questions were asked in the first place... Andrew
On 28/09/18 17:45, Andrew Lunn wrote: > Now is a good time to change the API, since we are moving to a netlink > socket. Which is why these questions were asked in the first place... OK, well, I've posted sfc's semantics and view-from-the-hardware*; now patiently waiting for other NIC vendors to chime in so we can try to converge on something consistent. Then again, since they've been CCed since the original patch three weeks ago, we might be waiting a while :-( Regarding Ariel Almog's suggested semantics, it seems like they have the 'auto' bit just encoding 'more than one non-auto bit', which is redundant (i.e. off|rs is always off|rs|auto, whereas rs is never rs|auto). I don't see how that would be useful. -Ed * One complication I left out: we actually have _three_ pairs of sup/req bits, because we separate 'BaseR for 10G/40G/100G' from 'BaseR for 25G/50G'. I don't know the details of why our HW does this (or why 100G isn't lumped in with the other 25ers) but I think it has to do with Horrific Ethernet Spec Arcana Man Was Not Meant To Know™.
diff --git a/ethtool.8.in b/ethtool.8.in index c8a902a..414eaa1 100644 --- a/ethtool.8.in +++ b/ethtool.8.in @@ -389,7 +389,8 @@ ethtool \- query or control network driver and hardware settings .HP .B ethtool \-\-set\-fec .I devname -.B4 encoding auto off rs baser +.B encoding +.BR auto | off | rs | baser [ , ...] . .\" Adjust lines (i.e. full justification) and hyphenate. .ad @@ -1119,8 +1120,12 @@ current FEC mode, the driver or firmware must take the link down administratively and report the problem in the system logs for users to correct. .RS 4 .TP -.A4 encoding auto off rs baser -Sets the FEC encoding for the device. +.BR encoding\ auto | off | rs | baser [ , ...] + +Sets the FEC encoding for the device. Combinations of options are specified as +e.g. +.B auto,rs +; the semantics of such combinations vary between drivers. .TS nokeep; lB l. diff --git a/ethtool.c b/ethtool.c index e8b7703..9997930 100644 --- a/ethtool.c +++ b/ethtool.c @@ -4967,20 +4967,48 @@ static int do_set_phy_tunable(struct cmd_context *ctx) static int fecmode_str_to_type(const char *str) { + if (!strcasecmp(str, "auto")) + return ETHTOOL_FEC_AUTO; + if (!strcasecmp(str, "off")) + return ETHTOOL_FEC_OFF; + if (!strcasecmp(str, "rs")) + return ETHTOOL_FEC_RS; + if (!strcasecmp(str, "baser")) + return ETHTOOL_FEC_BASER; + + return 0; +} + +/* Takes a comma-separated list of FEC modes, returns the bitwise OR of their + * corresponding ETHTOOL_FEC_* constants. + * Accepts repetitions (e.g. 'auto,auto') and trailing comma (e.g. 'off,'). + */ +static int parse_fecmode(const char *str) +{ int fecmode = 0; + char buf[6]; if (!str) - return fecmode; - - if (!strcasecmp(str, "auto")) - fecmode |= ETHTOOL_FEC_AUTO; - else if (!strcasecmp(str, "off")) - fecmode |= ETHTOOL_FEC_OFF; - else if (!strcasecmp(str, "rs")) - fecmode |= ETHTOOL_FEC_RS; - else if (!strcasecmp(str, "baser")) - fecmode |= ETHTOOL_FEC_BASER; + return 0; + while (*str) { + size_t next; + int mode; + next = strcspn(str, ","); + if (next >= 6) /* Bad mode, longest name is 5 chars */ + return 0; + /* Copy into temp buffer and nul-terminate */ + memcpy(buf, str, next); + buf[next] = 0; + mode = fecmode_str_to_type(buf); + if (!mode) /* Bad mode encountered */ + return 0; + fecmode |= mode; + str += next; + /* Skip over ',' (but not nul) */ + if (*str) + str++; + } return fecmode; } @@ -5028,7 +5056,7 @@ static int do_sfec(struct cmd_context *ctx) if (!fecmode_str) exit_bad_args(); - fecmode = fecmode_str_to_type(fecmode_str); + fecmode = parse_fecmode(fecmode_str); if (!fecmode) exit_bad_args(); diff --git a/test-cmdline.c b/test-cmdline.c index a94edea..9c51cca 100644 --- a/test-cmdline.c +++ b/test-cmdline.c @@ -266,6 +266,15 @@ static struct test_case { { 0, "--set-eee devname tx-timer 42 advertise 0x4321" }, { 1, "--set-eee devname tx-timer foo" }, { 1, "--set-eee devname advertise foo" }, + { 1, "--set-fec devname" }, + { 0, "--set-fec devname encoding auto" }, + { 0, "--set-fec devname encoding off," }, + { 0, "--set-fec devname encoding baser,rs" }, + { 0, "--set-fec devname encoding auto,auto," }, + { 1, "--set-fec devname encoding foo" }, + { 1, "--set-fec devname encoding auto,foo" }, + { 1, "--set-fec devname encoding auto,," }, + { 1, "--set-fec devname auto" }, /* can't test --set-priv-flags yet */ { 0, "-h" }, { 0, "--help" },
Of the three drivers that currently support FEC configuration, two (sfc and cxgb4[vf]) accept configurations with more than one bit set in the feccmd.fec bitmask. (The precise semantics of these combinations vary.) Thus, this patch adds the ability to specify such combinations through a comma-separated list of FEC modes in the 'encoding' argument on the command line. Also adds --set-fec tests to test-cmdline.c, and corrects the man page (the encoding argument is not optional) while updating it. Signed-off-by: Edward Cree <ecree@solarflare.com> --- I've CCed the maintainers of the other drivers (cxgb4, nfp) that support --set-fec, in case they have opinions on this. I'm not totally happy with the man page changebar; it might be clearer just to leave the comma-less version in the syntax synopsis and only mention the commas in the running-text. ethtool.8.in | 11 ++++++++--- ethtool.c | 50 +++++++++++++++++++++++++++++++++++++++----------- test-cmdline.c | 9 +++++++++ 3 files changed, 56 insertions(+), 14 deletions(-)