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 <ec...@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" },
>
--
John W. Linville Someday the world will need a hero, and you
linvi...@tuxdriver.com might be all we have. Be ready.
