[PATCH] plist: include -DDEBUG if CONFIG_DEBUG_PI_LIST

[PATCH] plist: include -DDEBUG if CONFIG_DEBUG_PI_LIST

[Dan Streetman]

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

Re: [PATCH] plist: include -DDEBUG if CONFIG_DEBUG_PI_LIST

[Steven Rostedt]

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

Re: [PATCH] plist: include -DDEBUG if CONFIG_DEBUG_PI_LIST

[Dan Streetman]

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
>

[PATCH] plist: replace pr_debug with printk in plist_test()

[Dan Streetman]

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

Re: [PATCH] plist: replace pr_debug with printk in plist_test()

[Steven Rostedt]

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

Re: [PATCH] plist: replace pr_debug with printk in plist_test()

[Andrew Morton]

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 ;)

Re: [PATCH] plist: replace pr_debug with printk in plist_test()

[Joe Perches]

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...

Re: [PATCH] plist: replace pr_debug with printk in plist_test()

[Dan Streetman]

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 ;)

[PATCH] Documentation: expand/clarify debug documentation

[Dan Streetman]

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

Re: [PATCH] plist: replace pr_debug with printk in plist_test()

[Fabian Frederick]

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

Re: [PATCH] plist: replace pr_debug with printk in plist_test()

[Dan Streetman]

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
>

Re: [PATCH] plist: replace pr_debug with printk in plist_test()

[Steven Rostedt]

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

Re: [PATCH] plist: replace pr_debug with printk in plist_test()

[Joe Perches]

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.

Re: [PATCH] plist: replace pr_debug with printk in plist_test()

[Steven Rostedt]

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

Re: [PATCH] plist: replace pr_debug with printk in plist_test()

[Joe Perches]

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