1. Patchew
  2. linux
  3. View series

[PATCH] kgdboc: fix return value of __setup handler

Randy Dunlap posted 1 patch 4 years, 3 months ago
There is a newer version of this series
drivers/tty/serial/kgdboc.c |    6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
[PATCH] kgdboc: fix return value of __setup handler
Posted by Randy Dunlap 4 years, 3 months ago 
__setup() handlers should return 1 to indicate that the boot option
has been handled. A return of 0 causes the boot option/value to be
listed as an Unknown kernel parameter and added to init's (limited)
environment strings. So return 1 from kgdboc_option_setup().

Unknown kernel command line parameters "BOOT_IMAGE=/boot/bzImage-517rc7
  kgdboc=kbd kgdbts=", will be passed to user space.

 Run /sbin/init as init process
   with arguments:
     /sbin/init
   with environment:
     HOME=/
     TERM=linux
     BOOT_IMAGE=/boot/bzImage-517rc7
     kgdboc=kbd
     kgdbts=

Fixes: 1cd25cbb2fed ("kgdboc: Fix warning with module build")
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: Laura Abbott <labbott@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: kgdb-bugreport@lists.sourceforge.net
Cc: Jason Wessel <jason.wessel@windriver.com>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: linux-serial@vger.kernel.org
---
 drivers/tty/serial/kgdboc.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- lnx-517-rc7.orig/drivers/tty/serial/kgdboc.c
