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

2018-10-04 Thread 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. LinvilleSomeday the world will need a hero, and you
linvi...@tuxdriver.com  might be all we have.  Be ready.


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

2018-10-04 Thread 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

2018-10-04 Thread 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 
> > > Signed-off-by: Edward Cree 
> > > ---
> > >  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,  _str,  },
> > > - };
> > > - int changed;
> > > - int fecmode;
> > > - int rv;
> > > + int fecmode = 0, newmode;
> > > + int rv, i;
> > >  
> > > - parse_generic_cmdline(ctx, , 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 

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

2018-10-04 Thread 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 
> > Signed-off-by: Edward Cree 
> > ---
> >  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,  _str,  },
> > -   };
> > -   int changed;
> > -   int fecmode;
> > -   int rv;
> > +   int fecmode = 0, newmode;
> > +   int rv, i;
> >  
> > -   parse_generic_cmdline(ctx, , 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 @@ 

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

2018-10-01 Thread 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 
> Signed-off-by: Edward Cree 
> ---
>  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,  _str,  },
> - };
> - int changed;
> - int fecmode;
> - int rv;
> + int fecmode = 0, newmode;
> + int rv, i;
>  
> - parse_generic_cmdline(ctx, , 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 

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

2018-09-20 Thread 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 
> Signed-off-by: Edward Cree 
> ---

Looks good, thank you.

Reviewed-by: Michal Kubecek 


>  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,  _str,  },
> - };
> - int changed;
> - int fecmode;
> - int rv;
> + int fecmode = 0, newmode;
> + int rv, i;
>  
> - parse_generic_cmdline(ctx, , 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," },
> -

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

2018-09-19 Thread 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 
Signed-off-by: Edward Cree 
---
 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,  _str,  },
-   };
-   int changed;
-   int fecmode;
-   int rv;
+   int fecmode = 0, newmode;
+   int rv, i;
 
-   parse_generic_cmdline(ctx, , 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,