Re: [PATCH v12 33/40] dyndbg: Ignore additional arguments from pr_fmt

On Tue, Oct 13, 2026 at 5:33 AM Petr Mladek [off-list ref] wrote:

On Mon 2026-10-12 20:37:04, Jim Cromie via B4 Relay wrote:

quoted

From: Philipp Hahn <redacted>

pr_fmt can be used to add a common prefix to any output from a module:
      #define pr_fmt(fmt) KBUILD_MODNAME ".%s " fmt, __func__

But adding additional arguments breaks dynamic debug:

quoted

error: macro "DEFINE_DYNAMIC_DEBUG_METADATA_CLS" passed 4 arguments, but takes just 3
|         pr_debug_ratelimited("%s", "Hello world!");
|                                                  ^
note: macro "DEFINE_DYNAMIC_DEBUG_METADATA_CLS" defined here
| #define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt)       \
|
error: ‘DEFINE_DYNAMIC_DEBUG_METADATA_CLS’ undeclared (first use in this function)
|         DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, _DPRINTK_CLASS_DFLT, fmt)
|         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I wonder what this error about undeclared macro means.

quoted

quoted

note: in expansion of macro ‘DEFINE_DYNAMIC_DEBUG_METADATA’
|         DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, pr_fmt(fmt));         \
|         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: in expansion of macro ‘pr_debug_ratelimited’
|         pr_debug_ratelimited("%s", "Hello world!");
|         ^~~~~~~~~~~~~~~~~~~~

Add an additional ', ...' to DEFINE_DYNAMIC_DEBUG_METADATA_CLS to slurp
any additional argument, which `pr_fmt` might add.

--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h

@@ -351,7 +351,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
              dump_stack();                                   \
 }

-#define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt)    \
+#define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt, ...)       \
      static struct _ddebug  __aligned(8)                     \
      __section("__dyndbg_descs") name = {                    \
              .modname = KBUILD_MODNAME,                      \

diff --git a/include/linux/printk.h b/include/linux/printk.h
index f594c1266bfd..5d701952e739 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h

@@ -742,7 +742,7 @@ do {                                                                      \
      static DEFINE_RATELIMIT_STATE(_rs,                              \
                                    DEFAULT_RATELIMIT_INTERVAL,       \
                                    DEFAULT_RATELIMIT_BURST);         \
-     DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, pr_fmt(fmt));         \
+     DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);         \

IMHO, this changes the existing behavior. The prefix added by
pr_fmt() will no longer be stored in DEFINE_DYNAMIC_DEBUG_METADATA().
Is this intentional, please?

yes, its intentional.

515a9adce0f0 ("include/linux/printk.h: include pr_fmt in pr_debug_ratelimited")
added pr-fmt to both the METADATA macro call, and to the printk() just below it.
The commit-msg was clearly about adding it to the __dynamic_pr_debug() itself,
the METADATA addition was overzealous.

Removing it brings pr_debug_ratelimited() into conformance with the other
*_pr_debug_* calls:
    dynamic_pr_debug, dev_dbg_ratelimited, net_dbg_ratelimited.

and as you noted, this makes the ... variadic unnecessary.

Ive added the Fixes: tag accordingly.

thanks for the review & pushback.
and apologies for the delay, I picked up a FLU shortly after sending v12.

Anyway, if we do not longer pass pr_fmt(fmt) then we should not
need to add "..." into DEFINE_DYNAMIC_DEBUG_METADATA_CLS().

By other words, we newly pass just the format "fmt" without the parametes.
The 4th parameter was added by pr_fmt().


IMHO, the right solution is to keep pr_fmt(fmt) here and extend both
BEFINE_DYNAMIC_DEBUG_METADATA() and
DEFINE_DYNAMIC_DEBUG_METADATA_CLS()
to accept var args. I would expect something like:

#define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt, ...)  \
...

#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt, ...)           \
        DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, _DPRINTK_CLASS_DFLT, fmt, ##__VA_ARGS__)


or maybe, we could "eat" the extra parameters in
DEFINE_DYNAMIC_DEBUG_METADATA. It would mean to keep
DEFINE_DYNAMIC_DEBUG_METADATA_CLS() as is and just add the "..." into
DEFINE_DYNAMIC_DEBUG_METADATA(), aka

#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt, ...)           \
        DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, _DPRINTK_CLASS_DFLT, fmt)


Or do I miss something, please?

It is possible that the macros work another way. Or this depends
on another change. I checked only this patch...

Best Regards,
Petr

PS: I was curious what Sashiko says about this change and it seems
    that it does not review this patchset from some reasons.

I checked shortly after sending, it was "embargoed" for 24 hrs,
perhaps cuz it was so soon after rc7

quoted

      if (DYNAMIC_DEBUG_BRANCH(descriptor) &&                         \
          __ratelimit(&_rs))                                          \
              __dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__);    \