Messages in this thread | | | Date | Fri, 25 Nov 2022 10:44:01 -0800 | From | Stephen Hemminger <> | Subject | Re: [PATCH net-next] Revert "net: stmmac: use sysfs_streq() instead of strncmp()" |
| |
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.
| |