Re: blink driver power saving

Re: blink driver power saving

[Indan Zupancic]

On Mon, July 2, 2007 01:59, Andi Kleen wrote:
> 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)

Pavel's lost key events, mine stopped reacting altogether.

> 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.

Dmitry already has a patch for that. He limited it to one event each 50 ms.

> 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.

CC'ing Dmitry, as I think he doesn't like the blink driver much either. ;-)

Greetings,

Indan

Re: blink driver power saving

[Andi Kleen]

On Monday 02 July 2007 13:43:23 Indan Zupancic wrote:
> On Mon, July 2, 2007 01:59, Andi Kleen wrote:
> > 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)
> 
> Pavel's lost key events, mine stopped reacting altogether.

Did you try if the network was still alive? Perhaps it was
just a locked up keyboard.

> 
> > 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.
> 
> Dmitry already has a patch for that. He limited it to one event each 50 ms.

Great.

> 
> > 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.
> 
> CC'ing Dmitry, as I think he doesn't like the blink driver much either. ;-)

Perhaps one of you geniuses who all hate it can find a better way to 
solve the "video output dead after kexec; but need visual feedback to the user
while crash dumping" problem. I'm waiting for your patches.

-Andi

Re: blink driver power saving

[Indan Zupancic]

On Mon, July 2, 2007 13:51, Andi Kleen wrote:
> On Monday 02 July 2007 13:43:23 Indan Zupancic wrote:
>> On Mon, July 2, 2007 01:59, Andi Kleen wrote:
>> > 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)
>>
>> Pavel's lost key events, mine stopped reacting altogether.
>
> Did you try if the network was still alive? Perhaps it was
> just a locked up keyboard.

No, it wasn't locked up, because the leds were blinking.
I think if I waited long enough it eventually reacted to other
key presses too, sometimes. At least flooding it with Ctrl+C
stopped the setleds loop long after I had any hope left.

>>
>> > 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.
>>
>> CC'ing Dmitry, as I think he doesn't like the blink driver much either. ;-)
>
> Perhaps one of you geniuses who all hate it can find a better way to
> solve the "video output dead after kexec; but need visual feedback to the user
> while crash dumping" problem. I'm waiting for your patches.

While someone's at it, perhaps USB keyboards could be made working too.
And make it generic infrastructure too so everyone can (ab)use it?
What about adopting the approach here and share the code:
http://lkml.org/lkml/2007/6/19/112

At least it works with all keyboards...

Greetings,

Indan

Re: blink driver power saving

[Dmitry Torokhov]

On 7/2/07, Andi Kleen <ak@suse.de> wrote:
> On Monday 02 July 2007 13:43:23 Indan Zupancic wrote:
> > On Mon, July 2, 2007 01:59, Andi Kleen wrote:
> > > 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)
> >
> > Pavel's lost key events, mine stopped reacting altogether.
>
> Did you try if the network was still alive? Perhaps it was
> just a locked up keyboard.
>
> >
> > > 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.
> >
> > Dmitry already has a patch for that. He limited it to one event each 50 ms.
>
> Great.
>
> >
> > > 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.
> >
> > CC'ing Dmitry, as I think he doesn't like the blink driver much either. ;-)
>
> Perhaps one of you geniuses who all hate it can find a better way to
> solve the "video output dead after kexec; but need visual feedback to the user
> while crash dumping" problem. I'm waiting for your patches.
>

I don't don't like it ;) Unfortunately too many people end up enabling
it and having issues with their keyboards. Can we have it depend on
DEBUG_KERNEL? And probably KEXEC as well?

Another option would be for it not use panic_blink. Do your kexec
kernels have atkbd support enabled? You could write an new "blink"
input handler that would latch to keyboards supporting leds and blink
by sending EV_LED events.

-- 
Dmitry

Re: blink driver power saving

[Andi Kleen]

> > Perhaps one of you geniuses who all hate it can find a better way to
> > solve the "video output dead after kexec; but need visual feedback to the user
> > while crash dumping" problem. I'm waiting for your patches.
> >
> 
> I don't don't like it ;) Unfortunately too many people end up enabling

Yes that's pretty weird. I admit I hadn't expected
that problem. blink is equivalent to "annoy me" and it
is a mystery why so many people should willingly ask their computer to 
annoy them.

Or perhaps they update their configs with yes | make oldconfig? 

User psychology can be mysterious.

I wonder if the kernel offered a CONFIG_FORMAT_FILESYSTEMS_AT_BOOT
how many people would enable that @) Might be an interesting experiment
for next April.

> it and having issues with their keyboards.

Forcing a suitable slow rate should fix that shouldn't it? We need
that anyways to stop the "setleds DOS". 

> Can we have it depend on 
> DEBUG_KERNEL? 

Yes that would be probably a good idea; even though it is technically
not correct: the debug kernel doesn't try to debug itself. But anyways,
it's probably the best place.

> And probably KEXEC as well? 

The kcrash kernel doesn't necessarily need to have kexec enabled by
itself.

> Another option would be for it not use panic_blink. Do your kexec
> kernels have atkbd support enabled? You could write an new "blink"
> input handler that would latch to keyboards supporting leds and blink
> by sending EV_LED events.

Yes that would be probably a better implementation. Also hook something
for USB keyboards. iirc Bernhard Walle (cc'ed) was looking at that.

-Andi

Re: blink driver power saving

[Bernhard Walle]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

* Andi Kleen <ak@suse.de> [2007-07-02 14:39]:
> 
> > Another option would be for it not use panic_blink. Do your kexec
> > kernels have atkbd support enabled? You could write an new "blink"
> > input handler that would latch to keyboards supporting leds and blink
> > by sending EV_LED events.
> 
> Yes that would be probably a better implementation. Also hook something
> for USB keyboards. iirc Bernhard Walle (cc'ed) was looking at that.

The problem is that most distributions use USB as module (in initrd,
or later). If you're able to load modules, you're also able to run a
userspace programs that blinks. That's the way I implemented it in
SUSE now. In my tests it takes about < 5 seconds from the sysrq-c to
the time the first blink. That's ok IMO.

And if a person compiles his kernel manually, he doesn't need keyboard
LED blinking for kdump because he can look at the HDD LED to see what
happens (or serial console). ... :)

I don't know if it's worth to apply a patch that uses the keyboard
interface in the kernel, because there are several small changes
necessary (see patch, that's what my thought was).


Thanks,
   Bernhard

[-- Attachment #2: add_setledstate_global --]
[-- Type: text/plain, Size: 1082 bytes --]

---
 drivers/char/keyboard.c  |    7 +++++++
 include/linux/keyboard.h |    7 +++++++
 2 files changed, 14 insertions(+)

Index: linux-2.6.21.1/drivers/char/keyboard.c
===================================================================
--- linux-2.6.21.1.orig/drivers/char/keyboard.c
+++ linux-2.6.21.1/drivers/char/keyboard.c
@@ -956,6 +956,13 @@ void setledstate(struct kbd_struct *kbd,
 	set_leds();
 }
 
+void setledstate_fgconsole(unsigned int led)
+{
+	struct kbd_struct *kbd = kbd_table + fg_console;
+	setledstate(kbd, led);
+}
+EXPORT_SYMBOL_GPL(setledstate_fgconsole);
+
 static inline unsigned char getleds(void)
 {
 	struct kbd_struct *kbd = kbd_table + fg_console;
Index: linux-2.6.21.1/include/linux/keyboard.h
===================================================================
--- linux-2.6.21.1.orig/include/linux/keyboard.h
+++ linux-2.6.21.1/include/linux/keyboard.h
@@ -441,4 +441,11 @@ extern unsigned short plain_map[NR_KEYS]
 #define NR_BRL		9
 
 #define MAX_DIACR	256
+
+
+
+#ifdef __KERNEL__
+void setledstate_fgconsole(unsigned int led);
+#endif
+
 #endif

Re: blink driver power saving

[Dmitry Torokhov]

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]

On 7/2/07, Andi Kleen <ak@suse.de> wrote:
>
> > > Perhaps one of you geniuses who all hate it can find a better way to
> > > solve the "video output dead after kexec; but need visual feedback to the user
> > > while crash dumping" problem. I'm waiting for your patches.
> > >
> >
> > I don't don't like it ;) Unfortunately too many people end up enabling
>
> Yes that's pretty weird. I admit I hadn't expected
> that problem. blink is equivalent to "annoy me" and it
> is a mystery why so many people should willingly ask their computer to
> annoy them.
>
> Or perhaps they update their configs with yes | make oldconfig?
>
> User psychology can be mysterious.
>
> I wonder if the kernel offered a CONFIG_FORMAT_FILESYSTEMS_AT_BOOT
> how many people would enable that @) Might be an interesting experiment
> for next April.
>

Heh ;) That could be interesting.

