linux-kernel - Re: [PATCH] printk: add halt_delay parameter for printk delay in halt phase

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 2 Jun 2009 21:27:05 +0800
From:	Dave Young <hidave.darkstar@...il.com>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] printk: add halt_delay parameter for printk delay in halt 
	phase

On Tue, Jun 2, 2009 at 6:56 PM, Ingo Molnar <mingo@...e.hu> wrote:
>
> * Dave Young <hidave.darkstar@...il.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@...il.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/