Re: [RFC 1/2] printk/dynamic_debug: Remove cyclic dependency between printk.h and dynamic_debug.h

[Rasmus Villemoes]

On 12/01/2022 13.12, Petr Mladek wrote:
> 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.

I'm not against splitting up printk.h, I'm very familiar with the
problems with too-large headers. But I think the specific problem with
dynamic_debug.h has a much smaller and simpler resolution (as I've
suggested), which would also clean up the code a bit, and make a later
refactoring of printk.h simpler - because there's one less user to care
about.

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

No, not really, anything in dynamic_debug.h is part of every TU that
includes that header, even if the static inline is only used by one of
them. And since the module code anyway needs to have some knowledge of
dyndbg, I don't see why we can't move the meat of that static inline
into module.c.

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

No, not if I'm reading the code and the old commit logs right. For a
built-in module, that thing gets handled by
ddebug_dyndbg_boot_param_cb(), just as the comment in
ddebug_dyndbg_param_cb() says.

But in the call from ddebug_dyndbg_module_param_cb(), param is expected
to be the plain parameter name; it is (userspace) modprobe which parses
/proc/cmdline for any occurrence of <module>.foo=bar and passes that on,
in the format foo=bar, along with any other module parameters given
explicitly in the modprobe call. So if I'm reading the code right, a
CONFIG_DYNAMIC_DEBUG=n kernel would print the "dyndbg param is supported
only in ..." warning if one loads a module and gives a
HALLEdyndbgLUJA=123 parameter.

Rasmus