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@xxxxxxxxx> wrote:
> On Tue, 6 May 2014 08:30:56 -0400
> Dan Streetman <ddstreet@xxxxxxxx> wrote:
>
>> 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...
>
> 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@xxxxxxxxx>
> ---
>  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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/