[PATCH] parisc: blink loadavg LEDs on Oops

[PATCH] parisc: blink loadavg LEDs on Oops

[Helge Deller]

- blink loadavg LEDs only (not all LEDs) twice a second on Oops
- cancel_rearming_delayed_workqueue() is obsolete, 
  use cancel_delayed_work_sync() instead

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
index f9b1266..0cb4862 100644
--- a/drivers/parisc/led.c
+++ b/drivers/parisc/led.c
@@ -3,7 +3,7 @@
  *
  *      (c) Copyright 2000 Red Hat Software
  *      (c) Copyright 2000 Helge Deller <hdeller@redhat.com>
- *      (c) Copyright 2001-2005 Helge Deller <deller@gmx.de>
+ *      (c) Copyright 2001-2009 Helge Deller <deller@gmx.de>
  *      (c) Copyright 2001 Randolph Chung <tausq@debian.org>
  *
  *      This program is free software; you can redistribute it and/or modify
@@ -463,9 +463,13 @@ static void led_work_func (struct work_struct *unused)
 	if (likely(led_lanrxtx))  currentleds |= led_get_net_activity();
 	if (likely(led_diskio))   currentleds |= led_get_diskio_activity();
 
-	/* blink all LEDs twice a second if we got an Oops (HPMC) */
-	if (unlikely(oops_in_progress)) 
-		currentleds = (count_HZ<=(HZ/2)) ? 0 : 0xff;
+	/* blink loadavg LEDs twice per second if we got an Oops (HPMC) */
+	if (unlikely(oops_in_progress)) {
+		if (count_HZ <= (HZ/2))
+			currentleds &= ~(LED4|LED5|LED6|LED7);
+		else
+			currentleds |= (LED4|LED5|LED6|LED7);
+	}
 
 	if (currentleds != lastleds)
 	{
@@ -511,7 +515,7 @@ static int led_halt(struct notifier_block *nb, unsigned long event, void *buf)
 	
 	/* Cancel the work item and delete the queue */
 	if (led_wq) {
-		cancel_rearming_delayed_workqueue(led_wq, &led_task);
+		cancel_delayed_work_sync(&led_task);
 		destroy_workqueue(led_wq);
 		led_wq = NULL;
 	}
@@ -630,7 +634,7 @@ int lcd_print( const char *str )
 	
 	/* temporarily disable the led work task */
 	if (led_wq)
-		cancel_rearming_delayed_workqueue(led_wq, &led_task);
+		cancel_delayed_work_sync(&led_task);
 
 	/* copy display string to buffer for procfs */
 	strlcpy(lcd_text, str, sizeof(lcd_text));

Re: [PATCH] parisc: blink loadavg LEDs on Oops

[Thibaut VARENE]

On Tue, Jan 6, 2009 at 2:23 PM, Helge Deller <deller@gmx.de> wrote:
> - blink loadavg LEDs only (not all LEDs) twice a second on Oops

Just out of curiosity, why wouldn't you want to have all leds blinking
on something as serious as an Oops? I kind of liked to clearly see
when something went wrong...

Cheers

-- 
Thibaut VARENE
http://www.parisc-linux.org/~varenet/

Re: [PATCH] parisc: blink loadavg LEDs on Oops

[Helge Deller]

Thibaut VARENE wrote:
> On Tue, Jan 6, 2009 at 2:23 PM, Helge Deller <deller@gmx.de> wrote:
>> - blink loadavg LEDs only (not all LEDs) twice a second on Oops
> 
> Just out of curiosity, why wouldn't you want to have all leds blinking
> on something as serious as an Oops? I kind of liked to clearly see
> when something went wrong...

Yeah, valid question.

While doing some bugfixing on the HIL drivers I noticed that suddenly
all LEDs were lit on the 715/64. It was strange, as I still was using
the machine and it was running fine.

The reason why the LEDs were lit was, that oops_in_progress was set due
a bug in my code. In dmesg I saw:
BUG: rwlock cpu recursion on CPU#0, modprobe/1098, 00a1599c
Backtrace:
 [<102a9c78>] _raw_write_lock+0x54/0x8c
 ....

So, whenever the kernel hits such a WARNING(), all LEDs will blink.
Now after my patch, the 4 LEDs on the left side blink (they are currently
not used anyway), while Heartbeat, disk-IO and LAN in/out still work
as usual. So, you will notice the 4 LEDs at once as well, but in addition
you still have the possibility to see if LAN goes in/out (e.g. if you ping
the machine) or if the heartbeat still works (and so your machine should 
still be reachable).

Helge

Re: [PATCH] parisc: blink loadavg LEDs on Oops

[Helge Deller]

Helge Deller wrote:
> Thibaut VARENE wrote:
>> On Tue, Jan 6, 2009 at 2:23 PM, Helge Deller <deller@gmx.de> wrote:
>>> - blink loadavg LEDs only (not all LEDs) twice a second on Oops
>> Just out of curiosity, why wouldn't you want to have all leds blinking
>> on something as serious as an Oops? I kind of liked to clearly see
>> when something went wrong...
> 
> Yeah, valid question.
> 
> While doing some bugfixing on the HIL drivers I noticed that suddenly
> all LEDs were lit on the 715/64. It was strange, as I still was using...

I meant: The LEDs were blinking twice per second, not full-time on.

Re: [PATCH] parisc: blink loadavg LEDs on Oops

[Thibaut VARENE]

On Tue, Jan 6, 2009 at 4:40 PM, Helge Deller <deller@gmx.de> wrote:

> Yeah, valid question.
....
>
> So, whenever the kernel hits such a WARNING(), all LEDs will blink.
> Now after my patch, the 4 LEDs on the left side blink (they are currently
> not used anyway), while Heartbeat, disk-IO and LAN in/out still work
> as usual. So, you will notice the 4 LEDs at once as well, but in addition
> you still have the possibility to see if LAN goes in/out (e.g. if you ping
> the machine) or if the heartbeat still works (and so your machine should
> still be reachable).

The thing is that you have a machine with more than 4 LEDs. I have
B180/B132s which have only 4 leds. Thus I won't notice anything when
an Oops occurs. If you really don't want to have the old behaviour
(which, again, I really found nice), how about having the heartbeat
LED fixed when an oops occurs.

Even better, a way to "reset" the state of the leds through /proc
would solve both problems: an oops occurs, all/some leds flash/remain
lit, you (try to) login, figure out the oops wasn't so serious, echo
something > /proc/pdc/led and the LEDs start blinking normally again.

Anyway, the key point here is that not all machines without LCD have
more than 4 LEDs ;)

