2014-04-14 22:55:30

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH v2] mm: convert some level-less printks to pr_*

On Thu, 27 Mar 2014 10:54:19 -0700 Mitchel Humpherys <[email protected]> wrote:

> printk is meant to be used with an associated log level. There are some
> instances of printk scattered around the mm code where the log level is
> missing. Add a log level and adhere to suggestions by
> scripts/checkpatch.pl by moving to the pr_* macros.
>

hm, this is a functional change.

> --- a/mm/bounce.c
> +++ b/mm/bounce.c
> @@ -3,6 +3,8 @@
> * - Split from highmem.c
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Because of this.

> #include <linux/mm.h>
> #include <linux/export.h>
> #include <linux/swap.h>
> @@ -15,6 +17,7 @@
> #include <linux/hash.h>
> #include <linux/highmem.h>
> #include <linux/bootmem.h>
> +#include <linux/printk.h>
> #include <asm/tlbflush.h>
>
> #include <trace/events/block.h>
> @@ -34,7 +37,7 @@ static __init int init_emergency_pool(void)
>
> page_pool = mempool_create_page_pool(POOL_SIZE, 0);
> BUG_ON(!page_pool);
> - printk("bounce pool size: %d pages\n", POOL_SIZE);
> + pr_info("bounce pool size: %d pages\n", POOL_SIZE);

This used to print "bounce pool size: N pages" but will now print
"bounce: bounce pool size: N pages".

It isn't necessarily a *bad* change but perhaps a little more thought
could be put into it. In this example it would be better remove the
redundancy by using

pr_info("pool size: %d pages\n"...);

And all of this should be described and justified in the changelog,
please.


2014-04-15 23:58:16

by Mitchel Humpherys

[permalink] [raw]
Subject: Re: [PATCH v2] mm: convert some level-less printks to pr_*

On Mon, Apr 14 2014 at 03:55:26 PM, Andrew Morton <[email protected]> wrote:
> On Thu, 27 Mar 2014 10:54:19 -0700 Mitchel Humpherys <[email protected]> wrote:
>> #include <linux/mm.h>
>> #include <linux/export.h>
>> #include <linux/swap.h>
>> @@ -15,6 +17,7 @@
>> #include <linux/hash.h>
>> #include <linux/highmem.h>
>> #include <linux/bootmem.h>
>> +#include <linux/printk.h>
>> #include <asm/tlbflush.h>
>>
>> #include <trace/events/block.h>
>> @@ -34,7 +37,7 @@ static __init int init_emergency_pool(void)
>>
>> page_pool = mempool_create_page_pool(POOL_SIZE, 0);
>> BUG_ON(!page_pool);
>> - printk("bounce pool size: %d pages\n", POOL_SIZE);
>> + pr_info("bounce pool size: %d pages\n", POOL_SIZE);
>
> This used to print "bounce pool size: N pages" but will now print
> "bounce: bounce pool size: N pages".
>
> It isn't necessarily a *bad* change but perhaps a little more thought
> could be put into it. In this example it would be better remove the
> redundancy by using
>
> pr_info("pool size: %d pages\n"...);

Yes I noticed this in my boot-test... I'll change it to remove the
redundancy. The others all seem okay.

>
> And all of this should be described and justified in the changelog,
> please.

Will send a v3 shortly. Thanks for your comments.

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2014-04-16 00:05:48

by Mitchel Humpherys

[permalink] [raw]
Subject: Re: [PATCH v2] mm: convert some level-less printks to pr_*

On Tue, Apr 15 2014 at 04:58:21 PM, Mitchel Humpherys <[email protected]> wrote:
> On Mon, Apr 14 2014 at 03:55:26 PM, Andrew Morton <[email protected]> wrote:
>> And all of this should be described and justified in the changelog,
>> please.
>
> Will send a v3 shortly. Thanks for your comments.

Make that a v4, I actually already sent a v3. You'd think I could get a
printk change right on v1 :). We'll get there.

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation