blink: Only blink when parameter is set

PATCH In review Bernhard Walle 6 participants 11 messages Jun 17, 2007 – Jun 22, 2007 Char and misc drivers

Share & tools

Bernhard Walle

author

2007-06-17 08:39 UTC #0 lore raw

PATCH 1/1 blink: Only blink when parameter is set

This patch in the blink driver changes the module to only blink when the parameter 'blink' is set to true. This is to allow the module to be compiled in the kernel and not as module.

As the blink module was initially written for kdump, and as the kernel is relocatable on lots of architectures, there's no need to compile a separate kdump kernel. The blinking can now enabled via the boot command line for the kdump kernel when necessary.

The patch also adds some author/license information and marks the init function as '__init'.

Signed-off-by Bernhard Walle <[email protected]>

---
 drivers/misc/blink.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/misc/blink.c +12 -1

@@ -3,6 +3,10 @@
 #include <linux/timer.h>
 #include <linux/jiffies.h>
 
+static int blink = 0;
+module_param(blink, bool, S_IRUGO);
+MODULE_PARM_DESC(blink, "Enable blinking (without that, the module does nothing)\n");
+
 static void do_blink(unsigned long data);
 
 static DEFINE_TIMER(blink_timer, do_blink, 0 ,0);

@@ -19,9 +23,16 @@ static void do_blink(unsigned long data)
 static int blink_init(void)
 {
 	printk(KERN_INFO "Enabling keyboard blinking\n");
-	do_blink(0);
+
+	if (blink)
+		do_blink(0);
+
 	return 0;
 }
 
 module_init(blink_init);
 
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Andi Kleen <[email protected]>");
+MODULE_DESCRIPTION("Blinks the keyboard LEDs");
+

Arjan van de Ven

2007-06-17 16:14 UTC #1 lore raw

Quoted 3 lines · Bernhard Walle

This patch in the blink driver changes the module to only blink when the parameter 'blink' is set to true. This is to allow the module to be compiled in the kernel and not as module.

it also has a 1000Hz timer in it... which sucks power at a high rate.. Any chance of making the timer more benign?

Randy Dunlap

2007-06-18 04:25 UTC #2 lore raw

In reply to Bernhard Walle

Quoted 21 lines · On Sun, 17 Jun 2007 10:39:04 +0200 Bernhard Walle wrote:

This patch in the blink driver changes the module to only blink when the parameter 'blink' is set to true. This is to allow the module to be compiled in the kernel and not as module.
As the blink module was initially written for kdump, and as the kernel is relocatable on lots of architectures, there's no need to compile a separate kdump kernel. The blinking can now enabled via the boot command line for the kdump kernel when necessary.
The patch also adds some author/license information and marks the init function as '__init'.
Signed-off-by Bernhard Walle <[email protected]>
---
 drivers/misc/blink.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/misc/blink.c line 6

Quoted 4 lines

drivers/misc/blink.c +1 -0

@@ -3,6 +3,10 @@
 #include <linux/timer.h>
 #include <linux/jiffies.h>
 
+static int blink = 0;

no need to init to 0.

drivers/misc/blink.c line 8

Quoted 2 lines

drivers/misc/blink.c +2 -0

+module_param(blink, bool, S_IRUGO);
+MODULE_PARM_DESC(blink, "Enable blinking (without that, the module does nothing)\n");

unneeded "\n"

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Bernhard Walle

author

2007-06-18 07:18 UTC #3 lore raw

Quoted 3 lines · Randy Dunlap

Quoted 1 line
+static int blink = 0;
no need to init to 0.

Does it harm?

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+module_param(blink, bool, S_IRUGO);
+MODULE_PARM_DESC(blink, "Enable blinking (without that, the module does nothing)\n");

unneeded "\n"

Fixed. Please use the following patch.

-----

This patch in the blink driver changes the module to only blink when the parameter 'blink' is set to true. This is to allow the module to be compiled in the kernel and not as module.

The patch also adds some author/license information and marks the init function as '__init'.

Signed-off-by Bernhard Walle <[email protected]>

---
 drivers/misc/blink.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

drivers/misc/blink.c +13 -2

@@ -3,6 +3,10 @@
 #include <linux/timer.h>
 #include <linux/jiffies.h>
 
+static int blink;
+module_param(blink, bool, S_IRUGO);
+MODULE_PARM_DESC(blink, "Enable blinking (without that, the module does nothing)");
+
 static void do_blink(unsigned long data);
 
 static DEFINE_TIMER(blink_timer, do_blink, 0 ,0);

@@ -16,12 +20,19 @@ static void do_blink(unsigned long data)
 	add_timer(&blink_timer);
 }
 
-static int blink_init(void)
+static int __init blink_init(void)
 {
 	printk(KERN_INFO "Enabling keyboard blinking\n");
-	do_blink(0);
+
+	if (blink)
+		do_blink(0);
+
 	return 0;
 }
 
 module_init(blink_init);
 
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Andi Kleen <[email protected]>");
+MODULE_DESCRIPTION("Blinks the keyboard LEDs");
+