Thoughts?

Cheers,
T-Bone

-- 
Thibaut VARENE
http://www.parisc-linux.org/~varenet/

Re: [PATCH] parisc: blink loadavg LEDs on Oops

[Helge Deller]

Thibaut VARENE wrote:
> On Tue, Jan 6, 2009 at 4:40 PM, Helge Deller <deller@gmx.de> wrote:
> 
>> Yeah, valid question.
> ....
>> So, whenever the kernel hits such a WARNING(), all LEDs will blink.
>> Now after my patch, the 4 LEDs on the left side blink (they are currently
>> not used anyway), while Heartbeat, disk-IO and LAN in/out still work
>> as usual. So, you will notice the 4 LEDs at once as well, but in addition
>> you still have the possibility to see if LAN goes in/out (e.g. if you ping
>> the machine) or if the heartbeat still works (and so your machine should
>> still be reachable).
> 
> The thing is that you have a machine with more than 4 LEDs. I have
> B180/B132s which have only 4 leds. Thus I won't notice anything when
> an Oops occurs. If you really don't want to have the old behaviour
> (which, again, I really found nice), how about having the heartbeat
> LED fixed when an oops occurs.
> 
> Even better, a way to "reset" the state of the leds through /proc
> would solve both problems: an oops occurs, all/some leds flash/remain
> lit, you (try to) login, figure out the oops wasn't so serious, echo
> something > /proc/pdc/led and the LEDs start blinking normally again.
> 
> Anyway, the key point here is that not all machines without LCD have
> more than 4 LEDs ;)

I think you are right. My b160L has 4 LEDs only as well.
So, just changing the "upper" 4 LEDs is the wrong way.
Then I was thinking to just check for the machine type or processor type
and make it dependend on that. No good idea either.

In the end, I think it's the fault of CONFIG_DEBUG_SPINLOCK.
If this is enabled, it just turns sets oops_in_progress=1 as soon as
a spinlock was wrong. Either way, if this happens something is
wrong, and the LEDs should flash.
That said, I think we should just drop my patch. If they all flash
as soon as an error happened this is just right.
In a normal productive system a bad spinlock shouldn't happen either, so
my patch just adds some unnecessary noise.

Helge