| Message ID | 1415287626-25802-1-git-send-email-daniel.thompson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, 2014-11-06 at 15:27 +0000, Daniel Thompson wrote: > Currently when kdb traps printk messages then the raw log level prefix > (consisting of '\001' followed by a numeral) does not get stripped off > before the message is issued to the various I/O handlers supported by > kdb. This causes annoying visual noise as well as causing problems > grepping for ^. It is also a change of behaviour compared to normal usage > of printk() usage. For example <SysRq>-h ends up with different to that of > kdb's "sr h". > > This patch addresses the problem by stripping log levels from messages > before they are issued to the I/O handlers. Perhaps instead of stripping the logging level, maybe a KERN_SOH_ASCII 'char' sequence should be emitted as '<' 'char' '>' (see: printk:print_prefix) Maybe this should be added to stable from v3.6 when KERN_SOH_ASCII was first added.
On 06/11/14 16:13, Joe Perches wrote: > On Thu, 2014-11-06 at 15:27 +0000, Daniel Thompson wrote: >> Currently when kdb traps printk messages then the raw log level prefix >> (consisting of '\001' followed by a numeral) does not get stripped off >> before the message is issued to the various I/O handlers supported by >> kdb. This causes annoying visual noise as well as causing problems >> grepping for ^. It is also a change of behaviour compared to normal usage >> of printk() usage. For example <SysRq>-h ends up with different to that of >> kdb's "sr h". >> >> This patch addresses the problem by stripping log levels from messages >> before they are issued to the I/O handlers. > > Perhaps instead of stripping the logging level, > maybe a KERN_SOH_ASCII 'char' sequence should be > emitted as '<' 'char' '>' (see: printk:print_prefix) > > Maybe this should be added to stable from v3.6 > when KERN_SOH_ASCII was first added. You mean call the problem a regression and try to restore the original 3.5 behaviour? Maybe. However I have to confess that I don't really like the old behaviour. I'd view it as contradicting the normal behaviours of consoles (including the kgdbcon console). Why should printk() inside kdb show different text to printk() outside kdb? For me, having <5> and <c> scribbled all over the output of an "sr" command (which I think is probably the heaviest user of printk() inside kdb) never struck me as adding much value. Is the above paragraph convincing? To be honest I'd happy to make the change if this is critical for you since almost anything is better than dumping a non-printable character (which does all sorts of different things depending on the combination of terminal emuluator/screen/minicom I need to access the board or emulator). On the other hand if you really mean "perhaps and maybe" then I'd prefer to leave it as it it. Daniel.
On Thu, 2014-11-06 at 17:22 +0000, Daniel Thompson wrote: > On 06/11/14 16:13, Joe Perches wrote: > > On Thu, 2014-11-06 at 15:27 +0000, Daniel Thompson wrote: > >> Currently when kdb traps printk messages then the raw log level prefix > >> (consisting of '\001' followed by a numeral) does not get stripped off > >> before the message is issued to the various I/O handlers supported by > >> kdb. This causes annoying visual noise as well as causing problems > >> grepping for ^. It is also a change of behaviour compared to normal usage > >> of printk() usage. For example <SysRq>-h ends up with different to that of > >> kdb's "sr h". > >> > >> This patch addresses the problem by stripping log levels from messages > >> before they are issued to the I/O handlers. > > > > Perhaps instead of stripping the logging level, > > maybe a KERN_SOH_ASCII 'char' sequence should be > > emitted as '<' 'char' '>' (see: printk:print_prefix) > > > > Maybe this should be added to stable from v3.6 > > when KERN_SOH_ASCII was first added. > > You mean call the problem a regression and try to restore the original > 3.5 behaviour? Yes. I added KERN_SOH_ASCII so to me it's a regression. > However I have to confess that I don't really like the old behaviour. > I'd view it as contradicting the normal behaviours of consoles > (including the kgdbcon console). Why should printk() inside kdb show > different text to printk() outside kdb? For me, having <5> and <c> > scribbled all over the output of an "sr" command (which I think is > probably the heaviest user of printk() inside kdb) never struck me as > adding much value. > > Is the above paragraph convincing? I don't use it so I have a useful opinion. I don't recall that anyone has reported it in the 2+ years since so it doesn't seem widely used. But then again, this is a resend and I don't recall seeing it the first time either. > On the other hand if you really mean "perhaps and maybe" then I'd prefer > to leave it as it it. Your choice. btw: in the patch I suggest using printk_skip_level instead of the direct test here: + cp = kdb_buffer; + if (cp[0] == KERN_SOH_ASCII && cp[1] != '\0') + cp += 2; so this could be cp = printk_skip_level(kdb_buffer);
On 06/11/14 17:43, Joe Perches wrote: > On Thu, 2014-11-06 at 17:22 +0000, Daniel Thompson wrote: >> On 06/11/14 16:13, Joe Perches wrote: >>> On Thu, 2014-11-06 at 15:27 +0000, Daniel Thompson wrote: >>>> Currently when kdb traps printk messages then the raw log level prefix >>>> (consisting of '\001' followed by a numeral) does not get stripped off >>>> before the message is issued to the various I/O handlers supported by >>>> kdb. This causes annoying visual noise as well as causing problems >>>> grepping for ^. It is also a change of behaviour compared to normal usage >>>> of printk() usage. For example <SysRq>-h ends up with different to that of >>>> kdb's "sr h". >>>> >>>> This patch addresses the problem by stripping log levels from messages >>>> before they are issued to the I/O handlers. >>> >>> Perhaps instead of stripping the logging level, >>> maybe a KERN_SOH_ASCII 'char' sequence should be >>> emitted as '<' 'char' '>' (see: printk:print_prefix) >>> >>> Maybe this should be added to stable from v3.6 >>> when KERN_SOH_ASCII was first added. >> >> You mean call the problem a regression and try to restore the original >> 3.5 behaviour? > > Yes. > > I added KERN_SOH_ASCII so to me it's a regression. > >> However I have to confess that I don't really like the old behaviour. >> I'd view it as contradicting the normal behaviours of consoles >> (including the kgdbcon console). Why should printk() inside kdb show >> different text to printk() outside kdb? For me, having <5> and <c> >> scribbled all over the output of an "sr" command (which I think is >> probably the heaviest user of printk() inside kdb) never struck me as >> adding much value. >> >> Is the above paragraph convincing? > > I don't use it so I have a useful opinion. > > I don't recall that anyone has reported it in the > 2+ years since so it doesn't seem widely used. Does that arguing against Cc:ing stable@ or you think I should just put that on anyway and leave it for the stable committee? > But then again, this is a resend and I don't recall > seeing it the first time either. > >> On the other hand if you really mean "perhaps and maybe" then I'd prefer >> to leave it as it it. > > Your choice. > > btw: in the patch I suggest using printk_skip_level > instead of the direct test here: > > + cp = kdb_buffer; > + if (cp[0] == KERN_SOH_ASCII && cp[1] != '\0') > + cp += 2; > > so this could be > > cp = printk_skip_level(kdb_buffer); Excellent. I'll do that.
On Fri, 2014-11-07 at 09:57 +0000, Daniel Thompson wrote: > Does that arguing against Cc:ing stable@ or you think I should just put > that on anyway and leave it for the stable committee? It's a regression, I think you should send a fix for it to stable. It's up to the stable folks to figure out if they want to take it.
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 7c70812..381f297 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -691,19 +691,22 @@ kdb_printit: * Write to all consoles. */ retlen = strlen(kdb_buffer); + cp = kdb_buffer; + if (cp[0] == KERN_SOH_ASCII && cp[1] != '\0') + cp += 2; if (!dbg_kdb_mode && kgdb_connected) { - gdbstub_msg_write(kdb_buffer, retlen); + gdbstub_msg_write(cp, retlen - (cp - kdb_buffer)); } else { if (dbg_io_ops && !dbg_io_ops->is_console) { - len = retlen; - cp = kdb_buffer; + len = retlen - (cp - kdb_buffer); + cp2 = cp; while (len--) { - dbg_io_ops->write_char(*cp); - cp++; + dbg_io_ops->write_char(*cp2); + cp2++; } } while (c) { - c->write(c, kdb_buffer, retlen); + c->write(c, cp, retlen - (cp - kdb_buffer)); touch_nmi_watchdog(); c = c->next; } @@ -711,7 +714,10 @@ kdb_printit: if (logging) { saved_loglevel = console_loglevel; console_loglevel = CONSOLE_LOGLEVEL_SILENT; - printk(KERN_INFO "%s", kdb_buffer); + if (cp == kdb_buffer) + printk(KERN_INFO "%s", kdb_buffer); + else + printk("%s", kdb_buffer); } if (KDB_STATE(PAGER)) {
Currently when kdb traps printk messages then the raw log level prefix (consisting of '\001' followed by a numeral) does not get stripped off before the message is issued to the various I/O handlers supported by kdb. This causes annoying visual noise as well as causing problems grepping for ^. It is also a change of behaviour compared to normal usage of printk() usage. For example <SysRq>-h ends up with different to that of kdb's "sr h". This patch addresses the problem by stripping log levels from messages before they are issued to the I/O handlers. printk(), which is used as an I/O handler for logging purposes, is handled as a special case; if the caller provided a log level then this will be preserved. Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Cc: Jason Wessel <jason.wessel@windriver.com> --- Notes: This patch is tested both on arm (kgdboc=ttyAMA0) and x86_64 (kgdboc=kdb,ttyS0). kernel/debug/kdb/kdb_io.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) -- 1.9.3