To keep panic_timeout accuracy when running under a hypervisor, the
current implementation only spins on long time (1 second) calls to
mdelay. That brings a good effect, but the problem is the keyboard
LEDs don't blink at all on that situation.
This patch changes to call to panic_blink_enter() between every mdelay
and keeps blinking in spite of long spin timer mode.
The default time to call to mdelay is 1ms. If the speed of blinking
is slow enough, the time to call to mdelay will be automatically
switched to longer. This is suitable when running under a hypervisor.
Signed-off-by: TAMUKI Shoichi <[email protected]>
---
Changes since v1:
- get rid of panic_longspin kernel parameter
- add the explanation of panicblink= to kernel-parameters.txt
- panic_blink_enter(): should be static
- next_count in panic_blink_enter(): unnecessary to initialize
- move sanity checks in panic_blink_enter() to an appropriate place
Documentation/kernel-parameters.txt | 8 +-
arch/arm/mach-s3c2440/mach-gta02.c | 17 +----
drivers/input/serio/i8042.c | 25 +-------
include/linux/kernel.h | 2
kernel/panic.c | 77 +++++++++++++++++---------
5 files changed, 67 insertions(+), 62 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ff9f1a8..70d7f2e 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -917,9 +917,6 @@ and is between 256 and 4096 characters. It is defined in the file
controller
i8042.nopnp [HW] Don't use ACPIPnP / PnPBIOS to discover KBD/AUX
controllers
- i8042.panicblink=
- [HW] Frequency with which keyboard LEDs should blink
- when kernel panics (default is 0.5 sec)
i8042.reset [HW] Reset the controller during init and cleanup
i8042.unlock [HW] Unlock (ignore) the keylock
@@ -1871,6 +1868,11 @@ and is between 256 and 4096 characters. It is defined in the file
panic= [KNL] Kernel behaviour on panic
Format: <timeout>
+ panicblink= [KNL] The speed of panic blink (default is 12 wpm)
+ The period of panic blink can be computed by the
+ formula T = 7200 / W, where T is the period in milli-
+ seconds, W is the speed in wpm (words per minute).
+ Should be 5 or less when running under a hypervisor
parkbd.port= [HW] Parallel port number the keyboard adapter is
connected to, default is 0.
diff --git a/arch/arm/mach-s3c2440/mach-gta02.c b/arch/arm/mach-s3c2440/mach-gta02.c
index 9e39faa..deaabe8 100644
--- a/arch/arm/mach-s3c2440/mach-gta02.c
+++ b/arch/arm/mach-s3c2440/mach-gta02.c
@@ -90,24 +90,17 @@
static struct pcf50633 *gta02_pcf;
/*
- * This gets called every 1ms when we paniced.
+ * This gets called frequently when we paniced.
*/
-static long gta02_panic_blink(long count)
+static long gta02_panic_blink(int state)
{
long delay = 0;
- static long last_blink;
- static char led;
+ char led;
- /* Fast blink: 200ms period. */
- if (count - last_blink < 100)
- return 0;
-
- led ^= 1;
+ led = (state) ? 1 : 0;
gpio_direction_output(GTA02_GPIO_AUX_LED, led);
- last_blink = count;
-
return delay;
}
@@ -556,7 +549,7 @@ static void gta02_poweroff(void)
static void __init gta02_machine_init(void)
{
- /* Set the panic callback to make AUX LED blink at ~5Hz. */
+ /* Set the panic callback to turn AUX LED on or off. */
panic_blink = gta02_panic_blink;
s3c_pm_init();
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 6440a8f..c32d9b3 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -61,10 +61,6 @@ static bool i8042_noloop;
module_param_named(noloop, i8042_noloop, bool, 0);
MODULE_PARM_DESC(noloop, "Disable the AUX Loopback command while probing for the AUX port");
-static unsigned int i8042_blink_frequency = 500;
-module_param_named(panicblink, i8042_blink_frequency, uint, 0600);
-MODULE_PARM_DESC(panicblink, "Frequency with which keyboard LEDs should blink when kernel panics");
-
#ifdef CONFIG_X86
static bool i8042_dritek;
module_param_named(dritek, i8042_dritek, bool, 0);
@@ -1032,8 +1028,8 @@ static void i8042_controller_reset(void)
/*
- * i8042_panic_blink() will flash the keyboard LEDs and is called when
- * kernel panics. Flashing LEDs is useful for users running X who may
+ * i8042_panic_blink() will turn the keyboard LEDs on or off and is called
+ * when kernel panics. Flashing LEDs is useful for users running X who may
* not see the console and will help distingushing panics from "real"
* lockups.
*
@@ -1043,22 +1039,12 @@ static void i8042_controller_reset(void)
#define DELAY do { mdelay(1); if (++delay > 10) return delay; } while(0)
-static long i8042_panic_blink(long count)
+static long i8042_panic_blink(int state)
{
long delay = 0;
- static long last_blink;
- static char led;
-
- /*
- * We expect frequency to be about 1/2s. KDB uses about 1s.
- * Make sure they are different.
- */
- if (!i8042_blink_frequency)
- return 0;
- if (count - last_blink < i8042_blink_frequency)
- return 0;
+ char led;
- led ^= 0x01 | 0x04;
+ led = (state) ? 0x01 | 0x04 : 0;
while (i8042_read_status() & I8042_STR_IBF)
DELAY;
dbg("%02x -> i8042 (panic blink)", 0xed);
@@ -1071,7 +1057,6 @@ static long i8042_panic_blink(long count)
dbg("%02x -> i8042 (panic blink)", led);
i8042_write_data(led);
DELAY;
- last_blink = count;
return delay;
}
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 8317ec4..7d4552a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -172,7 +172,7 @@ static inline void might_fault(void)
#endif
extern struct atomic_notifier_head panic_notifier_list;
-extern long (*panic_blink)(long time);
+extern long (*panic_blink)(int state);
NORET_TYPE void panic(const char * fmt, ...)
__attribute__ ((NORET_AND format (printf, 1, 2))) __cold;
extern void oops_enter(void);
diff --git a/kernel/panic.c b/kernel/panic.c
index 3b16cd9..6ee47b9 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -31,39 +31,43 @@ static int pause_on_oops_flag;
static DEFINE_SPINLOCK(pause_on_oops_lock);
int panic_timeout;
+static int panic_blink_wpm = 12;
ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
EXPORT_SYMBOL(panic_notifier_list);
+static long no_blink(int state)
+{
+ return 0;
+}
+
/* Returns how long it waited in ms */
-long (*panic_blink)(long time);
+long (*panic_blink)(int state);
EXPORT_SYMBOL(panic_blink);
-static void panic_blink_one_second(void)
+static long panic_blink_enter(long count)
{
- static long i = 0, end;
-
- if (panic_blink) {
- end = i + MSEC_PER_SEC;
-
- while (i < end) {
- i += panic_blink(i);
- mdelay(1);
- i++;
- }
+ int len;
+ long delay = 0;
+ static int state = 1;
+ static int first = 1;
+ static long next_count;
+
+ len = 3600 / panic_blink_wpm;
+ if (!first && count - next_count < 0)
+ return 0;
+ if (state) {
+ delay += panic_blink(0);
+ state = 0;
+ if (first)
+ first = 0;
} else {
- /*
- * When running under a hypervisor a small mdelay may get
- * rounded up to the hypervisor timeslice. For example, with
- * a 1ms in 10ms hypervisor timeslice we might inflate a
- * mdelay(1) loop by 10x.
- *
- * If we have nothing to blink, spin on 1 second calls to
- * mdelay to avoid this.
- */
- mdelay(MSEC_PER_SEC);
+ delay += panic_blink(1);
+ state = 1;
}
+ next_count = count + len;
+ return delay;
}
/**
@@ -79,6 +83,7 @@ NORET_TYPE void panic(const char * fmt, ...)
static char buf[1024];
va_list args;
long i;
+ int step;
/*
* It's possible to come here directly from a panic-assertion and
@@ -117,6 +122,23 @@ NORET_TYPE void panic(const char * fmt, ...)
bust_spinlocks(0);
+ if (panic_blink_wpm <= 0 || panic_blink_wpm > 100)
+ panic_blink_wpm = 12;
+
+ if (!panic_blink)
+ panic_blink = no_blink;
+
+ /*
+ * When running under a hypervisor a small mdelay may get
+ * rounded up to the hypervisor timeslice. For example, with
+ * a 1ms in 10ms hypervisor timeslice we might inflate a
+ * mdelay(1) loop by 10x.
+ *
+ * If panic_blink_wpm is 5 or less, spin on 240 millisecond
+ * calls to mdelay to avoid this.
+ */
+ step = (panic_blink_wpm <= 5) ? 240 : 1;
+
if (panic_timeout > 0) {
/*
* Delay timeout seconds before rebooting the machine.
@@ -124,9 +146,10 @@ NORET_TYPE void panic(const char * fmt, ...)
*/
printk(KERN_EMERG "Rebooting in %d seconds..", panic_timeout);
- for (i = 0; i < panic_timeout; i++) {
+ for (i = 0; i < panic_timeout * 1000; i += step) {
touch_nmi_watchdog();
- panic_blink_one_second();
+ i += panic_blink_enter(i);
+ mdelay(step);
}
/*
* This will not be a clean reboot, with everything
@@ -152,9 +175,10 @@ NORET_TYPE void panic(const char * fmt, ...)
}
#endif
local_irq_enable();
- while (1) {
+ for (i = 0; ; i += step) {
touch_softlockup_watchdog();
- panic_blink_one_second();
+ i += panic_blink_enter(i);
+ mdelay(step);
}
}
@@ -437,4 +461,5 @@ EXPORT_SYMBOL(__stack_chk_fail);
#endif
core_param(panic, panic_timeout, int, 0644);
+core_param(panicblink, panic_blink_wpm, int, 0644);
core_param(pause_on_oops, pause_on_oops, int, 0644);
On Fri, 04 Jun 2010 07:01:16 +0900
TAMUKI Shoichi <[email protected]> wrote:
> To keep panic_timeout accuracy when running under a hypervisor, the
> current implementation only spins on long time (1 second) calls to
> mdelay. That brings a good effect, but the problem is the keyboard
> LEDs don't blink at all on that situation.
>
> This patch changes to call to panic_blink_enter() between every mdelay
> and keeps blinking in spite of long spin timer mode.
>
> The default time to call to mdelay is 1ms. If the speed of blinking
> is slow enough, the time to call to mdelay will be automatically
> switched to longer. This is suitable when running under a hypervisor.
>
> ...
>
> Documentation/kernel-parameters.txt | 8 +-
> arch/arm/mach-s3c2440/mach-gta02.c | 17 +----
> drivers/input/serio/i8042.c | 25 +-------
> include/linux/kernel.h | 2
> kernel/panic.c | 77 +++++++++++++++++---------
> 5 files changed, 67 insertions(+), 62 deletions(-)
It cleans up the led-blinking implementations nicely. That's good.
>
> ...
>
> + panicblink= [KNL] The speed of panic blink (default is 12 wpm)
> + The period of panic blink can be computed by the
> + formula T = 7200 / W, where T is the period in milli-
> + seconds, W is the speed in wpm (words per minute).
> + Should be 5 or less when running under a hypervisor
Nobody will know what "wpm" means. What is a "word" in this context?
Unclear.
How about "bpm": "blinks per minute". That's nice and direct.
>
> ...
>
>
> + if (panic_blink_wpm <= 0 || panic_blink_wpm > 100)
> + panic_blink_wpm = 12;
hm, OK. Or we could do
if (panic_blink_wpm <= 0)
panic_blink_wpm = 1;
if (panic_blink_wpm > 100)
panic_blink_wpm = 100;
which is handily encapsulated in the clamp() macro which nobody uses.
> + if (!panic_blink)
> + panic_blink = no_blink;
Can we initialise panic_blink to no_blink at compile-time then remove this?
>
> ...
>
Hello,
Thank you for the review.
On Thu, 3 Jun 2010 15:30:16 -0700 Andrew Morton wrote:
> > + panicblink= [KNL] The speed of panic blink (default is 12 wpm)
> > + The period of panic blink can be computed by the
> > + formula T = 7200 / W, where T is the period in milli-
> > + seconds, W is the speed in wpm (words per minute).
> > + Should be 5 or less when running under a hypervisor
>
> Nobody will know what "wpm" means. What is a "word" in this context?
> Unclear.
>
> How about "bpm": "blinks per minute". That's nice and direct.
The current explanation of panicblink= still makes no sense, indeed.
"bpm" seems to be nice and direct, but the range at a practicable
blinking speed will be 8.33 to 833 bpm. That is too wide and the step
size is too small for the speed of panic blink. That will be hard to
deal with.
OTOH, the range at a practicable blinking speed in "wpm" fits in 1 to
100. I think that will sit well with us.
Now, I need to explain what "wpm" means and what a "word" in the con-
text is:
The speed of morse code is measured in wpm, which defines the speed of
morse transmission as the timing needed to send the word "PARIS" a
given number of times per minute.
The time for one (minimum) unit can be computed by the formula T =
1200 / W, where T is the unit time in milliseconds, W is the speed in
wpm. The panic blink here is assumed as a word of infinite length to
which "T" continues (i.e. "TTTTTTTT..."). The letter "T" represents
three units long and the short gap (between letters) also represents
three units long. The period of panic blink thus can be computed by
the formula T = 7200 / W.
However, IMO, it is gauche to say such a explanation of panicblink=
in kernel-parameters.txt.
After all, I will just explain concisely the important matters (range
is 1 to 100, the lower the slower, the higher the faster, default is
12).
> > + if (panic_blink_wpm <= 0 || panic_blink_wpm > 100)
> > + panic_blink_wpm = 12;
>
> hm, OK. Or we could do
>
> if (panic_blink_wpm <= 0)
> panic_blink_wpm = 1;
> if (panic_blink_wpm > 100)
> panic_blink_wpm = 100;
>
> which is handily encapsulated in the clamp() macro which nobody uses.
OK, I will use the clamp() macro here.
> > + if (!panic_blink)
> > + panic_blink = no_blink;
>
> Can we initialise panic_blink to no_blink at compile-time then remove this?
Perhaps, we cannot initialize panic_blink to no_blink at compile time
because panic_blink is dynamically set/unset the panic callback when
module_init()/module_exit() is called.
Since there are few changes this time, I will post the patch as PATCH
v2.1. Andrew, would you please readd the patch to the -mm tree.
Regards,
TAMUKI Shoichi
To keep panic_timeout accuracy when running under a hypervisor, the
current implementation only spins on long time (1 second) calls to
mdelay. That brings a good effect, but the problem is the keyboard
LEDs don't blink at all on that situation.
This patch changes to call to panic_blink_enter() between every mdelay
and keeps blinking in spite of long spin timer mode.
The default time to call to mdelay is 1ms. If the speed of blinking
is slow enough, the time to call to mdelay will be automatically
switched to longer. This is suitable when running under a hypervisor.
Signed-off-by: TAMUKI Shoichi <[email protected]>
---
Changes since v2:
- brush up the explanation of panicblink= in kernel-parameters.txt
- use the clamp() macro
Documentation/kernel-parameters.txt | 6 +-
arch/arm/mach-s3c2440/mach-gta02.c | 17 +----
drivers/input/serio/i8042.c | 25 +-------
include/linux/kernel.h | 2
kernel/panic.c | 76 +++++++++++++++++---------
5 files changed, 64 insertions(+), 62 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ff9f1a8..e1fe7e3 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -917,9 +917,6 @@ and is between 256 and 4096 characters. It is defined in the file
controller
i8042.nopnp [HW] Don't use ACPIPnP / PnPBIOS to discover KBD/AUX
controllers
- i8042.panicblink=
- [HW] Frequency with which keyboard LEDs should blink
- when kernel panics (default is 0.5 sec)
i8042.reset [HW] Reset the controller during init and cleanup
i8042.unlock [HW] Unlock (ignore) the keylock
@@ -1871,6 +1868,9 @@ and is between 256 and 4096 characters. It is defined in the file
panic= [KNL] Kernel behaviour on panic
Format: <timeout>
+ panicblink= [KNL] The speed of panic blink (range is 1 to 100, the
+ lower the slower, the higher the faster, default is 12)
+ Should be 5 or less when running under a hypervisor
parkbd.port= [HW] Parallel port number the keyboard adapter is
connected to, default is 0.
diff --git a/arch/arm/mach-s3c2440/mach-gta02.c b/arch/arm/mach-s3c2440/mach-gta02.c
index 9e39faa..deaabe8 100644
--- a/arch/arm/mach-s3c2440/mach-gta02.c
+++ b/arch/arm/mach-s3c2440/mach-gta02.c
@@ -90,24 +90,17 @@
static struct pcf50633 *gta02_pcf;
/*
- * This gets called every 1ms when we paniced.
+ * This gets called frequently when we paniced.
*/
-static long gta02_panic_blink(long count)
+static long gta02_panic_blink(int state)
{
long delay = 0;
- static long last_blink;
- static char led;
+ char led;
- /* Fast blink: 200ms period. */
- if (count - last_blink < 100)
- return 0;
-
- led ^= 1;
+ led = (state) ? 1 : 0;
gpio_direction_output(GTA02_GPIO_AUX_LED, led);
- last_blink = count;
-
return delay;
}
@@ -556,7 +549,7 @@ static void gta02_poweroff(void)
static void __init gta02_machine_init(void)
{
- /* Set the panic callback to make AUX LED blink at ~5Hz. */
+ /* Set the panic callback to turn AUX LED on or off. */
panic_blink = gta02_panic_blink;
s3c_pm_init();
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 6440a8f..c32d9b3 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -61,10 +61,6 @@ static bool i8042_noloop;
module_param_named(noloop, i8042_noloop, bool, 0);
MODULE_PARM_DESC(noloop, "Disable the AUX Loopback command while probing for the AUX port");
-static unsigned int i8042_blink_frequency = 500;
-module_param_named(panicblink, i8042_blink_frequency, uint, 0600);
-MODULE_PARM_DESC(panicblink, "Frequency with which keyboard LEDs should blink when kernel panics");
-
#ifdef CONFIG_X86
static bool i8042_dritek;
module_param_named(dritek, i8042_dritek, bool, 0);
@@ -1032,8 +1028,8 @@ static void i8042_controller_reset(void)
/*
- * i8042_panic_blink() will flash the keyboard LEDs and is called when
- * kernel panics. Flashing LEDs is useful for users running X who may
+ * i8042_panic_blink() will turn the keyboard LEDs on or off and is called
+ * when kernel panics. Flashing LEDs is useful for users running X who may
* not see the console and will help distingushing panics from "real"
* lockups.
*
@@ -1043,22 +1039,12 @@ static void i8042_controller_reset(void)
#define DELAY do { mdelay(1); if (++delay > 10) return delay; } while(0)
-static long i8042_panic_blink(long count)
+static long i8042_panic_blink(int state)
{
long delay = 0;
- static long last_blink;
- static char led;
-
- /*
- * We expect frequency to be about 1/2s. KDB uses about 1s.
- * Make sure they are different.
- */
- if (!i8042_blink_frequency)
- return 0;
- if (count - last_blink < i8042_blink_frequency)
- return 0;
+ char led;
- led ^= 0x01 | 0x04;
+ led = (state) ? 0x01 | 0x04 : 0;
while (i8042_read_status() & I8042_STR_IBF)
DELAY;
dbg("%02x -> i8042 (panic blink)", 0xed);
@@ -1071,7 +1057,6 @@ static long i8042_panic_blink(long count)
dbg("%02x -> i8042 (panic blink)", led);
i8042_write_data(led);
DELAY;
- last_blink = count;
return delay;
}
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 8317ec4..7d4552a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -172,7 +172,7 @@ static inline void might_fault(void)
#endif
extern struct atomic_notifier_head panic_notifier_list;
-extern long (*panic_blink)(long time);
+extern long (*panic_blink)(int state);
NORET_TYPE void panic(const char * fmt, ...)
__attribute__ ((NORET_AND format (printf, 1, 2))) __cold;
extern void oops_enter(void);
diff --git a/kernel/panic.c b/kernel/panic.c
index 3b16cd9..601858a 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -31,39 +31,43 @@ static int pause_on_oops_flag;
static DEFINE_SPINLOCK(pause_on_oops_lock);
int panic_timeout;
+static int panic_blink_wpm = 12;
ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
EXPORT_SYMBOL(panic_notifier_list);
+static long no_blink(int state)
+{
+ return 0;
+}
+
/* Returns how long it waited in ms */
-long (*panic_blink)(long time);
+long (*panic_blink)(int state);
EXPORT_SYMBOL(panic_blink);
-static void panic_blink_one_second(void)
+static long panic_blink_enter(long count)
{
- static long i = 0, end;
-
- if (panic_blink) {
- end = i + MSEC_PER_SEC;
-
- while (i < end) {
- i += panic_blink(i);
- mdelay(1);
- i++;
- }
+ int len;
+ long delay = 0;
+ static int state = 1;
+ static int first = 1;
+ static long next_count;
+
+ len = 3600 / panic_blink_wpm;
+ if (!first && count - next_count < 0)
+ return 0;
+ if (state) {
+ delay += panic_blink(0);
+ state = 0;
+ if (first)
+ first = 0;
} else {
- /*
- * When running under a hypervisor a small mdelay may get
- * rounded up to the hypervisor timeslice. For example, with
- * a 1ms in 10ms hypervisor timeslice we might inflate a
- * mdelay(1) loop by 10x.
- *
- * If we have nothing to blink, spin on 1 second calls to
- * mdelay to avoid this.
- */
- mdelay(MSEC_PER_SEC);
+ delay += panic_blink(1);
+ state = 1;
}
+ next_count = count + len;
+ return delay;
}
/**
@@ -79,6 +83,7 @@ NORET_TYPE void panic(const char * fmt, ...)
static char buf[1024];
va_list args;
long i;
+ int step;
/*
* It's possible to come here directly from a panic-assertion and
@@ -117,6 +122,22 @@ NORET_TYPE void panic(const char * fmt, ...)
bust_spinlocks(0);
+ panic_blink_wpm = clamp(panic_blink_wpm, 1, 100);
+
+ if (!panic_blink)
+ panic_blink = no_blink;
+
+ /*
+ * When running under a hypervisor a small mdelay may get
+ * rounded up to the hypervisor timeslice. For example, with
+ * a 1ms in 10ms hypervisor timeslice we might inflate a
+ * mdelay(1) loop by 10x.
+ *
+ * If panic_blink_wpm is 5 or less, spin on 240 millisecond
+ * calls to mdelay to avoid this.
+ */
+ step = (panic_blink_wpm <= 5) ? 240 : 1;
+
if (panic_timeout > 0) {
/*
* Delay timeout seconds before rebooting the machine.
@@ -124,9 +145,10 @@ NORET_TYPE void panic(const char * fmt, ...)
*/
printk(KERN_EMERG "Rebooting in %d seconds..", panic_timeout);
- for (i = 0; i < panic_timeout; i++) {
+ for (i = 0; i < panic_timeout * 1000; i += step) {
touch_nmi_watchdog();
- panic_blink_one_second();
+ i += panic_blink_enter(i);
+ mdelay(step);
}
/*
* This will not be a clean reboot, with everything
@@ -152,9 +174,10 @@ NORET_TYPE void panic(const char * fmt, ...)
}
#endif
local_irq_enable();
- while (1) {
+ for (i = 0; ; i += step) {
touch_softlockup_watchdog();
- panic_blink_one_second();
+ i += panic_blink_enter(i);
+ mdelay(step);
}
}
@@ -437,4 +460,5 @@ EXPORT_SYMBOL(__stack_chk_fail);
#endif
core_param(panic, panic_timeout, int, 0644);
+core_param(panicblink, panic_blink_wpm, int, 0644);
core_param(pause_on_oops, pause_on_oops, int, 0644);
On Sun, 06 Jun 2010 22:19:18 +0900
TAMUKI Shoichi <[email protected]> wrote:
> Hello,
>
> Thank you for the review.
>
> On Thu, 3 Jun 2010 15:30:16 -0700 Andrew Morton wrote:
>
> > > + panicblink= [KNL] The speed of panic blink (default is 12 wpm)
> > > + The period of panic blink can be computed by the
> > > + formula T = 7200 / W, where T is the period in milli-
> > > + seconds, W is the speed in wpm (words per minute).
> > > + Should be 5 or less when running under a hypervisor
> >
> > Nobody will know what "wpm" means. What is a "word" in this context?
> > Unclear.
> >
> > How about "bpm": "blinks per minute". That's nice and direct.
>
> The current explanation of panicblink= still makes no sense, indeed.
>
> "bpm" seems to be nice and direct, but the range at a practicable
> blinking speed will be 8.33 to 833 bpm. That is too wide and the step
> size is too small for the speed of panic blink. That will be hard to
> deal with.
>
> OTOH, the range at a practicable blinking speed in "wpm" fits in 1 to
> 100. I think that will sit well with us.
>
> Now, I need to explain what "wpm" means and what a "word" in the con-
> text is:
>
> The speed of morse code is measured in wpm, which defines the speed of
> morse transmission as the timing needed to send the word "PARIS" a
> given number of times per minute.
>
> The time for one (minimum) unit can be computed by the formula T =
> 1200 / W, where T is the unit time in milliseconds, W is the speed in
> wpm. The panic blink here is assumed as a word of infinite length to
> which "T" continues (i.e. "TTTTTTTT..."). The letter "T" represents
> three units long and the short gap (between letters) also represents
> three units long. The period of panic blink thus can be computed by
> the formula T = 7200 / W.
Morse code? Kidding?
Sorry, no. Nobody who uses this feature will know what what
"words per minute" means. It's nutty!
Please remind me why we're making this configurable at all. Can't we
just hardwire the thing to 1Hz or something? Add an
im_using_a_hypervisor boot option or something, if necessary?
Hi,
> Morse code? Kidding?
>
> Sorry, no. Nobody who uses this feature will know what what
> "words per minute" means. It's nutty!
>
> Please remind me why we're making this configurable at all. Can't we
> just hardwire the thing to 1Hz or something? Add an
> im_using_a_hypervisor boot option or something, if necessary?
I agree. The panic_blink() interface is quite painful and I have no idea
why someone would want to configure the blink frequency of their keyboard
LED when panicing. Maybe they want to match it to the beat of their techno
music.
Since the keyboard LED default is a transition every 0.5s, why don't we just
remove i8042.panicblink and change all users (all 2 of them) to expect a 2 HZ
call rate? The hypervisor case should be fine with 0.5s mdelays, so we end up
removing that special case.
I would have said 1 HZ, but it seems like the default was chosen to be
different to kdb:
/*
* We expect frequency to be about 1/2s. KDB uses about 1s.
* Make sure they are different.
*/
No idea if that comment is still valid.
Anton
Hello,
On Wed, 9 Jun 2010 15:15:52 -0700 Andrew Morton wrote:
> Morse code? Kidding?
I'm sorry, I'm confusing you.
> Sorry, no. Nobody who uses this feature will know what what
> "words per minute" means. It's nutty!
That is just a strained interpretation where the measure of the
blinking speed comes from. The description of "words per minute"
has already disappeared in the latest patch.
> Please remind me why we're making this configurable at all. Can't we
> just hardwire the thing to 1Hz or something? Add an
> im_using_a_hypervisor boot option or something, if necessary?
For now, gta02_panic_blink(), one of panic_blink() users, expects a
10Hz call rate. IOW, it blinks at 5Hz (i.e. panicblink=36). I think
that the desirable blinking speed is different according to devices.
Regards,
TAMUKI Shoichi
Hello,
On Thu, 10 Jun 2010 12:30:33 +1000 Anton Blanchard wrote:
> > Please remind me why we're making this configurable at all. Can't we
> > just hardwire the thing to 1Hz or something? Add an
> > im_using_a_hypervisor boot option or something, if necessary?
>
> I agree. The panic_blink() interface is quite painful and I have no idea
> why someone would want to configure the blink frequency of their keyboard
> LED when panicing. Maybe they want to match it to the beat of their techno
> music.
That's cool. Joking aside, we might want to know which machine has
paniced among two or more overcrowded machines. Maybe it is a good
idea to change the blinking speed of each machine so as to distinguish
whether the machine has paniced or not.
> Since the keyboard LED default is a transition every 0.5s, why don't we just
> remove i8042.panicblink and change all users (all 2 of them) to expect a 2 HZ
> call rate? The hypervisor case should be fine with 0.5s mdelays, so we end up
> removing that special case.
If the speed of blinking is slow enough (i.e. panicblink<=5), both the
native case and the hypervisor case should be fine, indeed. However,
gta02_panic_blink(), one of panic_blink() users, expects a 10Hz call
rate at the native case. IOW, it blinks at 5Hz (i.e. panicblink=36).
I think that the desirable blinking speed is different according to
devices.
> I would have said 1 HZ, but it seems like the default was chosen to be
> different to kdb:
>
> /*
> * We expect frequency to be about 1/2s. KDB uses about 1s.
> * Make sure they are different.
> */
>
> No idea if that comment is still valid.
That comment maybe still valid according to the following:
On Sun, 30 May 2010 08:55:22 +0200 Andi Kleen wrote:
> Changing the frequency is ok, the only requirement is that it is
> visible and different from the frequency kdb uses.
Regards,
TAMUKI Shoichi
On Fri, 11 Jun 2010 07:31:13 +0900
TAMUKI Shoichi <[email protected]> wrote:
> Hello,
>
> On Wed, 9 Jun 2010 15:15:52 -0700 Andrew Morton wrote:
>
> > Morse code? Kidding?
>
> I'm sorry, I'm confusing you.
>
> > Sorry, no. Nobody who uses this feature will know what what
> > "words per minute" means. It's nutty!
>
> That is just a strained interpretation where the measure of the
> blinking speed comes from. The description of "words per minute"
> has already disappeared in the latest patch.
>
> > Please remind me why we're making this configurable at all. Can't we
> > just hardwire the thing to 1Hz or something? Add an
> > im_using_a_hypervisor boot option or something, if necessary?
>
> For now, gta02_panic_blink(), one of panic_blink() users, expects a
> 10Hz call rate. IOW, it blinks at 5Hz (i.e. panicblink=36). I think
> that the desirable blinking speed is different according to devices.
>
gta02_panic_blink() simply toggles a gpio output. It's trivial to
convert that function to implement whatever behaviour we decide upon.
Err, in fact your patch already does that.
I still don't think we should have a kernel boot option for this - it
just doesn't seem useful enough. Can we please just hard-wire the
blinking frequency? And once we've done that, we don't have to jump
through hoops with this "wpm" thing. We can use Hz.
Hello,
On Mon, 14 Jun 2010 14:14:47 -0700 Andrew Morton wrote:
> I still don't think we should have a kernel boot option for this - it
> just doesn't seem useful enough. Can we please just hard-wire the
> blinking frequency? And once we've done that, we don't have to jump
> through hoops with this "wpm" thing. We can use Hz.
Hmm, OK. I understand your request.
I will repost new patch in a few days.
Please wait for a while.
Regards,
TAMUKI Shoichi