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@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon,  5 May 2014 10:43:05 -0400 Dan Streetman <ddstreet@xxxxxxxx> 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 ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/