Messages in this thread Patch in this message | | | Subject | Re: [RFC 1/2] printk/dynamic_debug: Remove cyclic dependency between printk.h and dynamic_debug.h | From | Rasmus Villemoes <> | Date | Tue, 11 Jan 2022 17:01:35 +0100 |
| |
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
| |