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

[Michal Kubecek <mkubecek@suse.cz>]

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