> > it and having issues with their keyboards.
>
> Forcing a suitable slow rate should fix that shouldn't it? We need
> that anyways to stop the "setleds DOS".
>

I already have a patch that throttles AT keyboard when switching LED
state. However there is another problem - i8042's interrupt hanler is
racing with panic_blink polling the keyboar controller and they both
don;t quite like that.

> > Can we have it depend on
> > DEBUG_KERNEL?
>
> Yes that would be probably a good idea; even though it is technically
> not correct: the debug kernel doesn't try to debug itself. But anyways,
> it's probably the best place.
>
> > And probably KEXEC as well?
>
> The kcrash kernel doesn't necessarily need to have kexec enabled by
> itself.
>

OK.

> > Another option would be for it not use panic_blink. Do your kexec
> > kernels have atkbd support enabled? You could write an new "blink"
> > input handler that would latch to keyboards supporting leds and blink
> > by sending EV_LED events.
>
> Yes that would be probably a better implementation. Also hook something
> for USB keyboards. iirc Bernhard Walle (cc'ed) was looking at that.
>

I was thinking about something like the atached (untested and sorry
for using attachment). It shoudl blink just one led (numLock) on any
keyboard that has such LED (and allows to control it).

-- 
Dmitry

[-- Attachment #2: blink.c --]
[-- Type: text/plain, Size: 2852 bytes --]

/*
 * Debug driver that continuosly blink LEDs on keyboards
 *
 * Copyright (c) 2007 Dmitry Torokhov
 */

/*
 * This program is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 as published
 * by the Free Software Foundation.
 */

#include <linux/module.h>
#include <linux/input.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
#include <linux/init.h>

MODULE_AUTHOR("Dmitry Torokhov <dtor@mail.ru>");
MODULE_DESCRIPTION("Blink driver");
MODULE_LICENSE("GPL");

struct blinker {
	struct delayed_work work;
	struct input_handle handle;
	int state;
};

static void blink_task_handler(struct work_struct *work)
{
	struct blinker *blinker = container_of(work, struct blinker, work.work);

	blinker->state = !blinker->state;
	input_inject_event(&blinker->handle, EV_LED, LED_NUML, blinker->state);
	schedule_delayed_work(&blinker->work, msecs_to_jiffies(250));
}

static void blink_event(struct input_handle *handle, unsigned int type,
		        unsigned int code, int down)
{
	/*
	 * This is a very rare handler that does not process any input
	 * events; just injects them.
	 */
}

static int blink_connect(struct input_handler *handler, struct input_dev *dev,
			  const struct input_device_id *id)
{
	struct blinker *blinker;
	struct input_handle *handle;
	int error;

	blinker = kzalloc(sizeof(struct blinker), GFP_KERNEL);
	if (!blinker)
		return -ENOMEM;

	INIT_DELAYED_WORK(&blinker->work, blink_task_handler);

	handle = &blinker->handle;
	handle->dev = dev;
	handle->handler = handler;
	handle->name = "blink";
	handle->private = blinker;

	error = input_register_handle(handle);
	if (error)
		goto err_free_handle;

	error = input_open_device(handle);
	if (error)
		goto err_unregister_handle;

	schedule_delayed_work(&blinker->work, 0);

	return 0;

 err_unregister_handle:
	input_unregister_handle(handle);
 err_free_handle:
	kfree(handle);
	return error;
}

static void blink_disconnect(struct input_handle *handle)
{
	struct blinker *blinker = handle->private;

	cancel_rearming_delayed_work(&blinker->work);
	input_close_device(handle);
	input_unregister_handle(handle);
	kfree(blinker);
}

static const struct input_device_id blink_ids[] = {
	{
		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_LEDBIT,
		.evbit = { BIT(EV_LED) },
		.keybit = { [LONG(LED_NUML)] = BIT(LED_NUML) },
	},
	{ }
};

static struct input_handler blink_handler = {
	.event		= blink_event;
	.connect	= blink_connect,
	.disconnect	= blink_disconnect,
	.name		= "blink",
	.id_table	= blink_ids,
};

static int __init blink_handler_init(void)
{
	return input_register_handler(&blink_handler);
}

static void __exit blink_handler_exit(void)
{
	input_unregister_handler(&blink_handler);
	flush_scheduled_work();
}

module_init(blink_handler_init);
module_exit(blink_handler_exit);

Re: blink driver power saving

[Dmitry Torokhov]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

On 7/2/07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> I was thinking about something like the atached (untested and sorry
> for using attachment). It shoudl blink just one led (numLock) on any
> keyboard that has such LED (and allows to control it).
>

Argh, bad one. This one shoudl be better.

-- 
Dmitry

[-- Attachment #2: blink.c --]
[-- Type: text/plain, Size: 2852 bytes --]

/*
 * Debug driver that continuosly blink LEDs on keyboards
 *
 * Copyright (c) 2007 Dmitry Torokhov
 */

/*
 * This program is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 as published
 * by the Free Software Foundation.
 */

#include <linux/module.h>
#include <linux/input.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
#include <linux/init.h>

MODULE_AUTHOR("Dmitry Torokhov <dtor@mail.ru>");
MODULE_DESCRIPTION("Blink driver");
MODULE_LICENSE("GPL");

struct blinker {
	struct delayed_work work;
	struct input_handle handle;
	int state;
};

static void blink_task_handler(struct work_struct *work)
{
	struct blinker *blinker = container_of(work, struct blinker, work.work);

	blinker->state = !blinker->state;
	input_inject_event(&blinker->handle, EV_LED, LED_NUML, blinker->state);
	schedule_delayed_work(&blinker->work, msecs_to_jiffies(250));
}

static void blink_event(struct input_handle *handle, unsigned int type,
		        unsigned int code, int down)
{
	/*
	 * This is a very rare handler that does not process any input
	 * events; just injects them.
	 */
}

static int blink_connect(struct input_handler *handler, struct input_dev *dev,
			  const struct input_device_id *id)
{
	struct blinker *blinker;
	struct input_handle *handle;
	int error;

	blinker = kzalloc(sizeof(struct blinker), GFP_KERNEL);
	if (!blinker)
		return -ENOMEM;

	INIT_DELAYED_WORK(&blinker->work, blink_task_handler);

	handle = &blinker->handle;
	handle->dev = dev;
	handle->handler = handler;
	handle->name = "blink";
	handle->private = blinker;

	error = input_register_handle(handle);
	if (error)
		goto err_free_handle;

	error = input_open_device(handle);
	if (error)
		goto err_unregister_handle;

	schedule_delayed_work(&blinker->work, 0);

	return 0;

 err_unregister_handle:
	input_unregister_handle(handle);
 err_free_handle:
	kfree(handle);
	return error;
}

static void blink_disconnect(struct input_handle *handle)
{
	struct blinker *blinker = handle->private;

	cancel_rearming_delayed_work(&blinker->work);
	input_close_device(handle);
	input_unregister_handle(handle);
	kfree(blinker);
}

static const struct input_device_id blink_ids[] = {
	{
		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_LEDBIT,
		.evbit = { BIT(EV_LED) },
		.ledbit = { [LONG(LED_NUML)] = BIT(LED_NUML) },
	},
	{ }
};

static struct input_handler blink_handler = {
	.event		= blink_event,
	.connect	= blink_connect,
	.disconnect	= blink_disconnect,
	.name		= "blink",
	.id_table	= blink_ids,
};

static int __init blink_handler_init(void)
{
	return input_register_handler(&blink_handler);
}

static void __exit blink_handler_exit(void)
{
	input_unregister_handler(&blink_handler);
	flush_scheduled_work();
}

module_init(blink_handler_init);
module_exit(blink_handler_exit);

Re: blink driver power saving

[Bernhard Walle]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> [2007-07-02 15:43]:
> On 7/2/07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>
>> I was thinking about something like the atached (untested and sorry
>> for using attachment). It shoudl blink just one led (numLock) on any
>> keyboard that has such LED (and allows to control it).
>>
>
> Argh, bad one. This one shoudl be better.

Ok, I didn't think of that possibility. That looks better than my
attempt to modify low-level console stuff ... :)


Thanks,
   Bernhard

Re: blink driver power saving

[Pavel Machek]

On Mon 2007-07-02 14:39:27, Andi Kleen wrote:
> 
> > > Perhaps one of you geniuses who all hate it can find a better way to
> > > solve the "video output dead after kexec; but need visual feedback to the user
> > > while crash dumping" problem. I'm waiting for your patches.
> > >
> > 
> > I don't don't like it ;) Unfortunately too many people end up enabling
> 
> Yes that's pretty weird. I admit I hadn't expected
> that problem. blink is equivalent to "annoy me" and it
> is a mystery why so many people should willingly ask their computer to 
> annoy them.

tristate "Keyboard blink driver"

...drivers are not expected to act on their own. I was expecting to
get nice /sys/class/led* interface to my keyboard leds.

BTW ... I still believe we should have /sys/class/led* interface to
those leds. I'd like to make them blink with hdd activity on some
machines... of course, that needs non-buggy KBC.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Dmitry Torokhov]

