Subject: Re: [RFC 1/2] printk/dynamic_debug: Remove cyclic dependency between
 printk.h and dynamic_debug.h
To:     Petr Mladek <pmladek@suse.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     John Ogness <john.ogness@linutronix.de>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        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>
References: <20220111143046.14680-1-pmladek@suse.com>
 <20220111143046.14680-2-pmladek@suse.com>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <996a7cf5-b047-5038-c86b-f10820364465@rasmusvillemoes.dk>
Date:   Tue, 11 Jan 2022 17:01:35 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <20220111143046.14680-2-pmladek@suse.com>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
> 
> Anyway, it would be great to get rid of this dependency because it is
> tricky and it might hit us in the future. Also it might hide another
> more complicated cyclic dependencies.
> 
> One solution would be to move the inlined ddebug_dyndbg_module_param_cb()
> and dynamic_debug_exec_queries() from 'dynamic_debug.h' into some .c so
> that it will not be needed to inline printk() in 'dynamic_debug.h'.
> 
> The obvious location would be 'lib/dynamic_debug.c'. But it is built
> only when CONFIG_DYNAMIC_DEBUG_CORE is set. And the problematic
> inline variants are used only when this config option is _not_ set.
> So that it is not that easy.
> 
> 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.

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

What's with the strstr, btw? Shouldn't it just be !strcmp()?

> @@ -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?

Rasmus