[RFC] Remove most all #define pr_fmt(fmt) lines

[RFC] Remove most all #define pr_fmt(fmt) lines

[Joe Perches]

For the next RC window I'd like to convert the
current pr_fmt #define in include/linux/printk.h
so that non-debug uses of pr_<level> always get a
standardized KBUILD_MODNAME prefix when pr_fmt is
otherwise undefined.

Basically, add #define pr_fmt_std and pr_fmt_dbg
and convert the existing pr_fmt uses in printk.h
to these new #defines.

This will allow deletion of all of the 600 or so
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
sprinkled around the kernel tree.

This will also allow separate and possibly clearer
uses of dynamic_debugging or adding __func__ only
to pr_debug uses.

Below this rfc patch is more commentary and another
suggested patch to a Makefile to improve prefixing.

 include/linux/printk.h |  114 ++++++++++++++++++++++++++++++------------------
 1 files changed, 72 insertions(+), 42 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 0525927..d9c05f4 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -158,48 +158,78 @@ static inline void setup_log_buf(int early)
 
 extern void dump_stack(void) __cold;
 
+/* Define separate possible prefixes for pr_debug and other pr_<level> uses
+ *
+ * pr_fmt_dbg for pr_debug and pr_devel (default: blank)
+ * pr_fmt_std for everything else (default: KBUILD_MODNAME ": ")
+ *
+ * if pr_fmt is defined before this inclusion, it is
+ * used for both pr_fmt_dbg and pr_fmt_std
+ *
+ * nb: Most debugging can now be done with dynamic debug.
+ *     dynamic debug can optionally prefix KBUILD_MODNAME and/or __func__
+ *     see Documentation/dynamic-debug-howto.txt
+ */
+
 #ifndef pr_fmt
+
+#ifndef pr_fmt_std
+#define pr_fmt_std(fmt) KBUILD_MODNAME ": " fmt
+#endif
+#ifndef pr_fmt_dbg
+#define pr_fmt_dbg(fmt) fmt
+#endif
 #define pr_fmt(fmt) fmt
+
+#else
+
+#ifndef pr_fmt_std
+#define pr_fmt_std(fmt...) pr_fmt(fmt)
+#endif
+#ifndef pr_fmt_dbg
+#define pr_fmt_dbg(fmt...) pr_fmt(fmt)
+#endif
+
 #endif
 
