Prev: TP_STORE_SIGINFO(SEND_SIG_FORCED) == OOPS
Next: staging/wlan-ng: Switch from wext to cfg80211
From: Andrew Morton on 3 Jun 2010 18:40
On Fri, 04 Jun 2010 07:01:16 +0900
TAMUKI Shoichi <tamuki(a)linet.gr.jp> 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?

>
> ...
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 9 Jun 2010 18:20
On Sun, 06 Jun 2010 22:19:18 +0900
TAMUKI Shoichi <tamuki(a)linet.gr.jp> 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?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Anton Blanchard on 9 Jun 2010 22:40

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 14 Jun 2010 17:20
On Fri, 11 Jun 2010 07:31:13 +0900
TAMUKI Shoichi <tamuki(a)linet.gr.jp> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 17 Jun 2010 18:30
On Fri, 18 Jun 2010 06:36:31 +0900
TAMUKI Shoichi <tamuki(a)linet.gr.jp> 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. It will be switched to
> longer if the CONFIG_PANIC_LONGSPIN_TIMER kernel configuration option
> is enabled. This feature is helpful when running under a hypervisor.
>
> Signed-off-by: TAMUKI Shoichi <tamuki(a)linet.gr.jp>
> ---
> Changes since v2.1:
> - get rid of panicblink= kernel parameter
> - introduce new kernel config option CONFIG_PANIC_LONGSPIN_TIMER
>

I still don't get it :(

Why can't we simply do

for (i = 0; ; i++) {
(*panic_blink)(i & 1);
mdelay(250);
touch_nmi_watchdog();
}

on all kernels, regardless of virtualisation, etc?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: TP_STORE_SIGINFO(SEND_SIG_FORCED) == OOPS
Next: staging/wlan-ng: Switch from wext to cfg80211