[ethtool] ethtool: support combinations of FEC modes

Message ID 518b8b8b-0151-1053-3798-6009044ed53a@solarflare.com
State Accepted
Delegated to: John Linville
Headers show
Series
  • [ethtool] ethtool: support combinations of FEC modes
Related show

Commit Message

Edward Cree Sept. 5, 2018, 5:54 p.m.
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(-)

Comments

John W. Linville Sept. 17, 2018, 7:52 p.m. | #1
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" },
>

Patch

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" },