Patchwork [v3,2/5] ARM: Add a printk loglevel modifier

login
register
mail settings
Submitter Uwe Kleine-König
Date March 14, 2012, 10:21 a.m.
Message ID <1331720516-11801-1-git-send-email-u.kleine-koenig@pengutronix.de>
Download mbox | patch
Permalink /patch/146593/
State New
Headers show

Comments

Uwe Kleine-König - March 14, 2012, 10:21 a.m.
This is a needed followup for

	ARM: protect usage of cr_alignment by #ifdef CONFIG_CPU_CP15

otherwise no newline is printed for !CONFIG_CPU_CP15

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Should this be folded into the

	ARM: protect usage of cr_alignment by #ifdef CONFIG_CPU_CP15

patch? Or should it come before it?

 arch/arm/kernel/setup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Arnd Bergmann - March 14, 2012, 1:12 p.m.
On Wednesday 14 March 2012, Uwe Kleine-König wrote:
> This is a needed followup for
> 
>         ARM: protect usage of cr_alignment by #ifdef CONFIG_CPU_CP15
> 
> otherwise no newline is printed for !CONFIG_CPU_CP15
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Should this be folded into the
> 
>         ARM: protect usage of cr_alignment by #ifdef CONFIG_CPU_CP15
> 
> patch? Or should it come before it?
> 

I think this patch needs a better explanation. It looks to me that
it's simply a bug fix, because every printk that is not a continuation
of a previous printk needs a KERN_* line. In that case, it should
just get applied as a bug fix independent of the other patches.

However, if you sometimes don't have a newline after the previous
printk, this patch is actually wrong and should not get applied at
all.

	Arnd
Nicolas Pitre - March 14, 2012, 3:26 p.m.
On Wed, 14 Mar 2012, Uwe Kleine-König wrote:

> This is a needed followup for
> 
> 	ARM: protect usage of cr_alignment by #ifdef CONFIG_CPU_CP15
> 
> otherwise no newline is printed for !CONFIG_CPU_CP15
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Should this be folded into the
> 
> 	ARM: protect usage of cr_alignment by #ifdef CONFIG_CPU_CP15
> 
> patch? Or should it come before it?

Given my comments on that other patch, this one could be dropped 
altogether.


Nicolas
Uwe Kleine-König - March 14, 2012, 8:06 p.m.
On Wed, Mar 14, 2012 at 01:12:51PM +0000, Arnd Bergmann wrote:
> On Wednesday 14 March 2012, Uwe Kleine-König wrote:
> > This is a needed followup for
> > 
> >         ARM: protect usage of cr_alignment by #ifdef CONFIG_CPU_CP15
> > 
> > otherwise no newline is printed for !CONFIG_CPU_CP15
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Should this be folded into the
> > 
> >         ARM: protect usage of cr_alignment by #ifdef CONFIG_CPU_CP15
> > 
> > patch? Or should it come before it?
> > 
> 
> I think this patch needs a better explanation. It looks to me that
> it's simply a bug fix, because every printk that is not a continuation
> of a previous printk needs a KERN_* line. In that case, it should
> just get applied as a bug fix independent of the other patches.
> 
> However, if you sometimes don't have a newline after the previous
> printk, this patch is actually wrong and should not get applied at
> all.
I don't get your reasoning here.

An (independant) printk (that is expected to start at a new line) must
start with KERN_SOMETHING iff the previous printk always ends in \n?
Probably not. 

Independently of the correctness of the first patch that splits the
printk I'm just taking it as an example:

In this case the two printks in question are:

		printk("CPU: %s [%08x] revision %d (ARMv%s)", ...);
	#ifdef CONFIG_CPU_CP15
		printk(KERN_CONT ", cr=%08lx\n", cr_alignment);
	#endif

and

		printk("CPU: %s data cache, %s instruction cache\n", ...)

In this case it's correct to add KERN_INFO to the last printk, isn't it?
When would you consider it to be wrong?

If you want to have the first group of printks to always end in \n, I'd
have to do:

		printk("CPU: %s [%08x] revision %d (ARMv%s)", ...);
	#ifdef CONFIG_CPU_CP15
		printk(KERN_CONT ", cr=%08lx", cr_alignment);
	#endif
		printk(KERN_CONT "\n");

but ISTR Linus (Torvalds) telling me that KERN_CONT "\n" is wrong and
should just be skipped.

Best regards
Uwe
Arnd Bergmann - March 14, 2012, 9:40 p.m.
On Wednesday 14 March 2012, Uwe Kleine-König wrote:
> I don't get your reasoning here.
> 
> An (independant) printk (that is expected to start at a new line) must
> start with KERN_SOMETHING iff the previous printk always ends in \n?
> Probably not. 
> 
> Independently of the correctness of the first patch that splits the
> printk I'm just taking it as an example:
> 
> In this case the two printks in question are:
> 
>                 printk("CPU: %s [%08x] revision %d (ARMv%s)", ...);
>         #ifdef CONFIG_CPU_CP15
>                 printk(KERN_CONT ", cr=%08lx\n", cr_alignment);
>         #endif
> 
> and
> 
>                 printk("CPU: %s data cache, %s instruction cache\n", ...)
> 
> In this case it's correct to add KERN_INFO to the last printk, isn't it?
> When would you consider it to be wrong?

I forgot about KERN_CONT, which did not exist until a few years ago
and is still not all that common. Initially, KERN_CONT was just an
empty string that was used to make it clear when a printk was
intentionally used as a continuation of the previous line, rather
than having no KERN_* at all.

In the example above, the first and the second line should get a KERN_INFO
or one of the others.

> If you want to have the first group of printks to always end in \n, I'd
> have to do:
> 
>                 printk("CPU: %s [%08x] revision %d (ARMv%s)", ...);
>         #ifdef CONFIG_CPU_CP15
>                 printk(KERN_CONT ", cr=%08lx", cr_alignment);
>         #endif
>                 printk(KERN_CONT "\n");
> 
> but ISTR Linus (Torvalds) telling me that KERN_CONT "\n" is wrong and
> should just be skipped.

Ok, didn't know that.

	Arnd

Patch

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 50d3df8..23893b1 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -325,7 +325,7 @@  static void __init cacheid_init(void)
 		cacheid = CACHEID_VIVT;
 	}
 
-	printk("CPU: %s data cache, %s instruction cache\n",
+	printk(KERN_INFO "CPU: %s data cache, %s instruction cache\n",
 		cache_is_vivt() ? "VIVT" :
 		cache_is_vipt_aliasing() ? "VIPT aliasing" :
 		cache_is_vipt_nonaliasing() ? "PIPT / VIPT nonaliasing" : "unknown",