Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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. 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). -- Bill Davidsen <[EMAIL PROTECTED]> "We have more to fear from the bungling of the incompetent than from the machinations of the wicked." - from Slashdot - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 [EMAIL PROTECTED] We have more to fear from the bungling of the incompetent than from the machinations of the wicked. - from Slashdot - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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(>work, 0);
> ...
> > + INIT_DELAYED_WORK(>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 000..2805de8
--- /dev/null
+++ b/drivers/leds/leds-input.c
@@ -0,0 +1,151 @@
+/*
+ * LED <-> input subsystem glue
+ *
+ * Copyright 2007 Pavel Machek <[EMAIL PROTECTED]>
+ * Copyright 2007 Dmitry Torokhov
+ * Copyright 2005-2006 Openedhand Ltd.
+ *
+ * Author: Pavel Machek <[EMAIL PROTECTED]>
+ * Based on code by: Richard Purdie <[EMAIL PROTECTED]>
+ *
+ *
+ * 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
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+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(>work);
+}
+
+static void blink_task_handler(struct work_struct *work)
+{
+ struct blinker *blinker = container_of(work, struct blinker, work);
+ input_inject_event(>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(>work, blink_task_handler);
+
+ led_dev = >dev;
+ led_dev->name = kmalloc(10, GFP_KERNEL);
+ sprintf(led_dev->name, "input%d", counter++);
+ led_dev->brightness_set = inputled_set;
+
+ handle = >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(>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,
Re: blink driver power saving
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 <[EMAIL PROTECTED]>
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 000..8caca35
--- /dev/null
+++ b/drivers/leds/leds-input.c
@@ -0,0 +1,153 @@
+/*
+ * LED <-> input subsystem glue
+ *
+ * Copyright 2007 Pavel Machek <[EMAIL PROTECTED]>
+ * Copyright 2007 Dmitry Torokhov
+ * Copyright 2005-2006 Openedhand Ltd.
+ *
+ * Author: Pavel Machek <[EMAIL PROTECTED]>
+ * Based on code by: Richard Purdie <[EMAIL PROTECTED]>
+ *
+ *
+ * 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
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+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(>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(>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(>work, blink_task_handler);
+
+ led_dev = >dev;
+ led_dev->name = kmalloc(10, GFP_KERNEL);
+ sprintf(led_dev->name, "input%d", counter++);
+ led_dev->brightness_set = inputled_set;
+
+ handle = >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(>dev);
+ cancel_rearming_delayed_work(>work);
+ input_close_device(handle);
+ input_unregister_handle(handle);
+ kfree(blinker);
+}
+
+static const struct input_device_id blink_ids[] = {
+ {
+ .flags =
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 <[EMAIL PROTECTED]>
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 000..8caca35
--- /dev/null
+++ b/drivers/leds/leds-input.c
@@ -0,0 +1,153 @@
+/*
+ * LED <-> input subsystem glue
+ *
+ * Copyright 2007 Pavel Machek <[EMAIL PROTECTED]>
+ * Copyright 2007 Dmitry Torokhov
+ * Copyright 2005-2006 Openedhand Ltd.
+ *
+ * Author: Pavel Machek <[EMAIL PROTECTED]>
+ * Based on code by: Richard Purdie <[EMAIL PROTECTED]>
+ *
+ *
+ * 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
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+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(>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(>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(>work, blink_task_handler);
+
+ led_dev = >dev;
+ led_dev->name = kmalloc(10, GFP_KERNEL);
+ sprintf(led_dev->name, "input%d", counter++);
+ led_dev->brightness_set = inputled_set;
+
+ handle = >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;
+
+
Re: blink driver power saving
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 <[EMAIL PROTECTED]>
/*
* LED <-> input subsystem glue
*
* Copyright 2007 Pavel Machek <[EMAIL PROTECTED]>
* Copyright 2007 Dmitry Torokhov
* Copyright 2005-2006 Openedhand Ltd.
*
* Author: Pavel Machek <[EMAIL PROTECTED]>
* Based on code by: Richard Purdie <[EMAIL PROTECTED]>
*
*
* 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
#include
#include
#include
#include
#include
#include
#include
#include
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(>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(>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(>work, blink_task_handler);
handle = >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);
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(_led);
cancel_rearming_delayed_work(>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(_handler);
}
static void __exit blink_handler_exit(void)
{
input_unregister_handler(_handler);
flush_scheduled_work();
}
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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(>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 <[EMAIL PROTECTED]>
* Copyright 2007 Dmitry Torokhov
* Copyright 2005-2006 Openedhand Ltd.
*
* Author: Pavel Machek <[EMAIL PROTECTED]>
* Based on code by: Richard Purdie <[EMAIL PROTECTED]>
*
*
* 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
#include
#include
#include
#include
#include
#include
#include
#include
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(>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(>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(>work, blink_task_handler);
handle = >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);
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(_led);
cancel_rearming_delayed_work(>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)
{
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 [EMAIL PROTECTED]
* Copyright 2007 Dmitry Torokhov
* Copyright 2005-2006 Openedhand Ltd.
*
* Author: Pavel Machek [EMAIL PROTECTED]
* Based on code by: Richard Purdie [EMAIL PROTECTED]
*
*
* 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
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 [EMAIL PROTECTED]
/*
* LED - input subsystem glue
*
* Copyright 2007 Pavel Machek [EMAIL PROTECTED]
* Copyright 2007 Dmitry Torokhov
* Copyright 2005-2006 Openedhand Ltd.
*
* Author: Pavel Machek [EMAIL PROTECTED]
* Based on code by: Richard Purdie [EMAIL PROTECTED]
*
*
* 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)
{
Re: blink driver power saving
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 [EMAIL PROTECTED]
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 000..8caca35
--- /dev/null
+++ b/drivers/leds/leds-input.c
@@ -0,0 +1,153 @@
+/*
+ * LED - input subsystem glue
+ *
+ * Copyright 2007 Pavel Machek [EMAIL PROTECTED]
+ * Copyright 2007 Dmitry Torokhov
+ * Copyright 2005-2006 Openedhand Ltd.
+ *
+ * Author: Pavel Machek [EMAIL PROTECTED]
+ * Based on code by: Richard Purdie [EMAIL PROTECTED]
+ *
+ *
+ * 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
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 [EMAIL PROTECTED]
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 000..8caca35
--- /dev/null
+++ b/drivers/leds/leds-input.c
@@ -0,0 +1,153 @@
+/*
+ * LED - input subsystem glue
+ *
+ * Copyright 2007 Pavel Machek [EMAIL PROTECTED]
+ * Copyright 2007 Dmitry Torokhov
+ * Copyright 2005-2006 Openedhand Ltd.
+ *
+ * Author: Pavel Machek [EMAIL PROTECTED]
+ * Based on code by: Richard Purdie [EMAIL PROTECTED]
+ *
+ *
+ * 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);
+
Re: blink driver power saving
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 000..2805de8
--- /dev/null
+++ b/drivers/leds/leds-input.c
@@ -0,0 +1,151 @@
+/*
+ * LED - input subsystem glue
+ *
+ * Copyright 2007 Pavel Machek [EMAIL PROTECTED]
+ * Copyright 2007 Dmitry Torokhov
+ * Copyright 2005-2006 Openedhand Ltd.
+ *
+ * Author: Pavel Machek [EMAIL PROTECTED]
+ * Based on code by: Richard Purdie [EMAIL PROTECTED]
+ *
+ *
+ * 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);
+}
+
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
* Pavel Machek <[EMAIL PROTECTED]> [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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
* Pavel Machek [EMAIL PROTECTED] [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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 \ --initrd= --args-linux \ --append="root= " 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
* Dmitry Torokhov <[EMAIL PROTECTED]> [2007-07-02 15:43]: > On 7/2/07, Dmitry Torokhov <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
On 7/2/07, Stephen Hemminger <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
On Mon, 2 Jul 2007 01:59:50 +0200 Andi Kleen <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
> > 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
On 7/2/07, Dmitry Torokhov <[EMAIL PROTECTED]> 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
/*
* 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
#include
#include
#include
#include
MODULE_AUTHOR("Dmitry Torokhov <[EMAIL PROTECTED]>");
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(>handle, EV_LED, LED_NUML, blinker->state);
schedule_delayed_work(>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(>work, blink_task_handler);
handle = >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(>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(>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(_handler);
}
static void __exit blink_handler_exit(void)
{
input_unregister_handler(_handler);
flush_scheduled_work();
}
module_init(blink_handler_init);
module_exit(blink_handler_exit);
Re: blink driver power saving
On 7/2/07, Andi Kleen <[EMAIL PROTECTED]> 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
/*
* 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
#include
#include
#include
#include
MODULE_AUTHOR("Dmitry Torokhov <[EMAIL PROTECTED]>");
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(>handle, EV_LED, LED_NUML, blinker->state);
schedule_delayed_work(>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(>work, blink_task_handler);
handle = >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(>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(>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
Re: blink driver power saving
Andi Kleen <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED] [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
* Andi Kleen <[EMAIL PROTECTED]> [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
---
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
> > 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
On 7/2/07, Andi Kleen <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
On 7/2/07, Andi Kleen [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
* Andi Kleen [EMAIL PROTECTED] [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
---
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
Andi Kleen [EMAIL PROTECTED] 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: [EMAIL PROTECTED] [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
On 7/2/07, Andi Kleen [EMAIL PROTECTED] 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
/*
* 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 [EMAIL PROTECTED]);
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,
Re: blink driver power saving
On 7/2/07, Dmitry Torokhov [EMAIL PROTECTED] 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
/*
* 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 [EMAIL PROTECTED]);
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
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
On Mon, 2 Jul 2007 01:59:50 +0200 Andi Kleen [EMAIL PROTECTED] 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
On 7/2/07, Stephen Hemminger [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
* Dmitry Torokhov [EMAIL PROTECTED] [2007-07-02 15:43]: On 7/2/07, Dmitry Torokhov [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
blink driver power saving
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 PROTECTED]>
--- a/drivers/misc/blink.c 2007-06-29 22:56:20.0 -0400
+++ b/drivers/misc/blink.c 2007-07-01 12:44:37.0 -0400
@@ -16,12 +16,30 @@ static void do_blink(unsigned long data)
add_timer(_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(_notifier_list, _notify);
+ return 0;
+}
+
+static __exit void blink_remove(void)
+{
+ del_timer_sync(_timer);
+ atomic_notifier_chain_unregister(_notifier_list, _notify);
+}
+
module_init(blink_init);
+module_exit(blink_remove);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
blink driver power saving
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 PROTECTED]
--- a/drivers/misc/blink.c 2007-06-29 22:56:20.0 -0400
+++ b/drivers/misc/blink.c 2007-07-01 12:44:37.0 -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);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: blink driver power saving
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