On Monday 02 July 2007 19:08, Pavel Machek wrote:
> On Mon 2007-07-02 14:39:27, Andi Kleen wrote:
> > 
> > > > Perhaps one of you geniuses who all hate it can find a better way to
> > > > solve the "video output dead after kexec; but need visual feedback to the user
> > > > while crash dumping" problem. I'm waiting for your patches.
> > > >
> > > 
> > > I don't don't like it ;) Unfortunately too many people end up enabling
> > 
> > Yes that's pretty weird. I admit I hadn't expected
> > that problem. blink is equivalent to "annoy me" and it
> > is a mystery why so many people should willingly ask their computer to 
> > annoy them.
> 
> tristate "Keyboard blink driver"
> 
> ...drivers are not expected to act on their own. I was expecting to
> get nice /sys/class/led* interface to my keyboard leds.
> 
> BTW ... I still believe we should have /sys/class/led* interface to
> those leds. I'd like to make them blink with hdd activity on some
> machines... of course, that needs non-buggy KBC.

I'll take patches. Ofcourse we'll have to keep the current EV_LED interface
for compatibility.

-- 
Dmitry

Re: blink driver power saving

[Bernhard Walle]

* Pavel Machek <pavel@ucw.cz> [2007-07-03 01:08]:
> On Mon 2007-07-02 14:39:27, Andi Kleen wrote:
> > 
> > > > Perhaps one of you geniuses who all hate it can find a better way to
> > > > solve the "video output dead after kexec; but need visual feedback to the user
> > > > while crash dumping" problem. I'm waiting for your patches.
> > > >
> > > 
> > > I don't don't like it ;) Unfortunately too many people end up enabling
> > 
> > Yes that's pretty weird. I admit I hadn't expected
> > that problem. blink is equivalent to "annoy me" and it
> > is a mystery why so many people should willingly ask their computer to 
> > annoy them.
> 
> tristate "Keyboard blink driver"
> 
> ...drivers are not expected to act on their own. I was expecting to
> get nice /sys/class/led* interface to my keyboard leds.

What's the benefit of such an interface? If you're able to trigger
keyboard LEDs via that interface, you're also able to use the ioctl()
on /dev/console.

I think the intention of the blink driver was to have a *early* blink,
i.e. before initrd (and on systems without intrd, before the first
init script runs).



Thanks,
   Bernhard

Re: blink driver power saving

[Pavel Machek]

Hi!

> > > > > Perhaps one of you geniuses who all hate it can find a better way to
> > > > > solve the "video output dead after kexec; but need visual feedback to the user
> > > > > while crash dumping" problem. I'm waiting for your patches.
> > > > >
> > > > 
> > > > I don't don't like it ;) Unfortunately too many people end up enabling
> > > 
> > > Yes that's pretty weird. I admit I hadn't expected
> > > that problem. blink is equivalent to "annoy me" and it
> > > is a mystery why so many people should willingly ask their computer to 
> > > annoy them.
> > 
> > tristate "Keyboard blink driver"
> > 
> > ...drivers are not expected to act on their own. I was expecting to
> > get nice /sys/class/led* interface to my keyboard leds.
> 
> What's the benefit of such an interface? If you're able to trigger
> keyboard LEDs via that interface, you're also able to use the ioctl()
> on /dev/console.

Well, at least it is standartized interface... plus it can do stuff
like "blink that led on disk access".

> I think the intention of the blink driver was to have a *early* blink,
> i.e. before initrd (and on systems without intrd, before the first
> init script runs).

...and yes, it can autoblink, too. It should be even possible to set
default behaviour of led to blink, doing what the blink driver does,
but in a clean way.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Pavel Machek]

Hi!

> > > > > Perhaps one of you geniuses who all hate it can find a better way to
> > > > > solve the "video output dead after kexec; but need visual feedback to the user
> > > > > while crash dumping" problem. I'm waiting for your patches.
> > > > >
> > > > 
> > > > I don't don't like it ;) Unfortunately too many people end up enabling
> > > 
> > > Yes that's pretty weird. I admit I hadn't expected
> > > that problem. blink is equivalent to "annoy me" and it
> > > is a mystery why so many people should willingly ask their computer to 
> > > annoy them.
> > 
> > tristate "Keyboard blink driver"
> > 
> > ...drivers are not expected to act on their own. I was expecting to
> > get nice /sys/class/led* interface to my keyboard leds.
> > 
> > BTW ... I still believe we should have /sys/class/led* interface to
> > those leds. I'd like to make them blink with hdd activity on some
> > machines... of course, that needs non-buggy KBC.
> 
> I'll take patches. Ofcourse we'll have to keep the current EV_LED interface
> for compatibility.