-#define pr_emerg(fmt, ...) \
-	printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_alert(fmt, ...) \
-	printk(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_crit(fmt, ...) \
-	printk(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_err(fmt, ...) \
-	printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_warning(fmt, ...) \
-	printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_emerg(fmt, ...)					\
+	printk(KERN_EMERG pr_fmt_std(fmt), ##__VA_ARGS__)
+#define pr_alert(fmt, ...)					\
+	printk(KERN_ALERT pr_fmt_std(fmt), ##__VA_ARGS__)
+#define pr_crit(fmt, ...)					\
+	printk(KERN_CRIT pr_fmt_std(fmt), ##__VA_ARGS__)
+#define pr_err(fmt, ...)					\
+	printk(KERN_ERR pr_fmt_std(fmt), ##__VA_ARGS__)
+#define pr_warning(fmt, ...)					\
+	printk(KERN_WARNING pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_warn pr_warning
-#define pr_notice(fmt, ...) \
-	printk(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_info(fmt, ...) \
-	printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_cont(fmt, ...) \
+#define pr_notice(fmt, ...)					\
+	printk(KERN_NOTICE pr_fmt_std(fmt), ##__VA_ARGS__)
+#define pr_info(fmt, ...)					\
+	printk(KERN_INFO pr_fmt_std(fmt), ##__VA_ARGS__)
+#define pr_cont(fmt, ...)					\
 	printk(KERN_CONT fmt, ##__VA_ARGS__)
 
 /* pr_devel() should produce zero code unless DEBUG is defined */
 #ifdef DEBUG
-#define pr_devel(fmt, ...) \
-	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_devel(fmt, ...)					\
+	printk(KERN_DEBUG pr_fmt_dbg(fmt), ##__VA_ARGS__)
 #else
-#define pr_devel(fmt, ...) \
-	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_devel(fmt, ...)					\
+	no_printk(KERN_DEBUG pr_fmt_dbg(fmt), ##__VA_ARGS__)
 #endif
 
 /* If you are writing a driver, please use dev_dbg instead */
 #if defined(CONFIG_DYNAMIC_DEBUG)
 /* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
-#define pr_debug(fmt, ...) \
+#define pr_debug(fmt, ...)					\
 	dynamic_pr_debug(fmt, ##__VA_ARGS__)
 #elif defined(DEBUG)
-#define pr_debug(fmt, ...) \
-	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_debug(fmt, ...)					\
+	printk(KERN_DEBUG pr_fmt_dbg(fmt), ##__VA_ARGS__)
 #else
-#define pr_debug(fmt, ...) \
-	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_debug(fmt, ...) 					\
+	no_printk(KERN_DEBUG pr_fmt_dbg(fmt), ##__VA_ARGS__)
 #endif
 
 /*
@@ -222,28 +252,28 @@ extern void dump_stack(void) __cold;
 #endif
 
 #define pr_emerg_once(fmt, ...)					\
-	printk_once(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
+	printk_once(KERN_EMERG pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_alert_once(fmt, ...)					\
-	printk_once(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
+	printk_once(KERN_ALERT pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_crit_once(fmt, ...)					\
-	printk_once(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
+	printk_once(KERN_CRIT pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_err_once(fmt, ...)					\
-	printk_once(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
+	printk_once(KERN_ERR pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_warn_once(fmt, ...)					\
-	printk_once(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+	printk_once(KERN_WARNING pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_notice_once(fmt, ...)				\
-	printk_once(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
+	printk_once(KERN_NOTICE pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_info_once(fmt, ...)					\
-	printk_once(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
+	printk_once(KERN_INFO pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_cont_once(fmt, ...)					\
 	printk_once(KERN_CONT pr_fmt(fmt), ##__VA_ARGS__)
 /* If you are writing a driver, please use dev_dbg instead */
 #if defined(DEBUG)
 #define pr_debug_once(fmt, ...)					\
-	printk_once(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+	printk_once(KERN_DEBUG pr_fmt_dbg(fmt), ##__VA_ARGS__)
 #else
 #define pr_debug_once(fmt, ...)					\
-	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+	no_printk(KERN_DEBUG pr_fmt_dbg(fmt), ##__VA_ARGS__)
 #endif
 
 /*
@@ -266,27 +296,27 @@ extern void dump_stack(void) __cold;
 #endif
 
 #define pr_emerg_ratelimited(fmt, ...)					\
-	printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
+	printk_ratelimited(KERN_EMERG pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_alert_ratelimited(fmt, ...)					\
-	printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
+	printk_ratelimited(KERN_ALERT pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_crit_ratelimited(fmt, ...)					\
-	printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
+	printk_ratelimited(KERN_CRIT pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_err_ratelimited(fmt, ...)					\
-	printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
+	printk_ratelimited(KERN_ERR pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_warn_ratelimited(fmt, ...)					\
-	printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+	printk_ratelimited(KERN_WARNING pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_notice_ratelimited(fmt, ...)					\
-	printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
+	printk_ratelimited(KERN_NOTICE pr_fmt_std(fmt), ##__VA_ARGS__)
 #define pr_info_ratelimited(fmt, ...)					\
-	printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
+	printk_ratelimited(KERN_INFO pr_fmt_std(fmt), ##__VA_ARGS__)
 /* no pr_cont_ratelimited, don't do that... */
 /* If you are writing a driver, please use dev_dbg instead */
 #if defined(DEBUG)
 #define pr_debug_ratelimited(fmt, ...)					\
-	printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+	printk_ratelimited(KERN_DEBUG pr_fmt_dbg(fmt), ##__VA_ARGS__)
 #else
 #define pr_debug_ratelimited(fmt, ...) \
-	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+	no_printk(KERN_DEBUG pr_fmt_dbg(fmt), ##__VA_ARGS__)
 #endif
 
 enum {

-------------------------

This does have some side-effects to current uses of
pr_<level> without a pre-existing pr_fmt.

All non-debug pr_<level> uses will have a prefix.

pr_debug can be prefixed by dynamic_debug or have
__func__ added independently of other pr_<level> uses.

For instance, drivers/regulator/ uses pr_err and pr_crit
for dmesg output, but the messages are not prefixed
by subsystem at all.

With this change, objects like drivers/regulator/core.o
are prefixed with "core: ".  This is somewhat senseless
and may prompt Makefile changes to make the prefixes for
some objects more sensible.

For example, modifying the Makefile to bundle objects
together with a specific name can change the prefixes.
Here the prefix becomes "regulator: "

diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 94b5274..93fff05 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -2,8 +2,10 @@
 # Makefile for regulator drivers.
 #
 
+regulator-y := core.o dummy.o fixed-helper.o
+regulator-objs := $(regulator-y)
 
-obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o
+obj-$(CONFIG_REGULATOR) += regulator.o
 obj-$(CONFIG_OF) += of_regulator.o
 obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o

------------------

Any objections or other suggestions/improvements?

Re: [RFC] Remove most all #define pr_fmt(fmt) lines

[Clemens Ladisch]

Joe Perches wrote:
> With this change, objects like drivers/regulator/core.o
> are prefixed with "core: ".  This is somewhat senseless
> and may prompt Makefile changes to make the prefixes for
> some objects more sensible.
>
> For example, modifying the Makefile to bundle objects
> together with a specific name can change the prefixes.
> Here the prefix becomes "regulator: "
>
> +++ b/drivers/regulator/Makefile
>
> +regulator-y := core.o dummy.o fixed-helper.o
> +regulator-objs := $(regulator-y)
>
> -obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o
> +obj-$(CONFIG_REGULATOR) += regulator.o
>
> ------------------
>
> Any objections or other suggestions/improvements?

Instead of doing a Makefile change that has no _obvious_ connection with
printk, wouldn't it be better to just define pr_fmt with "regulator: "?


Regards,
Clemens

Re: [RFC] Remove most all #define pr_fmt(fmt) lines

[Joe Perches]

On Wed, 2012-03-28 at 09:27 +0200, Clemens Ladisch wrote:
> Joe Perches wrote:
> > With this change, objects like drivers/regulator/core.o
> > are prefixed with "core: ".  This is somewhat senseless
> > and may prompt Makefile changes to make the prefixes for
> > some objects more sensible.
> >
> > For example, modifying the Makefile to bundle objects
> > together with a specific name can change the prefixes.
> > Here the prefix becomes "regulator: "
> >
> > +++ b/drivers/regulator/Makefile
> >
> > +regulator-y := core.o dummy.o fixed-helper.o
> > +regulator-objs := $(regulator-y)
> >
> > -obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o
> > +obj-$(CONFIG_REGULATOR) += regulator.o
> >
> > ------------------
> >
> > Any objections or other suggestions/improvements?
> 
> Instead of doing a Makefile change that has no _obvious_ connection with
> printk, wouldn't it be better to just define pr_fmt with "regulator: "?

Maybe, maybe not.

Bundling objects in a Makefile like this is pretty common
and can also produce better module names.

Re: [RFC] Remove most all #define pr_fmt(fmt) lines

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On Wed, Mar 28, 2012 at 12:30:03AM -0700, Joe Perches wrote:
> On Wed, 2012-03-28 at 09:27 +0200, Clemens Ladisch wrote:
> > Joe Perches wrote:

> > > +regulator-y := core.o dummy.o fixed-helper.o
> > > +regulator-objs := $(regulator-y)

> > > -obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o
> > > +obj-$(CONFIG_REGULATOR) += regulator.o

> > > Any objections or other suggestions/improvements?

This seems an incredibly obscure approach.

> > Instead of doing a Makefile change that has no _obvious_ connection with
> > printk, wouldn't it be better to just define pr_fmt with "regulator: "?

This seems like a much better idea if we're going to do anything; it
means that we don't end up embedding module names in things (which are
after all a bit of an implementation detail) and get to pick the name so
we can do something like get the prefix which is used for the symbols in
the code even if things are split over multiple modules.

> Maybe, maybe not.

> Bundling objects in a Makefile like this is pretty common
> and can also produce better module names.

In the case above we don't support modular build in the first place.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] Remove most all #define pr_fmt(fmt) lines

[Joe Perches]

On Wed, 2012-03-28 at 10:46 +0100, Mark Brown wrote:
> On Wed, Mar 28, 2012 at 12:30:03AM -0700, Joe Perches wrote:
> > On Wed, 2012-03-28 at 09:27 +0200, Clemens Ladisch wrote:
> > > Joe Perches wrote:
> > > > +regulator-y := core.o dummy.o fixed-helper.o
> > > > +regulator-objs := $(regulator-y)
> > > > -obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o
> > > > +obj-$(CONFIG_REGULATOR) += regulator.o
> > > > Any objections or other suggestions/improvements?
> This seems an incredibly obscure approach.
> > > Instead of doing a Makefile change that has no _obvious_ connection with
> > > printk, wouldn't it be better to just define pr_fmt with "regulator: "?
> This seems like a much better idea if we're going to do anything; it
> means that we don't end up embedding module names in things (which are
> after all a bit of an implementation detail) and get to pick the name so
> we can do something like get the prefix which is used for the symbols in
> the code even if things are split over multiple modules.

A negative is that requires #defines in multiple
source files or rearranging #includes to centralize
that #define.

A negative of the Makefile approach is the name is
obscurely chosen.  A positive is it's only chosen
once.

> In the case above we don't support modular build in the first place.

Unrelated but is there any particular reason why
the regulator core code couldn't be build as a
module?

Re: [RFC] Remove most all #define pr_fmt(fmt) lines

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

On Wed, Mar 28, 2012 at 09:22:45AM -0700, Joe Perches wrote:
> On Wed, 2012-03-28 at 10:46 +0100, Mark Brown wrote:

Please at least leave the blank lines others put in their mails when
quoting even if you don't add any between paragraphs yourself, it really
makes things much more legible.

> > > > Instead of doing a Makefile change that has no _obvious_ connection with
> > > > printk, wouldn't it be better to just define pr_fmt with "regulator: "?

> > This seems like a much better idea if we're going to do anything; it
> > means that we don't end up embedding module names in things (which are
> > after all a bit of an implementation detail) and get to pick the name so
> > we can do something like get the prefix which is used for the symbols in
> > the code even if things are split over multiple modules.

> A negative is that requires #defines in multiple
> source files or rearranging #includes to centralize
> that #define.

You only need to do this in cases where the module name isn't a good
choice which hopefully should be relatively rare, and I'd expect a lot
of the time things will be in one source file anyway (as with the
regulator case).

> A negative of the Makefile approach is the name is
> obscurely chosen.  A positive is it's only chosen
> once.

I don't think this is an either/or thing - you can default to using the
module name and override it where needed which like I say should
hopefully be relatively rare (though the sound tree will probably need
to do this pretty much everywhere due to the naming convention it uses).

> > In the case above we don't support modular build in the first place.

> Unrelated but is there any particular reason why
> the regulator core code couldn't be build as a
> module?

It's not worth bothering, it's needed spectacularly early on in most
practical systems (including by board files that are always built in as
bits of them run prior to I/O mappings and whatnot being set up).  The
main practical result would be lots more hassle with dependencies in
Kconfig so the randconfig folks can do their stuff and no change in
actual configurations that people deploy.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] Remove most all #define pr_fmt(fmt) lines

[Geert Uytterhoeven]

On Tue, Mar 27, 2012 at 19:23, Joe Perches <joe@perches.com> wrote:
>  #ifndef pr_fmt
> +
> +#ifndef pr_fmt_std
> +#define pr_fmt_std(fmt) KBUILD_MODNAME ": " fmt
> +#endif
> +#ifndef pr_fmt_dbg
> +#define pr_fmt_dbg(fmt) fmt
> +#endif
>  #define pr_fmt(fmt) fmt

What's the rationale behind the above line?
I had expected

     #define pr_fmt(fmt) pr_fmt_std(fmt)

> +
> +#else
> +
> +#ifndef pr_fmt_std
> +#define pr_fmt_std(fmt...) pr_fmt(fmt)

Why the "fmt..." here, and "fmt" in the definitions above?

> +#endif
> +#ifndef pr_fmt_dbg
> +#define pr_fmt_dbg(fmt...) pr_fmt(fmt)
> +#endif
> +
>  #endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC] Remove most all #define pr_fmt(fmt) lines

[Joe Perches]

On Wed, 2012-03-28 at 09:42 +0200, Geert Uytterhoeven wrote:
> On Tue, Mar 27, 2012 at 19:23, Joe Perches <joe@perches.com> wrote:
> >  #ifndef pr_fmt
> > +
> > +#ifndef pr_fmt_std
> > +#define pr_fmt_std(fmt) KBUILD_MODNAME ": " fmt
> > +#endif
> > +#ifndef pr_fmt_dbg
> > +#define pr_fmt_dbg(fmt) fmt
> > +#endif
> >  #define pr_fmt(fmt) fmt
> 
> What's the rationale behind the above line?
> I had expected
> 
>      #define pr_fmt(fmt) pr_fmt_std(fmt)

Keeping current pr_debug output as similar to
possible to the output produced by this patch.

pr_debug predates pr_fmt by quite awhile, it
dates back from kernel version 2.0 days, and
there are _many_ instances of pr_debug in files
without a pr_fmt define.

The other defines of pr_<level> were introduced
more or less the same time as pr_fmt so there
aren't _too_ many that couldn't reasonably be
prefixed.

commit d091c2f58ba32029495a933b721e8e02fbd12caa
Author: Martin Schwidefsky <schwidefsky@de.ibm.com>
Date:   Wed Nov 12 21:16:43 2008 +0100

    Add 'pr_fmt()' format modifier to pr_xyz macros.

commit 1f7c8234c7a68c2ccc2a33f3b7d48057980e7c35
Author: Emil Medve <Emilian.Medve@Freescale.com>
Date:   Tue Oct 16 23:29:48 2007 -0700

    Make the pr_*() family of macros in kernel.h complete

> > +#ifndef pr_fmt_std
> > +#define pr_fmt_std(fmt...) pr_fmt(fmt)
> 
> Why the "fmt..." here, and "fmt" in the definitions above?

Where pr_fmt is already defined, these pr_fmt_std and _dbg
macros must work with pr_fmt defines like:

#define pr_fmt(fmt) "my special prefix:%s:%d", __func__, __LINE__