In the ongoing effort to reduce the power consumption of the linux kernel [story] and take better advantage of the tickless kernel patch [story], Stephen Hemminger posted a patch to make it possible to unload the keyboard blink driver, "the blink driver wakes up every jiffy which wastes power unnecessarily. Using a notifier gives same effect. Also add ability to unload module." The blink driver was only recently merged, described as a "simple driver that blinks the keyboard LEDs when loaded. Useful for checking that the kernel is still alive or for crashdumping."

Linus Torvalds reviewed the driver and retorted, "I really get the feeling this thing should be removed entirely. Wasting power is the _least_ of its problems." When it was pointed out that the driver is only a debugging tool, Linus listed his complaints, "it has been a total disaster from beginning to end. It wastes power. It hangs machines when it tries to blink," going on to add, "its main problem is that PEOPLE SHOULD NOT USE IT, but it sounds cool, so people end up configuring the damn thing even though they shouldn't." Ultimately, Linus removed the driver before the 2.6.22 release [story] noting, "we could have just disabled it, but there's work on a new one that isn't as fundamentally broken, so there really doesn't seem to be any point in keeping it around."

From:	Stephen Hemminger [email blocked]
Subject: blink driver power saving
Date:	Sun, 1 Jul 2007 12:50:35 -0400

The blink driver wakes up every jiffies which wastes power unnecessarily.
Using a notifier gives same effect. Also add ability to unload module.

Signed-off-by: Stephen Hemminger [email blocked]

--- a/drivers/misc/blink.c	2007-06-29 22:56:20.000000000 -0400
+++ b/drivers/misc/blink.c	2007-07-01 12:44:37.000000000 -0400
@@ -16,12 +16,30 @@ static void do_blink(unsigned long data)
 	add_timer(&blink_timer);
 }
 
-static int blink_init(void)
+static int blink_panic_event(struct notifier_block *blk,
+			     unsigned long event, void *arg)
 {
-	printk(KERN_INFO "Enabling keyboard blinking\n");
 	do_blink(0);
 	return 0;
 }
 
+static struct notifier_block blink_notify = {
+	.notifier_call = blink_panic_event,
+};
+
+static __init int blink_init(void)
+{
+	printk(KERN_INFO "Enabling keyboard blinking\n");
+	atomic_notifier_chain_register(&panic_notifier_list, &blink_notify);
+	return 0;
+}
+
+static __exit void blink_remove(void)
+{
+	del_timer_sync(&blink_timer);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &blink_notify);
+}
+
 module_init(blink_init);
+module_exit(blink_remove);
 


From:	Andi Kleen [email blocked]
To:	Stephen Hemminger [email blocked]
Subject: Re: blink driver power saving
Date:	Sun, 1 Jul 2007 23:26:29 +0200

On Sunday 01 July 2007 18:50:35 Stephen Hemminger wrote:
> The blink driver wakes up every jiffies which wastes power unnecessarily.
> Using a notifier gives same effect. Also add ability to unload module.

It's really a debugging tool where you normally don't care about
details like this. I wrote it to get visual feedback during kdump,
but it is nothing you should ever run during normal operation.

But I don't get how your patch is supposed to work. The blink driver
is not supposed to blink after panic -- panic does that anyways --
but always. So hooking it into the panic notifier chain which
is only called on panic makes about zero sense. 

If it's really needed to fix the wakeup issue the interface to
the keyboard blinking would need to be changed.

-Andi



From:	Linus Torvalds [email blocked]
To:	Stephen Hemminger [email blocked]
Subject: Re: blink driver power saving
Date:	Sun, 1 Jul 2007 11:07:21 -0700 (PDT)



On Sun, 1 Jul 2007, Stephen Hemminger wrote:
>
> The blink driver wakes up every jiffies which wastes power unnecessarily.
> Using a notifier gives same effect. Also add ability to unload module.

I really get the feeling this thing should be removed entirely. Wasting 
power is the _least_ of its problems.

I'll apply the patch, but I wonder if I should also just mark it BROKEN in 
the config file to make sure nobody enables it without realizing how bad 
it is to do so.

		Linus



From:	Andi Kleen [email blocked]
To:	Linus Torvalds [email blocked]
Subject: Re: blink driver power saving
Date:	Sun, 1 Jul 2007 23:29:52 +0200

On Sunday 01 July 2007 20:07:21 Linus Torvalds wrote:
> 
> On Sun, 1 Jul 2007, Stephen Hemminger wrote:
> >
> > The blink driver wakes up every jiffies which wastes power unnecessarily.
> > Using a notifier gives same effect. Also add ability to unload module.
> 
> I really get the feeling this thing should be removed entirely. Wasting 
> power is the _least_ of its problems.

What is so bad with it? Note it's a debugging facility and used 
for kcrash kernels where the video output doesn't work. But they
normally only run a few minutes to dump the previous state to disk
and then reboot. 

> I'll apply the patch, but I wonder if I should also just mark it BROKEN in 
> the config file to make sure nobody enables it without realizing how bad 
> it is to do so.

Well I suspect it will not work anymore after Stephen's patch
(or rather try to blink redundantly after panic which is quite dumb) 
If that is your aim then it might be cleaner to remove it.

But then I don't think it hurts anybody.  It's main problem
is that it won't blink for people with USB keyboard.

-Andi



