[PATCH] net: sxgbe: fix return value of __setup handler

[PATCH] net: sxgbe: fix return value of __setup handler

[Randy Dunlap]

__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(-)

--- 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);

Re: [PATCH] net: sxgbe: fix return value of __setup handler

[Jakub Kicinski]

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?

Re: [PATCH] net: sxgbe: fix return value of __setup handler

[Randy Dunlap]

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. :)

-- 
~Randy

Re: [PATCH] net: sxgbe: fix return value of __setup handler

[Jakub Kicinski]

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!

Re: [PATCH] net: sxgbe: fix return value of __setup handler

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html