[PATCH ethtool] ethtool: support combinations of FEC modes

[PATCH ethtool] ethtool: support combinations of FEC modes

[Edward Cree]

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(-)

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

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[John W. Linville]

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

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[Michal Kubecek]

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

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[Andrew Lunn]

> 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

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[Michal Kubecek]

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

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[Edward Cree]

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

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[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

[RFC PATCH ethtool] ethtool: better syntax for combinations of FEC modes

[Edward Cree]

Instead of commas, just have them as separate argvs.

The parsing state machine might look heavyweight, but it makes it easy to add
 more parameters later and distinguish parameter names from encoding names.

Suggested-by: Michal Kubecek <mkubecek@suse.cz>
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 ethtool.8.in   |  6 +++---
 ethtool.c      | 63 ++++++++++++++++------------------------------------------
 test-cmdline.c | 10 +++++-----
 3 files changed, 25 insertions(+), 54 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index 414eaa1..7ea2cc0 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -390,7 +390,7 @@ ethtool \- query or control network driver and hardware settings
 .B ethtool \-\-set\-fec
 .I devname
 .B encoding
-.BR auto | off | rs | baser [ , ...]
+.BR auto | off | rs | baser \ [...]
 .
 .\" Adjust lines (i.e. full justification) and hyphenate.
 .ad
@@ -1120,11 +1120,11 @@ 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
-.BR encoding\ auto | off | rs | baser [ , ...]
+.BR encoding\ auto | off | rs | baser \ [...]
 
 Sets the FEC encoding for the device.  Combinations of options are specified as
 e.g.
-.B auto,rs
+.B encoding auto rs
 ; the semantics of such combinations vary between drivers.
 .TS
 nokeep;
diff --git a/ethtool.c b/ethtool.c
index 9997930..2f7e96b 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4979,39 +4979,6 @@ static int fecmode_str_to_type(const char *str)
 	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 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;
-}
-
 static int do_gfec(struct cmd_context *ctx)
 {
 	struct ethtool_fecparam feccmd = { 0 };
@@ -5041,22 +5008,26 @@ static int do_gfec(struct cmd_context *ctx)
 
 static int do_sfec(struct cmd_context *ctx)
 {
-	char *fecmode_str = NULL;
+	enum { ARG_NONE, ARG_ENCODING } state = ARG_NONE;
 	struct ethtool_fecparam feccmd;
-	struct cmdline_info cmdline_fec[] = {
-		{ "encoding", CMDL_STR,  &fecmode_str,  &feccmd.fec},
-	};
-	int changed;
-	int fecmode;
-	int rv;
+	int fecmode = 0, newmode;
+	int rv, i;
 
-	parse_generic_cmdline(ctx, &changed, cmdline_fec,
-			      ARRAY_SIZE(cmdline_fec));
-
-	if (!fecmode_str)
+	for (i = 0; i < ctx->argc; i++) {
+		if (!strcmp(ctx->argp[i], "encoding")) {
+			state = ARG_ENCODING;
+			continue;
+		}
+		if (state == ARG_ENCODING) {
+			newmode = fecmode_str_to_type(ctx->argp[i]);
+			if (!newmode)
+				exit_bad_args();
+			fecmode |= newmode;
+			continue;
+		}
 		exit_bad_args();
+	}
 
-	fecmode = parse_fecmode(fecmode_str);
 	if (!fecmode)
 		exit_bad_args();
 
@@ -5265,7 +5236,7 @@ static const struct option {
 	  "		[ all ]\n"},
 	{ "--show-fec", 1, do_gfec, "Show FEC settings"},
 	{ "--set-fec", 1, do_sfec, "Set FEC settings",
-	  "		[ encoding auto|off|rs|baser ]\n"},
+	  "		[ encoding auto|off|rs|baser [...]]\n"},
 	{ "-h|--help", 0, show_usage, "Show this help" },
 	{ "--version", 0, do_version, "Show version number" },
 	{}
diff --git a/test-cmdline.c b/test-cmdline.c
index 9c51cca..84630a5 100644
--- a/test-cmdline.c
+++ b/test-cmdline.c
@@ -268,12 +268,12 @@ static struct test_case {
 	{ 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," },
+	{ 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 encoding auto foo" },
+	{ 1, "--set-fec devname encoding none" },
 	{ 1, "--set-fec devname auto" },
 	/* can't test --set-priv-flags yet */
 	{ 0, "-h" },

Re: [RFC PATCH ethtool] ethtool: better syntax for combinations of FEC modes

[Michal Kubecek]

On Wed, Sep 19, 2018 at 05:06:25PM +0100, Edward Cree wrote:
> Instead of commas, just have them as separate argvs.
> 
> The parsing state machine might look heavyweight, but it makes it easy to add
>  more parameters later and distinguish parameter names from encoding names.
> 
> Suggested-by: Michal Kubecek <mkubecek@suse.cz>
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---

Looks good, thank you.

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>


>  ethtool.8.in   |  6 +++---
>  ethtool.c      | 63 ++++++++++++++++------------------------------------------
>  test-cmdline.c | 10 +++++-----
>  3 files changed, 25 insertions(+), 54 deletions(-)
> 
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 414eaa1..7ea2cc0 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -390,7 +390,7 @@ ethtool \- query or control network driver and hardware settings
>  .B ethtool \-\-set\-fec
>  .I devname
>  .B encoding
> -.BR auto | off | rs | baser [ , ...]
> +.BR auto | off | rs | baser \ [...]
>  .
>  .\" Adjust lines (i.e. full justification) and hyphenate.
>  .ad
> @@ -1120,11 +1120,11 @@ 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
> -.BR encoding\ auto | off | rs | baser [ , ...]
> +.BR encoding\ auto | off | rs | baser \ [...]
>  
>  Sets the FEC encoding for the device.  Combinations of options are specified as
>  e.g.
> -.B auto,rs
> +.B encoding auto rs
>  ; the semantics of such combinations vary between drivers.
>  .TS
>  nokeep;
> diff --git a/ethtool.c b/ethtool.c
> index 9997930..2f7e96b 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -4979,39 +4979,6 @@ static int fecmode_str_to_type(const char *str)
>  	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 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;
> -}
> -
>  static int do_gfec(struct cmd_context *ctx)
>  {
>  	struct ethtool_fecparam feccmd = { 0 };
> @@ -5041,22 +5008,26 @@ static int do_gfec(struct cmd_context *ctx)
>  
>  static int do_sfec(struct cmd_context *ctx)
>  {
> -	char *fecmode_str = NULL;
> +	enum { ARG_NONE, ARG_ENCODING } state = ARG_NONE;
>  	struct ethtool_fecparam feccmd;
> -	struct cmdline_info cmdline_fec[] = {
> -		{ "encoding", CMDL_STR,  &fecmode_str,  &feccmd.fec},
> -	};
> -	int changed;
> -	int fecmode;
> -	int rv;
> +	int fecmode = 0, newmode;
> +	int rv, i;
>  
> -	parse_generic_cmdline(ctx, &changed, cmdline_fec,
> -			      ARRAY_SIZE(cmdline_fec));
> -
> -	if (!fecmode_str)
> +	for (i = 0; i < ctx->argc; i++) {
> +		if (!strcmp(ctx->argp[i], "encoding")) {
> +			state = ARG_ENCODING;
> +			continue;
> +		}
> +		if (state == ARG_ENCODING) {
> +			newmode = fecmode_str_to_type(ctx->argp[i]);
> +			if (!newmode)
> +				exit_bad_args();
> +			fecmode |= newmode;
> +			continue;
> +		}
>  		exit_bad_args();
> +	}
>  
> -	fecmode = parse_fecmode(fecmode_str);
>  	if (!fecmode)
>  		exit_bad_args();
>  
> @@ -5265,7 +5236,7 @@ static const struct option {
>  	  "		[ all ]\n"},
>  	{ "--show-fec", 1, do_gfec, "Show FEC settings"},
>  	{ "--set-fec", 1, do_sfec, "Set FEC settings",
> -	  "		[ encoding auto|off|rs|baser ]\n"},
> +	  "		[ encoding auto|off|rs|baser [...]]\n"},
>  	{ "-h|--help", 0, show_usage, "Show this help" },
>  	{ "--version", 0, do_version, "Show version number" },
>  	{}
> diff --git a/test-cmdline.c b/test-cmdline.c
> index 9c51cca..84630a5 100644
> --- a/test-cmdline.c
> +++ b/test-cmdline.c
> @@ -268,12 +268,12 @@ static struct test_case {
>  	{ 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," },
> +	{ 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 encoding auto foo" },
> +	{ 1, "--set-fec devname encoding none" },
>  	{ 1, "--set-fec devname auto" },
>  	/* can't test --set-priv-flags yet */
>  	{ 0, "-h" },

Re: [RFC PATCH ethtool] ethtool: better syntax for combinations of FEC modes

[John W. Linville]

Is this patch still RFC?

On Wed, Sep 19, 2018 at 05:06:25PM +0100, Edward Cree wrote:
> Instead of commas, just have them as separate argvs.
> 
> The parsing state machine might look heavyweight, but it makes it easy to add
>  more parameters later and distinguish parameter names from encoding names.
> 
> Suggested-by: Michal Kubecek <mkubecek@suse.cz>
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
>  ethtool.8.in   |  6 +++---
>  ethtool.c      | 63 ++++++++++++++++------------------------------------------
>  test-cmdline.c | 10 +++++-----
>  3 files changed, 25 insertions(+), 54 deletions(-)
> 
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 414eaa1..7ea2cc0 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -390,7 +390,7 @@ ethtool \- query or control network driver and hardware settings
>  .B ethtool \-\-set\-fec
>  .I devname
>  .B encoding
> -.BR auto | off | rs | baser [ , ...]
> +.BR auto | off | rs | baser \ [...]
>  .
>  .\" Adjust lines (i.e. full justification) and hyphenate.
>  .ad
> @@ -1120,11 +1120,11 @@ 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
> -.BR encoding\ auto | off | rs | baser [ , ...]
> +.BR encoding\ auto | off | rs | baser \ [...]
>  
>  Sets the FEC encoding for the device.  Combinations of options are specified as
>  e.g.
> -.B auto,rs
> +.B encoding auto rs
>  ; the semantics of such combinations vary between drivers.
>  .TS
>  nokeep;
> diff --git a/ethtool.c b/ethtool.c
> index 9997930..2f7e96b 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -4979,39 +4979,6 @@ static int fecmode_str_to_type(const char *str)
>  	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 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;
> -}
> -
>  static int do_gfec(struct cmd_context *ctx)
>  {
>  	struct ethtool_fecparam feccmd = { 0 };
> @@ -5041,22 +5008,26 @@ static int do_gfec(struct cmd_context *ctx)
>  
>  static int do_sfec(struct cmd_context *ctx)
>  {
> -	char *fecmode_str = NULL;
> +	enum { ARG_NONE, ARG_ENCODING } state = ARG_NONE;
>  	struct ethtool_fecparam feccmd;
> -	struct cmdline_info cmdline_fec[] = {
> -		{ "encoding", CMDL_STR,  &fecmode_str,  &feccmd.fec},
> -	};
> -	int changed;
> -	int fecmode;
> -	int rv;
> +	int fecmode = 0, newmode;
> +	int rv, i;
>  
> -	parse_generic_cmdline(ctx, &changed, cmdline_fec,
> -			      ARRAY_SIZE(cmdline_fec));
> -
> -	if (!fecmode_str)
> +	for (i = 0; i < ctx->argc; i++) {
> +		if (!strcmp(ctx->argp[i], "encoding")) {
> +			state = ARG_ENCODING;
> +			continue;
> +		}
> +		if (state == ARG_ENCODING) {
> +			newmode = fecmode_str_to_type(ctx->argp[i]);
> +			if (!newmode)
> +				exit_bad_args();
> +			fecmode |= newmode;
> +			continue;
> +		}
>  		exit_bad_args();
> +	}
>  
> -	fecmode = parse_fecmode(fecmode_str);
>  	if (!fecmode)
>  		exit_bad_args();
>  
> @@ -5265,7 +5236,7 @@ static const struct option {
>  	  "		[ all ]\n"},
>  	{ "--show-fec", 1, do_gfec, "Show FEC settings"},
>  	{ "--set-fec", 1, do_sfec, "Set FEC settings",
> -	  "		[ encoding auto|off|rs|baser ]\n"},
> +	  "		[ encoding auto|off|rs|baser [...]]\n"},
>  	{ "-h|--help", 0, show_usage, "Show this help" },
>  	{ "--version", 0, do_version, "Show version number" },
>  	{}
> diff --git a/test-cmdline.c b/test-cmdline.c
> index 9c51cca..84630a5 100644
> --- a/test-cmdline.c
> +++ b/test-cmdline.c
> @@ -268,12 +268,12 @@ static struct test_case {
>  	{ 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," },
> +	{ 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 encoding auto foo" },
> +	{ 1, "--set-fec devname encoding none" },
>  	{ 1, "--set-fec devname auto" },
>  	/* can't test --set-priv-flags yet */
>  	{ 0, "-h" },
> 

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [RFC PATCH ethtool] ethtool: better syntax for combinations of FEC modes

[John W. Linville]

Ping?

On Mon, Oct 01, 2018 at 02:59:10PM -0400, John W. Linville wrote:
> Is this patch still RFC?
> 
> On Wed, Sep 19, 2018 at 05:06:25PM +0100, Edward Cree wrote:
> > Instead of commas, just have them as separate argvs.
> > 
> > The parsing state machine might look heavyweight, but it makes it easy to add
> >  more parameters later and distinguish parameter names from encoding names.
> > 
> > Suggested-by: Michal Kubecek <mkubecek@suse.cz>
> > Signed-off-by: Edward Cree <ecree@solarflare.com>
> > ---
> >  ethtool.8.in   |  6 +++---
> >  ethtool.c      | 63 ++++++++++++++++------------------------------------------
> >  test-cmdline.c | 10 +++++-----
> >  3 files changed, 25 insertions(+), 54 deletions(-)
> > 
> > diff --git a/ethtool.8.in b/ethtool.8.in
> > index 414eaa1..7ea2cc0 100644
> > --- a/ethtool.8.in
> > +++ b/ethtool.8.in
> > @@ -390,7 +390,7 @@ ethtool \- query or control network driver and hardware settings
> >  .B ethtool \-\-set\-fec
> >  .I devname
> >  .B encoding
> > -.BR auto | off | rs | baser [ , ...]
> > +.BR auto | off | rs | baser \ [...]
> >  .
> >  .\" Adjust lines (i.e. full justification) and hyphenate.
> >  .ad
> > @@ -1120,11 +1120,11 @@ 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
> > -.BR encoding\ auto | off | rs | baser [ , ...]
> > +.BR encoding\ auto | off | rs | baser \ [...]
> >  
> >  Sets the FEC encoding for the device.  Combinations of options are specified as
> >  e.g.
> > -.B auto,rs
> > +.B encoding auto rs
> >  ; the semantics of such combinations vary between drivers.
> >  .TS
> >  nokeep;
> > diff --git a/ethtool.c b/ethtool.c
> > index 9997930..2f7e96b 100644
> > --- a/ethtool.c
> > +++ b/ethtool.c
> > @@ -4979,39 +4979,6 @@ static int fecmode_str_to_type(const char *str)
> >  	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 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;
> > -}
> > -
> >  static int do_gfec(struct cmd_context *ctx)
> >  {
> >  	struct ethtool_fecparam feccmd = { 0 };
> > @@ -5041,22 +5008,26 @@ static int do_gfec(struct cmd_context *ctx)
> >  
> >  static int do_sfec(struct cmd_context *ctx)
> >  {
> > -	char *fecmode_str = NULL;
> > +	enum { ARG_NONE, ARG_ENCODING } state = ARG_NONE;
> >  	struct ethtool_fecparam feccmd;
> > -	struct cmdline_info cmdline_fec[] = {
> > -		{ "encoding", CMDL_STR,  &fecmode_str,  &feccmd.fec},
> > -	};
> > -	int changed;
> > -	int fecmode;
> > -	int rv;
> > +	int fecmode = 0, newmode;
> > +	int rv, i;
> >  
> > -	parse_generic_cmdline(ctx, &changed, cmdline_fec,
> > -			      ARRAY_SIZE(cmdline_fec));
> > -
> > -	if (!fecmode_str)
> > +	for (i = 0; i < ctx->argc; i++) {
> > +		if (!strcmp(ctx->argp[i], "encoding")) {
> > +			state = ARG_ENCODING;
> > +			continue;
> > +		}
> > +		if (state == ARG_ENCODING) {
> > +			newmode = fecmode_str_to_type(ctx->argp[i]);
> > +			if (!newmode)
> > +				exit_bad_args();
> > +			fecmode |= newmode;
> > +			continue;
> > +		}
> >  		exit_bad_args();
> > +	}
> >  
> > -	fecmode = parse_fecmode(fecmode_str);
> >  	if (!fecmode)
> >  		exit_bad_args();
> >  
> > @@ -5265,7 +5236,7 @@ static const struct option {
> >  	  "		[ all ]\n"},
> >  	{ "--show-fec", 1, do_gfec, "Show FEC settings"},
> >  	{ "--set-fec", 1, do_sfec, "Set FEC settings",
> > -	  "		[ encoding auto|off|rs|baser ]\n"},
> > +	  "		[ encoding auto|off|rs|baser [...]]\n"},
> >  	{ "-h|--help", 0, show_usage, "Show this help" },
> >  	{ "--version", 0, do_version, "Show version number" },
> >  	{}
> > diff --git a/test-cmdline.c b/test-cmdline.c
> > index 9c51cca..84630a5 100644
> > --- a/test-cmdline.c
> > +++ b/test-cmdline.c
> > @@ -268,12 +268,12 @@ static struct test_case {
> >  	{ 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," },
> > +	{ 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 encoding auto foo" },
> > +	{ 1, "--set-fec devname encoding none" },
> >  	{ 1, "--set-fec devname auto" },
> >  	/* can't test --set-priv-flags yet */
> >  	{ 0, "-h" },
> > 
> 
> -- 
> John W. Linville		Someday the world will need a hero, and you
> linville@tuxdriver.com			might be all we have.  Be ready.
> 

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [RFC PATCH ethtool] ethtool: better syntax for combinations of FEC modes

[Michal Kubecek]

On Thu, Oct 04, 2018 at 10:08:14AM -0400, John W. Linville wrote:
> Ping?
> 
> On Mon, Oct 01, 2018 at 02:59:10PM -0400, John W. Linville wrote:
> > Is this patch still RFC?

As far as I'm concerned, it can be taken as it is.

Michal Kubecek

> > 
> > On Wed, Sep 19, 2018 at 05:06:25PM +0100, Edward Cree wrote:
> > > Instead of commas, just have them as separate argvs.
> > > 
> > > The parsing state machine might look heavyweight, but it makes it easy to add
> > >  more parameters later and distinguish parameter names from encoding names.
> > > 
> > > Suggested-by: Michal Kubecek <mkubecek@suse.cz>
> > > Signed-off-by: Edward Cree <ecree@solarflare.com>
> > > ---
> > >  ethtool.8.in   |  6 +++---
> > >  ethtool.c      | 63 ++++++++++++++++------------------------------------------
> > >  test-cmdline.c | 10 +++++-----
> > >  3 files changed, 25 insertions(+), 54 deletions(-)
> > > 
> > > diff --git a/ethtool.8.in b/ethtool.8.in
> > > index 414eaa1..7ea2cc0 100644
> > > --- a/ethtool.8.in
> > > +++ b/ethtool.8.in
> > > @@ -390,7 +390,7 @@ ethtool \- query or control network driver and hardware settings
> > >  .B ethtool \-\-set\-fec
> > >  .I devname
> > >  .B encoding
> > > -.BR auto | off | rs | baser [ , ...]
> > > +.BR auto | off | rs | baser \ [...]
> > >  .
> > >  .\" Adjust lines (i.e. full justification) and hyphenate.
> > >  .ad
> > > @@ -1120,11 +1120,11 @@ 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
> > > -.BR encoding\ auto | off | rs | baser [ , ...]
> > > +.BR encoding\ auto | off | rs | baser \ [...]
> > >  
> > >  Sets the FEC encoding for the device.  Combinations of options are specified as
> > >  e.g.
> > > -.B auto,rs
> > > +.B encoding auto rs
> > >  ; the semantics of such combinations vary between drivers.
> > >  .TS
> > >  nokeep;
> > > diff --git a/ethtool.c b/ethtool.c
> > > index 9997930..2f7e96b 100644
> > > --- a/ethtool.c
> > > +++ b/ethtool.c
> > > @@ -4979,39 +4979,6 @@ static int fecmode_str_to_type(const char *str)
> > >  	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 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;
> > > -}
> > > -
> > >  static int do_gfec(struct cmd_context *ctx)
> > >  {
> > >  	struct ethtool_fecparam feccmd = { 0 };
> > > @@ -5041,22 +5008,26 @@ static int do_gfec(struct cmd_context *ctx)
> > >  
> > >  static int do_sfec(struct cmd_context *ctx)
> > >  {
> > > -	char *fecmode_str = NULL;
> > > +	enum { ARG_NONE, ARG_ENCODING } state = ARG_NONE;
> > >  	struct ethtool_fecparam feccmd;
> > > -	struct cmdline_info cmdline_fec[] = {
> > > -		{ "encoding", CMDL_STR,  &fecmode_str,  &feccmd.fec},
> > > -	};
> > > -	int changed;
> > > -	int fecmode;
> > > -	int rv;
> > > +	int fecmode = 0, newmode;
> > > +	int rv, i;
> > >  
> > > -	parse_generic_cmdline(ctx, &changed, cmdline_fec,
> > > -			      ARRAY_SIZE(cmdline_fec));
> > > -
> > > -	if (!fecmode_str)
> > > +	for (i = 0; i < ctx->argc; i++) {
> > > +		if (!strcmp(ctx->argp[i], "encoding")) {
> > > +			state = ARG_ENCODING;
> > > +			continue;
> > > +		}
> > > +		if (state == ARG_ENCODING) {
> > > +			newmode = fecmode_str_to_type(ctx->argp[i]);
> > > +			if (!newmode)
> > > +				exit_bad_args();
> > > +			fecmode |= newmode;
> > > +			continue;
> > > +		}
> > >  		exit_bad_args();
> > > +	}
> > >  
> > > -	fecmode = parse_fecmode(fecmode_str);
> > >  	if (!fecmode)
> > >  		exit_bad_args();
> > >  
> > > @@ -5265,7 +5236,7 @@ static const struct option {
> > >  	  "		[ all ]\n"},
> > >  	{ "--show-fec", 1, do_gfec, "Show FEC settings"},
> > >  	{ "--set-fec", 1, do_sfec, "Set FEC settings",
> > > -	  "		[ encoding auto|off|rs|baser ]\n"},
> > > +	  "		[ encoding auto|off|rs|baser [...]]\n"},
> > >  	{ "-h|--help", 0, show_usage, "Show this help" },
> > >  	{ "--version", 0, do_version, "Show version number" },
> > >  	{}
> > > diff --git a/test-cmdline.c b/test-cmdline.c
> > > index 9c51cca..84630a5 100644
> > > --- a/test-cmdline.c
> > > +++ b/test-cmdline.c
> > > @@ -268,12 +268,12 @@ static struct test_case {
> > >  	{ 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," },
> > > +	{ 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 encoding auto foo" },
> > > +	{ 1, "--set-fec devname encoding none" },
> > >  	{ 1, "--set-fec devname auto" },
> > >  	/* can't test --set-priv-flags yet */
> > >  	{ 0, "-h" },
> > > 
> > 
> > -- 
> > John W. Linville		Someday the world will need a hero, and you
> > linville@tuxdriver.com			might be all we have.  Be ready.
> > 
> 
> -- 
> John W. Linville		Someday the world will need a hero, and you
> linville@tuxdriver.com			might be all we have.  Be ready.

Re: [RFC PATCH ethtool] ethtool: better syntax for combinations of FEC modes

[Edward Cree]

On 04/10/18 15:08, John W. Linville wrote:
> Ping?
>
> On Mon, Oct 01, 2018 at 02:59:10PM -0400, John W. Linville wrote:
>> Is this patch still RFC?
Feel free to de-RFC and apply it.

Re: [RFC PATCH ethtool] ethtool: better syntax for combinations of FEC modes

[John W. Linville]

On Thu, Oct 04, 2018 at 05:06:29PM +0100, Edward Cree wrote:
> On 04/10/18 15:08, John W. Linville wrote:
> > Ping?
> >
> > On Mon, Oct 01, 2018 at 02:59:10PM -0400, John W. Linville wrote:
> >> Is this patch still RFC?
> Feel free to de-RFC and apply it.

Great -- queued for next release.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[Ariel Almog]

> --- 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

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[Edward Cree]

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

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[Andrew Lunn]

> 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

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[Edward Cree]

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

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[Andrew Lunn]

> 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

Re: [PATCH ethtool] ethtool: support combinations of FEC modes

[Edward Cree]

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™.