It was simpler then I thought... but it has one small problem. It does
not actually work :-(..

it reaches the 

        printk("Setting led to %d\n", blinker->state);
        input_inject_event(&blinker->handle, EV_LED, LED_NUML, blinker->state);

code, but my numlock led does not actually change. Any ideas?
									Pavel

(leds-input.c:)

/*
 * LED <-> input subsystem glue
 *
 * Copyright 2007 Pavel Machek <pavel@suse.cz>
 * Copyright 2007 Dmitry Torokhov
 * Copyright 2005-2006 Openedhand Ltd.
 *
 * Author: Pavel Machek <pavel@suse.cz>
 * Based on code by: Richard Purdie <rpurdie@openedhand.com>
 * 		     
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 *
 */

#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/leds.h>

#include <linux/module.h>
#include <linux/input.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
#include <linux/init.h>

struct blinker {
	struct delayed_work work;
	struct input_handle handle;
	int state;
};

struct blinker *blinker;

static void inputled_set(struct led_classdev *led_cdev, enum led_brightness value)
{
	blinker->state = value;
	schedule_delayed_work(&blinker->work, 0);
}

static struct led_classdev input_led = {
	.name			= "input",
	.default_trigger	= "sharpsl-charge",
	.brightness_set		= inputled_set,
};

static void blink_task_handler(struct work_struct *work)
{
	struct blinker *blinker = container_of(work, struct blinker, work.work);
	printk("Setting led to %d\n", blinker->state);
	input_inject_event(&blinker->handle, EV_LED, LED_NUML, blinker->state);
}

static void blink_event(struct input_handle *handle, unsigned int type,
		        unsigned int code, int down)
{
	/*
	 * This is a very rare handler that does not process any input
	 * events; just injects them.
	 */
}

static int blink_connect(struct input_handler *handler, struct input_dev *dev,
			  const struct input_device_id *id)
{
	struct input_handle *handle;
	int error;

	blinker = kzalloc(sizeof(struct blinker), GFP_KERNEL);
	if (!blinker)
		return -ENOMEM;

	INIT_DELAYED_WORK(&blinker->work, blink_task_handler);

	handle = &blinker->handle;
	handle->dev = dev;
	handle->handler = handler;
	handle->name = "blink";
	handle->private = blinker;

	error = input_register_handle(handle);
	if (error)
		goto err_free_handle;

	error = input_open_device(handle);
	if (error)
		goto err_unregister_handle;

	error = led_classdev_register(NULL, &input_led);
	if (error < 0)
		goto err_input_close_device;

	return 0;

 err_input_close_device:
	input_close_device(handle);
 err_unregister_handle:
	input_unregister_handle(handle);
 err_free_handle:
	kfree(handle);
	return error;
}

static void blink_disconnect(struct input_handle *handle)
{
	struct blinker *blinker = handle->private;

	led_classdev_unregister(&input_led);
	cancel_rearming_delayed_work(&blinker->work);
	input_close_device(handle);
	input_unregister_handle(handle);
	kfree(blinker);
}

static const struct input_device_id blink_ids[] = {
	{
		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_LEDBIT,
		.evbit = { BIT(EV_LED) },
		.ledbit = { [LONG(LED_NUML)] = BIT(LED_NUML) },
	},
	{ }
};

static struct input_handler blink_handler = {
	.event		= blink_event,
	.connect	= blink_connect,
	.disconnect	= blink_disconnect,
	.name		= "blink",
	.id_table	= blink_ids,
};

static int __init blink_handler_init(void)
{
	return input_register_handler(&blink_handler);
}

static void __exit blink_handler_exit(void)
{
	input_unregister_handler(&blink_handler);
	flush_scheduled_work();
}

module_init(blink_handler_init);
module_exit(blink_handler_exit);




-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Pavel Machek]

Hi!

> >I was thinking about something like the atached (untested and sorry
> >for using attachment). It shoudl blink just one led (numLock) on any
> >keyboard that has such LED (and allows to control it).
> >
> 
> Argh, bad one. This one shoudl be better.

Does it blink for you? It does not seem to do anything here :-(.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Pavel Machek]

On Tue 2007-07-03 01:42:35, Dmitry Torokhov wrote:
> On Monday 02 July 2007 19:08, Pavel Machek wrote:
> > On Mon 2007-07-02 14:39:27, Andi Kleen wrote:
> > > 
> > > > > Perhaps one of you geniuses who all hate it can find a better way to
> > > > > solve the "video output dead after kexec; but need visual feedback to the user
> > > > > while crash dumping" problem. I'm waiting for your patches.
> > > > >
> > > > 
> > > > I don't don't like it ;) Unfortunately too many people end up enabling
> > > 
> > > Yes that's pretty weird. I admit I hadn't expected
> > > that problem. blink is equivalent to "annoy me" and it
> > > is a mystery why so many people should willingly ask their computer to 
> > > annoy them.
> > 
> > tristate "Keyboard blink driver"
> > 
> > ...drivers are not expected to act on their own. I was expecting to
> > get nice /sys/class/led* interface to my keyboard leds.
> > 
> > BTW ... I still believe we should have /sys/class/led* interface to
> > those leds. I'd like to make them blink with hdd activity on some
> > machines... of course, that needs non-buggy KBC.
> 
> I'll take patches. Ofcourse we'll have to keep the current EV_LED interface
> for compatibility.

Update.. both your and mine code seem to do something here -- but they
only keep turning the led off. If I press numlock to turn it on, they
will turn it off. Strange.
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Pavel Machek]

Hi!

> > > > > Perhaps one of you geniuses who all hate it can find a better way to
> > > > > solve the "video output dead after kexec; but need visual feedback to the user
> > > > > while crash dumping" problem. I'm waiting for your patches.
> > > > >
> > > > 
> > > > I don't don't like it ;) Unfortunately too many people end up enabling
> > > 
> > > Yes that's pretty weird. I admit I hadn't expected
> > > that problem. blink is equivalent to "annoy me" and it
> > > is a mystery why so many people should willingly ask their computer to 
> > > annoy them.
> > 
> > tristate "Keyboard blink driver"
> > 
> > ...drivers are not expected to act on their own. I was expecting to
> > get nice /sys/class/led* interface to my keyboard leds.
> > 
> > BTW ... I still believe we should have /sys/class/led* interface to
> > those leds. I'd like to make them blink with hdd activity on some
> > machines... of course, that needs non-buggy KBC.
> 
> I'll take patches. Ofcourse we'll have to keep the current EV_LED interface
> for compatibility.

Here's a working version. For some reason, it only works with capslock
led. (But so does your code, is that thinkpad problem?)

Andi, by making default trigger "heartbeat", you can get very nice
visual feedback :-).

Signed-off-by: Pavel Machek <pavel@suse.cz>

/*
 * LED <-> input subsystem glue
 *
 * Copyright 2007 Pavel Machek <pavel@suse.cz>
 * Copyright 2007 Dmitry Torokhov
 * Copyright 2005-2006 Openedhand Ltd.
 *
 * Author: Pavel Machek <pavel@suse.cz>
 * Based on code by: Richard Purdie <rpurdie@openedhand.com>
 * 		     
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 *
 */

#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/leds.h>

#include <linux/module.h>
#include <linux/input.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
#include <linux/init.h>

struct blinker {
	struct delayed_work work;
	struct input_handle handle;
	int state;
};

struct blinker *blinker;

static void inputled_set(struct led_classdev *led_cdev, enum led_brightness value)
{
	blinker->state = value;
	schedule_delayed_work(&blinker->work, 0);
}

