From mboxrd@z Thu Jan 1 00:00:00 1970 From: "John W. Linville" Subject: Re: [PATCH ethtool] ethtool: support combinations of FEC modes Date: Mon, 17 Sep 2018 15:52:25 -0400 Message-ID: <20180917195225.GA27394@tuxdriver.com> References: <518b8b8b-0151-1053-3798-6009044ed53a@solarflare.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev , Ganesh Goudar , Jakub Kicinski , Dustin Byford , Dirk van der Merwe To: Edward Cree Return-path: Received: from charlotte.tuxdriver.com ([70.61.120.58]:49379 "EHLO smtp.tuxdriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727089AbeIRB3H (ORCPT ); Mon, 17 Sep 2018 21:29:07 -0400 Content-Disposition: inline In-Reply-To: <518b8b8b-0151-1053-3798-6009044ed53a@solarflare.com> Sender: netdev-owner@vger.kernel.org List-ID: 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 > --- > 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 linville@tuxdriver.com might be all we have. Be ready.