| Message ID | 1226959111-11221-1-git-send-email-tpiepho@freescale.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
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
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?
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
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.
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;
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
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.
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;
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(-)