static struct led_classdev input_led = {
	.name			= "input",
	.default_trigger	= "none",
	.brightness_set		= inputled_set,
};

static void blink_task_handler(struct work_struct *work)
{
	struct blinker *blinker = container_of(work, struct blinker, work.work);
	input_inject_event(&blinker->handle, EV_LED, LED_CAPSL, !!blinker->state);
}

static void blink_event(struct input_handle *handle, unsigned int type,
		        unsigned int code, int down)
{
	/*
	 * This is a very rare handler that does not process any input
	 * events; just injects them.
	 */
}

static int blink_connect(struct input_handler *handler, struct input_dev *dev,
			  const struct input_device_id *id)
{
	struct input_handle *handle;
	int error;

	blinker = kzalloc(sizeof(struct blinker), GFP_KERNEL);
	if (!blinker)
		return -ENOMEM;

	INIT_DELAYED_WORK(&blinker->work, blink_task_handler);

	handle = &blinker->handle;
	handle->dev = dev;
	handle->handler = handler;
	handle->name = "blink";
	handle->private = blinker;

	error = input_register_handle(handle);
	if (error)
		goto err_free_handle;

	error = input_open_device(handle);
	if (error)
		goto err_unregister_handle;

	error = led_classdev_register(NULL, &input_led);
	if (error < 0)
		goto err_input_close_device;

	return 0;

 err_input_close_device:
	input_close_device(handle);
 err_unregister_handle:
	input_unregister_handle(handle);
 err_free_handle:
	kfree(handle);
	return error;
}

static void blink_disconnect(struct input_handle *handle)
{
	struct blinker *blinker = handle->private;

	led_classdev_unregister(&input_led);
	cancel_rearming_delayed_work(&blinker->work);
	input_close_device(handle);
	input_unregister_handle(handle);
	kfree(blinker);
}

static const struct input_device_id blink_ids[] = {
	{
		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_LEDBIT,
		.evbit = { BIT(EV_LED) },
		.ledbit = { [LONG(LED_CAPSL)] = BIT(LED_CAPSL) },
	},
	{ }
};

static struct input_handler blink_handler = {
	.event		= blink_event,
	.connect	= blink_connect,
	.disconnect	= blink_disconnect,
	.name		= "blink",
	.id_table	= blink_ids,
};

static int __init blink_handler_init(void)
{
	return input_register_handler(&blink_handler);
}

static void __exit blink_handler_exit(void)
{
	input_unregister_handler(&blink_handler);
	flush_scheduled_work();
}

module_init(blink_handler_init);
module_exit(blink_handler_exit);




-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Pavel Machek]

Hi!

> > tristate "Keyboard blink driver"
> > 
> > ...drivers are not expected to act on their own. I was expecting to
> > get nice /sys/class/led* interface to my keyboard leds.
> > 
> > BTW ... I still believe we should have /sys/class/led* interface to
> > those leds. I'd like to make them blink with hdd activity on some
> > machines... of course, that needs non-buggy KBC.
> 
> I'll take patches. Ofcourse we'll have to keep the current EV_LED interface
> for compatibility.

Actually here's one that does not immediately oops when I plug USB
keyboard in.

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 87d2046..716620c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -95,6 +95,13 @@ config LEDS_COBALT
 	help
 	  This option enables support for the front LED on Cobalt Server
 
+config LEDS_INPUT
+	tristate "LED Support for input layer keyboards"
+	depends on LEDS_CLASS
+	help
+	  This option enables support for LEDs on keyboards handled by
+	  input layer.
+
 comment "LED Triggers"
 
 config LEDS_TRIGGERS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index aa2c18e..ea58020 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -16,8 +16,10 @@ obj-$(CONFIG_LEDS_NET48XX)		+= leds-net4
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
 obj-$(CONFIG_LEDS_H1940)		+= leds-h1940.o
 obj-$(CONFIG_LEDS_COBALT)		+= leds-cobalt.o
+obj-$(CONFIG_LEDS_INPUT)		+= leds-input.o
 
 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
 obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)	+= ledtrig-ide-disk.o
 obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
