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

[Ariel Almog <arielalmogworkemails@gmail.com>]

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