[PATCH] powerpc: Silence timebase sync code

[PATCH] powerpc: Silence timebase sync code

[Trent Piepho]

It's over a dozen lines of output and doesn't appear to provide any useful
information.  Even after looking at the code, I'm in the dark about what
"score 299, offset 250" means.

Signed-off-by: Trent Piepho <tpiepho@freescale.com>
---
 arch/powerpc/kernel/smp-tbsync.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/smp-tbsync.c b/arch/powerpc/kernel/smp-tbsync.c
index bc892e6..b590135 100644
--- a/arch/powerpc/kernel/smp-tbsync.c
+++ b/arch/powerpc/kernel/smp-tbsync.c
@@ -113,7 +113,7 @@ void __devinit smp_generic_give_timebase(void)
 {
 	int i, score, score2, old, min=0, max=5000, offset=1000;
 
-	printk("Synchronizing timebase\n");
+	pr_info("Synchronizing timebase\n");
 
 	/* if this fails then this kernel won't work anyway... */
 	tbsync = kzalloc( sizeof(*tbsync), GFP_KERNEL );
@@ -123,14 +123,10 @@ void __devinit smp_generic_give_timebase(void)
 	while (!tbsync->ack)
 		barrier();
 
-	printk("Got ack\n");
-
 	/* binary search */
 	for (old = -1; old != offset ; offset = (min+max) / 2) {
 		score = start_contest(kSetAndTest, offset, NUM_ITER);
 
-		printk("score %d, offset %d\n", score, offset );
-
 		if( score > 0 )
 			max = offset;
 		else
@@ -140,8 +136,8 @@ void __devinit smp_generic_give_timebase(void)
 	score = start_contest(kSetAndTest, min, NUM_ITER);
 	score2 = start_contest(kSetAndTest, max, NUM_ITER);
 
-	printk("Min %d (score %d), Max %d (score %d)\n",
-	       min, score, max, score2);
+	pr_debug("Min %d (score %d), Max %d (score %d)\n",
+		 min, score, max, score2);
 	score = abs(score);
 	score2 = abs(score2);
 	offset = (score < score2) ? min : max;
@@ -155,7 +151,7 @@ void __devinit smp_generic_give_timebase(void)
 		if (score2 <= score || score2 < 20)
 			break;
 	}
-	printk("Final offset: %d (%d/%d)\n", offset, score2, NUM_ITER );
+	pr_debug("Final offset: %d (%d/%d)\n", offset, score2, NUM_ITER);
 
 	/* exiting */
 	tbsync->cmd = kExit;
-- 
1.5.4.1

Re: [PATCH] powerpc: Silence timebase sync code

[Kumar Gala]

On Nov 17, 2008, at 3:58 PM, Trent Piepho wrote:

> It's over a dozen lines of output and doesn't appear to provide any  
> useful
> information.  Even after looking at the code, I'm in the dark about  
> what
> "score 299, offset 250" means.
>
> Signed-off-by: Trent Piepho <tpiepho@freescale.com>
> ---
> arch/powerpc/kernel/smp-tbsync.c |   12 ++++--------
> 1 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/kernel/smp-tbsync.c b/arch/powerpc/kernel/ 
> smp-tbsync.c
> index bc892e6..b590135 100644
> --- a/arch/powerpc/kernel/smp-tbsync.c
> +++ b/arch/powerpc/kernel/smp-tbsync.c
> @@ -113,7 +113,7 @@ void __devinit smp_generic_give_timebase(void)
> {
> 	int i, score, score2, old, min=0, max=5000, offset=1000;
>
> -	printk("Synchronizing timebase\n");
> +	pr_info("Synchronizing timebase\n");

I think its useful to leave this as a printk.

- k

Re: [PATCH] powerpc: Silence timebase sync code

[Trent Piepho]

On Mon, 17 Nov 2008, Kumar Gala wrote:
> On Nov 17, 2008, at 3:58 PM, Trent Piepho wrote:
>
>> It's over a dozen lines of output and doesn't appear to provide any useful
>> information.  Even after looking at the code, I'm in the dark about what
>> "score 299, offset 250" means.
>> 
>> Signed-off-by: Trent Piepho <tpiepho@freescale.com>
>> ---
>> arch/powerpc/kernel/smp-tbsync.c |   12 ++++--------
>> 1 files changed, 4 insertions(+), 8 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/smp-tbsync.c 
>> b/arch/powerpc/kernel/smp-tbsync.c
>> index bc892e6..b590135 100644
>> --- a/arch/powerpc/kernel/smp-tbsync.c
>> +++ b/arch/powerpc/kernel/smp-tbsync.c
>> @@ -113,7 +113,7 @@ void __devinit smp_generic_give_timebase(void)
>> {
>>  int i, score, score2, old, min=0, max=5000, offset=1000;
>> 
>> -	printk("Synchronizing timebase\n");
>> +	pr_info("Synchronizing timebase\n");
>
> I think its useful to leave this as a printk.

#define pr_info(fmt, arg...) \
         printk(KERN_INFO fmt, ##arg)

Isn't printk with no level tag the same as KERN_INFO?

Re: [PATCH] powerpc: Silence timebase sync code

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]

On Mon, 2008-11-17 at 14:22 -0800, Trent Piepho wrote:
> On Mon, 17 Nov 2008, Kumar Gala wrote:
> > On Nov 17, 2008, at 3:58 PM, Trent Piepho wrote:
> >
> >> It's over a dozen lines of output and doesn't appear to provide any useful
> >> information.  Even after looking at the code, I'm in the dark about what
> >> "score 299, offset 250" means.
> >> 
> >> Signed-off-by: Trent Piepho <tpiepho@freescale.com>
> >> ---
> >> arch/powerpc/kernel/smp-tbsync.c |   12 ++++--------
> >> 1 files changed, 4 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/arch/powerpc/kernel/smp-tbsync.c 
> >> b/arch/powerpc/kernel/smp-tbsync.c
> >> index bc892e6..b590135 100644
> >> --- a/arch/powerpc/kernel/smp-tbsync.c
> >> +++ b/arch/powerpc/kernel/smp-tbsync.c
> >> @@ -113,7 +113,7 @@ void __devinit smp_generic_give_timebase(void)
> >> {
> >>  int i, score, score2, old, min=0, max=5000, offset=1000;
> >> 
> >> -	printk("Synchronizing timebase\n");
> >> +	pr_info("Synchronizing timebase\n");
> >
> > I think its useful to leave this as a printk.
> 
> #define pr_info(fmt, arg...) \
>          printk(KERN_INFO fmt, ##arg)
> 
> Isn't printk with no level tag the same as KERN_INFO?

Stuff like this should IMHO be printk(KERN_DEBUG ..)

That way it will show up in the log as long as you boot with 'debug' on
your command line, it doesn't require a kernel recompile to turn on. And
at the same time it doesn't spam the boot log for a normal boot.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] powerpc: Silence timebase sync code

[Trent Piepho]

On Tue, 18 Nov 2008, Michael Ellerman wrote:
> On Mon, 2008-11-17 at 14:22 -0800, Trent Piepho wrote:
>> On Mon, 17 Nov 2008, Kumar Gala wrote:
>>> On Nov 17, 2008, at 3:58 PM, Trent Piepho wrote:
>>>
>>>> It's over a dozen lines of output and doesn't appear to provide any useful
>>>> information.  Even after looking at the code, I'm in the dark about what
>>>> "score 299, offset 250" means.
>>>>
>>>> Signed-off-by: Trent Piepho <tpiepho@freescale.com>
>>>> ---
>>>> +++ b/arch/powerpc/kernel/smp-tbsync.c
>>>> @@ -113,7 +113,7 @@ void __devinit smp_generic_give_timebase(void)
>>>> {
>>>>  int i, score, score2, old, min=0, max=5000, offset=1000;
>>>>
>>>> -	printk("Synchronizing timebase\n");
>>>> +	pr_info("Synchronizing timebase\n");
>>>
>>> I think its useful to leave this as a printk.
>>
>> #define pr_info(fmt, arg...) \
>>          printk(KERN_INFO fmt, ##arg)
>>
>> Isn't printk with no level tag the same as KERN_INFO?
>
> Stuff like this should IMHO be printk(KERN_DEBUG ..)
>
> That way it will show up in the log as long as you boot with 'debug' on
> your command line, it doesn't require a kernel recompile to turn on. And
> at the same time it doesn't spam the boot log for a normal boot.

Originally my patched changed it to debug, but Kumar requested I keep it at
info level.  The timebase sync code might hang or be slow, so it's nice to
give a status output before doing it.  It seems just as good as most of the
info printks during boot.

Re: [PATCH] powerpc: Silence timebase sync code

[Kumar Gala]

On Nov 17, 2008, at 4:22 PM, Trent Piepho wrote:

> On Mon, 17 Nov 2008, Kumar Gala wrote:
>> On Nov 17, 2008, at 3:58 PM, Trent Piepho wrote:
>>
>>> It's over a dozen lines of output and doesn't appear to provide  
>>> any useful
>>> information.  Even after looking at the code, I'm in the dark  
>>> about what
>>> "score 299, offset 250" means.
>>>
>>> Signed-off-by: Trent Piepho <tpiepho@freescale.com>
>>> ---
>>> arch/powerpc/kernel/smp-tbsync.c |   12 ++++--------
>>> 1 files changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/smp-tbsync.c
>>> b/arch/powerpc/kernel/smp-tbsync.c
>>> index bc892e6..b590135 100644
>>> --- a/arch/powerpc/kernel/smp-tbsync.c
>>> +++ b/arch/powerpc/kernel/smp-tbsync.c
>>> @@ -113,7 +113,7 @@ void __devinit smp_generic_give_timebase(void)
>>> {
>>> int i, score, score2, old, min=0, max=5000, offset=1000;
>>>
>>> -	printk("Synchronizing timebase\n");
>>> +	pr_info("Synchronizing timebase\n");
>>
>> I think its useful to leave this as a printk.
>
> #define pr_info(fmt, arg...) \
>         printk(KERN_INFO fmt, ##arg)
>
> Isn't printk with no level tag the same as KERN_INFO?

oops, sorry.. read the pr_info() as pr_debug()

- k

Re: [PATCH] powerpc: Silence timebase sync code

[Benjamin Herrenschmidt]

On Mon, 2008-11-17 at 13:58 -0800, Trent Piepho wrote:
> It's over a dozen lines of output and doesn't appear to provide any useful
> information.  Even after looking at the code, I'm in the dark about what
> "score 299, offset 250" means.

Hah ! I had almost the same patch in my pile for some time :-)

> Signed-off-by: Trent Piepho <tpiepho@freescale.com>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> ---
>  arch/powerpc/kernel/smp-tbsync.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/smp-tbsync.c b/arch/powerpc/kernel/smp-tbsync.c
> index bc892e6..b590135 100644
> --- a/arch/powerpc/kernel/smp-tbsync.c
> +++ b/arch/powerpc/kernel/smp-tbsync.c
> @@ -113,7 +113,7 @@ void __devinit smp_generic_give_timebase(void)
>  {
>  	int i, score, score2, old, min=0, max=5000, offset=1000;
>  
> -	printk("Synchronizing timebase\n");
> +	pr_info("Synchronizing timebase\n");
>  
>  	/* if this fails then this kernel won't work anyway... */
>  	tbsync = kzalloc( sizeof(*tbsync), GFP_KERNEL );
> @@ -123,14 +123,10 @@ void __devinit smp_generic_give_timebase(void)
>  	while (!tbsync->ack)
>  		barrier();
>  
> -	printk("Got ack\n");
> -
>  	/* binary search */
>  	for (old = -1; old != offset ; offset = (min+max) / 2) {
>  		score = start_contest(kSetAndTest, offset, NUM_ITER);
>  
> -		printk("score %d, offset %d\n", score, offset );
> -
>  		if( score > 0 )
>  			max = offset;
>  		else
> @@ -140,8 +136,8 @@ void __devinit smp_generic_give_timebase(void)
>  	score = start_contest(kSetAndTest, min, NUM_ITER);
>  	score2 = start_contest(kSetAndTest, max, NUM_ITER);
>  
> -	printk("Min %d (score %d), Max %d (score %d)\n",
> -	       min, score, max, score2);
> +	pr_debug("Min %d (score %d), Max %d (score %d)\n",
> +		 min, score, max, score2);
>  	score = abs(score);
>  	score2 = abs(score2);
>  	offset = (score < score2) ? min : max;
> @@ -155,7 +151,7 @@ void __devinit smp_generic_give_timebase(void)
>  		if (score2 <= score || score2 < 20)
>  			break;
>  	}
> -	printk("Final offset: %d (%d/%d)\n", offset, score2, NUM_ITER );
> +	pr_debug("Final offset: %d (%d/%d)\n", offset, score2, NUM_ITER);
>  
>  	/* exiting */
>  	tbsync->cmd = kExit;

Re: [PATCH] powerpc: Silence timebase sync code

[Paul Mackerras]

Trent Piepho writes:

> It's over a dozen lines of output and doesn't appear to provide any useful
> information.  Even after looking at the code, I'm in the dark about what
> "score 299, offset 250" means.

I already put a very similar patch from Ben Herrenschmidt into my
powerpc.git tree next branch, back on 5 November.

Paul.