On Tue, May 6, 2014 at 11:44 AM, Fabian Frederick <f...@skynet.be> wrote: > On Tue, 6 May 2014 08:30:56 -0400 > Dan Streetman <ddstr...@ieee.org> wrote: > >> On Mon, May 5, 2014 at 4:35 PM, Andrew Morton <a...@linux-foundation.org> >> wrote: >> > On Mon, 5 May 2014 10:43:05 -0400 Dan Streetman <ddstr...@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 <f...@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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/