| Message ID | 20221125105304.3012153-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next] Revert "net: stmmac: use sysfs_streq() instead of strncmp()" | expand |
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@nxp.com> Ah the infamous string handling in C... Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> 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 >
On Fri, 25 Nov 2022 12:58:23 +0100 Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote: > 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@nxp.com> > > Ah the infamous string handling in C... > > Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > 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 > > Configuring via module options is bad idea. If you have to do it don't roll your own key/value parsing. If the driver just used regular module_param() for this it wouldn't have this crap.
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Fri, 25 Nov 2022 12:53:04 +0200 you 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: > > [...] Here is the summary with links: - [net-next] Revert "net: stmmac: use sysfs_streq() instead of strncmp()" https://git.kernel.org/netdev/net-next/c/469d258d9e11 You are awesome, thank you!
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; }
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@nxp.com> --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)