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

[Fabian Frederick <fabf@skynet.be>]

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