[PATCH] blink: Only blink when parameter is set

[PATCH] blink: Only blink when parameter is set

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

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 <bwalle@suse.de>

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

--- a/drivers/misc/blink.c
+++ b/drivers/misc/blink.c
@@ -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 <ak@suse.de>");
+MODULE_DESCRIPTION("Blinks the keyboard LEDs");
+

Re: [PATCH] blink: Only blink when parameter is set

[Arjan van de Ven]

On Sun, 2007-06-17 at 10:39 +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.


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

Re: [PATCH] blink: Only blink when parameter is set

[Randy Dunlap]

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 <bwalle@suse.de>
> 
> ---
>  drivers/misc/blink.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> --- a/drivers/misc/blink.c
> +++ b/drivers/misc/blink.c
> @@ -3,6 +3,10 @@
>  #include <linux/timer.h>
>  #include <linux/jiffies.h>
>  
> +static int blink = 0;

no need to init to 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 ***

Re: [PATCH] blink: Only blink when parameter is set

[Bernhard Walle]

* Randy Dunlap <randy.dunlap@oracle.com> [2007-06-18 06:26]:
> > +static int blink = 0;
> 
> no need to init to 0.

Does it harm?

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

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 <bwalle@suse.de>

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

--- a/drivers/misc/blink.c
+++ b/drivers/misc/blink.c
@@ -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 <ak@suse.de>");
+MODULE_DESCRIPTION("Blinks the keyboard LEDs");
+

Re: [PATCH] blink: Only blink when parameter is set

[Randy Dunlap]

Bernhard Walle wrote:
> * Randy Dunlap <randy.dunlap@oracle.com> [2007-06-18 06:26]:
>>> +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.

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


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

Re: [PATCH] blink: Only blink when parameter is set

[Pavel Machek]

Hi!

> 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 <bwalle@suse.de>

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] blink: Only blink when parameter is set

[Jiri Kosina]

On Wed, 20 Jun 2007, Pavel Machek wrote:

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

-- 
Jiri Kosina

Re: [PATCH] blink: Only blink when parameter is set

[Pavel Machek]

Hi!

> > * 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] blink: Only blink when parameter is set

[Jiri Kosina]

On Thu, 21 Jun 2007, Pavel Machek wrote:

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

-- 
Jiri Kosina

Re: [PATCH] blink: Only blink when parameter is set

[Dmitry Torokhov]

On Wednesday 20 June 2007 20:24, Jiri Kosina wrote:
> On Thu, 21 Jun 2007, Pavel Machek wrote:
> 
> > > 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?

> > We should rename CONFIG_BLINK to
> > CONFIG_BREAK_THINKPAD_KEYBOARD_AND_MORE.

Also CONFIG_KILL_MY_NOHZ_SAVINGS ;)

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

-- 
Dmitry

Re: [PATCH] blink: Only blink when parameter is set

[Pavel Machek]

Hi!

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

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