lkml.org 
[lkml]   [2009]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] printk: add halt_delay parameter for printk delay in halt phase
From
On Tue, Jun 2, 2009 at 6:56 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Dave Young <hidave.darkstar@gmail.com> wrote:
>
>> Add a halt_delay module parameter in printk.c used to read the printk
>> messages in halt/poweroff/restart phase, delay each printk messages
>> by halt_delay milliseconds. It is useful for debugging if there's no
>> other way to dump kernel messages that time.
>
> nice idea! We frequently have kernel-death warnings/messages that
> scroll off too fast and which cannot be captured.

Thanks! It is for this case indeed.

>
>> halt_delay default value is 0, change it by:
>>
>> echo xxx > /sys/module/printk/parameters/halt_delay
>>
>> Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
>> ---
>> kernel/printk.c   |   21 +++++++++++++++++++++
>> lib/Kconfig.debug |   10 ++++++++++
>> 2 files changed, 31 insertions(+)
>>
>> --- linux-2.6.orig/kernel/printk.c    2009-04-09 16:23:03.000000000 +0800
>> +++ linux-2.6/kernel/printk.c 2009-06-02 17:45:54.000000000 +0800
>> @@ -250,6 +250,26 @@ static inline void boot_delay_msec(void)
>>  }
>>  #endif
>>
>> +#ifdef CONFIG_HALT_PRINTK_DELAY
>
> (this #ifdef is ugly - see below.)
>
>> +/* msecs delay after each halt/poweroff/restart phase printk */
>> +static unsigned int halt_delay;
>> +
>> +static inline void halt_delay_msec(void)
>> +{
>> +     if (halt_delay == 0 || !(system_state == SYSTEM_HALT
>> +                             || system_state == SYSTEM_POWER_OFF
>> +                             || system_state == SYSTEM_RESTART))
>> +             return;
>> +
>> +     mdelay(halt_delay);
>
>>
>>       boot_delay_msec();
>> +     halt_delay_msec();
>
>
> i think it should be done in boot_delay_msec() and the function
> should be renamed to print_delay_msec() or so.

I have two concerns:

1. boot_delay use a busy looping with lpj preset because it's too
early that mdelay is probably not ready yet. Furthermore the delay is
not accurate. For halt issue we can just use mdelay()

2. boot_delay is set by kernel boot paramenter, but IMHO, for halt
delay use sysfs file is more convenient.

>
>>       preempt_disable();
>>       /* This stops the holder of console_sem just where we want him */
>> --- linux-2.6.orig/lib/Kconfig.debug  2009-05-12 11:27:22.000000000 +0800
>> +++ linux-2.6/lib/Kconfig.debug       2009-06-02 17:50:15.000000000 +0800
>> @@ -647,6 +647,16 @@ config BOOT_PRINTK_DELAY
>>         BOOT_PRINTK_DELAY also may cause DETECT_SOFTLOCKUP to detect
>>         what it believes to be lockup conditions.
>>
>> +config HALT_PRINTK_DELAY
>> +     bool "Delay halt/poweroff/restart printk message by N milliseconds"
>> +     depends on DEBUG_KERNEL && PRINTK && SYSFS
>> +     default n
>> +     help
>> +       This build option allows you to read kernel messages in
>> +       halt/poweroff/restart phase by inserting a short delay after
>> +       each one.  The delay is specified in milliseconds on the
>> +       module parameter: /sys/module/printk/halt_delay.
>
> No need for a Kconfig option - this should be an unconditional
> feature like boot-delay.
>
> This will further simplify the patch and will get rid of that ugly
> #ifdef.

What do you think removing the Kconfig option, then unconditionally
use halt_delay functions?

>
>        Ingo
>



--
Regards
dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2009-06-02 15:29    [W:0.297 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site