+++ lnx-517-rc7/drivers/tty/serial/kgdboc.c
@@ -403,16 +403,16 @@ static int kgdboc_option_setup(char *opt
 {
 	if (!opt) {
 		pr_err("config string not provided\n");
-		return -EINVAL;
+		return 1;
 	}
 
 	if (strlen(opt) >= MAX_CONFIG_LEN) {
 		pr_err("config string too long\n");
-		return -ENOSPC;
+		return 1;
 	}
 	strcpy(config, opt);
 
-	return 0;
+	return 1;
 }
 
 __setup("kgdboc=", kgdboc_option_setup);
Re: [PATCH] kgdboc: fix return value of __setup handler
Posted by Doug Anderson 4 years, 3 months ago 
Hi,

On Mon, Mar 7, 2022 at 7:32 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> __setup() handlers should return 1 to indicate that the boot option
> has been handled. A return of 0 causes the boot option/value to be
> listed as an Unknown kernel parameter and added to init's (limited)
> environment strings. So return 1 from kgdboc_option_setup().

This took me about 20 minutes to trace through the code to confirm,
but it appears you're correct. It's pretty twisted that early_param()
and __setup(), both of which add things to the same list, work exactly
opposite here. :( Any chance I could convince you to:

1. Add a comment before the definition of __setup_param() explaining
that 0 means error and 1 means no error. There's a comment next to
early_param() that _implies_ that setup is the opposite(), but it'd be
nice to see documentation of __setup(). I know __setup() is supposed
to be "only for core code", but still seems like we could document it.

2. Add something to your commit message helping someone find the place
where the return value is checked. Basically just mention
obsolete_checksetup() to give people a hint.


> Unknown kernel command line parameters "BOOT_IMAGE=/boot/bzImage-517rc7
>   kgdboc=kbd kgdbts=", will be passed to user space.
>
>  Run /sbin/init as init process
>    with arguments:
>      /sbin/init
>    with environment:
>      HOME=/
>      TERM=linux
>      BOOT_IMAGE=/boot/bzImage-517rc7
>      kgdboc=kbd
>      kgdbts=
>
> Fixes: 1cd25cbb2fed ("kgdboc: Fix warning with module build")

Are you certain about this "Fixes" line? That commit was just code
motion to move the code inside the #ifdef. It sure looks like it was
broken even before this.


> 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: Laura Abbott <labbott@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: kgdb-bugreport@lists.sourceforge.net
> Cc: Jason Wessel <jason.wessel@windriver.com>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: linux-serial@vger.kernel.org
> ---
>  drivers/tty/serial/kgdboc.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> --- lnx-517-rc7.orig/drivers/tty/serial/kgdboc.c
> +++ lnx-517-rc7/drivers/tty/serial/kgdboc.c
> @@ -403,16 +403,16 @@ static int kgdboc_option_setup(char *opt
>  {
>         if (!opt) {
>                 pr_err("config string not provided\n");
> -               return -EINVAL;
> +               return 1;

Shouldn't it return 0 in the error cases? If __setup() functions are
supposed to return "1" no matter what then what was the purpose of
having a return value in the first place?
Re: [PATCH] kgdboc: fix return value of __setup handler
Posted by Randy Dunlap 4 years, 3 months ago 
Hi Doug,

On 3/8/22 08:04, Doug Anderson wrote:
> Hi,
> 
> On Mon, Mar 7, 2022 at 7:32 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>> __setup() handlers should return 1 to indicate that the boot option
>> has been handled. A return of 0 causes the boot option/value to be
>> listed as an Unknown kernel parameter and added to init's (limited)
>> environment strings. So return 1 from kgdboc_option_setup().
> 
> This took me about 20 minutes to trace through the code to confirm,
> but it appears you're correct. It's pretty twisted that early_param()
> and __setup(), both of which add things to the same list, work exactly
> opposite here. :( Any chance I could convince you to:
> 
> 1. Add a comment before the definition of __setup_param() explaining
> that 0 means error and 1 means no error. There's a comment next to
> early_param() that _implies_ that setup is the opposite(), but it'd be
> nice to see documentation of __setup(). I know __setup() is supposed
> to be "only for core code", but still seems like we could document it.

I have already done this. The patch is in Andrew's mmotm tree (patch queue).

> 2. Add something to your commit message helping someone find the place
> where the return value is checked. Basically just mention
> obsolete_checksetup() to give people a hint.
> 

Sure, no problem. Good idea.

> 
>> Unknown kernel command line parameters "BOOT_IMAGE=/boot/bzImage-517rc7
>>   kgdboc=kbd kgdbts=", will be passed to user space.
>>
>>  Run /sbin/init as init process
>>    with arguments:
>>      /sbin/init
>>    with environment:
>>      HOME=/
>>      TERM=linux
>>      BOOT_IMAGE=/boot/bzImage-517rc7
>>      kgdboc=kbd
>>      kgdbts=
>>
>> Fixes: 1cd25cbb2fed ("kgdboc: Fix warning with module build")
> 
> Are you certain about this "Fixes" line? That commit was just code
> motion to move the code inside the #ifdef. It sure looks like it was
> broken even before this.
> 

Yes, but I am not enough of a git user to be able to backtrack
to see where this code was added. :(
(help?)

> 
>> 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: Laura Abbott <labbott@redhat.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Jiri Slaby <jirislaby@kernel.org>
>> Cc: kgdb-bugreport@lists.sourceforge.net
>> Cc: Jason Wessel <jason.wessel@windriver.com>
>> Cc: Daniel Thompson <daniel.thompson@linaro.org>
>> Cc: Douglas Anderson <dianders@chromium.org>
>> Cc: linux-serial@vger.kernel.org
>> ---
>>  drivers/tty/serial/kgdboc.c |    6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> --- lnx-517-rc7.orig/drivers/tty/serial/kgdboc.c
>> +++ lnx-517-rc7/drivers/tty/serial/kgdboc.c
>> @@ -403,16 +403,16 @@ static int kgdboc_option_setup(char *opt
>>  {
>>         if (!opt) {
>>                 pr_err("config string not provided\n");
>> -               return -EINVAL;
>> +               return 1;
> 
> Shouldn't it return 0 in the error cases? If __setup() functions are
> supposed to return "1" no matter what then what was the purpose of
> having a return value in the first place?

It should return 0 if the string(s) should be added to init's arg or env
strings, which is probably very rare. I don't know why it has a return
value in the first place. Someone else has already suggested that __setup()
functions should be void. Maybe they should one day, but that's a much
larger patch.

I'll send a v2.

thanks.
-- 
~Randy
Re: [PATCH] kgdboc: fix return value of __setup handler
Posted by Doug Anderson 4 years, 3 months ago 
Hi,

On Tue, Mar 8, 2022 at 1:19 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> Hi Doug,
>
> >> Unknown kernel command line parameters "BOOT_IMAGE=/boot/bzImage-517rc7
> >>   kgdboc=kbd kgdbts=", will be passed to user space.
> >>
> >>  Run /sbin/init as init process
> >>    with arguments:
> >>      /sbin/init
> >>    with environment:
> >>      HOME=/
> >>      TERM=linux
> >>      BOOT_IMAGE=/boot/bzImage-517rc7
> >>      kgdboc=kbd
> >>      kgdbts=
> >>
> >> Fixes: 1cd25cbb2fed ("kgdboc: Fix warning with module build")
> >
> > Are you certain about this "Fixes" line? That commit was just code
> > motion to move the code inside the #ifdef. It sure looks like it was
> > broken even before this.
> >
>
> Yes, but I am not enough of a git user to be able to backtrack
> to see where this code was added. :(
> (help?)

I always just chain back w/ git blame. In this case:

git blame 1cd25cbb2fed~ -- drivers/tty/serial/kgdboc.c

...then search for __setup there and it finds:

Fixes: f2d937f3bf00 ("consoles: polling support, kgdboc")

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.