All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] WARN(): add a \n to the message printk
@ 2009-06-15  7:08 Arjan van de Ven
  2009-06-15  9:09 ` Alan Cox
  2009-06-15 16:38 ` Linus Torvalds
  0 siblings, 2 replies; 12+ messages in thread
From: Arjan van de Ven @ 2009-06-15  7:08 UTC (permalink / raw)
  To: linux-kernel; +Cc: torvalds

>From 14cc1a7592e46a8b57081f90d7d54ab274ab7610 Mon Sep 17 00:00:00 2001
From: Arjan van de Ven <arjan@linux.intel.com>
Date: Mon, 15 Jun 2009 00:05:39 -0700
Subject: [PATCH] WARN(): add a \n to the message printk

many (most) users of WARN() don't have a \n at the end of their string;
as is understandable from the API usage point of view. But this means
that the backend needs to add this \n or the warning message
gets corrupted (as is seen by kerneloops.org).

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 kernel/panic.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 984b3ec..08ce451 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -355,8 +355,10 @@ static void warn_slowpath_common(const char *file, int line, void *caller, struc
 	if (board)
 		printk(KERN_WARNING "Hardware name: %s\n", board);
 
-	if (args)
+	if (args) {
 		vprintk(args->fmt, args->args);
+		printk("\n");
+	}
 
 	print_modules();
 	dump_stack();
-- 
1.6.0.6



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-15  7:08 [PATCH] WARN(): add a \n to the message printk Arjan van de Ven
@ 2009-06-15  9:09 ` Alan Cox
  2009-06-15 14:13   ` Arjan van de Ven
  2009-06-15 16:38 ` Linus Torvalds
  1 sibling, 1 reply; 12+ messages in thread
From: Alan Cox @ 2009-06-15  9:09 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: linux-kernel, torvalds

> +	if (args) {
>  		vprintk(args->fmt, args->args);
> +		printk("\n");

What stops another printk occuring between those two ?

Alan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-15  9:09 ` Alan Cox
@ 2009-06-15 14:13   ` Arjan van de Ven
  0 siblings, 0 replies; 12+ messages in thread
From: Arjan van de Ven @ 2009-06-15 14:13 UTC (permalink / raw)
  To: Alan Cox; +Cc: linux-kernel, torvalds

On Mon, 15 Jun 2009 10:09:51 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > +	if (args) {
> >  		vprintk(args->fmt, args->args);
> > +		printk("\n");
> 
> What stops another printk occuring between those two ?
> 

absolutely nothing... and in that case you get what you get in a
million other places in the kernel; a bit of a mix.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-15  7:08 [PATCH] WARN(): add a \n to the message printk Arjan van de Ven
  2009-06-15  9:09 ` Alan Cox
@ 2009-06-15 16:38 ` Linus Torvalds
  2009-06-15 16:58   ` Linus Torvalds
  1 sibling, 1 reply; 12+ messages in thread
From: Linus Torvalds @ 2009-06-15 16:38 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: linux-kernel



On Mon, 15 Jun 2009, Arjan van de Ven wrote:
>  
> -	if (args)
> +	if (args) {
>  		vprintk(args->fmt, args->args);
> +		printk("\n");
> +	}

I really don't like this. What if the format already does have a '\n'? And 
what if some other CPU is printing at the same time?

I'd almost be open to adding a "flags" field to vprintk, and allow setting 
things like "finish line with \n" there. Or perhaps even better, have a 
"vprintk_line()" function that does it with no dynamic flags. Maybe make 
it static, and move all these panic helper funtions into kernel/printk.c 
(since this really is a special case).	

I dunno. I'm just throwing out suggestions. I just don't think the above 
patch is very nice.

		Linus

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-15 16:38 ` Linus Torvalds
@ 2009-06-15 16:58   ` Linus Torvalds
  2009-06-15 17:10     ` Ingo Molnar
  2009-06-15 17:57     ` Linus Torvalds
  0 siblings, 2 replies; 12+ messages in thread
From: Linus Torvalds @ 2009-06-15 16:58 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Linux Kernel Mailing List, Alan Cox



On Mon, 15 Jun 2009, Linus Torvalds wrote:
> 
> On Mon, 15 Jun 2009, Arjan van de Ven wrote:
> >  
> > -	if (args)
> > +	if (args) {
> >  		vprintk(args->fmt, args->args);
> > +		printk("\n");
> > +	}
> 
> I really don't like this. What if the format already does have a '\n'? And 
> what if some other CPU is printing at the same time?
> 
> I'd almost be open to adding a "flags" field to vprintk, and allow setting 
> things like "finish line with \n" there. Or perhaps even better, have a 
> "vprintk_line()" function that does it with no dynamic flags. Maybe make 
> it static, and move all these panic helper funtions into kernel/printk.c 
> (since this really is a special case).	
> 
> I dunno. I'm just throwing out suggestions. I just don't think the above 
> patch is very nice.

Oh, I actually think I have a preference.

I think we should _always_ cause a line break at the beginning of a new 
line, unless the new printk() starts with a KERN_CONT thing.

Right now KERN_CONT is "", but we could easily make it an explicit 
"loglevel" thing. Like this.

NOTE! This is, of course, totally untested. And we're bound to have 
continuation printk's that don't have the KERN_CONT at front, and need 
them added, but I think this is generally a saner model than what we have 
now, or your suggested explicit addition of '\n'.

Basically, it tries to guarantee that new messages always get a newline, 
unless they _explicitly_ say that they don't want one. Doesn't that make 
sense?

		Linus

---
 include/linux/kernel.h |    2 +-
 kernel/printk.c        |   16 +++++++++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 883cd44..066bb1e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -102,7 +102,7 @@ extern const char linux_proc_banner[];
  * line that had no enclosing \n). Only to be used by core/arch code
  * during early bootup (a continued line is not SMP-safe otherwise).
  */
