Re: [RFC 1/2] printk/dynamic_debug: Remove cyclic dependency between printk.h and dynamic_debug.h
From: Chris Down
Date: Tue Jan 11 2022 - 09:53:09 EST
Hey Petr,
Petr Mladek writes:
`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.
Sounds reasonable.
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'.
Note that it is not enough to declare _printk() in 'dynamic_debug.h'.
It would break build with CONFIG_PRINTK where it is an inlined nope.
s/nope/nop/, and you mean !CONFIG_PRINTK instead of CONFIG_PRINTK. Or did I
misunderstand?
The drawback of this approach is that _printk() does not add metadata for
printk indexing and the message is not listed in <debugfs>/printk/index/.
But it should be acceptable here. The strings are used only for debugging.
Agreed that these are fine to omit from a printk indexing perspective.
The advantage is that this approach might be used to solve other cyclic
dependencies, for example in bug.h.
Reported-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
Thanks! Looks good to me with changelog fixes.
Acked-by: Chris Down <chris@xxxxxxxxxxxxxx>
---
MAINTAINERS | 1 +
include/linux/dynamic_debug.h | 10 ++---
include/linux/printk.h | 57 +-------------------------
include/linux/printk_core.h | 76 +++++++++++++++++++++++++++++++++++
4 files changed, 83 insertions(+), 61 deletions(-)
create mode 100644 include/linux/printk_core.h
diff --git a/MAINTAINERS b/MAINTAINERS
index fb18ce7168aa..eadb77da01db 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15340,6 +15340,7 @@ R: Steven Rostedt <rostedt@xxxxxxxxxxx>
R: John Ogness <john.ogness@xxxxxxxxxxxxx>
S: Maintained
F: include/linux/printk.h
+F: include/linux/printk_core.h
F: kernel/printk/
PRINTK INDEXING
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index dce631e678dd..63f6ebd6a14c 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -185,7 +185,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
#include <linux/string.h>
#include <linux/errno.h>
-#include <linux/printk.h>
+#include <linux/printk_core.h>
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;
@@ -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;
}
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9497f6b98339..c20f55df7fa6 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -5,6 +5,7 @@
#include <linux/stdarg.h>
#include <linux/init.h>
#include <linux/kern_levels.h>
+#include <linux/printk_core.h>
#include <linux/linkage.h>
#include <linux/cache.h>
#include <linux/ratelimit_types.h>
@@ -144,32 +145,6 @@ void early_printk(const char *s, ...) { }
struct dev_printk_info;
#ifdef CONFIG_PRINTK
-asmlinkage __printf(4, 0)
-int vprintk_emit(int facility, int level,
- const struct dev_printk_info *dev_info,
- const char *fmt, va_list args);
-
-asmlinkage __printf(1, 0)
-int vprintk(const char *fmt, va_list args);
-
-asmlinkage __printf(1, 2) __cold
-int _printk(const char *fmt, ...);
-
-/*
- * Special printk facility for scheduler/timekeeping use only, _DO_NOT_USE_ !
- */
-__printf(1, 2) __cold int _printk_deferred(const char *fmt, ...);
-
-extern void __printk_safe_enter(void);
-extern void __printk_safe_exit(void);
-/*
- * The printk_deferred_enter/exit macros are available only as a hack for
- * some code paths that need to defer all printk console printing. Interrupts
- * must be disabled for the deferred duration.
- */
-#define printk_deferred_enter __printk_safe_enter
-#define printk_deferred_exit __printk_safe_exit
-
/*
* Please don't use printk_ratelimit(), because it shares ratelimiting state
* with all other unrelated printk_ratelimit() callsites. Instead use
@@ -189,7 +164,6 @@ devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, void *buf,
extern void wake_up_klogd(void);
-char *log_buf_addr_get(void);
u32 log_buf_len_get(void);
void log_buf_vmcoreinfo_setup(void);
void __init setup_log_buf(int early);
@@ -200,30 +174,6 @@ extern asmlinkage void dump_stack_lvl(const char *log_lvl) __cold;
extern asmlinkage void dump_stack(void) __cold;
void printk_trigger_flush(void);
#else
-static inline __printf(1, 0)
-int vprintk(const char *s, va_list args)
-{
- return 0;
-}
-static inline __printf(1, 2) __cold
-int _printk(const char *s, ...)
-{
- return 0;
-}
-static inline __printf(1, 2) __cold
-int _printk_deferred(const char *s, ...)
-{
- return 0;
-}
-
-static inline void printk_deferred_enter(void)
-{
-}
-
-static inline void printk_deferred_exit(void)
-{
-}
-
static inline int printk_ratelimit(void)
{
return 0;
@@ -238,11 +188,6 @@ static inline void wake_up_klogd(void)
{
}
-static inline char *log_buf_addr_get(void)
-{
- return NULL;
-}
-
static inline u32 log_buf_len_get(void)
{
return 0;
diff --git a/include/linux/printk_core.h b/include/linux/printk_core.h
new file mode 100644
index 000000000000..a2b8727a2873
--- /dev/null
+++ b/include/linux/printk_core.h
@@ -0,0 +1,76 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __KERNEL_PRINTK_CORE__
+#define __KERNEL_PRINTK_CORE__
+
+#include <linux/stdarg.h>
+#include <linux/kern_levels.h>
+#include <linux/linkage.h>
+
+/* Low level printk API. Use carefully! */
+
+#ifdef CONFIG_PRINTK
+
+struct dev_printk_info;
+
+asmlinkage __printf(4, 0)
+int vprintk_emit(int facility, int level,
+ const struct dev_printk_info *dev_info,
+ const char *fmt, va_list args);
+
+asmlinkage __printf(1, 0)
+int vprintk(const char *fmt, va_list args);
+
+asmlinkage __printf(1, 2) __cold
+int _printk(const char *fmt, ...);
+
+/*
+ * Special printk facility for scheduler/timekeeping use only, _DO_NOT_USE_ !
+ */
+__printf(1, 2) __cold int _printk_deferred(const char *fmt, ...);
+
+extern void __printk_safe_enter(void);
+extern void __printk_safe_exit(void);
+/*
+ * The printk_deferred_enter/exit macros are available only as a hack for
+ * some code paths that need to defer all printk console printing. Interrupts
+ * must be disabled for the deferred duration.
+ */
+#define printk_deferred_enter __printk_safe_enter
+#define printk_deferred_exit __printk_safe_exit
+
+char *log_buf_addr_get(void);
+
+#else /* CONFIG_PRINTK */
+
+static inline __printf(1, 0)
+int vprintk(const char *s, va_list args)
+{
+ return 0;
+}
+static inline __printf(1, 2) __cold
+int _printk(const char *s, ...)
+{
+ return 0;
+}
+static inline __printf(1, 2) __cold
+int _printk_deferred(const char *s, ...)
+{
+ return 0;
+}
+
+static inline void printk_deferred_enter(void)
+{
+}
+
+static inline void printk_deferred_exit(void)
+{
+}
+
+static inline char *log_buf_addr_get(void)
+{
+ return NULL;
+}
+
+#endif /* CONFING_PRINTK */
+
+#endif
--
2.26.2