All of lore.kernel.org
 help / color / mirror / Atom feed
From: yajun.deng@linux.dev
To: "Marco Elver" <elver@google.com>
Cc: keescook@chromium.org, samitolvanen@google.com, ojeda@kernel.org,
	masahiroy@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] init: fix the wrong __setup_param() definition
Date: Tue, 21 Dec 2021 02:08:54 +0000	[thread overview]
Message-ID: <56d8e86231eec9f46abf9117596ffe07@linux.dev> (raw)
In-Reply-To: <CANpmjNPsCVkq7SsVL-xpktGe3RsJnRGTJxEPZ60VUt1w_5QgPQ@mail.gmail.com>

December 20, 2021 7:20 PM, "Marco Elver" <elver@google.com> wrote:

> On Mon, 20 Dec 2021 at 04:55, Yajun Deng <yajun.deng@linux.dev> wrote:
> 
>> The parameters in __setup_param() should be four rather than three when
>> MODULE isn't definited.
> 
> This is actually "when MODULE is defined". __setup_param() becomes a
> nop when compiling as a module.
> 
> But that begs the question: why hasn't this been caught before?
> Probably because nobody should be using __setup_param() if something
> can also be compiled as a module, in which case module_param() and
> friends should be used. But perhaps there are valid usecases where i
> t's meant to become a nop if MODULE.
> 
> I don't object this fix, since the !MODULE __setup_param() seems like
> it was meant to be defined.
> 
> Just curious: did you actually encounter a problem with some new code
> using __setup_param()?

NO, it is just code inspection. But for the current code, it's really a bug.
vim drivers/clk/imx/clk.c  +161

#ifndef MODULE

...
__setup_param("earlycon", imx_keep_uart_earlycon,
              imx_keep_uart_clocks_param, 0); 
__setup_param("earlyprintk", imx_keep_uart_earlyprintk,
              imx_keep_uart_clocks_param, 0); 


> 
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
>> ---
>> include/linux/init.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/include/linux/init.h b/include/linux/init.h
>> index d82b4b2e1d25..62a77850f10e 100644
>> --- a/include/linux/init.h
>> +++ b/include/linux/init.h
>> @@ -355,7 +355,7 @@ void __init parse_early_options(char *cmdline);
>> 
>> #else /* MODULE */
>> 
>> -#define __setup_param(str, unique_id, fn) /* nothing */
>> +#define __setup_param(str, unique_id, fn, early)/* nothing */
>> #define __setup(str, func) /* nothing */
>> #endif
>> 
>> --
>> 2.32.0

      reply	other threads:[~2021-12-21  2:08 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-20  3:54 [PATCH] init: fix the wrong __setup_param() definition Yajun Deng
2021-12-20 11:20 ` Marco Elver
2021-12-21  2:08   ` yajun.deng [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56d8e86231eec9f46abf9117596ffe07@linux.dev \
    --to=yajun.deng@linux.dev \
    --cc=elver@google.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=masahiroy@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=samitolvanen@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.