-#define	KERN_CONT	""
+#define	KERN_CONT	"<c>"
 
 extern int console_printk[];
 
diff --git a/kernel/printk.c b/kernel/printk.c
index 5052b54..6f416fd 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -691,7 +691,21 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 	 * Copy the output into log_buf.  If the caller didn't provide
 	 * appropriate log level tags, we insert them here
 	 */
-	for (p = printk_buf; *p; p++) {
+	p = printk_buf;
+
+	/* Are we continuing a previous printk? */
+	if (!new_text_line) {
+		if (!memcmp(p, KERN_CONT, 3)) {
+			/* We intended to do that continued printk! */
+			p += 3;
+		} else {
+			/* Force a line break */
+			emit_log_char('\n');
+			new_text_line = 1;
+		}
+	}
+
+	for ( ; *p; p++) {
 		if (new_text_line) {
 			/* If a token, set current_log_level and skip over */
 			if (p[0] == '<' && p[1] >= '0' && p[1] <= '7' &&

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-15 16:58   ` Linus Torvalds
@ 2009-06-15 17:10     ` Ingo Molnar
  2009-06-16  4:04       ` Linus Torvalds
  2009-06-15 17:57     ` Linus Torvalds
  1 sibling, 1 reply; 12+ messages in thread
From: Ingo Molnar @ 2009-06-15 17:10 UTC (permalink / raw)
  To: Linus Torvalds, Frédéric Weisbecker, Li Zefan
  Cc: Arjan van de Ven, Linux Kernel Mailing List, Alan Cox


* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, 15 Jun 2009, Linus Torvalds wrote:
> > 
> > On Mon, 15 Jun 2009, Arjan van de Ven wrote:
> > >  
> > > -	if (args)
> > > +	if (args) {
> > >  		vprintk(args->fmt, args->args);
> > > +		printk("\n");
> > > +	}
> > 
> > I really don't like this. What if the format already does have a '\n'? And 
> > what if some other CPU is printing at the same time?
> > 
> > I'd almost be open to adding a "flags" field to vprintk, and allow setting 
> > things like "finish line with \n" there. Or perhaps even better, have a 
> > "vprintk_line()" function that does it with no dynamic flags. Maybe make 
> > it static, and move all these panic helper funtions into kernel/printk.c 
> > (since this really is a special case).	
> > 
> > I dunno. I'm just throwing out suggestions. I just don't think the above 
> > patch is very nice.
> 
> Oh, I actually think I have a preference.
> 
> I think we should _always_ cause a line break at the beginning of a new 
> line, unless the new printk() starts with a KERN_CONT thing.
> 
> Right now KERN_CONT is "", but we could easily make it an explicit 
> "loglevel" thing. Like this.
> 
> NOTE! This is, of course, totally untested. And we're bound to have 
> continuation printk's that don't have the KERN_CONT at front, and need 
> them added, but I think this is generally a saner model than what we have 
> now, or your suggested explicit addition of '\n'.
> 
> Basically, it tries to guarantee that new messages always get a newline, 
> unless they _explicitly_ say that they don't want one. Doesn't that make 
> sense?
> 
> 		Linus
> 
> ---
>  include/linux/kernel.h |    2 +-
>  kernel/printk.c        |   16 +++++++++++++++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 883cd44..066bb1e 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -102,7 +102,7 @@ extern const char linux_proc_banner[];
>   * line that had no enclosing \n). Only to be used by core/arch code
>   * during early bootup (a continued line is not SMP-safe otherwise).
>   */
> -#define	KERN_CONT	""
> +#define	KERN_CONT	"<c>"
>  
>  extern int console_printk[];
>  
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 5052b54..6f416fd 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -691,7 +691,21 @@ asmlinkage int vprintk(const char *fmt, va_list args)
>  	 * Copy the output into log_buf.  If the caller didn't provide
>  	 * appropriate log level tags, we insert them here
>  	 */
> -	for (p = printk_buf; *p; p++) {
> +	p = printk_buf;
> +
> +	/* Are we continuing a previous printk? */
> +	if (!new_text_line) {
> +		if (!memcmp(p, KERN_CONT, 3)) {
> +			/* We intended to do that continued printk! */
> +			p += 3;
> +		} else {
> +			/* Force a line break */
> +			emit_log_char('\n');
> +			new_text_line = 1;
> +		}
> +	}
> +

Nice idea ...

Puts some pressure on current intentionally 'naked' printks (there's 
still a few of them) - but that's OK, it's not like KERN_CONT (or 
pr_cont()) is that hard to add.

( Plus many of our boot printks (where most of the 'naked' printks 
  are currently occuring) are development leftovers and should 
  really be removed, so it's good to shake them up a bit. )

	Ingo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-15 16:58   ` Linus Torvalds
  2009-06-15 17:10     ` Ingo Molnar
@ 2009-06-15 17:57     ` Linus Torvalds
  2009-06-15 18:39       ` Ingo Molnar
  2009-06-15 18:53       ` Frederic Weisbecker
  1 sibling, 2 replies; 12+ messages in thread
From: Linus Torvalds @ 2009-06-15 17:57 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Linux Kernel Mailing List, Alan Cox



On Mon, 15 Jun 2009, Linus Torvalds wrote:
> 
> NOTE! This is, of course, totally untested. And we're bound to have 
> continuation printk's that don't have the KERN_CONT at front, and need 
> them added, but I think this is generally a saner model than what we have 
> now, or your suggested explicit addition of '\n'.

Ok, it's tested now.

It works, and yes, it does show cases of insanity: both missing KERN_CONT 
(common), and _extra_ KERN_CONT (odd).

For example, the ACPI printk's seem to have pointless KERN_CONT's in them, 
an I get printouts like:

	[    0.000000] <c>ACPI: RSDP 00000000000fe020 00024 (v02 INTEL )
	[    0.000000] <c>ACPI: XSDT 00000000bf7fe120 0006C (v01 INTEL  DX58SO   0000084F      01000013)
	[    0.000000] <c>ACPI: FACP 00000000bf7fd000 000F4 (v03 INTEL  DX58SO   0000084F MSFT 0100000D)
	...

where that "<c>" is just because ACPI does something odd and pointless.

The lack of KERN_CONT shows up in printouts like

	[   26.626492] CPU: L1 I cache: 32K
	[   26.626492] , L1 D cache: 32K
	...
	[   26.826201] ACPI: (supports S0
	[   26.826243]  S5
	[   26.826326] )
	...
	[   26.839617] ACPI: PCI Interrupt Link [LNKA] (IRQs
	[   26.839660]  3
	[   26.839741]  4
	[   26.839823]  5
	[   26.839904]  6
	[   26.839985]  7
	[   26.840067]  9
	[   26.840148]  10
	[   26.840230]  *11
	[   26.840313]  12
	[   26.840395]  14
	[   26.840476]  15
	[   26.840558] )
	...
	[   26.964999] ACPI: CPU0 (power states:
	[   26.965040]  C1[C1]
	[   26.965123]  C2[C3]
	[   26.965205] )
	...
	[   27.231268] ata_piix 0000:00:1f.5: MAP [
	[   27.231309]  P0
	[   27.231390]  --
	[   27.231472]  P1
	[   27.231553]  --
	[   27.231635]  ]
	...
	[   28.092534]  sda:
	[   28.092820]  sda1
	[   28.092910]  sda2
	...

where the kernel now added a newline because they were separate printk's 
and didn't have KERN_CONT on the continuation.

But despite seeing these kinds of things, I do think the patch is the 
right thing to do. It just shows that since KERN_CONT didn't use to _do_ 
anything, people obviously mis-used it. It's the old "if it wasn't tested, 
it's buggy" thing, but none of these look in the least like serious 
problems to the approach.

Comments? We could make it remove the unnecessary '<c>' things (so that 
you can always add KERN_CONT if you _want_ to), but the patch as-is will 
show them because I think it's useful to see them. 

			Linus

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-15 17:57     ` Linus Torvalds
@ 2009-06-15 18:39       ` Ingo Molnar
  2009-06-15 18:53       ` Frederic Weisbecker
  1 sibling, 0 replies; 12+ messages in thread
From: Ingo Molnar @ 2009-06-15 18:39 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Arjan van de Ven, Linux Kernel Mailing List, Alan Cox


* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, 15 Jun 2009, Linus Torvalds wrote:
> > 
> > NOTE! This is, of course, totally untested. And we're bound to have 
> > continuation printk's that don't have the KERN_CONT at front, and need 
> > them added, but I think this is generally a saner model than what we have 
> > now, or your suggested explicit addition of '\n'.
> 
> Ok, it's tested now.
> 
> It works, and yes, it does show cases of insanity: both missing KERN_CONT 
> (common), and _extra_ KERN_CONT (odd).
> 
> For example, the ACPI printk's seem to have pointless KERN_CONT's in them, 
> an I get printouts like:
> 
> 	[    0.000000] <c>ACPI: RSDP 00000000000fe020 00024 (v02 INTEL )
> 	[    0.000000] <c>ACPI: XSDT 00000000bf7fe120 0006C (v01 INTEL  DX58SO   0000084F      01000013)
> 	[    0.000000] <c>ACPI: FACP 00000000bf7fd000 000F4 (v03 INTEL  DX58SO   0000084F MSFT 0100000D)
> 	...
> 
> where that "<c>" is just because ACPI does something odd and pointless.
> 
> The lack of KERN_CONT shows up in printouts like
> 
> 	[   26.626492] CPU: L1 I cache: 32K
> 	[   26.626492] , L1 D cache: 32K
> 	...
> 	[   26.826201] ACPI: (supports S0
> 	[   26.826243]  S5
> 	[   26.826326] )
> 	...
> 	[   26.839617] ACPI: PCI Interrupt Link [LNKA] (IRQs
> 	[   26.839660]  3
> 	[   26.839741]  4
> 	[   26.839823]  5
> 	[   26.839904]  6
> 	[   26.839985]  7
> 	[   26.840067]  9
> 	[   26.840148]  10
> 	[   26.840230]  *11
> 	[   26.840313]  12
> 	[   26.840395]  14
> 	[   26.840476]  15
> 	[   26.840558] )
> 	...
> 	[   26.964999] ACPI: CPU0 (power states:
> 	[   26.965040]  C1[C1]
> 	[   26.965123]  C2[C3]
> 	[   26.965205] )
> 	...
> 	[   27.231268] ata_piix 0000:00:1f.5: MAP [
> 	[   27.231309]  P0
> 	[   27.231390]  --
> 	[   27.231472]  P1
> 	[   27.231553]  --
> 	[   27.231635]  ]
> 	...
> 	[   28.092534]  sda:
> 	[   28.092820]  sda1
> 	[   28.092910]  sda2
> 	...
> 
> where the kernel now added a newline because they were separate 
> printk's and didn't have KERN_CONT on the continuation.
> 
> But despite seeing these kinds of things, I do think the patch is 
> the right thing to do. It just shows that since KERN_CONT didn't 
> use to _do_ anything, people obviously mis-used it. It's the old 
> "if it wasn't tested, it's buggy" thing, but none of these look in 
> the least like serious problems to the approach.
> 
> Comments? We could make it remove the unnecessary '<c>' things (so 
> that you can always add KERN_CONT if you _want_ to), but the patch 
> as-is will show them because I think it's useful to see them.

I like this - it makes KERN_CONT truly functional. (Should have 
thought of that myself when i added KERN_CONT in 474925277.)

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-15 17:57     ` Linus Torvalds
  2009-06-15 18:39       ` Ingo Molnar
@ 2009-06-15 18:53       ` Frederic Weisbecker
  1 sibling, 0 replies; 12+ messages in thread
From: Frederic Weisbecker @ 2009-06-15 18:53 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Arjan van de Ven, Linux Kernel Mailing List, Alan Cox

On Mon, Jun 15, 2009 at 10:57:15AM -0700, Linus Torvalds wrote:
> 
> 
> On Mon, 15 Jun 2009, Linus Torvalds wrote:
> > 
> > NOTE! This is, of course, totally untested. And we're bound to have 
> > continuation printk's that don't have the KERN_CONT at front, and need 
> > them added, but I think this is generally a saner model than what we have 
> > now, or your suggested explicit addition of '\n'.
> 
> Ok, it's tested now.
> 
> It works, and yes, it does show cases of insanity: both missing KERN_CONT 
> (common), and _extra_ KERN_CONT (odd).
> 
> For example, the ACPI printk's seem to have pointless KERN_CONT's in them, 
> an I get printouts like:
> 
> 	[    0.000000] <c>ACPI: RSDP 00000000000fe020 00024 (v02 INTEL )
> 	[    0.000000] <c>ACPI: XSDT 00000000bf7fe120 0006C (v01 INTEL  DX58SO   0000084F      01000013)
> 	[    0.000000] <c>ACPI: FACP 00000000bf7fd000 000F4 (v03 INTEL  DX58SO   0000084F MSFT 0100000D)
> 	...
> 
> where that "<c>" is just because ACPI does something odd and pointless.
> 
> The lack of KERN_CONT shows up in printouts like
> 
> 	[   26.626492] CPU: L1 I cache: 32K
> 	[   26.626492] , L1 D cache: 32K
> 	...
> 	[   26.826201] ACPI: (supports S0
> 	[   26.826243]  S5
> 	[   26.826326] )
> 	...
> 	[   26.839617] ACPI: PCI Interrupt Link [LNKA] (IRQs
> 	[   26.839660]  3
> 	[   26.839741]  4
> 	[   26.839823]  5
> 	[   26.839904]  6
> 	[   26.839985]  7
> 	[   26.840067]  9
> 	[   26.840148]  10
> 	[   26.840230]  *11
> 	[   26.840313]  12
> 	[   26.840395]  14
> 	[   26.840476]  15
> 	[   26.840558] )
> 	...
> 	[   26.964999] ACPI: CPU0 (power states:
> 	[   26.965040]  C1[C1]
> 	[   26.965123]  C2[C3]
> 	[   26.965205] )
> 	...
> 	[   27.231268] ata_piix 0000:00:1f.5: MAP [
> 	[   27.231309]  P0
> 	[   27.231390]  --
> 	[   27.231472]  P1
> 	[   27.231553]  --
> 	[   27.231635]  ]
> 	...
> 	[   28.092534]  sda:
> 	[   28.092820]  sda1
> 	[   28.092910]  sda2
> 	...
> 
> where the kernel now added a newline because they were separate printk's 
> and didn't have KERN_CONT on the continuation.
> 
> But despite seeing these kinds of things, I do think the patch is the 
> right thing to do. It just shows that since KERN_CONT didn't use to _do_ 
> anything, people obviously mis-used it. It's the old "if it wasn't tested, 
> it's buggy" thing, but none of these look in the least like serious 
> problems to the approach.
> 
> Comments? We could make it remove the unnecessary '<c>' things (so that 
> you can always add KERN_CONT if you _want_ to), but the patch as-is will 
> show them because I think it's useful to see them. 
> 
> 			Linus

Nice, eventually KERN_CONT have now a real sense.
IMHO it's good to keep <c> in the beginning of the line for misuses like
ACPI does. It provides easy and quick checks to find them.

Frederic.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-15 17:10     ` Ingo Molnar
@ 2009-06-16  4:04       ` Linus Torvalds
  2009-06-16  4:16         ` Linus Torvalds
  2009-06-16  5:46         ` Arjan van de Ven
  0 siblings, 2 replies; 12+ messages in thread
From: Linus Torvalds @ 2009-06-16  4:04 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Frédéric Weisbecker, Li Zefan, Arjan van de Ven,
	Linux Kernel Mailing List, Alan Cox



On Mon, 15 Jun 2009, Ingo Molnar wrote:
> 
> Nice idea ...
> 
> Puts some pressure on current intentionally 'naked' printks (there's 
> still a few of them) - but that's OK, it's not like KERN_CONT (or 
> pr_cont()) is that hard to add.

I looked some more, and there's a _ton_ of these naked printk's in the 
partition handling code.

So while I think the patch was a good idea, I don't feel like exposing 
quite that many old printk's and forcing people to use KERN_CONT. Here's 
an alternate patch that has a somewhat similar approach, but tries much 
harder to leave naked printk's as-is.

So instead of always adding a '\n' if it doesn't say KERN_CONT, it just 
adds '\n' if it has a KERN_xyz level. It also modifies the code to _only_ 
look at the beginning of the printk - if you have a multi-line printk, 
it will take the log-level from the beginning of the printk, and nowhere 
else.

And it will take the log-level from the beginning of the printk 
*regardless* of whether it thinks you're at the beginning of a line or 
not.

So with this, KERN_CONT is not as important, but it_is_ meaningful: if you 
want to print out something like "<%d>", then you _have_ to have a 
KERN_xyz header, and if you don't want to force a new line, you have to do

	printk(KERN_CONT "<%d>", n);

because otherwise the printk code will think that what you want to print 
out is the loglevel.

But for all the traditional printk()'s that don't have KERN_CONT (or other 
loglevel info), and print out strings that are not of that "<.>" form, 
they'll still work as they used to.

And no, this does not necessarily fix Arjan's problem: it only adds the 
newline before printk's that _do_ have a KERN_<lvl> format. So now, in 
order to get the extra '\n' after the WARN_ON() line, somebody needs to 
make sure that the printk's in the warning printing have loglevels.

Arjan?

		Linus

---
 include/linux/kernel.h |    2 +-
 kernel/printk.c        |   31 ++++++++++++++++++++++---------
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 883cd44..066bb1e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -102,7 +102,7 @@ extern const char linux_proc_banner[];
  * line that had no enclosing \n). Only to be used by core/arch code
  * during early bootup (a continued line is not SMP-safe otherwise).
  */
-#define	KERN_CONT	""
+#define	KERN_CONT	"<c>"
 
 extern int console_printk[];
 
diff --git a/kernel/printk.c b/kernel/printk.c
index 5052b54..a87770c 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -687,20 +687,33 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 				  sizeof(printk_buf) - printed_len, fmt, args);
 
 
+	p = printk_buf;
+
+	/* Do we have a loglevel in the string? */
+	if (p[0] == '<') {
+		unsigned char c = p[1];
+		if (c && p[2] == '>') {
+			switch (c) {
+			case '0' ... '7': /* loglevel */
+				current_log_level = c - '0';
+				if (!new_text_line) {
+					emit_log_char('\n');
+					new_text_line = 1;
+				}
+			/* Fallthrough - skip the loglevel */
+			case 'c': /* KERN_CONT */
+				p += 3;
+				break;
+			}
+		}
+	}
+
 	/*
 	 * Copy the output into log_buf.  If the caller didn't provide
 	 * appropriate log level tags, we insert them here
 	 */
-	for (p = printk_buf; *p; p++) {
+	for ( ; *p; p++) {
 		if (new_text_line) {
-			/* If a token, set current_log_level and skip over */
-			if (p[0] == '<' && p[1] >= '0' && p[1] <= '7' &&
-			    p[2] == '>') {
-				current_log_level = p[1] - '0';
-				p += 3;
-				printed_len -= 3;
-			}
-
 			/* Always output the token */
 			emit_log_char('<');
 			emit_log_char(current_log_level + '0');

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-16  4:04       ` Linus Torvalds
@ 2009-06-16  4:16         ` Linus Torvalds
  2009-06-16  5:46         ` Arjan van de Ven
  1 sibling, 0 replies; 12+ messages in thread
From: Linus Torvalds @ 2009-06-16  4:16 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Frédéric Weisbecker, Li Zefan, Arjan van de Ven,
	Linux Kernel Mailing List, Alan Cox



On Mon, 15 Jun 2009, Linus Torvalds wrote:
> 
> And no, this does not necessarily fix Arjan's problem: it only adds the 
> newline before printk's that _do_ have a KERN_<lvl> format. So now, in 
> order to get the extra '\n' after the WARN_ON() line, somebody needs to 
> make sure that the printk's in the warning printing have loglevels.
> 
> Arjan?

The "print_modules()" function needs a KERN_WARNING in front of it.

Or something like this (on top of the patch I just sent out), which allows 
you to specify loglevel that is just the default one, whatever that 
happens to be. Using KERN_DEFAULT, of course.

Hmm?

		Linus

---
 include/linux/kernel.h |    2 ++
 kernel/module.c        |    2 +-
 kernel/printk.c        |    2 ++
 3 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 066bb1e..1b2e174 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -97,6 +97,8 @@ extern const char linux_proc_banner[];
 #define	KERN_INFO	"<6>"	/* informational			*/
 #define	KERN_DEBUG	"<7>"	/* debug-level messages			*/
 
+/* Use the default kernel loglevel */
+#define KERN_DEFAULT	"<d>"
 /*
  * Annotation for a "continued" line of log printout (only done after a
  * line that had no enclosing \n). Only to be used by core/arch code
diff --git a/kernel/module.c b/kernel/module.c
index e4ab36c..215aaab 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2899,7 +2899,7 @@ void print_modules(void)
 	struct module *mod;
 	char buf[8];
 
-	printk("Modules linked in:");
+	printk(KERN_DEFAULT "Modules linked in:");
 	/* Most callers should already have preempt disabled, but make sure */
 	preempt_disable();
 	list_for_each_entry_rcu(mod, &modules, list)
diff --git a/kernel/printk.c b/kernel/printk.c
index a87770c..b4d97b5 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -696,6 +696,8 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 			switch (c) {
 			case '0' ... '7': /* loglevel */
 				current_log_level = c - '0';
+			/* Fallthrough - make sure we're on a new line */
+			case 'd': /* KERN_DEFAULT */
 				if (!new_text_line) {
 					emit_log_char('\n');
 					new_text_line = 1;

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] WARN(): add a \n to the message printk
  2009-06-16  4:04       ` Linus Torvalds
  2009-06-16  4:16         ` Linus Torvalds
@ 2009-06-16  5:46         ` Arjan van de Ven
  1 sibling, 0 replies; 12+ messages in thread
From: Arjan van de Ven @ 2009-06-16  5:46 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Ingo Molnar, Frédéric Weisbecker, Li Zefan,
	Linux Kernel Mailing List, Alan Cox

On Mon, 15 Jun 2009 21:04:25 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> And no, this does not necessarily fix Arjan's problem: it only adds
> the newline before printk's that _do_ have a KERN_<lvl> format. So
> now, in order to get the extra '\n' after the WARN_ON() line,
> somebody needs to make sure that the printk's in the warning printing
> have loglevels.
> 
> Arjan?
> 

I liked the first patch more :-) ... but practical considerations make
this one more useful I suppose. I'll make the warnings work.

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2009-06-16  5:46 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-15  7:08 [PATCH] WARN(): add a \n to the message printk Arjan van de Ven
2009-06-15  9:09 ` Alan Cox
2009-06-15 14:13   ` Arjan van de Ven
2009-06-15 16:38 ` Linus Torvalds
2009-06-15 16:58   ` Linus Torvalds
2009-06-15 17:10     ` Ingo Molnar
2009-06-16  4:04       ` Linus Torvalds
2009-06-16  4:16         ` Linus Torvalds
2009-06-16  5:46         ` Arjan van de Ven
2009-06-15 17:57     ` Linus Torvalds
2009-06-15 18:39       ` Ingo Molnar
2009-06-15 18:53       ` Frederic Weisbecker

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.