* [PATCH] plist: include -DDEBUG if CONFIG_DEBUG_PI_LIST @ 2014-05-02 20:23 Dan Streetman 2014-05-02 20:41 ` Steven Rostedt 0 siblings, 1 reply; 15+ messages in thread From: Dan Streetman @ 2014-05-02 20:23 UTC (permalink / raw) To: Paul Gortmaker, Steven Rostedt, Thomas Gleixner, Andrew Morton Cc: Dan Streetman, linux-kernel lib/plist.c uses pr_debug() in its test function, which is compiled and run only when CONFIG_DEBUG_PI_LIST in set; however pr_debug() is compiled out unless -DDEBUG is set for the file. Update lib/Makefile to add -DDEBUG to CFLAGS_plist.o if CONFIG_DEBUG_PI_LIST is set, so that the pr_debug() output from plist_test() is shown. Signed-off-by: Dan Streetman <ddstreet@ieee.org> --- lib/Makefile | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/Makefile b/lib/Makefile index 0cd7b68..fd1f4c8 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -39,6 +39,10 @@ CFLAGS_kobject.o += -DDEBUG CFLAGS_kobject_uevent.o += -DDEBUG endif +ifeq ($(CONFIG_DEBUG_PI_LIST),y) +CFLAGS_plist.o += -DDEBUG +endif + obj-$(CONFIG_GENERIC_IOMAP) += iomap.o obj-$(CONFIG_GENERIC_PCI_IOMAP) += pci_iomap.o obj-$(CONFIG_HAS_IOMEM) += iomap_copy.o devres.o -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: include -DDEBUG if CONFIG_DEBUG_PI_LIST 2014-05-02 20:23 [PATCH] plist: include -DDEBUG if CONFIG_DEBUG_PI_LIST Dan Streetman @ 2014-05-02 20:41 ` Steven Rostedt 2014-05-05 14:35 ` Dan Streetman 0 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2014-05-02 20:41 UTC (permalink / raw) To: Dan Streetman Cc: Paul Gortmaker, Thomas Gleixner, Andrew Morton, linux-kernel On Fri, 2 May 2014 16:23:43 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > lib/plist.c uses pr_debug() in its test function, which is compiled > and run only when CONFIG_DEBUG_PI_LIST in set; however pr_debug() > is compiled out unless -DDEBUG is set for the file. > > Update lib/Makefile to add -DDEBUG to CFLAGS_plist.o if > CONFIG_DEBUG_PI_LIST is set, so that the pr_debug() output from > plist_test() is shown. Why not just use printk(KERN_DEBUG ...) then if we always want to print it? You could just place #define DEBUG in plist.c as well. Although I think just switching to printk() is better. -- Steve > > Signed-off-by: Dan Streetman <ddstreet@ieee.org> > --- > lib/Makefile | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/lib/Makefile b/lib/Makefile > index 0cd7b68..fd1f4c8 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -39,6 +39,10 @@ CFLAGS_kobject.o += -DDEBUG > CFLAGS_kobject_uevent.o += -DDEBUG > endif > > +ifeq ($(CONFIG_DEBUG_PI_LIST),y) > +CFLAGS_plist.o += -DDEBUG > +endif > + > obj-$(CONFIG_GENERIC_IOMAP) += iomap.o > obj-$(CONFIG_GENERIC_PCI_IOMAP) += pci_iomap.o > obj-$(CONFIG_HAS_IOMEM) += iomap_copy.o devres.o ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: include -DDEBUG if CONFIG_DEBUG_PI_LIST 2014-05-02 20:41 ` Steven Rostedt @ 2014-05-05 14:35 ` Dan Streetman 2014-05-05 14:43 ` [PATCH] plist: replace pr_debug with printk in plist_test() Dan Streetman 0 siblings, 1 reply; 15+ messages in thread From: Dan Streetman @ 2014-05-05 14:35 UTC (permalink / raw) To: Steven Rostedt Cc: Paul Gortmaker, Thomas Gleixner, Andrew Morton, linux-kernel On Fri, May 2, 2014 at 4:41 PM, Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 2 May 2014 16:23:43 -0400 > Dan Streetman <ddstreet@ieee.org> wrote: > >> lib/plist.c uses pr_debug() in its test function, which is compiled >> and run only when CONFIG_DEBUG_PI_LIST in set; however pr_debug() >> is compiled out unless -DDEBUG is set for the file. >> >> Update lib/Makefile to add -DDEBUG to CFLAGS_plist.o if >> CONFIG_DEBUG_PI_LIST is set, so that the pr_debug() output from >> plist_test() is shown. > > Why not just use printk(KERN_DEBUG ...) then if we always want to print > it? > > You could just place > > #define DEBUG > > in plist.c as well. Although I think just switching to printk() is > better. Agreed - since the entire function is already inside CONFIG_DEBUG_PI_LIST, there's no need to additionally require DEBUG so we should just use printk. New patch to follow. Thanks! > > -- Steve > >> >> Signed-off-by: Dan Streetman <ddstreet@ieee.org> >> --- >> lib/Makefile | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/lib/Makefile b/lib/Makefile >> index 0cd7b68..fd1f4c8 100644 >> --- a/lib/Makefile >> +++ b/lib/Makefile >> @@ -39,6 +39,10 @@ CFLAGS_kobject.o += -DDEBUG >> CFLAGS_kobject_uevent.o += -DDEBUG >> endif >> >> +ifeq ($(CONFIG_DEBUG_PI_LIST),y) >> +CFLAGS_plist.o += -DDEBUG >> +endif >> + >> obj-$(CONFIG_GENERIC_IOMAP) += iomap.o >> obj-$(CONFIG_GENERIC_PCI_IOMAP) += pci_iomap.o >> obj-$(CONFIG_HAS_IOMEM) += iomap_copy.o devres.o > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-05 14:35 ` Dan Streetman @ 2014-05-05 14:43 ` Dan Streetman 2014-05-05 14:52 ` Steven Rostedt 2014-05-05 20:35 ` Andrew Morton 0 siblings, 2 replies; 15+ messages in thread From: Dan Streetman @ 2014-05-05 14:43 UTC (permalink / raw) To: Paul Gortmaker, Steven Rostedt, Thomas Gleixner, Andrew Morton, Borislav Petkov Cc: Dan Streetman, linux-kernel Replace pr_debug() in lib/plist.c test function plist_test() with printk(KERN_DEBUG ...). Without DEBUG defined, pr_debug() is complied out, but the entire plist_test() function is already inside CONFIG_DEBUG_PI_LIST, so printk should just be used directly. Signed-off-by: Dan Streetman <ddstreet@ieee.org> Cc: Steven Rostedt <rostedt@goodmis.org> --- This is a trivial patch so this isn't really "v2", but for completeness the "v1" was here: https://lkml.org/lkml/2014/5/2/539 That added -DDEBUG to the lib/Makefile, but Steve suggested this approach and I agree. lib/plist.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/plist.c b/lib/plist.c index 1ebc95f..b883a7a 100644 --- a/lib/plist.c +++ b/lib/plist.c @@ -175,7 +175,7 @@ static int __init plist_test(void) int nr_expect = 0, i, loop; unsigned int r = local_clock(); - pr_debug("start plist test\n"); + printk(KERN_DEBUG "start plist test\n"); plist_head_init(&test_head); for (i = 0; i < ARRAY_SIZE(test_node); i++) plist_node_init(test_node + i, 0); @@ -203,7 +203,7 @@ static int __init plist_test(void) plist_test_check(nr_expect); } - pr_debug("end plist test\n"); + printk(KERN_DEBUG "end plist test\n"); return 0; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-05 14:43 ` [PATCH] plist: replace pr_debug with printk in plist_test() Dan Streetman @ 2014-05-05 14:52 ` Steven Rostedt 2014-05-05 20:35 ` Andrew Morton 1 sibling, 0 replies; 15+ messages in thread From: Steven Rostedt @ 2014-05-05 14:52 UTC (permalink / raw) To: Dan Streetman Cc: Paul Gortmaker, Thomas Gleixner, Andrew Morton, Borislav Petkov, linux-kernel On Mon, 5 May 2014 10:43:05 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > Replace pr_debug() in lib/plist.c test function plist_test() with > printk(KERN_DEBUG ...). > > Without DEBUG defined, pr_debug() is complied out, but the entire > plist_test() function is already inside CONFIG_DEBUG_PI_LIST, so > printk should just be used directly. > > Signed-off-by: Dan Streetman <ddstreet@ieee.org> > Cc: Steven Rostedt <rostedt@goodmis.org> Reviewed-by: Steven Rostedt <rostedt@goodmis.org> -- Steve ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-05 14:43 ` [PATCH] plist: replace pr_debug with printk in plist_test() Dan Streetman 2014-05-05 14:52 ` Steven Rostedt @ 2014-05-05 20:35 ` Andrew Morton 2014-05-05 20:52 ` Joe Perches 2014-05-06 12:30 ` Dan Streetman 1 sibling, 2 replies; 15+ messages in thread From: Andrew Morton @ 2014-05-05 20:35 UTC (permalink / raw) To: Dan Streetman Cc: Paul Gortmaker, Steven Rostedt, Thomas Gleixner, Borislav Petkov, linux-kernel, Joe Perches, Fabian Frederick On Mon, 5 May 2014 10:43:05 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > Replace pr_debug() in lib/plist.c test function plist_test() with > printk(KERN_DEBUG ...). > > Without DEBUG defined, pr_debug() is complied out, but the entire > plist_test() function is already inside CONFIG_DEBUG_PI_LIST, so > printk should just be used directly. > > --- a/lib/plist.c > +++ b/lib/plist.c > @@ -175,7 +175,7 @@ static int __init plist_test(void) > int nr_expect = 0, i, loop; > unsigned int r = local_clock(); > > - pr_debug("start plist test\n"); > + printk(KERN_DEBUG "start plist test\n"); Now someone will come along and helpfully switch it back to pr_debug() again :( What about adding a #define DEBUG? This aspect of pr_debug() is rather surprising and unfortunate and I guess we screwed it up. pr_debug() should unconditionally do the printk, just like pr_warn, pr_emerg, etc. And there should be a separate pr_debug_cond() which honours the DEBUG setting. akpm3:/usr/src/linux-3.15-rc4> grep -r pr_debug . | wc -l 10286 Boy, that's going to be a big patch ;) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-05 20:35 ` Andrew Morton @ 2014-05-05 20:52 ` Joe Perches 2014-05-06 12:30 ` Dan Streetman 1 sibling, 0 replies; 15+ messages in thread From: Joe Perches @ 2014-05-05 20:52 UTC (permalink / raw) To: Andrew Morton Cc: Dan Streetman, Paul Gortmaker, Steven Rostedt, Thomas Gleixner, Borislav Petkov, linux-kernel, Fabian Frederick On Mon, 2014-05-05 at 13:35 -0700, Andrew Morton wrote: > On Mon, 5 May 2014 10:43:05 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > > > Replace pr_debug() in lib/plist.c test function plist_test() with > > printk(KERN_DEBUG ...). > > > > Without DEBUG defined, pr_debug() is complied out, but the entire > > plist_test() function is already inside CONFIG_DEBUG_PI_LIST, so > > printk should just be used directly. > > > > --- a/lib/plist.c > > +++ b/lib/plist.c > > @@ -175,7 +175,7 @@ static int __init plist_test(void) > > int nr_expect = 0, i, loop; > > unsigned int r = local_clock(); > > > > - pr_debug("start plist test\n"); > > + printk(KERN_DEBUG "start plist test\n"); > > Now someone will come along and helpfully switch it back to pr_debug() > again :( > > What about adding a #define DEBUG? That can be unfortunate when there are other #if DEBUG some noisy or unnecessary code... #endif blocks in the same compilation unit. > This aspect of pr_debug() is rather surprising and unfortunate and I > guess we screwed it up. meh. It's been this way for quite awhile. I have over long periods suggested adding variants like pr_vdbg and pr_dbg_always. https://lkml.org/lkml/2009/10/13/634 http://patchwork.ozlabs.org/patch/63513/ Never had much success or visibility. > pr_debug() should unconditionally do the > printk, just like pr_warn, pr_emerg, etc. I think that would not be good. That would set an expectation for dev_dbg which has never been enabled unconditionally. > And there should be a > separate pr_debug_cond() which honours the DEBUG setting. I think that's suboptimal. > akpm3:/usr/src/linux-3.15-rc4> grep -r pr_debug . | wc -l > 10286 > > Boy, that's going to be a big patch ;) $ git grep -E "\bprintk\s*\(s*KERN_DEBUG\b" | wc -l 4412 Not to mention all the macros that use those and are then themselves used hundreds or thousands of times... ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-05 20:35 ` Andrew Morton 2014-05-05 20:52 ` Joe Perches @ 2014-05-06 12:30 ` Dan Streetman 2014-05-06 15:08 ` [PATCH] Documentation: expand/clarify debug documentation Dan Streetman 2014-05-06 15:44 ` [PATCH] plist: replace pr_debug with printk in plist_test() Fabian Frederick 1 sibling, 2 replies; 15+ messages in thread From: Dan Streetman @ 2014-05-06 12:30 UTC (permalink / raw) To: Andrew Morton Cc: Paul Gortmaker, Steven Rostedt, Thomas Gleixner, Borislav Petkov, linux-kernel, Joe Perches, Fabian Frederick On Mon, May 5, 2014 at 4:35 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Mon, 5 May 2014 10:43:05 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > >> Replace pr_debug() in lib/plist.c test function plist_test() with >> printk(KERN_DEBUG ...). >> >> Without DEBUG defined, pr_debug() is complied out, but the entire >> plist_test() function is already inside CONFIG_DEBUG_PI_LIST, so >> printk should just be used directly. >> >> --- a/lib/plist.c >> +++ b/lib/plist.c >> @@ -175,7 +175,7 @@ static int __init plist_test(void) >> int nr_expect = 0, i, loop; >> unsigned int r = local_clock(); >> >> - pr_debug("start plist test\n"); >> + printk(KERN_DEBUG "start plist test\n"); > > Now someone will come along and helpfully switch it back to pr_debug() > again :( > > What about adding a #define DEBUG? > > > > This aspect of pr_debug() is rather surprising and unfortunate and I > guess we screwed it up. pr_debug() should unconditionally do the > printk, just like pr_warn, pr_emerg, etc. And there should be a > separate pr_debug_cond() which honours the DEBUG setting. I agree, it's definitely surprising and not obvious. At the least, maybe some clearer comments/docs would help; besides actually reviewing the printk.h code, the only other indication of this behavior is CodingStyle which currently says: "For messages that aren't associated with a particular device, <linux/printk.h> defines pr_debug() and pr_info()." Listing pr_debug() and pr_info() on the same line with no qualifications kind of implies they behave the same. Maybe the example should be pr_err() and pr_info(), or really anything besides pr_debug(), which is discussed in (very brief) detail in the next paragraph... "Such messages should be compiled out when the DEBUG symbol is not defined (that is, by default they are not included). When you use dev_dbg() or pr_debug(), that's automatic. Many subsystems have Kconfig options to turn on -DDEBUG." While that does explain that pr_debug() won't actually print anything without DEBUG defined, it's hardly in a way that jumps out, clearly indicating that pr_debug() is unlike all the other pr_XXX() functions. I'll try sending a patch to update the docs to make pr_debug()'s behavior clearer... > > akpm3:/usr/src/linux-3.15-rc4> grep -r pr_debug . | wc -l > 10286 > > Boy, that's going to be a big patch ;) ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] Documentation: expand/clarify debug documentation 2014-05-06 12:30 ` Dan Streetman @ 2014-05-06 15:08 ` Dan Streetman 2014-05-06 15:44 ` [PATCH] plist: replace pr_debug with printk in plist_test() Fabian Frederick 1 sibling, 0 replies; 15+ messages in thread From: Dan Streetman @ 2014-05-06 15:08 UTC (permalink / raw) To: linux-kernel, Joe Perches, Andrew Morton; +Cc: Dan Streetman, Steven Rostedt The pr_debug() and related debug print macros all differ from the normal pr_XXX() macros, in that the normal ones print unconditionally, while the debug macros are compiled out unless DEBUG is defined or CONFIG_DYNAMIC_DEBUG is set. This isn't obvious, and the only way to find this out is either to review the actual printk.h code or to read CodingStyle, and the message there doesn't highlight the fact. Change Documentation/CodingStyle to clearly indicate that pr_debug() and related debug printing macros behave differently than all other pr_XXX() macros, and attempt to clarify when and where the different debug printing methods might be used. Add short comment to printk.h above the pr_XXX() macros indicating that while these macros print unconditionally, pr_debug() does not. Signed-off-by: Dan Streetman <ddstreet@ieee.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Joe Perches <joe@perches.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: linux-kernel <linux-kernel@vger.kernel.org> --- Documentation/CodingStyle | 25 ++++++++++++++++++------- include/linux/printk.h | 6 ++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle index 7fe0546..f9ae2c8 100644 --- a/Documentation/CodingStyle +++ b/Documentation/CodingStyle @@ -660,15 +660,23 @@ There are a number of driver model diagnostic macros in <linux/device.h> which you should use to make sure messages are matched to the right device and driver, and are tagged with the right level: dev_err(), dev_warn(), dev_info(), and so forth. For messages that aren't associated with a -particular device, <linux/printk.h> defines pr_debug() and pr_info(). +particular device, <linux/printk.h> defines pr_notice(), pr_info(), +pr_warn(), pr_err(), etc. Coming up with good debugging messages can be quite a challenge; and once -you have them, they can be a huge help for remote troubleshooting. Such -messages should be compiled out when the DEBUG symbol is not defined (that -is, by default they are not included). When you use dev_dbg() or pr_debug(), -that's automatic. Many subsystems have Kconfig options to turn on -DDEBUG. -A related convention uses VERBOSE_DEBUG to add dev_vdbg() messages to the -ones already enabled by DEBUG. +you have them, they can be a huge help for remote troubleshooting. However +debug message printing is handled differently than printing other non-debug +messages. While the other pr_XXX() functions print unconditionally, +pr_debug() does not; it is compiled out by default, unless either DEBUG is +defined or CONFIG_DYNAMIC_DEBUG is set. That is true for dev_dbg() also, +and a related convention uses VERBOSE_DEBUG to add dev_vdbg() messages to +the ones already enabled by DEBUG. + +Many subsystems have Kconfig debug options to turn on -DDEBUG in the +corresponding Makefile; in other cases specific files #define DEBUG. And +when a debug message should be unconditionally printed, such as if it is +already inside a debug-related #ifdef secton, printk(KERN_DEBUG ...) can be +used. Chapter 14: Allocating memory diff --git a/include/linux/printk.h b/include/linux/printk.h index 8752f75..88b1f3a 100644 --- a/include/linux/printk.h +++ b/include/linux/printk.h @@ -210,6 +210,12 @@ extern asmlinkage void dump_stack(void) __cold; #define pr_fmt(fmt) fmt #endif +/* + * These can be used to print at the various log levels. + * All of these will print unconditionally, although note that pr_debug() + * and other debug macros are compiled out unless either DEBUG is defined + * or CONFIG_DYNAMIC_DEBUG is set. + */ #define pr_emerg(fmt, ...) \ printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__) #define pr_alert(fmt, ...) \ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-06 12:30 ` Dan Streetman 2014-05-06 15:08 ` [PATCH] Documentation: expand/clarify debug documentation Dan Streetman @ 2014-05-06 15:44 ` Fabian Frederick 2014-05-07 14:21 ` Dan Streetman 1 sibling, 1 reply; 15+ messages in thread From: Fabian Frederick @ 2014-05-06 15:44 UTC (permalink / raw) To: Dan Streetman Cc: Andrew Morton, Paul Gortmaker, Steven Rostedt, Thomas Gleixner, Borislav Petkov, linux-kernel, Joe Perches On Tue, 6 May 2014 08:30:56 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > On Mon, May 5, 2014 at 4:35 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > On Mon, 5 May 2014 10:43:05 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > > > >> Replace pr_debug() in lib/plist.c test function plist_test() with > >> printk(KERN_DEBUG ...). > >> > >> Without DEBUG defined, pr_debug() is complied out, but the entire > >> plist_test() function is already inside CONFIG_DEBUG_PI_LIST, so > >> printk should just be used directly. > >> > >> --- a/lib/plist.c > >> +++ b/lib/plist.c > >> @@ -175,7 +175,7 @@ static int __init plist_test(void) > >> int nr_expect = 0, i, loop; > >> unsigned int r = local_clock(); > >> > >> - pr_debug("start plist test\n"); > >> + printk(KERN_DEBUG "start plist test\n"); > > > > Now someone will come along and helpfully switch it back to pr_debug() > > again :( > > > > What about adding a #define DEBUG? > > > > > > > > This aspect of pr_debug() is rather surprising and unfortunate and I > > guess we screwed it up. pr_debug() should unconditionally do the > > printk, just like pr_warn, pr_emerg, etc. And there should be a > > separate pr_debug_cond() which honours the DEBUG setting. > > I agree, it's definitely surprising and not obvious. At the least, > maybe some clearer comments/docs would help; besides actually > reviewing the printk.h code, the only other indication of this > behavior is CodingStyle which currently says: > > "For messages that aren't associated with a particular device, > <linux/printk.h> defines pr_debug() and pr_info()." > > Listing pr_debug() and pr_info() on the same line with no > qualifications kind of implies they behave the same. Maybe the > example should be pr_err() and pr_info(), or really anything besides > pr_debug(), which is discussed in (very brief) detail in the next > paragraph... > > "Such messages should be compiled out when the DEBUG symbol is not > defined (that is, by default they are not included). When you use > dev_dbg() or pr_debug(), that's automatic. Many subsystems have > Kconfig options to turn on -DDEBUG." > > While that does explain that pr_debug() won't actually print anything > without DEBUG defined, it's hardly in a way that jumps out, clearly > indicating that pr_debug() is unlike all the other pr_XXX() functions. > > I'll try sending a patch to update the docs to make pr_debug()'s > behavior clearer... Admitting checkpatch is the authority in that matter and that per subsystem debug granularity would be kept, we could at least add some specification like below ? [PATCH 1/1] scripts/checkpatch.pl: add printk(KERN_DEBUG to pr_debug specification Such conversions can't be done as trivially as other printk occurences. Signed-off-by: Fabian Frederick <fabf@skynet.be> --- scripts/checkpatch.pl | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 34eb216..4e462d7 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2863,9 +2863,14 @@ sub process { my $level = lc($orig); $level = "warn" if ($level eq "warning"); my $level2 = $level; - $level2 = "dbg" if ($level eq "debug"); + my $note = ""; + if ($level eq "debug"){ + $level2 = "dbg"; + $note = "Note that printk(KERN_DEBUG conversions to pr_debug require local DEBUG definition.\n"; + } WARN("PREFER_PR_LEVEL", - "Prefer [subsystem eg: netdev]_$level2([subsystem]dev, ... then dev_$level2(dev, ... then pr_$level(... to printk(KERN_$orig ...\n" . $herecurr); + "Prefer [subsystem eg: netdev]_$level2([subsystem]dev, ... then dev_$level2(dev, ... then pr_$level(... to printk(KERN_$orig ...\n$note" . $herecurr); + } if ($line =~ /\bpr_warning\s*\(/) { -- 1.8.4.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-06 15:44 ` [PATCH] plist: replace pr_debug with printk in plist_test() Fabian Frederick @ 2014-05-07 14:21 ` Dan Streetman 2014-05-07 14:35 ` Steven Rostedt 0 siblings, 1 reply; 15+ messages in thread From: Dan Streetman @ 2014-05-07 14:21 UTC (permalink / raw) To: Fabian Frederick Cc: Andrew Morton, Paul Gortmaker, Steven Rostedt, Thomas Gleixner, Borislav Petkov, linux-kernel, Joe Perches On Tue, May 6, 2014 at 11:44 AM, Fabian Frederick <fabf@skynet.be> wrote: > On Tue, 6 May 2014 08:30:56 -0400 > Dan Streetman <ddstreet@ieee.org> wrote: > >> On Mon, May 5, 2014 at 4:35 PM, Andrew Morton <akpm@linux-foundation.org> wrote: >> > On Mon, 5 May 2014 10:43:05 -0400 Dan Streetman <ddstreet@ieee.org> wrote: >> > >> >> Replace pr_debug() in lib/plist.c test function plist_test() with >> >> printk(KERN_DEBUG ...). >> >> >> >> Without DEBUG defined, pr_debug() is complied out, but the entire >> >> plist_test() function is already inside CONFIG_DEBUG_PI_LIST, so >> >> printk should just be used directly. >> >> >> >> --- a/lib/plist.c >> >> +++ b/lib/plist.c >> >> @@ -175,7 +175,7 @@ static int __init plist_test(void) >> >> int nr_expect = 0, i, loop; >> >> unsigned int r = local_clock(); >> >> >> >> - pr_debug("start plist test\n"); >> >> + printk(KERN_DEBUG "start plist test\n"); >> > >> > Now someone will come along and helpfully switch it back to pr_debug() >> > again :( >> > >> > What about adding a #define DEBUG? >> > >> > >> > >> > This aspect of pr_debug() is rather surprising and unfortunate and I >> > guess we screwed it up. pr_debug() should unconditionally do the >> > printk, just like pr_warn, pr_emerg, etc. And there should be a >> > separate pr_debug_cond() which honours the DEBUG setting. >> >> I agree, it's definitely surprising and not obvious. At the least, >> maybe some clearer comments/docs would help; besides actually >> reviewing the printk.h code, the only other indication of this >> behavior is CodingStyle which currently says: >> >> "For messages that aren't associated with a particular device, >> <linux/printk.h> defines pr_debug() and pr_info()." >> >> Listing pr_debug() and pr_info() on the same line with no >> qualifications kind of implies they behave the same. Maybe the >> example should be pr_err() and pr_info(), or really anything besides >> pr_debug(), which is discussed in (very brief) detail in the next >> paragraph... >> >> "Such messages should be compiled out when the DEBUG symbol is not >> defined (that is, by default they are not included). When you use >> dev_dbg() or pr_debug(), that's automatic. Many subsystems have >> Kconfig options to turn on -DDEBUG." >> >> While that does explain that pr_debug() won't actually print anything >> without DEBUG defined, it's hardly in a way that jumps out, clearly >> indicating that pr_debug() is unlike all the other pr_XXX() functions. >> >> I'll try sending a patch to update the docs to make pr_debug()'s >> behavior clearer... > > Admitting checkpatch is the authority in that matter and that per subsystem > debug granularity would be kept, we could at least add some specification > like below ? It would be even better if the note could clarify that sometimes it is ok to use printk(KERN_DEBUG > > [PATCH 1/1] scripts/checkpatch.pl: add printk(KERN_DEBUG to pr_debug specification > > Such conversions can't be done as trivially as other printk occurences. > > Signed-off-by: Fabian Frederick <fabf@skynet.be> > --- > scripts/checkpatch.pl | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 34eb216..4e462d7 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2863,9 +2863,14 @@ sub process { > my $level = lc($orig); > $level = "warn" if ($level eq "warning"); > my $level2 = $level; > - $level2 = "dbg" if ($level eq "debug"); > + my $note = ""; > + if ($level eq "debug"){ > + $level2 = "dbg"; > + $note = "Note that printk(KERN_DEBUG conversions to pr_debug require local DEBUG definition.\n"; > + } > WARN("PREFER_PR_LEVEL", > - "Prefer [subsystem eg: netdev]_$level2([subsystem]dev, ... then dev_$level2(dev, ... then pr_$level(... to printk(KERN_$orig ...\n" . $herecurr); > + "Prefer [subsystem eg: netdev]_$level2([subsystem]dev, ... then dev_$level2(dev, ... then pr_$level(... to printk(KERN_$orig ...\n$note" . $herecurr); > + > } > > if ($line =~ /\bpr_warning\s*\(/) { > -- > 1.8.4.5 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-07 14:21 ` Dan Streetman @ 2014-05-07 14:35 ` Steven Rostedt 2014-05-07 18:10 ` Joe Perches 0 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2014-05-07 14:35 UTC (permalink / raw) To: Dan Streetman Cc: Fabian Frederick, Andrew Morton, Paul Gortmaker, Thomas Gleixner, Borislav Petkov, linux-kernel, Joe Perches On Wed, 7 May 2014 10:21:28 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > It would be even better if the note could clarify that sometimes it is > ok to use printk(KERN_DEBUG Exactly. I think it's rather stupid to have to do a #define DEBUG to have pr_debug() print in general. I see no reason to have pr_debug() be anything different than the other pr_*() functions. Perhaps the pr_debug() should have been called debug_print(), or dyn_print(), where it can be dynamic printk or enabled with a DEBUG macro. The plist code is a perfect scenario where printk(KERN_DEBUG...) is appropriate, and using pr_debug() with a hard coded #define DEBUG is just stupid. -- Steve ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-07 14:35 ` Steven Rostedt @ 2014-05-07 18:10 ` Joe Perches 2014-05-07 18:19 ` Steven Rostedt 0 siblings, 1 reply; 15+ messages in thread From: Joe Perches @ 2014-05-07 18:10 UTC (permalink / raw) To: Steven Rostedt Cc: Dan Streetman, Fabian Frederick, Andrew Morton, Paul Gortmaker, Thomas Gleixner, Borislav Petkov, linux-kernel On Wed, 2014-05-07 at 10:35 -0400, Steven Rostedt wrote: > On Wed, 7 May 2014 10:21:28 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > > It would be even better if the note could clarify that sometimes it is > > ok to use printk(KERN_DEBUG > > Exactly. I think it's rather stupid to have to do a #define DEBUG to > have pr_debug() print in general. > > I see no reason to have pr_debug() be anything different than the other > pr_*() functions. pr_debug is meant to be disabled and have _no_ runtime effect unless DEBUG is #defined. For embedded systems where printk is enabled, pr_debug does have some utility for code/text reduction and it can have a minor impact on performance. tracing is frequently a better option for development and is often a better runtime tool. CONFIG_DYNAMIC_DEBUG is not enabled by default in most defconfigs. The _only_ reason it's possible is because dynamic debug is runtime patched to be fairly cost free for relatively larger memory/higher performance systems. > The plist code is a perfect scenario where printk(KERN_DEBUG...) is > appropriate, and using pr_debug() with a hard coded #define DEBUG is > just stupid. yup. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-07 18:10 ` Joe Perches @ 2014-05-07 18:19 ` Steven Rostedt 2014-05-07 18:37 ` Joe Perches 0 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2014-05-07 18:19 UTC (permalink / raw) To: Joe Perches Cc: Dan Streetman, Fabian Frederick, Andrew Morton, Paul Gortmaker, Thomas Gleixner, Borislav Petkov, linux-kernel On Wed, 07 May 2014 11:10:38 -0700 Joe Perches <joe@perches.com> wrote: > On Wed, 2014-05-07 at 10:35 -0400, Steven Rostedt wrote: > > On Wed, 7 May 2014 10:21:28 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > > > > It would be even better if the note could clarify that sometimes it is > > > ok to use printk(KERN_DEBUG > > > > Exactly. I think it's rather stupid to have to do a #define DEBUG to > > have pr_debug() print in general. > > > > I see no reason to have pr_debug() be anything different than the other > > pr_*() functions. > > pr_debug is meant to be disabled and have _no_ runtime > effect unless DEBUG is #defined. I understand why it does it, but having pr_debug() named just like pr_info(), pr_notice(), pr_warning(), pr_err(), pr_crit(), pr_alert(), pr_emerg(), where all those are just printk(<LOGLEVEL>...) *except* for pr_debug(). That's inconsistent and wrong. pr_debug() should have been just printk(KERN_DEBUG ...) as that follows convention. Not something that gets disabled by default. For that, we should have given it a different name. That's the point I was trying to make. Yes, it's somewhat too late as pr_debug() is all over the place, but maybe when things slow down (Ha! like that will ever happen ... "are we done yet?"), then we could do a massive clean up and rename pr_debug() to something not so confusing in its usage compared to the other pr_*() prints. -- Steve ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] plist: replace pr_debug with printk in plist_test() 2014-05-07 18:19 ` Steven Rostedt @ 2014-05-07 18:37 ` Joe Perches 0 siblings, 0 replies; 15+ messages in thread From: Joe Perches @ 2014-05-07 18:37 UTC (permalink / raw) To: Steven Rostedt Cc: Dan Streetman, Fabian Frederick, Andrew Morton, Paul Gortmaker, Thomas Gleixner, Borislav Petkov, linux-kernel On Wed, 2014-05-07 at 14:19 -0400, Steven Rostedt wrote: > On Wed, 07 May 2014 11:10:38 -0700 > Joe Perches <joe@perches.com> wrote: > > > On Wed, 2014-05-07 at 10:35 -0400, Steven Rostedt wrote: > > > On Wed, 7 May 2014 10:21:28 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > > > > > > It would be even better if the note could clarify that sometimes it is > > > > ok to use printk(KERN_DEBUG > > > > > > Exactly. I think it's rather stupid to have to do a #define DEBUG to > > > have pr_debug() print in general. > > > > > > I see no reason to have pr_debug() be anything different than the other > > > pr_*() functions. > > > > pr_debug is meant to be disabled and have _no_ runtime > > effect unless DEBUG is #defined. > > I understand why it does it, but having pr_debug() named just like > pr_info(), pr_notice(), pr_warning(), pr_err(), pr_crit(), pr_alert(), > pr_emerg(), where all those are just printk(<LOGLEVEL>...) *except* for > pr_debug(). That's inconsistent and wrong. > > pr_debug() should have been just printk(KERN_DEBUG ...) as that follows > convention. The convention history is kind of inverted. As you probably know, all the other pr_<level> macros other than pr_info were added some years after pr_debug. > Yes, it's somewhat too late as pr_debug() is all over the place, but > maybe when things slow down (Ha! like that will ever happen ... "are we > done yet?"), then we could do a massive clean up and rename pr_debug() > to something not so confusing in its usage compared to the other pr_*() > prints. g'luck with that. Renaming pr_warning to pr_warn has taken 4 years and it's only a 2:1 ratio in favor of pr_warn and there are _more_ uses of pr_warning today than when pr_warn was introduced. (1006 to 773) cheers, Joe ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-05-07 18:37 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-05-02 20:23 [PATCH] plist: include -DDEBUG if CONFIG_DEBUG_PI_LIST Dan Streetman 2014-05-02 20:41 ` Steven Rostedt 2014-05-05 14:35 ` Dan Streetman 2014-05-05 14:43 ` [PATCH] plist: replace pr_debug with printk in plist_test() Dan Streetman 2014-05-05 14:52 ` Steven Rostedt 2014-05-05 20:35 ` Andrew Morton 2014-05-05 20:52 ` Joe Perches 2014-05-06 12:30 ` Dan Streetman 2014-05-06 15:08 ` [PATCH] Documentation: expand/clarify debug documentation Dan Streetman 2014-05-06 15:44 ` [PATCH] plist: replace pr_debug with printk in plist_test() Fabian Frederick 2014-05-07 14:21 ` Dan Streetman 2014-05-07 14:35 ` Steven Rostedt 2014-05-07 18:10 ` Joe Perches 2014-05-07 18:19 ` Steven Rostedt 2014-05-07 18:37 ` Joe Perches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).