Remove most all #define pr_fmt(fmt) lines
Subsystems (2)
RFC 1/1 Remove most all #define pr_fmt(fmt) lines
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(-)
include/linux/printk.h +72 -42
@@ -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: "
drivers/regulator/Makefile +3 -1
@@ -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?
Quoted 19 lines · Joe Perches
With this change, objects like
drivers/regulator/core.oare prefixed with "core: ". This is somewhat senseless and may promptMakefilechanges to make the prefixes for some objects more sensible.For example, modifying the
Makefileto bundle objects together with a specific name can change the prefixes. Here the prefix becomes "regulator: "+++ b/drivers/regulator/Makefile
drivers/regulator/Makefile +3
-2+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 ------------------
Quoted 1 line
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
Quoted 18 lines · Clemens Ladisch
Quoted 17 lines · Joe Perches
With this change, objects like
drivers/regulator/core.oare prefixed with "core: ". This is somewhat senseless and may promptMakefilechanges to make the prefixes for some objects more sensible.For example, modifying the
Makefileto bundle objects together with a specific name can change the prefixes. Here the prefix becomes "regulator: "+++ b/drivers/regulator/Makefile
drivers/regulator/Makefile +3
-1+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
Quoted 6 lines
Quoted 3 lines
------------------
Any objections or other suggestions/improvements?
Instead of doing a
Makefilechange that has no _obvious_ connection with printk, wouldn't it be better to just definepr_fmtwith "regulator: "?
Maybe, maybe not.
Bundling objects in a Makefile like this is pretty common and can also produce better module names.
Quoted 10 lines · Joe Perches
Quoted 9 lines · Clemens Ladisch
Quoted 7 lines · Joe Perches
drivers/regulator/Makefile +3
-1+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.oAny objections or other suggestions/improvements?
This seems an incredibly obscure approach.
Quoted 2 lines
Quoted 2 lines
Instead of doing a
Makefilechange that has no _obvious_ connection with printk, wouldn't it be better to just definepr_fmtwith "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.
Quoted 4 lines
Maybe, maybe not.
Bundling objects in a
Makefilelike 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.
Quoted 9 lines · Mark Brown
Quoted 7 lines · Joe Perches
Quoted 6 lines · Clemens Ladisch
Quoted 5 lines · Joe Perches
drivers/regulator/Makefile +3
-1+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.oAny objections or other suggestions/improvements?
This seems an incredibly obscure approach.
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Instead of doing a
Makefilechange that has no _obvious_ connection with printk, wouldn't it be better to just definepr_fmtwith "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.
Quoted 1 line
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?
Quoted 1 line · Joe Perches
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.
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Instead of doing a
Makefilechange that has no _obvious_ connection with printk, wouldn't it be better to just definepr_fmtwith "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.
Quoted 3 lines
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).
Quoted 3 lines
A negative of the
Makefileapproach 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).
Quoted 5 lines
Quoted 1 line
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.
Quoted 9 lines · Joe Perches
#ifndef
pr_fmtinclude/linux/printk.h +7
-0+ +#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)
include/linux/printk.h static inline void setup_log_buf(int early) · line 187
Quoted 5 lines
include/linux/printk.h +5
-0+ +#else + +#ifndef pr_fmt_std +#define pr_fmt_std(fmt...) pr_fmt(fmt)
Why the "fmt..." here, and "fmt" in the definitions above?
Quoted 6 lines
include/linux/printk.h +5
-0+#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 -- [email protected]
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
Quoted 14 lines · Geert Uytterhoeven
include/linux/printk.h static inline void setup_log_buf(int early) · line 182
Quoted 9 lines · Joe Perches
#ifndef
pr_fmtinclude/linux/printk.h +7
-0+ +#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) fmtWhat's the rationale behind the above line?
I had expected
Quoted 1 line
#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 <[email protected]> Date: Wed Nov 12 21:16:43 2008 +0100
Add 'pr_fmt()' format modifier to pr_xyz macros.
commit 1f7c8234c7a68c2ccc2a33f3b7d48057980e7c35
Author: Emil Medve <[email protected]> Date: Tue Oct 16 23:29:48 2007 -0700
Make the pr_*() family of macros in kernel.h complete
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#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__