From: Petr Mladek <pmladek@suse.com>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
John Ogness <john.ogness@linutronix.de>,
Sergey Senozhatsky <senozhatsky@chromium.org>,
Steven Rostedt <rostedt@goodmis.org>,
Chris Down <chris@chrisdown.name>, Marc Zyngier <maz@kernel.org>,
Andrew Scull <ascull@google.com>, Will Deacon <will@kernel.org>,
Jason Baron <jbaron@akamai.com>,
Peter Zijlstra <peterz@infradead.org>,
Josh Poimboeuf <jpoimboe@redhat.com>,
linux-kernel@vger.kernel.org,
Andy Shevchenko <andy.shevchenko@gmail.com>,
Jessica Yu <jeyu@kernel.org>, Jim Cromie <jim.cromie@gmail.com>
Subject: Re: [RFC 1/2] printk/dynamic_debug: Remove cyclic dependency between printk.h and dynamic_debug.h
Date: Wed, 12 Jan 2022 13:12:27 +0100 [thread overview]
Message-ID: <Yd7Fq6V1/Ynff6Qx@alley> (raw)
In-Reply-To: <996a7cf5-b047-5038-c86b-f10820364465@rasmusvillemoes.dk>
On Tue 2022-01-11 17:01:35, Rasmus Villemoes wrote:
> On 11/01/2022 15.30, Petr Mladek wrote:
> > `make headerdep` reports many circular dependencies with the same
> > pattern:
> >
> > In file included from linux/printk.h,
> > from linux/dynamic_debug.h:188
> > from linux/printk.h:559 <-- here
> >
> > It looks like false positive:
> >
> > + printk.h includes dynamic_debug.h when CONFIG_DYNAMIC_DEBUG_CORE
> > + dynamic_debug.h includes printk.h when !CONFIG_DYNAMIC_DEBUG_CORE
> >
> > Instead, this patch adds 'include/linux/printk_core.h' and moves some
> > lowlevel printk API there. Then the raw _printk() can be called from
> > the inlined code in 'dynamic_debug.h'.
>
> Urgh, this doesn't look like the right approach.
It is not ideal. But it allows to handle these cycles without
complicating external code. It is not only about dynamic_debug.h.
The problem is also in include/asm-generic/bug.h and possibly other
headers included directly or indirectly from printk.h.
Another small advantage is that printk_core.h might define other
printk API that is not intended for general use.
> > static inline int ddebug_add_module(struct _ddebug *tab, unsigned int n,
> > const char *modname)
> > @@ -202,9 +202,8 @@ static inline int ddebug_dyndbg_module_param_cb(char *param, char *val,
> > const char *modname)
> > {
> > if (strstr(param, "dyndbg")) {
> > - /* avoid pr_warn(), which wants pr_fmt() fully defined */
> > - printk(KERN_WARNING "dyndbg param is supported only in "
> > - "CONFIG_DYNAMIC_DEBUG builds\n");
> > + /* Use raw _printk() to avoid cyclic dependency. */
> > + _printk(KERN_WARNING "dyndbg param is supported only in CONFIG_DYNAMIC_DEBUG builds\n");
> > return 0; /* allow and ignore */
> > }
> > return -EINVAL;
>
> It looks like this has only one caller, kernel/module.c. I suggest
> simply moving the match logic into unknown_module_param_cb(), making it
> on par with the other "generic" module parameter async_probe. That is,
> do something like
>
> if (strstr(param, "dyndbg")) {
> if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
> return ddebug_dyndbg_module_param_cb(param, val, modname)
> }
> pr_warn("dyndbg param is supported only in ...");
> return 0; /* allow and ignore */
> }
>
> pr_warn("%s: unknown parameter '%s' ignored\n", modname, param);
> return 0;
>
> That makes it simpler to add more magic/generic module parameters in
> unknown_module_param_cb(). No need for a static inline stub, and no need
> for conditionally declaring ddebug_dyndbg_module_param_cb(). So all that
> is needed in dynamic_debug.h is to remove the static inline definition,
> and pull the declaration outside #if defined(CONFIG_DYNAMIC_DEBUG_CORE)
> protection.
I do not have strong opinion here. The advantage of the current code
is that it keeps the complexity in dynamic_debug code.
Adding Jessica into CC to know her preferences.
> What's with the strstr, btw? Shouldn't it just be !strcmp()?
"dyndbg" parameter might be used as <module>.dyndbg[="val"].
> > @@ -223,7 +222,8 @@ static inline int ddebug_dyndbg_module_param_cb(char *param, char *val,
> >
> > static inline int dynamic_debug_exec_queries(const char *query, const char *modname)
> > {
> > - pr_warn("kernel not built with CONFIG_DYNAMIC_DEBUG_CORE\n");
> > + /* Use raw _printk() to avoid cyclic dependency. */
> > + _printk(KERN_WARNING "kernel not built with CONFIG_DYNAMIC_DEBUG_CORE\n");
> > return 0;
> > }
>
> And for this one I think the solution is even simpler, as I can't find
> any in-tree callers. Perhaps just nuke it entirely?
Adding Jim into Cc whether he still has any plans to use this API.
Best Regards,
Petr
next prev parent reply other threads:[~2022-01-12 12:12 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-11 14:30 [RFC 0/2] printk: Remove cyclic include dependencies with printk.h Petr Mladek
2022-01-11 14:30 ` [RFC 1/2] printk/dynamic_debug: Remove cyclic dependency between printk.h and dynamic_debug.h Petr Mladek
2022-01-11 14:53 ` Chris Down
2022-01-11 16:01 ` Rasmus Villemoes
2022-01-12 12:12 ` Petr Mladek [this message]
2022-01-13 3:38 ` jim.cromie
2022-01-13 8:35 ` Petr Mladek
2022-01-17 22:39 ` jim.cromie
2022-01-13 9:02 ` Rasmus Villemoes
2022-01-11 14:30 ` [RFC 2/2] printk/bug: Remove cyclic dependency between bug.h and printk.h Petr Mladek
2022-01-11 14:54 ` Chris Down
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=Yd7Fq6V1/Ynff6Qx@alley \
--to=pmladek@suse.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=andy.shevchenko@gmail.com \
--cc=ascull@google.com \
--cc=chris@chrisdown.name \
--cc=jbaron@akamai.com \
--cc=jeyu@kernel.org \
--cc=jim.cromie@gmail.com \
--cc=john.ogness@linutronix.de \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=maz@kernel.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=senozhatsky@chromium.org \
--cc=will@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).