+
diff --git a/drivers/leds/leds-input.c b/drivers/leds/leds-input.c
new file mode 100644
index 0000000..8caca35
--- /dev/null
+++ b/drivers/leds/leds-input.c
@@ -0,0 +1,153 @@
+/*
+ * LED <-> input subsystem glue
+ *
+ * Copyright 2007 Pavel Machek <pavel@suse.cz>
+ * Copyright 2007 Dmitry Torokhov
+ * Copyright 2005-2006 Openedhand Ltd.
+ *
+ * Author: Pavel Machek <pavel@suse.cz>
+ * Based on code by: Richard Purdie <rpurdie@openedhand.com>
+ * 		     
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+
+#include <linux/module.h>
+#include <linux/input.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+#include <linux/init.h>
+
+struct blinker {
+	struct delayed_work work;
+	struct input_handle handle;
+	int state;
+
+	struct led_classdev dev;
+};
+
+struct blinker *blinker;
+
+static void inputled_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+	struct blinker *blinker = container_of(led_cdev, struct blinker, dev);
+	blinker->state = value;
+	schedule_delayed_work(&blinker->work, 0);
+}
+
+static void blink_task_handler(struct work_struct *work)
+{
+	struct blinker *blinker = container_of(work, struct blinker, work.work);
+	printk("Setting led to %d\n", blinker->state);
+	input_inject_event(&blinker->handle, EV_LED, LED_CAPSL, !!blinker->state);
+}
+
+static void blink_event(struct input_handle *handle, unsigned int type,
+		        unsigned int code, int down)
+{
+	/*
+	 * This is a very rare handler that does not process any input
+	 * events; just injects them.
+	 */
+}
+
+static int blink_connect(struct input_handler *handler, struct input_dev *dev,
+			  const struct input_device_id *id)
+{
+	struct input_handle *handle;
+	struct led_classdev *led_dev;
+	static int counter;
+	int error;
+
+	blinker = kzalloc(sizeof(struct blinker), GFP_KERNEL);
+	if (!blinker) {
+		return -ENOMEM;
+	}
+
+	INIT_DELAYED_WORK(&blinker->work, blink_task_handler);
+
+	led_dev = &blinker->dev;
+	led_dev->name = kmalloc(10, GFP_KERNEL);
+	sprintf(led_dev->name, "input%d", counter++);
+	led_dev->brightness_set = inputled_set;
+
+	handle = &blinker->handle;
+	handle->dev = dev;
+	handle->handler = handler;
+	handle->name = "blink";
+	handle->private = blinker;
+
+	error = input_register_handle(handle);
+	if (error)
+		goto err_free_handle;
+
+	error = input_open_device(handle);
+	if (error)
+		goto err_unregister_handle;
+
+	error = led_classdev_register(NULL, led_dev);
+	if (error < 0)
+		goto err_input_close_device;
+
+	return 0;
+
+ err_input_close_device:
+	input_close_device(handle);
+ err_unregister_handle:
+	input_unregister_handle(handle);
+ err_free_handle:
+	kfree(handle);
+	return error;
+}
+
+static void blink_disconnect(struct input_handle *handle)
+{
+	struct blinker *blinker = handle->private;
+
+	led_classdev_unregister(&blinker->dev);
+	cancel_rearming_delayed_work(&blinker->work);
+	input_close_device(handle);
+	input_unregister_handle(handle);
+	kfree(blinker);
+}
+
+static const struct input_device_id blink_ids[] = {
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_LEDBIT,
+		.evbit = { BIT(EV_LED) },
+		.ledbit = { [LONG(LED_CAPSL)] = BIT(LED_CAPSL) },
+	},
+	{ }
+};
+
+static struct input_handler blink_handler = {
+	.event		= blink_event,
+	.connect	= blink_connect,
+	.disconnect	= blink_disconnect,
+	.name		= "blink",
+	.id_table	= blink_ids,
+};
+
+static int __init blink_handler_init(void)
+{
+	return input_register_handler(&blink_handler);
+}
+
+static void __exit blink_handler_exit(void)
+{
+	input_unregister_handler(&blink_handler);
+	flush_scheduled_work();
+}
+
+module_init(blink_handler_init);
+module_exit(blink_handler_exit);
+
+
diff --git a/drivers/leds/leds-spitz.c b/drivers/leds/leds-spitz.c
index 126d09c..8d10274 100644
--- a/drivers/leds/leds-spitz.c
+++ b/drivers/leds/leds-spitz.c
@@ -1,5 +1,5 @@
 /*
- * LED Triggers Core
+ * SPITZ LED Driver
  *
  * Copyright 2005-2006 Openedhand Ltd.
  *






-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Linus Torvalds]

On Thu, 5 Jul 2007, Pavel Machek wrote:
>
> Actually here's one that does not immediately oops when I plug USB
> keyboard in.

Why do you use a delayed workqueue and then always use it without a delay? 
That seems silly.

		Linus

Re: blink driver power saving

[Pavel Machek]

Hi!

> > Actually here's one that does not immediately oops when I plug USB
> > keyboard in.
> 
> Why do you use a delayed workqueue and then always use it without a delay? 
> That seems silly.

I copied code from Dmitry :-); guess I copied too much. Here's updated
version:

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 87d2046..716620c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -95,6 +95,13 @@ config LEDS_COBALT
 	help
 	  This option enables support for the front LED on Cobalt Server
 
+config LEDS_INPUT
+	tristate "LED Support for input layer keyboards"
+	depends on LEDS_CLASS
+	help
+	  This option enables support for LEDs on keyboards handled by
+	  input layer.
+
 comment "LED Triggers"
 
 config LEDS_TRIGGERS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index aa2c18e..ea58020 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -16,8 +16,10 @@ obj-$(CONFIG_LEDS_NET48XX)		+= leds-net4
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
 obj-$(CONFIG_LEDS_H1940)		+= leds-h1940.o
 obj-$(CONFIG_LEDS_COBALT)		+= leds-cobalt.o
+obj-$(CONFIG_LEDS_INPUT)		+= leds-input.o
 
 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
 obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)	+= ledtrig-ide-disk.o
 obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
+
diff --git a/drivers/leds/leds-input.c b/drivers/leds/leds-input.c
new file mode 100644
index 0000000..8caca35
--- /dev/null
+++ b/drivers/leds/leds-input.c
@@ -0,0 +1,153 @@
+/*
+ * LED <-> input subsystem glue
+ *
+ * Copyright 2007 Pavel Machek <pavel@suse.cz>
+ * Copyright 2007 Dmitry Torokhov
+ * Copyright 2005-2006 Openedhand Ltd.
+ *
+ * Author: Pavel Machek <pavel@suse.cz>
+ * Based on code by: Richard Purdie <rpurdie@openedhand.com>
+ * 		     
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+
+#include <linux/module.h>
+#include <linux/input.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+#include <linux/init.h>
+
+struct blinker {
+	struct delayed_work work;
+	struct input_handle handle;
+	int state;
+
+	struct led_classdev dev;
+};
+
+struct blinker *blinker;
+
+static void inputled_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+	struct blinker *blinker = container_of(led_cdev, struct blinker, dev);
+	blinker->state = value;
+	schedule_delayed_work(&blinker->work, 0);
+}
+
+static void blink_task_handler(struct work_struct *work)
+{
+	struct blinker *blinker = container_of(work, struct blinker, work.work);
+	printk("Setting led to %d\n", blinker->state);
+	input_inject_event(&blinker->handle, EV_LED, LED_CAPSL, !!blinker->state);
+}
+
+static void blink_event(struct input_handle *handle, unsigned int type,
+		        unsigned int code, int down)
+{
+	/*
+	 * This is a very rare handler that does not process any input
+	 * events; just injects them.
+	 */
+}
+
+static int blink_connect(struct input_handler *handler, struct input_dev *dev,
+			  const struct input_device_id *id)
+{
+	struct input_handle *handle;
+	struct led_classdev *led_dev;
+	static int counter;
+	int error;
+
+	blinker = kzalloc(sizeof(struct blinker), GFP_KERNEL);
+	if (!blinker) {
+		return -ENOMEM;
+	}
+
+	INIT_DELAYED_WORK(&blinker->work, blink_task_handler);
+
+	led_dev = &blinker->dev;
+	led_dev->name = kmalloc(10, GFP_KERNEL);
+	sprintf(led_dev->name, "input%d", counter++);
+	led_dev->brightness_set = inputled_set;
+
+	handle = &blinker->handle;
+	handle->dev = dev;
+	handle->handler = handler;
+	handle->name = "blink";
+	handle->private = blinker;
+
+	error = input_register_handle(handle);
+	if (error)
+		goto err_free_handle;
+
+	error = input_open_device(handle);
+	if (error)
+		goto err_unregister_handle;
+
+	error = led_classdev_register(NULL, led_dev);
+	if (error < 0)
+		goto err_input_close_device;
+
+	return 0;
+
+ err_input_close_device:
+	input_close_device(handle);
+ err_unregister_handle:
+	input_unregister_handle(handle);
+ err_free_handle:
+	kfree(handle);
+	return error;
+}
+
+static void blink_disconnect(struct input_handle *handle)
+{
+	struct blinker *blinker = handle->private;
+
+	led_classdev_unregister(&blinker->dev);
+	cancel_rearming_delayed_work(&blinker->work);
+	input_close_device(handle);
+	input_unregister_handle(handle);
+	kfree(blinker);
+}
+
+static const struct input_device_id blink_ids[] = {
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_LEDBIT,
+		.evbit = { BIT(EV_LED) },
+		.ledbit = { [LONG(LED_CAPSL)] = BIT(LED_CAPSL) },
+	},
+	{ }
+};
+
+static struct input_handler blink_handler = {
+	.event		= blink_event,
+	.connect	= blink_connect,
+	.disconnect	= blink_disconnect,
+	.name		= "blink",
+	.id_table	= blink_ids,
+};
+
+static int __init blink_handler_init(void)
+{
+	return input_register_handler(&blink_handler);
+}
+
+static void __exit blink_handler_exit(void)
+{
+	input_unregister_handler(&blink_handler);
+	flush_scheduled_work();
+}
+
+module_init(blink_handler_init);
+module_exit(blink_handler_exit);
+
+
diff --git a/drivers/leds/leds-spitz.c b/drivers/leds/leds-spitz.c
index 126d09c..8d10274 100644
--- a/drivers/leds/leds-spitz.c
+++ b/drivers/leds/leds-spitz.c
@@ -1,5 +1,5 @@
 /*
- * LED Triggers Core
+ * SPITZ LED Driver
  *
  * Copyright 2005-2006 Openedhand Ltd.
  *


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Pavel Machek]

Hi!

> > I copied code from Dmitry :-); guess I copied too much. Here's updated
> > version:
> 
> Umm, it's updated exactly how?

Sorry. I updated it in my tree but produced wrong patch. Here's
updated updated version: (trimmed cc list).

> > +struct blinker {
> > +	struct delayed_work work;
> ...
> > +	schedule_delayed_work(&blinker->work, 0);
> ...
> > +	INIT_DELAYED_WORK(&blinker->work, blink_task_handler);


diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 87d2046..716620c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -95,6 +95,13 @@ config LEDS_COBALT
 	help
 	  This option enables support for the front LED on Cobalt Server
 
+config LEDS_INPUT
+	tristate "LED Support for input layer keyboards"
+	depends on LEDS_CLASS
+	help
+	  This option enables support for LEDs on keyboards handled by
+	  input layer.
+
 comment "LED Triggers"
 
 config LEDS_TRIGGERS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index aa2c18e..ea58020 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -16,8 +16,10 @@ obj-$(CONFIG_LEDS_NET48XX)		+= leds-net4
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
 obj-$(CONFIG_LEDS_H1940)		+= leds-h1940.o
 obj-$(CONFIG_LEDS_COBALT)		+= leds-cobalt.o
+obj-$(CONFIG_LEDS_INPUT)		+= leds-input.o
 
 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
 obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)	+= ledtrig-ide-disk.o
 obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
+
diff --git a/drivers/leds/leds-input.c b/drivers/leds/leds-input.c
new file mode 100644
index 0000000..2805de8
--- /dev/null
+++ b/drivers/leds/leds-input.c
@@ -0,0 +1,151 @@
+/*
+ * LED <-> input subsystem glue
+ *
+ * Copyright 2007 Pavel Machek <pavel@suse.cz>
+ * Copyright 2007 Dmitry Torokhov
+ * Copyright 2005-2006 Openedhand Ltd.
+ *
+ * Author: Pavel Machek <pavel@suse.cz>
+ * Based on code by: Richard Purdie <rpurdie@openedhand.com>
+ * 		     
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+
+#include <linux/module.h>
+#include <linux/input.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+#include <linux/init.h>
+
+struct blinker {
+	struct work_struct work;
+	struct input_handle handle;
+	int state;
+
+	struct led_classdev dev;
+};
+
+struct blinker *blinker;
+
+static void inputled_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+	struct blinker *blinker = container_of(led_cdev, struct blinker, dev);
+	blinker->state = value;
+	schedule_work(&blinker->work);
+}
+
+static void blink_task_handler(struct work_struct *work)
+{
+	struct blinker *blinker = container_of(work, struct blinker, work);
+	input_inject_event(&blinker->handle, EV_LED, LED_CAPSL, !!blinker->state);
+}
+
+static void blink_event(struct input_handle *handle, unsigned int type,
+		        unsigned int code, int down)
+{
+	/*
+	 * This is a very rare handler that does not process any input
+	 * events; just injects them.
+	 */
+}
+
+static int blink_connect(struct input_handler *handler, struct input_dev *dev,
+			  const struct input_device_id *id)
+{
+	struct input_handle *handle;
+	struct led_classdev *led_dev;
+	static int counter;
+	int error;
+
+	blinker = kzalloc(sizeof(struct blinker), GFP_KERNEL);
+	if (!blinker) {
+		return -ENOMEM;
+	}
+
+	INIT_WORK(&blinker->work, blink_task_handler);
+
+	led_dev = &blinker->dev;
+	led_dev->name = kmalloc(10, GFP_KERNEL);
+	sprintf(led_dev->name, "input%d", counter++);
+	led_dev->brightness_set = inputled_set;
+
+	handle = &blinker->handle;
+	handle->dev = dev;
+	handle->handler = handler;
+	handle->name = "blink";
+	handle->private = blinker;
+
+	error = input_register_handle(handle);
+	if (error)
+		goto err_free_handle;
+
+	error = input_open_device(handle);
+	if (error)
+		goto err_unregister_handle;
+
+	error = led_classdev_register(NULL, led_dev);
+	if (error < 0)
+		goto err_input_close_device;
+
+	return 0;
+
+ err_input_close_device:
+	input_close_device(handle);
+ err_unregister_handle:
+	input_unregister_handle(handle);
+ err_free_handle:
+	kfree(handle);
+	return error;
+}
+
+static void blink_disconnect(struct input_handle *handle)
+{
+	struct blinker *blinker = handle->private;
+
+	led_classdev_unregister(&blinker->dev);
+	input_close_device(handle);
+	input_unregister_handle(handle);
+	kfree(blinker);
+}
+
+static const struct input_device_id blink_ids[] = {
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_LEDBIT,
+		.evbit = { BIT(EV_LED) },
+		.ledbit = { [LONG(LED_CAPSL)] = BIT(LED_CAPSL) },
+	},
+	{ }
+};
+
+static struct input_handler blink_handler = {
+	.event		= blink_event,
+	.connect	= blink_connect,
+	.disconnect	= blink_disconnect,
+	.name		= "blink",
+	.id_table	= blink_ids,
+};
+
+static int __init blink_handler_init(void)
+{
+	return input_register_handler(&blink_handler);
+}
+
+static void __exit blink_handler_exit(void)
+{
+	input_unregister_handler(&blink_handler);
+	flush_scheduled_work();
+}
+
+module_init(blink_handler_init);
+module_exit(blink_handler_exit);
+
+
diff --git a/drivers/leds/leds-spitz.c b/drivers/leds/leds-spitz.c
index 126d09c..8d10274 100644
--- a/drivers/leds/leds-spitz.c
+++ b/drivers/leds/leds-spitz.c
@@ -1,5 +1,5 @@
 /*
- * LED Triggers Core
+ * SPITZ LED Driver
  *
  * Copyright 2005-2006 Openedhand Ltd.
  *

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Dmitry Torokhov]

On Wednesday 04 July 2007 17:47, Pavel Machek wrote:
> Hi!
> 
> > >I was thinking about something like the atached (untested and sorry
> > >for using attachment). It shoudl blink just one led (numLock) on any
> > >keyboard that has such LED (and allows to control it).
> > >
> > 
> > Argh, bad one. This one shoudl be better.
> 
> Does it blink for you? It does not seem to do anything here :-(.

Yes it does. Just tested it and it blinks NumLock led just fine.

-- 
Dmitry

Re: blink driver power saving

[Dmitry Torokhov]

On Wednesday 04 July 2007 18:11, Pavel Machek wrote:
> Hi!
> 
> > > > > > Perhaps one of you geniuses who all hate it can find a better way to
> > > > > > solve the "video output dead after kexec; but need visual feedback to the user
> > > > > > while crash dumping" problem. I'm waiting for your patches.
> > > > > >
> > > > > 
> > > > > I don't don't like it ;) Unfortunately too many people end up enabling
> > > > 
> > > > Yes that's pretty weird. I admit I hadn't expected
> > > > that problem. blink is equivalent to "annoy me" and it
> > > > is a mystery why so many people should willingly ask their computer to 
> > > > annoy them.
> > > 
> > > tristate "Keyboard blink driver"
> > > 
> > > ...drivers are not expected to act on their own. I was expecting to
> > > get nice /sys/class/led* interface to my keyboard leds.
> > > 
> > > BTW ... I still believe we should have /sys/class/led* interface to
> > > those leds. I'd like to make them blink with hdd activity on some
> > > machines... of course, that needs non-buggy KBC.
> > 
> > I'll take patches. Ofcourse we'll have to keep the current EV_LED interface
> > for compatibility.
> 
> Here's a working version. For some reason, it only works with capslock
> led. (But so does your code, is that thinkpad problem?)
>

My code does blink NumLock on my box (Dell) so I wonder why it does not
on your box...

Anyway, as far as the patch goes - I don't think we want to do it this
way for a couple of reasons:

1. It does not reflect the true device hierarchy - LEDs are not children
of input devices, they are children of whatever device that owns input
device. So in case of AT keyboard LEDs should grow from corresponding
serio port, not input device and so atkbd should register all of its leds.

2. LEDs will stop working if input device is grabbed via EVIOCSGRAB.
I think that grab should not cross subsystem boundaries.

-- 
Dmitry

Re: blink driver power saving

[Bill Davidsen]

Pavel Machek wrote:

>>> ...drivers are not expected to act on their own. I was expecting to
>>> get nice /sys/class/led* interface to my keyboard leds.
>> What's the benefit of such an interface? If you're able to trigger
>> keyboard LEDs via that interface, you're also able to use the ioctl()
>> on /dev/console.
> 
> Well, at least it is standartized interface... plus it can do stuff
> like "blink that led on disk access".
> 
One of many useful things for system without blinking lights, disk 
network, thermal alert, etc. And a cheap helper for handicapped folks 
who can't hear an audible alert.

>> I think the intention of the blink driver was to have a *early* blink,
>> i.e. before initrd (and on systems without intrd, before the first
>> init script runs).
> 
> ...and yes, it can autoblink, too. It should be even possible to set
> default behaviour of led to blink, doing what the blink driver does,
> but in a clean way.

Endlessly useful, alarm clock, non-fatal errors on boot, etc.

<it would be nice>
If this were done, priority levels would be nice, so the "I'm taking a 
dump" or "panic" would block lower level system use like disk or network 
lights, and user applications would have some policy to put them higher 
or lower than the pseudo disk light (or not).
</not nice>

-- 
Bill Davidsen <davidsen@tmr.com>
   "We have more to fear from the bungling of the incompetent than from
the machinations of the wicked."  - from Slashdot

Re: blink driver power saving

[Pavel Machek]

Hi!

> > > I'll take patches. Ofcourse we'll have to keep the current EV_LED interface
> > > for compatibility.
> > 
> > Here's a working version. For some reason, it only works with capslock
> > led. (But so does your code, is that thinkpad problem?)
> 
> My code does blink NumLock on my box (Dell) so I wonder why it does not
> on your box...

setleds does not work here, either, so...

> Anyway, as far as the patch goes - I don't think we want to do it this
> way for a couple of reasons:
> 
> 1. It does not reflect the true device hierarchy - LEDs are not children
> of input devices, they are children of whatever device that owns input
> device. So in case of AT keyboard LEDs should grow from corresponding
> serio port, not input device and so atkbd should register all of its
> leds.

I'm not sure what you mean here. Should the code be moved to atkbd.c?
...and then duplicated to usbkbd.c?

> 2. LEDs will stop working if input device is grabbed via EVIOCSGRAB.
> I think that grab should not cross subsystem boundaries.

Well, if userspace grabbed a LED, it would make sense to stop blinking
it from heartbeat, no?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Jiri Kosina]

On Thu, 12 Jul 2007, Pavel Machek wrote:

> I'm not sure what you mean here. Should the code be moved to atkbd.c? 
> ...and then duplicated to usbkbd.c?

Hi Pavel,

just a sidenote - usbkbd.c is probably a confusing misnomer, renaming it 
to something more appropriate is on my todo for one day. It is not the 
driver that controls the USB keyboard on the majority of systems - see 
Kconfig help text, that explains it very clearly. USB keyboards are 
normally handled by usbhid module (drivers/hid/usbhid/*).

-- 
Jiri Kosina

Re: blink driver power saving

[Pavel Machek]

Hi!

> > The patch makes sense. You don't need to poll every jiffie to find
> > out if system has panic.
> 
> The blink driver doesn't run on panic (or at least not on panic on
> the same kernel). It runs always. It was designed to do the blinking
> while the kdump kernel runs and writes the dump.
> 
> > But I agree with Linus, it is the kind 
> > of patch that doesn't belong in the mainline kernel.  Every developer
> > seems to have built up a set of crappy/fragile debug tools, but these
> > don't belong in the wild.
> 
> Would you argue then that kdump also doesn't belong into the kernel?
> The patch was designed to plug a hole in kdump (no visual feedback) 


   kexec -p <dump-capture-kernel-vmlinux-image> \
   --initrd=<initrd-for-dump-capture-kernel> --args-linux \
   --append="root=<root-dev> <arch-specific-options>"

....so we are already using initrd for dumping...? I'd say providing
visual feedback is surely an userspace task.

Heck, with the keyboard leds you could probably blink them in a way
telling user how much dump was done. Blink once then pause - 10%,
blink twice then pause - 20%, ...

And yes, we already have LED subsystem capable of doing all this.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: blink driver power saving

[Andi Kleen]

On Monday 02 July 2007 19:50:00 Stephen Hemminger wrote:

> The patch makes sense. You don't need to poll every jiffie to find
> out if system has panic.

The blink driver doesn't run on panic (or at least not on panic on
the same kernel). It runs always. It was designed to do the blinking
while the kdump kernel runs and writes the dump.

> But I agree with Linus, it is the kind 
> of patch that doesn't belong in the mainline kernel.  Every developer
> seems to have built up a set of crappy/fragile debug tools, but these
> don't belong in the wild.

Would you argue then that kdump also doesn't belong into the kernel?
The patch was designed to plug a hole in kdump (no visual feedback) 

-Andi

Re: blink driver power saving

[Dmitry Torokhov]

On 7/2/07, Stephen Hemminger <shemminger@linux-foundation.org> wrote:
>
> The patch makes sense. You don't need to poll every jiffie to find
> out if system has panic.  But I agree with Linus, it is the kind
> of patch that doesn't belong in the mainline kernel.  Every developer
> seems to have built up a set of crappy/fragile debug tools, but these
> don't belong in the wild.
>

Stephen,

The drivers blinks LEDs when kernel operates normally. During panic
the panic_blink() routine in i8042 gets called directly, without
involving the blink driver.

-- 
Dmitry

Re: blink driver power saving

[Stephen Hemminger]

On Mon, 2 Jul 2007 01:59:50 +0200
Andi Kleen <ak@suse.de> wrote:

> 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.

It can be generally useful. Until/unless we get proper screen support,
how do you know hang from panic? If you can talk to some other
device it would help.

> 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.

The patch makes sense. You don't need to poll every jiffie to find
out if system has panic.  But I agree with Linus, it is the kind
of patch that doesn't belong in the mainline kernel.  Every developer
seems to have built up a set of crappy/fragile debug tools, but these
don't belong in the wild.



-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: blink driver power saving

[Alan Cox]

> > 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

Re: blink driver power saving

[Linus Torvalds]

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

Re: blink driver power saving

[Bodo Eggert]

Andi Kleen <ak@suse.de> wrote:
> 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.

There may be some setups that would overheat due to the sudden load,
while still running fine under normal conditions. Especially if the
fan is off or broken.
-- 
Fun things to slip into your budget
An abacus

Friß, Spammer: FK2F@ugyhXY.7eggert.dyndns.org u@zq7ggfIq.7eggert.dyndns.org

Re: blink driver power saving

[Andi Kleen]

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

Re: blink driver power saving

[Linus Torvalds]

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

Re: blink driver power saving

[Andi Kleen]

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

Re: blink driver power saving

[Andi Kleen]

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

Re: blink driver power saving

[Linus Torvalds]

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

blink driver power saving

[Stephen Hemminger]

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 <shemminger@linux-foundation.org>

--- 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);