Re: [PATCH net-next] Revert "net: stmmac: use sysfs_streq() instead of strncmp()"

[Maciej Fijalkowski]

On Fri, Nov 25, 2022 at 12:53:04PM +0200, Vladimir Oltean wrote:
> This reverts commit f72cd76b05ea1ce9258484e8127932d0ea928f22.
> This patch is so broken, it hurts. Apparently no one reviewed it and it
> passed the build testing (because the code was compiled out), but it was
> obviously never compile-tested, since it produces the following build
> error, due to an incomplete conversion where an extra argument was left,
> although the function being called was left:
> 
> stmmac_main.c: In function ‘stmmac_cmdline_opt’:
> stmmac_main.c:7586:28: error: too many arguments to function ‘sysfs_streq’
>  7586 |                 } else if (sysfs_streq(opt, "pause:", 6)) {
>       |                            ^~~~~~~~~~~
> In file included from ../include/linux/bitmap.h:11,
>                  from ../include/linux/cpumask.h:12,
>                  from ../include/linux/smp.h:13,
>                  from ../include/linux/lockdep.h:14,
>                  from ../include/linux/mutex.h:17,
>                  from ../include/linux/notifier.h:14,
>                  from ../include/linux/clk.h:14,
>                  from ../drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:17:
> ../include/linux/string.h:185:13: note: declared here
>   185 | extern bool sysfs_streq(const char *s1, const char *s2);
>       |             ^~~~~~~~~~~
> 
> What's even worse is that the patch is flat out wrong. The stmmac_cmdline_opt()
> function does not parse sysfs input, but cmdline input such as
> "stmmaceth=tc:1,pause:1". The pattern of using strsep() followed by
> strncmp() for such strings is not unique to stmmac, it can also be found
> mainly in drivers under drivers/video/fbdev/.
> 
> With strncmp("tc:", 3), the code matches on the "tc:1" token properly.
> With sysfs_streq("tc:"), it doesn't.
> 
> Fixes: f72cd76b05ea ("net: stmmac: use sysfs_streq() instead of strncmp()")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>

Ah the infamous string handling in C...

Acked-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>

Even when there would be no build error I agree that we should have kept
the code as it was.

> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1a86e66e4560..3affb7d3a005 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7565,31 +7565,31 @@ static int __init stmmac_cmdline_opt(char *str)
>  	if (!str || !*str)
>  		return 1;
>  	while ((opt = strsep(&str, ",")) != NULL) {
> -		if (sysfs_streq(opt, "debug:")) {
> +		if (!strncmp(opt, "debug:", 6)) {
>  			if (kstrtoint(opt + 6, 0, &debug))
>  				goto err;
> -		} else if (sysfs_streq(opt, "phyaddr:")) {
> +		} else if (!strncmp(opt, "phyaddr:", 8)) {
>  			if (kstrtoint(opt + 8, 0, &phyaddr))
>  				goto err;
> -		} else if (sysfs_streq(opt, "buf_sz:")) {
> +		} else if (!strncmp(opt, "buf_sz:", 7)) {
>  			if (kstrtoint(opt + 7, 0, &buf_sz))
>  				goto err;
> -		} else if (sysfs_streq(opt, "tc:")) {
> +		} else if (!strncmp(opt, "tc:", 3)) {
>  			if (kstrtoint(opt + 3, 0, &tc))
>  				goto err;
> -		} else if (sysfs_streq(opt, "watchdog:")) {
> +		} else if (!strncmp(opt, "watchdog:", 9)) {
>  			if (kstrtoint(opt + 9, 0, &watchdog))
>  				goto err;
> -		} else if (sysfs_streq(opt, "flow_ctrl:")) {
> +		} else if (!strncmp(opt, "flow_ctrl:", 10)) {
>  			if (kstrtoint(opt + 10, 0, &flow_ctrl))
>  				goto err;
> -		} else if (sysfs_streq(opt, "pause:", 6)) {
> +		} else if (!strncmp(opt, "pause:", 6)) {
>  			if (kstrtoint(opt + 6, 0, &pause))
>  				goto err;
> -		} else if (sysfs_streq(opt, "eee_timer:")) {
> +		} else if (!strncmp(opt, "eee_timer:", 10)) {
>  			if (kstrtoint(opt + 10, 0, &eee_timer))
>  				goto err;
> -		} else if (sysfs_streq(opt, "chain_mode:")) {
> +		} else if (!strncmp(opt, "chain_mode:", 11)) {
>  			if (kstrtoint(opt + 11, 0, &chain_mode))
>  				goto err;
>  		}
> -- 
> 2.34.1
>