From:	Linus Torvalds [email blocked]
To:	Andi Kleen [email blocked]
Subject: Re: blink driver power saving
Date:	Sun, 1 Jul 2007 15:14:25 -0700 (PDT)



On Sun, 1 Jul 2007, Andi Kleen wrote:
> 
> What is so bad with it? Note it's a debugging facility and used 
> for kcrash kernels where the video output doesn't work. But they
> normally only run a few minutes to dump the previous state to disk
> and then reboot. 

It has been a total disaster from beginning to end.

It wastes power.

It hangs machines when it tries to blink.

To quote an earlier thread:

  "The driver uses panic_blink - something that we expect to work after 
   panic. It rapidly polls KBC status register to detect when it accepted 
   led command and does it without taking i8042_lock (because it may have 
   been taken before kernel panicked) so it is quite possible that that 
   interferes with atkbd operation."

and it has been confirmed to render unusable at least some thinkpads. I 
think it was Pavel who reported it last.

> But then I don't think it hurts anybody.  It's main problem
> is that it won't blink for people with USB keyboard.

No, its main problem is that PEOPLE SHOULD NOT USE IT, but it sounds cool, 
so people end up configuring the damn thing even though they shouldn't.

Which is why I think it should be marked broken.

And yes, it *does* hurt people.

		Linus



From:	Andi Kleen [email blocked]
To:	Linus Torvalds [email blocked]
Subject: Re: blink driver power saving
Date:	Mon, 2 Jul 2007 01:59:50 +0200

On Monday 02 July 2007 00:14, Linus Torvalds wrote:
> On Sun, 1 Jul 2007, Andi Kleen wrote:
> > What is so bad with it? Note it's a debugging facility and used
> > for kcrash kernels where the video output doesn't work. But they
> > normally only run a few minutes to dump the previous state to disk
> > and then reboot.
>
> It has been a total disaster from beginning to end.
>
> It wastes power.

Again:
It's not intended for normal kernels. It's a debugging feature.
It's not intended for normal kernels. It's a debugging feature.
It's not intended for normal kernels. It's a debugging feature.

Got it now? Power wasting or not just doesn't matter for it.
 
> It hangs machines when it tries to blink.

Yes, there seem to be more buggy keyboard controllers
around than I anticipated. Very sad that IBM couldn't even
get such a simple thing right.

Well only those that could be already hung from user space
with setleds (that was also confirmed).  Actually I thought
they didn't hang completely, but just stopped reacting to 
the keyboard (which is actually pretty bad for every user
to be able to trigger)

I guess the better way to handle those would be to find out the 
minimum frequency of blinking that is still ok and rate limit it to that in 
the keyboard driver. 

Anyways, Stephen's patch just doesn't make sense: 
he clearly didn't understand the code at all. Before you
apply it and cripple it better drop the driver completely.

-Andi



From:	Linus Torvalds [email blocked]
To:	Andi Kleen [email blocked]
Subject: Re: blink driver power saving
Date:	Mon, 2 Jul 2007 08:51:38 -0700 (PDT)



On Mon, 2 Jul 2007, Andi Kleen wrote:
>
> It's not intended for normal kernels. It's a debugging feature.
> It's not intended for normal kernels. It's a debugging feature.
> It's not intended for normal kernels. It's a debugging feature.
> 
> Got it now?

You seem to have some reading comprehension problems.

The email you replied to had this in it:

  "No, its main problem is that PEOPLE SHOULD NOT USE IT, but it sounds cool,
   so people end up configuring the damn thing even though they shouldn't."

but you seem to not have understood.

Got it now?

> > It hangs machines when it tries to blink.
> 
> Yes, there seem to be more buggy keyboard controllers
> around than I anticipated. Very sad that IBM couldn't even
> get such a simple thing right.

Well, I would say that the driver itself is buggy. It calls 
"panic_blink()", which doesn't do the proper locking (i8042_lock is 
required to protect the accesses, otherwise you can have different 
entities in the system writing to the command ports concurrently, and get 
random stuff happening!).

So blaming "buggy keyboard controllers" is pretty damn silly of you, when 
the real problem is that the driver is broken. That interface is for 
panic, and panic *only*, and avoids the lock exactly because it's meant to 
be called when the system is basically dead.

Why did you think that function is called "panic_blink()"?

Yes, it could be hidden by making it do the buggy calls less often. That 
makes some machines work, but it doesn't change the fact that it would 
still be buggy.

> Anyways, Stephen's patch just doesn't make sense: 
> he clearly didn't understand the code at all. Before you
> apply it and cripple it better drop the driver completely.

I think I will have to.

		Linus



From: Alan Cox [email blocked]
To:	Linus Torvalds [email blocked]
Subject: Re: blink driver power saving
Date:	Mon, 2 Jul 2007 17:59:26 +0100
 Windsor, Berkshire, SL4 1TE, Y Deyrnas Gyfunol. Cofrestrwyd yng Nghymru a
 Lloegr o'r rhif cofrestru 3798903

> > Anyways, Stephen's patch just doesn't make sense: 
> > he clearly didn't understand the code at all. Before you
> > apply it and cripple it better drop the driver completely.
> 
> I think I will have to.

Make it depend on the kernel debug options and if you are really paranoid
also add a module option to enable it at boot time. The module option
works pretty well and various old and new IDE drivers do it to stop make
allyesconfig accidents.

• Archive of above thread
• KernelTrap interview with Alan Cox