| Message ID | 20220224033528.24640-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 50e06ddceeea263f57fe92baa677c638ecd65bb6 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sxgbe: fix return value of __setup handler | expand |
On Wed, 23 Feb 2022 19:35:28 -0800 Randy Dunlap wrote: > __setup() handlers should return 1 on success, i.e., the parameter > has been handled. A return of 0 causes the "option=value" string to be > added to init's environment strings, polluting it. Meaning early_param_on_off() also returns the wrong thing? Or that's different? > Fixes: acc18c147b22 ("net: sxgbe: add EEE(Energy Efficient Ethernet) for Samsung sxgbe") > Fixes: 1edb9ca69e8a ("net: sxgbe: add basic framework for Samsung 10Gb ethernet driver") > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > Reported-by: Igor Zhbanov <i.zhbanov@omprussia.ru> > Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0defe3@omprussia.ru > > --- linux-next-20220223.orig/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c > +++ linux-next-20220223/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c > @@ -2285,18 +2285,18 @@ static int __init sxgbe_cmdline_opt(char > char *opt; > > if (!str || !*str) > - return -EINVAL; > + return 1; > while ((opt = strsep(&str, ",")) != NULL) { > if (!strncmp(opt, "eee_timer:", 10)) { > if (kstrtoint(opt + 10, 0, &eee_timer)) > goto err; > } > } > - return 0; > + return 1; > > err: > pr_err("%s: ERROR broken module parameter conversion\n", __func__); > - return -EINVAL; > + return 1; > } > > __setup("sxgbeeth=", sxgbe_cmdline_opt); Was the option of making __setup() return void considered? Sounds like we always want to return 1 so what's the point?
On 2/24/22 21:43, Jakub Kicinski wrote: > On Wed, 23 Feb 2022 19:35:28 -0800 Randy Dunlap wrote: >> __setup() handlers should return 1 on success, i.e., the parameter >> has been handled. A return of 0 causes the "option=value" string to be >> added to init's environment strings, polluting it. > > Meaning early_param_on_off() also returns the wrong thing? > Or that's different? early_param() and its varieties are different -- 0 for success, else error. >> Fixes: acc18c147b22 ("net: sxgbe: add EEE(Energy Efficient Ethernet) for Samsung sxgbe") >> Fixes: 1edb9ca69e8a ("net: sxgbe: add basic framework for Samsung 10Gb ethernet driver") >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >> Reported-by: Igor Zhbanov <i.zhbanov@omprussia.ru> >> Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0defe3@omprussia.ru >> >> --- linux-next-20220223.orig/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c >> +++ linux-next-20220223/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c >> @@ -2285,18 +2285,18 @@ static int __init sxgbe_cmdline_opt(char >> char *opt; >> >> if (!str || !*str) >> - return -EINVAL; >> + return 1; >> while ((opt = strsep(&str, ",")) != NULL) { >> if (!strncmp(opt, "eee_timer:", 10)) { >> if (kstrtoint(opt + 10, 0, &eee_timer)) >> goto err; >> } >> } >> - return 0; >> + return 1; >> >> err: >> pr_err("%s: ERROR broken module parameter conversion\n", __func__); >> - return -EINVAL; >> + return 1; >> } >> >> __setup("sxgbeeth=", sxgbe_cmdline_opt); > > Was the option of making __setup() return void considered? > Sounds like we always want to return 1 so what's the point? Well, AFAIK __setup() has been around forever (at least 22 years), so No, I don't think anyone has considered making it void. Returning 1 or 0 gives kernel parameter writers the option of how error input is handled, although 0 is usually wrong. :)
On Thu, 24 Feb 2022 23:30:42 -0800 Randy Dunlap wrote: > On 2/24/22 21:43, Jakub Kicinski wrote: > > On Wed, 23 Feb 2022 19:35:28 -0800 Randy Dunlap wrote: > >> __setup() handlers should return 1 on success, i.e., the parameter > >> has been handled. A return of 0 causes the "option=value" string to be > >> added to init's environment strings, polluting it. > > > > Meaning early_param_on_off() also returns the wrong thing? > > Or that's different? > > early_param() and its varieties are different -- 0 for success, else error. Making it an even stronger contender for the worst API ever award ;) > >> Fixes: acc18c147b22 ("net: sxgbe: add EEE(Energy Efficient Ethernet) for Samsung sxgbe") > >> Fixes: 1edb9ca69e8a ("net: sxgbe: add basic framework for Samsung 10Gb ethernet driver") > >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > >> Reported-by: Igor Zhbanov <i.zhbanov@omprussia.ru> > >> Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0defe3@omprussia.ru > >> > >> --- linux-next-20220223.orig/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c > >> +++ linux-next-20220223/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c > >> @@ -2285,18 +2285,18 @@ static int __init sxgbe_cmdline_opt(char > >> char *opt; > >> > >> if (!str || !*str) > >> - return -EINVAL; > >> + return 1; > >> while ((opt = strsep(&str, ",")) != NULL) { > >> if (!strncmp(opt, "eee_timer:", 10)) { > >> if (kstrtoint(opt + 10, 0, &eee_timer)) > >> goto err; > >> } > >> } > >> - return 0; > >> + return 1; > >> > >> err: > >> pr_err("%s: ERROR broken module parameter conversion\n", __func__); > >> - return -EINVAL; > >> + return 1; > >> } > >> > >> __setup("sxgbeeth=", sxgbe_cmdline_opt); > > > > Was the option of making __setup() return void considered? > > Sounds like we always want to return 1 so what's the point? > > Well, AFAIK __setup() has been around forever (at least 22 years), so No, > I don't think anyone has considered making it void. > > Returning 1 or 0 gives kernel parameter writers the option of how error > input is handled, although 0 is usually wrong. :) Yeah, well, we've been making bus ->remove() functions void IIUC so why not this. That's separate from fixing the current incorrect uses tho, so let me apply both of the netdev fixes, thank you!
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Wed, 23 Feb 2022 19:35:28 -0800 you wrote: > __setup() handlers should return 1 on success, i.e., the parameter > has been handled. A return of 0 causes the "option=value" string to be > added to init's environment strings, polluting it. > > Fixes: acc18c147b22 ("net: sxgbe: add EEE(Energy Efficient Ethernet) for Samsung sxgbe") > Fixes: 1edb9ca69e8a ("net: sxgbe: add basic framework for Samsung 10Gb ethernet driver") > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > Reported-by: Igor Zhbanov <i.zhbanov@omprussia.ru> > Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0defe3@omprussia.ru > Cc: Siva Reddy <siva.kallam@samsung.com> > Cc: Girish K S <ks.giri@samsung.com> > Cc: Byungho An <bh74.an@samsung.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > > [...] Here is the summary with links: - net: sxgbe: fix return value of __setup handler https://git.kernel.org/netdev/net/c/50e06ddceeea You are awesome, thank you!
--- linux-next-20220223.orig/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c +++ linux-next-20220223/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c @@ -2285,18 +2285,18 @@ static int __init sxgbe_cmdline_opt(char char *opt; if (!str || !*str) - return -EINVAL; + return 1; while ((opt = strsep(&str, ",")) != NULL) { if (!strncmp(opt, "eee_timer:", 10)) { if (kstrtoint(opt + 10, 0, &eee_timer)) goto err; } } - return 0; + return 1; err: pr_err("%s: ERROR broken module parameter conversion\n", __func__); - return -EINVAL; + return 1; } __setup("sxgbeeth=", sxgbe_cmdline_opt);
__setup() handlers should return 1 on success, i.e., the parameter has been handled. A return of 0 causes the "option=value" string to be added to init's environment strings, polluting it. Fixes: acc18c147b22 ("net: sxgbe: add EEE(Energy Efficient Ethernet) for Samsung sxgbe") Fixes: 1edb9ca69e8a ("net: sxgbe: add basic framework for Samsung 10Gb ethernet driver") Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Reported-by: Igor Zhbanov <i.zhbanov@omprussia.ru> Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0defe3@omprussia.ru Cc: Siva Reddy <siva.kallam@samsung.com> Cc: Girish K S <ks.giri@samsung.com> Cc: Byungho An <bh74.an@samsung.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> --- drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)