"make headerdep" reports two cycles where printk.h is involved. Both are
a bit complicated. All involved headers provide inlined functions that
have to be inlined because they add caller-specific metadata.
There are several possible solutions:
1. Ignore the problem because the cycles do not cause any real problem
at the moment. I would say that it works by chance. See the patches
for more details.
2. Move the printk-calls from the headers into .c sources so that printk.h
is included in .c instead of .h. It is relatively easy except that it
makes the code a bit more complicated.
3. Use a simple declaration somewhere. It is problematic because
the inlined functions are more complex.
But printk() is complex because it adds metadata for the list
of strings in /sys/kernel/debug/printk/index. The index already misses
a lot of strings that are printed via some subsystem specific wrappers.
It should be acceptable to miss the few strings used in the affected
headers.
This patchset implements the 3rd solution. It does not complicate
the existing code. It is quite the opposite. It splits the long
printk.h. It puts some low-level definitions into separate printk_core.h.
The lightweight header file might be useful also in other situations.
What do you think, please?
Petr Mladek (2):
printk/dynamic_debug: Remove cyclic dependency between printk.h and
dynamic_debug.h
printk/bug: Remove cyclic dependency between bug.h and printk.h
MAINTAINERS | 1 +
include/asm-generic/bug.h | 4 +-
include/linux/dynamic_debug.h | 10 ++--
include/linux/printk.h | 68 +--------------------------
include/linux/printk_core.h | 87 +++++++++++++++++++++++++++++++++++
5 files changed, 96 insertions(+), 74 deletions(-)
create mode 100644 include/linux/printk_core.h
--
2.26.2
`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'.
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.
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.
The advantage is that this approach might be used to solve other cyclic
dependencies, for example in bug.h.
Reported-by: Andy Shevchenko <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
---
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 <[email protected]>
R: John Ogness <[email protected]>
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
`make headerdep` reports many circular dependencies with the same
pattern:
In file included from linux/bug.h,
from linux/jump_label.h:262
from linux/dynamic_debug.h:6
from linux/printk.h:504
from asm-generic/bug.h:22
from linux/bug.h:32 <-- here
It does not cause real problem because 'asm-generic/bug.h' uses only
plain printk() and no_printk(). And these two functions are defined
in 'printk.h' before 'dynamic_debug.h' is included.
There is no easy solution because all affected code does some inline
tricks:
+ printk() adds struct pi_entry metadata
+ dynamic_pr_debug() adds struct _ddebug metadata
+ static_branch_likely() adds assembly that realizes the jump
+ BUG() prints __FILE__, __LINE__, __func__ where it is inlined
One solution would be to modify BUG() and pass __FILE__, __LINE__,
__func__ into a helper function implemented in a .c source file.
It will not require including "printk.h" in "bug.h". The drawback
is code complication.
Alternative solution is to include "printk_core.h" and use the raw
_printk(). The drawback is that the string will not be listed in
printk index. But it will afftect only few architectires that do
not define HAVE_ARCH_BUG.
This patch uses the alternative solution because it seems to be
easier to maintain. The BUG() definitions are already complicated
enough thanks to all the ifdefs.
Reported-by: Andy Shevchenko <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
---
include/asm-generic/bug.h | 4 ++--
include/linux/printk.h | 11 -----------
include/linux/printk_core.h | 11 +++++++++++
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index edb0e2a602a8..140afb8bdfe7 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -19,7 +19,7 @@
#ifndef __ASSEMBLY__
#include <linux/panic.h>
-#include <linux/printk.h>
+#include <linux/printk_core.h>
#ifdef CONFIG_BUG
@@ -55,7 +55,7 @@ struct bug_entry {
*/
#ifndef HAVE_ARCH_BUG
#define BUG() do { \
- printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
+ _printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
barrier_before_unreachable(); \
panic("BUG!"); \
} while (0)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index c20f55df7fa6..23530b0a2a07 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -123,17 +123,6 @@ struct va_format {
*/
#define DEPRECATED "[Deprecated]: "
-/*
- * Dummy printk for disabled debugging statements to use whilst maintaining
- * gcc's format checking.
- */
-#define no_printk(fmt, ...) \
-({ \
- if (0) \
- printk(fmt, ##__VA_ARGS__); \
- 0; \
-})
-
#ifdef CONFIG_EARLY_PRINTK
extern asmlinkage __printf(1, 2)
void early_printk(const char *fmt, ...);
diff --git a/include/linux/printk_core.h b/include/linux/printk_core.h
index a2b8727a2873..37fc0e13fdbd 100644
--- a/include/linux/printk_core.h
+++ b/include/linux/printk_core.h
@@ -6,6 +6,17 @@
#include <linux/kern_levels.h>
#include <linux/linkage.h>
+/*
+ * Dummy printk for disabled debugging statements to use whilst maintaining
+ * gcc's format checking.
+ */
+#define no_printk(fmt, ...) \
+({ \
+ if (0) \
+ _printk(fmt, ##__VA_ARGS__); \
+ 0; \
+})
+
/* Low level printk API. Use carefully! */
#ifdef CONFIG_PRINTK
--
2.26.2
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 <[email protected]>
>Signed-off-by: Petr Mladek <[email protected]>
Thanks! Looks good to me with changelog fixes.
Acked-by: Chris Down <[email protected]>
>---
> 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 <[email protected]>
> R: John Ogness <[email protected]>
> 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
>
Petr Mladek writes:
>`make headerdep` reports many circular dependencies with the same
>pattern:
>
>In file included from linux/bug.h,
> from linux/jump_label.h:262
> from linux/dynamic_debug.h:6
> from linux/printk.h:504
> from asm-generic/bug.h:22
> from linux/bug.h:32 <-- here
>
>It does not cause real problem because 'asm-generic/bug.h' uses only
>plain printk() and no_printk(). And these two functions are defined
>in 'printk.h' before 'dynamic_debug.h' is included.
>
>There is no easy solution because all affected code does some inline
>tricks:
>
> + printk() adds struct pi_entry metadata
> + dynamic_pr_debug() adds struct _ddebug metadata
> + static_branch_likely() adds assembly that realizes the jump
> + BUG() prints __FILE__, __LINE__, __func__ where it is inlined
>
>One solution would be to modify BUG() and pass __FILE__, __LINE__,
>__func__ into a helper function implemented in a .c source file.
>It will not require including "printk.h" in "bug.h". The drawback
>is code complication.
>
>Alternative solution is to include "printk_core.h" and use the raw
>_printk(). The drawback is that the string will not be listed in
>printk index. But it will afftect only few architectires that do
>not define HAVE_ARCH_BUG.
>
>This patch uses the alternative solution because it seems to be
>easier to maintain. The BUG() definitions are already complicated
>enough thanks to all the ifdefs.
>
>Reported-by: Andy Shevchenko <[email protected]>
>Signed-off-by: Petr Mladek <[email protected]>
Thank you! Looks good.
Acked-by: Chris Down <[email protected]>
>---
> include/asm-generic/bug.h | 4 ++--
> include/linux/printk.h | 11 -----------
> include/linux/printk_core.h | 11 +++++++++++
> 3 files changed, 13 insertions(+), 13 deletions(-)
>
>diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>index edb0e2a602a8..140afb8bdfe7 100644
>--- a/include/asm-generic/bug.h
>+++ b/include/asm-generic/bug.h
>@@ -19,7 +19,7 @@
>
> #ifndef __ASSEMBLY__
> #include <linux/panic.h>
>-#include <linux/printk.h>
>+#include <linux/printk_core.h>
>
> #ifdef CONFIG_BUG
>
>@@ -55,7 +55,7 @@ struct bug_entry {
> */
> #ifndef HAVE_ARCH_BUG
> #define BUG() do { \
>- printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
>+ _printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
> barrier_before_unreachable(); \
> panic("BUG!"); \
> } while (0)
>diff --git a/include/linux/printk.h b/include/linux/printk.h
>index c20f55df7fa6..23530b0a2a07 100644
>--- a/include/linux/printk.h
>+++ b/include/linux/printk.h
>@@ -123,17 +123,6 @@ struct va_format {
> */
> #define DEPRECATED "[Deprecated]: "
>
>-/*
>- * Dummy printk for disabled debugging statements to use whilst maintaining
>- * gcc's format checking.
>- */
>-#define no_printk(fmt, ...) \
>-({ \
>- if (0) \
>- printk(fmt, ##__VA_ARGS__); \
>- 0; \
>-})
>-
> #ifdef CONFIG_EARLY_PRINTK
> extern asmlinkage __printf(1, 2)
> void early_printk(const char *fmt, ...);
>diff --git a/include/linux/printk_core.h b/include/linux/printk_core.h
>index a2b8727a2873..37fc0e13fdbd 100644
>--- a/include/linux/printk_core.h
>+++ b/include/linux/printk_core.h
>@@ -6,6 +6,17 @@
> #include <linux/kern_levels.h>
> #include <linux/linkage.h>
>
>+/*
>+ * Dummy printk for disabled debugging statements to use whilst maintaining
>+ * gcc's format checking.
>+ */
>+#define no_printk(fmt, ...) \
>+({ \
>+ if (0) \
>+ _printk(fmt, ##__VA_ARGS__); \
>+ 0; \
>+})
>+
> /* Low level printk API. Use carefully! */
>
> #ifdef CONFIG_PRINTK
>--
>2.26.2
>
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
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
On Wed, Jan 12, 2022 at 5:12 AM Petr Mladek <[email protected]> wrote:
>
> On Tue 2022-01-11 17:01:35, Rasmus Villemoes wrote:
> > On 11/01/2022 15.30, Petr Mladek wrote:
> > > 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
This EXPORT can go.
For the user I envisioned, DRM, Ive done it with a callback
provided by dynamic-debug, which maps bits in __drm_debug,
to calls to ddebug_exec_queries, without the export.
https://lore.kernel.org/lkml/[email protected]/
This seems like a narrower/tighter interface,
and readily repeatable for other users, should they emerge.
Id welcome your inputs on the whole patchset.
Rasmus, I extend your Factory macros to add a .class_id
and use them to wrap __drm_dev_dbg etc
thanks
On Wed 2022-01-12 20:38:57, [email protected] wrote:
> On Wed, Jan 12, 2022 at 5:12 AM Petr Mladek <[email protected]> wrote:
> >
> > On Tue 2022-01-11 17:01:35, Rasmus Villemoes wrote:
> > > On 11/01/2022 15.30, Petr Mladek wrote:
>
>
> > > > 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
>
> This EXPORT can go.
Does it mean that the entire function might be removed or just
EXPORT_SYMBOL_GPL() macro, please?
I am especially interested whether we could remove pr_warn()
from the header file. It would help us the get rid of the
cyclic header dependency an easy way.
Best Regards,
Petr
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
On Thu, Jan 13, 2022 at 1:35 AM Petr Mladek <[email protected]> wrote:
>
> On Wed 2022-01-12 20:38:57, [email protected] wrote:
> > On Wed, Jan 12, 2022 at 5:12 AM Petr Mladek <[email protected]> wrote:
> > >
> > > On Tue 2022-01-11 17:01:35, Rasmus Villemoes wrote:
> > > > On 11/01/2022 15.30, Petr Mladek wrote:
> >
> >
> > > > > 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
> >
> > This EXPORT can go.
>
> Does it mean that the entire function might be removed or just
> EXPORT_SYMBOL_GPL() macro, please?
>
the whole function and export can be dropped.
its a thin wrapper on static ddebug_exec_queries().
And apologies, I thought Id sent this earlier.
> I am especially interested whether we could remove pr_warn()
> from the header file. It would help us the get rid of the
> cyclic header dependency an easy way.
>
> Best Regards,
> Petr