Re: [PATCH] kgdboc: fix return value of __setup handler

[Doug Anderson]

Hi,

On Mon, Mar 7, 2022 at 7:32 PM Randy Dunlap <rdunlap@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> Reported-by: Igor Zhbanov <i.zhbanov@xxxxxxxxxxxx>
> Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0defe3@xxxxxxxxxxxx
> Cc: Laura Abbott <labbott@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Jiri Slaby <jirislaby@xxxxxxxxxx>
> Cc: kgdb-bugreport@xxxxxxxxxxxxxxxxxxxxx
> Cc: Jason Wessel <jason.wessel@xxxxxxxxxxxxx>
> Cc: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Cc: linux-serial@xxxxxxxxxxxxxxx
> ---
>  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?