Randy Dunlap

2007-06-18 14:43 UTC #4 lore raw

Quoted 5 lines · Bernhard Walle

Quoted 2 lines · Randy Dunlap
Quoted 1 line
+static int blink = 0;
no need to init to 0.
Does it harm?

It adds space to the binary file in some cases and it is kernel convention not to init statics to NULL or 0 since that is already guaranteed for them.

Quoted 5 lines

Quoted 3 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+module_param(blink, bool, S_IRUGO);
+MODULE_PARM_DESC(blink, "Enable blinking (without that, the module does nothing)\n");
unneeded "\n"
Fixed. Please use the following patch.

Signature · 2 lines

~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Pavel Machek

2007-06-20 20:32 UTC #5 lore raw

In reply to Bernhard Walle

Hi!

Quoted 9 lines

This patch in the blink driver changes the module to only blink when the parameter 'blink' is set to true. This is to allow the module to be compiled in the kernel and not as module.
As the blink module was initially written for kdump, and as the kernel is relocatable on lots of architectures, there's no need to compile a separate kdump kernel. The blinking can now enabled via the boot command line for the kdump kernel when necessary.

Quoted 4 lines

The patch also adds some author/license information and marks the init function as '__init'.
Signed-off-by Bernhard Walle <[email protected]>

Patch looks good (and needed!) to me, but:

can we remove that driver, instead?

* It breaks keyboards. Yes, we are
talking about maybe-broken i8042s, but it still breaks thinkpads at least.

* It can be done in userspace. setleds +num; sleep 1; setleds -num < /dev/tty1 does not seem like rocket science to me.

* if we want to do this, perhaps we should use proper led interface (/sys/class/led) that can already auto-blink

Pavel

Signature · 2 lines

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

Jiri Kosina

2007-06-20 23:52 UTC #6 lore raw

Quoted 2 lines · Pavel Machek

* It breaks keyboards. Yes, we are talking about maybe-broken i8042s, but it still breaks thinkpads at least.

Hi Pavel,

this has probably been already solved by proper throttling - see
http://lkml.org/lkml/2007/6/15/22

Signature · 1 line

Jiri Kosina

Pavel Machek

2007-06-21 00:20 UTC #7 lore raw

Hi!

Quoted 7 lines

Quoted 2 lines
* It breaks keyboards. Yes, we are talking about maybe-broken i8042s, but it still breaks thinkpads at least.
Hi Pavel,
this has probably been already solved by proper throttling - see
http://lkml.org/lkml/2007/6/15/22

No, it was not. I still saw the problems with CONFIG_BLINK on, that is one blink per 5 seconds or something.

We should rename CONFIG_BLINK to
CONFIG_BREAK_THINKPAD_KEYBOARD_AND_MORE.

Pavel

Signature · 2 lines

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

Jiri Kosina

2007-06-21 00:25 UTC #8 lore raw

Quoted 6 lines · Pavel Machek

Quoted 2 lines
this has probably been already solved by proper throttling - see
http://lkml.org/lkml/2007/6/15/22
No, it was not. I still saw the problems with CONFIG_BLINK on, that is one blink per 5 seconds or something.
We should rename CONFIG_BLINK to CONFIG_BREAK_THINKPAD_KEYBOARD_AND_MORE.

In fact it looks quite weird that one blink per 5 seconds can break the keyboard, in fact.

Seems like Dmitry is missing in CC, added.

Thanks,

Signature · 1 line

Jiri Kosina

Dmitry Torokhov

2007-06-21 02:57 UTC #9 lore raw

Quoted 6 lines · Jiri Kosina

Quoted 4 lines · Pavel Machek
Quoted 2 lines
this has probably been already solved by proper throttling - see
http://lkml.org/lkml/2007/6/15/22
No, it was not. I still saw the problems with CONFIG_BLINK on, that is one blink per 5 seconds or something.

Pavel, does my patch fixes keyboard issues for you when blinking via setleds?

Quoted 2 lines

Quoted 2 lines
We should rename CONFIG_BLINK to
CONFIG_BREAK_THINKPAD_KEYBOARD_AND_MORE.

Also CONFIG_KILL_MY_NOHZ_SAVINGS ;)

Quoted 3 lines

In fact it looks quite weird that one blink per 5 seconds can break the keyboard, in fact.

Not wierd at all. 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.

IOW I am not really interested in reports regrading issues with keyboards and/or mice on boxes with blink driver loaded.

Signature · 1 line

Dmitry

Pavel Machek

2007-06-22 20:45 UTC #10 lore raw

Hi!

Quoted 5 lines

Quoted 2 lines
In fact it looks quite weird that one blink per 5 seconds can break the keyboard, in fact.
Not wierd at all. The driver uses panic_blink - something that we expect to work after panic. It rapidly polls KBC status register to detect when

Aha. Can we get rid of that driver? It is so broken it is not funny.

Signature · 2 lines

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