[PATCH] leds: heartbeat: stop on shutdown

[PATCH] leds: heartbeat: stop on shutdown

[Alexander Holler]

A halted kernel should not show a heartbeat.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/leds/ledtrig-heartbeat.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
index 759c0bb..76189b0 100644
--- a/drivers/leds/ledtrig-heartbeat.c
+++ b/drivers/leds/ledtrig-heartbeat.c
@@ -18,6 +18,7 @@
 #include <linux/timer.h>
 #include <linux/sched.h>
 #include <linux/leds.h>
+#include <linux/reboot.h>
 #include "leds.h"
 
 struct heartbeat_trig_data {
@@ -101,13 +102,26 @@ static struct led_trigger heartbeat_led_trigger = {
 	.deactivate = heartbeat_trig_deactivate,
 };
 
+static int heartbeat_reboot_notifier(struct notifier_block *nb,
+				     unsigned long code, void *unused)
+{
+	led_trigger_unregister(&heartbeat_led_trigger);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block heartbeat_reboot_nb = {
+	.notifier_call = heartbeat_reboot_notifier,
+};
+
 static int __init heartbeat_trig_init(void)
 {
+	register_reboot_notifier(&heartbeat_reboot_nb);
 	return led_trigger_register(&heartbeat_led_trigger);
 }
 
 static void __exit heartbeat_trig_exit(void)
 {
+	unregister_reboot_notifier(&heartbeat_reboot_nb);
 	led_trigger_unregister(&heartbeat_led_trigger);
 }
 
-- 
1.7.6.5

[PATCH v2] leds: heartbeat: stop on shutdown

[Alexander Holler]

A halted kernel should not show a heartbeat.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/leds/ledtrig-heartbeat.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
index 759c0bb..1c05bd9 100644
--- a/drivers/leds/ledtrig-heartbeat.c
+++ b/drivers/leds/ledtrig-heartbeat.c
@@ -18,6 +18,7 @@
 #include <linux/timer.h>
 #include <linux/sched.h>
 #include <linux/leds.h>
+#include <linux/reboot.h>
 #include "leds.h"
 
 struct heartbeat_trig_data {
@@ -101,13 +102,28 @@ static struct led_trigger heartbeat_led_trigger = {
 	.deactivate = heartbeat_trig_deactivate,
 };
 
+static int heartbeat_reboot_notifier(struct notifier_block *nb,
+				     unsigned long code, void *unused)
+{
+	led_trigger_unregister(&heartbeat_led_trigger);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block heartbeat_reboot_nb = {
+	.notifier_call = heartbeat_reboot_notifier,
+};
+
 static int __init heartbeat_trig_init(void)
 {
-	return led_trigger_register(&heartbeat_led_trigger);
+	int rc = led_trigger_register(&heartbeat_led_trigger);
+	if( ! rc )
+		register_reboot_notifier(&heartbeat_reboot_nb);
+	return rc;
 }
 
 static void __exit heartbeat_trig_exit(void)
 {
+	unregister_reboot_notifier(&heartbeat_reboot_nb);
 	led_trigger_unregister(&heartbeat_led_trigger);
 }
 
-- 
1.7.6.5

Re: [PATCH v2] leds: heartbeat: stop on shutdown

[Shuah Khan]

On Tue, 2012-04-24 at 17:07 +0200, Alexander Holler wrote:
> A halted kernel should not show a heartbeat.
> 
> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
> ---
>  drivers/leds/ledtrig-heartbeat.c |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
> index 759c0bb..1c05bd9 100644
> --- a/drivers/leds/ledtrig-heartbeat.c
> +++ b/drivers/leds/ledtrig-heartbeat.c
> @@ -18,6 +18,7 @@
>  #include <linux/timer.h>
>  #include <linux/sched.h>
>  #include <linux/leds.h>
> +#include <linux/reboot.h>
>  #include "leds.h"
>  
>  struct heartbeat_trig_data {
> @@ -101,13 +102,28 @@ static struct led_trigger heartbeat_led_trigger = {
>  	.deactivate = heartbeat_trig_deactivate,
>  };
>  
> +static int heartbeat_reboot_notifier(struct notifier_block *nb,
> +				     unsigned long code, void *unused)
> +{
> +	led_trigger_unregister(&heartbeat_led_trigger);
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block heartbeat_reboot_nb = {
> +	.notifier_call = heartbeat_reboot_notifier,
> +};
> +
>  static int __init heartbeat_trig_init(void)
>  {
> -	return led_trigger_register(&heartbeat_led_trigger);
> +	int rc = led_trigger_register(&heartbeat_led_trigger);
> +	if( ! rc )
> +		register_reboot_notifier(&heartbeat_reboot_nb);

Do you need to check return value here? I see return value being checked
in some places in the kernel and not in others. Not checking is fine for
now since register_reboot_notifier() always returns 0. 

Maybe adding return handling might not be a bad idea to be consistent
with other places that do check.

FYI - Andrew Morton is handling led patches these days. You might want
to send include him  <akpm@linux-foundation.org>

> +	return rc;
>  }
>  
>  static void __exit heartbeat_trig_exit(void)
>  {
> +	unregister_reboot_notifier(&heartbeat_reboot_nb);
>  	led_trigger_unregister(&heartbeat_led_trigger);
>  }
>  

Re: [PATCH v2] leds: heartbeat: stop on shutdown

[Alexander Holler]

Am 24.04.2012 17:24, schrieb Shuah Khan:
> On Tue, 2012-04-24 at 17:07 +0200, Alexander Holler wrote:
>> A halted kernel should not show a heartbeat.
>>
>> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
>> ---
>>  drivers/leds/ledtrig-heartbeat.c |   18 +++++++++++++++++-
>>  1 files changed, 17 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
>> index 759c0bb..1c05bd9 100644
>> --- a/drivers/leds/ledtrig-heartbeat.c
>> +++ b/drivers/leds/ledtrig-heartbeat.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/timer.h>
>>  #include <linux/sched.h>
>>  #include <linux/leds.h>
>> +#include <linux/reboot.h>
>>  #include "leds.h"
>>  
>>  struct heartbeat_trig_data {
>> @@ -101,13 +102,28 @@ static struct led_trigger heartbeat_led_trigger = {
>>  	.deactivate = heartbeat_trig_deactivate,
>>  };
>>  
>> +static int heartbeat_reboot_notifier(struct notifier_block *nb,
>> +				     unsigned long code, void *unused)
>> +{
>> +	led_trigger_unregister(&heartbeat_led_trigger);
>> +	return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block heartbeat_reboot_nb = {
>> +	.notifier_call = heartbeat_reboot_notifier,
>> +};
>> +
>>  static int __init heartbeat_trig_init(void)
>>  {
>> -	return led_trigger_register(&heartbeat_led_trigger);
>> +	int rc = led_trigger_register(&heartbeat_led_trigger);
>> +	if( ! rc )
>> +		register_reboot_notifier(&heartbeat_reboot_nb);
> 
> Do you need to check return value here? I see return value being checked
> in some places in the kernel and not in others. Not checking is fine for
> now since register_reboot_notifier() always returns 0.
> 
> Maybe adding return handling might not be a bad idea to be consistent
> with other places that do check.

What to do if it fails? As long as unregister_reboot_notifier() doesn't
do something bad with non-registered notifiers (which it doesn't), I
would let the init still succeed. And to not unregister a
non-registered-reboot_notifier, a long-living bool would be needed.

> FYI - Andrew Morton is handling led patches these days. You might want
> to send include him  <akpm@linux-foundation.org>

I have added him to CC, thanks.

Regards,

Alexander

Re: [PATCH v2] leds: heartbeat: stop on shutdown

[Shuah Khan]

On Tue, 2012-04-24 at 18:32 +0200, Alexander Holler wrote:
> Am 24.04.2012 17:24, schrieb Shuah Khan:
> > On Tue, 2012-04-24 at 17:07 +0200, Alexander Holler wrote:
> >> A halted kernel should not show a heartbeat.
> >>
> >> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
> >> ---
> >>  drivers/leds/ledtrig-heartbeat.c |   18 +++++++++++++++++-
> >>  1 files changed, 17 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
> >> index 759c0bb..1c05bd9 100644
> >> --- a/drivers/leds/ledtrig-heartbeat.c
> >> +++ b/drivers/leds/ledtrig-heartbeat.c
> >> @@ -18,6 +18,7 @@
> >>  #include <linux/timer.h>
> >>  #include <linux/sched.h>
> >>  #include <linux/leds.h>
> >> +#include <linux/reboot.h>
> >>  #include "leds.h"
> >>  
> >>  struct heartbeat_trig_data {
> >> @@ -101,13 +102,28 @@ static struct led_trigger heartbeat_led_trigger = {
> >>  	.deactivate = heartbeat_trig_deactivate,
> >>  };
> >>  
> >> +static int heartbeat_reboot_notifier(struct notifier_block *nb,
> >> +				     unsigned long code, void *unused)
> >> +{
> >> +	led_trigger_unregister(&heartbeat_led_trigger);
> >> +	return NOTIFY_DONE;
> >> +}
I am sorry I didn't catch this the last time. Do you need to call
unregister here? Looks like trigger is going away with a registered
reboot notifier. Might be fine, since reboot is going to happen.

> >> +
> >> +static struct notifier_block heartbeat_reboot_nb = {
> >> +	.notifier_call = heartbeat_reboot_notifier,
> >> +};
> >> +
> >>  static int __init heartbeat_trig_init(void)
> >>  {
> >> -	return led_trigger_register(&heartbeat_led_trigger);
> >> +	int rc = led_trigger_register(&heartbeat_led_trigger);
> >> +	if( ! rc )
> >> +		register_reboot_notifier(&heartbeat_reboot_nb);
> > 
> > Do you need to check return value here? I see return value being checked
> > in some places in the kernel and not in others. Not checking is fine for
> > now since register_reboot_notifier() always returns 0.
> > 
> > Maybe adding return handling might not be a bad idea to be consistent
> > with other places that do check.
> 
> What to do if it fails? As long as unregister_reboot_notifier() doesn't
> do something bad with non-registered notifiers (which it doesn't), I
> would let the init still succeed. 

Yes I agree that init should still succeed. It might be helpful to throw
out a kernel message when this occurs though since register shouldn't
fail unless there is an error.

> And to not unregister a
> non-registered-reboot_notifier, a long-living bool would be needed.

Yes keeping state is probably required to unregister conditionally.
Since unregistering is safe (simply returns -ENOENT) when called after
register fails. So I would think you can call unregister even when
register fails. 

-- Shuah

Re: [PATCH v2] leds: heartbeat: stop on shutdown

[Alexander Holler]

Am 24.04.2012 19:00, schrieb Shuah Khan:
> On Tue, 2012-04-24 at 18:32 +0200, Alexander Holler wrote:
>> Am 24.04.2012 17:24, schrieb Shuah Khan:
>>> On Tue, 2012-04-24 at 17:07 +0200, Alexander Holler wrote:
>>>> A halted kernel should not show a heartbeat.
>>>>
>>>> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
>>>> ---
>>>>  drivers/leds/ledtrig-heartbeat.c |   18 +++++++++++++++++-
>>>>  1 files changed, 17 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
>>>> index 759c0bb..1c05bd9 100644
>>>> --- a/drivers/leds/ledtrig-heartbeat.c
>>>> +++ b/drivers/leds/ledtrig-heartbeat.c
>>>> @@ -18,6 +18,7 @@
>>>>  #include <linux/timer.h>
>>>>  #include <linux/sched.h>
>>>>  #include <linux/leds.h>
>>>> +#include <linux/reboot.h>
>>>>  #include "leds.h"
>>>>  
>>>>  struct heartbeat_trig_data {
>>>> @@ -101,13 +102,28 @@ static struct led_trigger heartbeat_led_trigger = {
>>>>  	.deactivate = heartbeat_trig_deactivate,
>>>>  };
>>>>  
>>>> +static int heartbeat_reboot_notifier(struct notifier_block *nb,
>>>> +				     unsigned long code, void *unused)
>>>> +{
>>>> +	led_trigger_unregister(&heartbeat_led_trigger);
>>>> +	return NOTIFY_DONE;
>>>> +}
> I am sorry I didn't catch this the last time. Do you need to call
> unregister here? Looks like trigger is going away with a registered
> reboot notifier. Might be fine, since reboot is going to happen.

That unregister is the whole reason of the patch because it disables all
running heartbeats on reboot/shutdown. ;)

I still wonder why the timers are running after a kernel halted (so the
heardbeat was still alive), but there might be a good reason not just to
disable all running timers unconditionally on shutdown.

Regards,

Alexander

Re: [PATCH v2] leds: heartbeat: stop on shutdown

[Shuah Khan]

On Tue, 2012-04-24 at 19:27 +0200, Alexander Holler wrote:
> Am 24.04.2012 19:00, schrieb Shuah Khan:
> > On Tue, 2012-04-24 at 18:32 +0200, Alexander Holler wrote:
> >> Am 24.04.2012 17:24, schrieb Shuah Khan:
> >>> On Tue, 2012-04-24 at 17:07 +0200, Alexander Holler wrote:
> >>>> A halted kernel should not show a heartbeat.
> >>>>
> >>>> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
> >>>> ---
> >>>>  drivers/leds/ledtrig-heartbeat.c |   18 +++++++++++++++++-
> >>>>  1 files changed, 17 insertions(+), 1 deletions(-)
> >>>>
> >>>> diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
> >>>> index 759c0bb..1c05bd9 100644
> >>>> --- a/drivers/leds/ledtrig-heartbeat.c
> >>>> +++ b/drivers/leds/ledtrig-heartbeat.c
> >>>> @@ -18,6 +18,7 @@
> >>>>  #include <linux/timer.h>
> >>>>  #include <linux/sched.h>
> >>>>  #include <linux/leds.h>
> >>>> +#include <linux/reboot.h>
> >>>>  #include "leds.h"
> >>>>  
> >>>>  struct heartbeat_trig_data {
> >>>> @@ -101,13 +102,28 @@ static struct led_trigger heartbeat_led_trigger = {
> >>>>  	.deactivate = heartbeat_trig_deactivate,
> >>>>  };
> >>>>  
> >>>> +static int heartbeat_reboot_notifier(struct notifier_block *nb,
> >>>> +				     unsigned long code, void *unused)
> >>>> +{
> >>>> +	led_trigger_unregister(&heartbeat_led_trigger);
> >>>> +	return NOTIFY_DONE;
> >>>> +}
> > I am sorry I didn't catch this the last time. Do you need to call
> > unregister here? Looks like trigger is going away with a registered
> > reboot notifier. Might be fine, since reboot is going to happen.
> 
> That unregister is the whole reason of the patch because it disables all
> running heartbeats on reboot/shutdown. ;)

Sorry I should have been clear about which unregister. I meant
unregister_reboot_notifier()
> 
> I still wonder why the timers are running after a kernel halted (so the
> heardbeat was still alive), but there might be a good reason not just to
> disable all running timers unconditionally on shutdown.
> 
> Regards,
> 
> Alexander
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH v2] leds: heartbeat: stop on shutdown

[Alexander Holler]

Am 24.04.2012 19:31, schrieb Shuah Khan:
> On Tue, 2012-04-24 at 19:27 +0200, Alexander Holler wrote:
>> Am 24.04.2012 19:00, schrieb Shuah Khan:
>>> On Tue, 2012-04-24 at 18:32 +0200, Alexander Holler wrote:
>>>> Am 24.04.2012 17:24, schrieb Shuah Khan:
>>>>> On Tue, 2012-04-24 at 17:07 +0200, Alexander Holler wrote:
>>>>>> A halted kernel should not show a heartbeat.
>>>>>>
>>>>>> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
>>>>>> ---
>>>>>>  drivers/leds/ledtrig-heartbeat.c |   18 +++++++++++++++++-
>>>>>>  1 files changed, 17 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
>>>>>> index 759c0bb..1c05bd9 100644
>>>>>> --- a/drivers/leds/ledtrig-heartbeat.c
>>>>>> +++ b/drivers/leds/ledtrig-heartbeat.c
>>>>>> @@ -18,6 +18,7 @@
>>>>>>  #include <linux/timer.h>
>>>>>>  #include <linux/sched.h>
>>>>>>  #include <linux/leds.h>
>>>>>> +#include <linux/reboot.h>
>>>>>>  #include "leds.h"
>>>>>>  
>>>>>>  struct heartbeat_trig_data {
>>>>>> @@ -101,13 +102,28 @@ static struct led_trigger heartbeat_led_trigger = {
>>>>>>  	.deactivate = heartbeat_trig_deactivate,
>>>>>>  };
>>>>>>  
>>>>>> +static int heartbeat_reboot_notifier(struct notifier_block *nb,
>>>>>> +				     unsigned long code, void *unused)
>>>>>> +{
>>>>>> +	led_trigger_unregister(&heartbeat_led_trigger);
>>>>>> +	return NOTIFY_DONE;
>>>>>> +}
>>> I am sorry I didn't catch this the last time. Do you need to call
>>> unregister here? Looks like trigger is going away with a registered
>>> reboot notifier. Might be fine, since reboot is going to happen.
>>
>> That unregister is the whole reason of the patch because it disables all
>> running heartbeats on reboot/shutdown. ;)
> 
> Sorry I should have been clear about which unregister. I meant
> unregister_reboot_notifier()

That is needed in case the module will be unloaded.

Regards,

Alexander

[PATCH v3] leds: heartbeat: stop on shutdown, reboot or panic

[Alexander Holler]

A halted kernel should not show a heartbeat.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/leds/ledtrig-heartbeat.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
index 759c0bb..8cfa7df 100644
--- a/drivers/leds/ledtrig-heartbeat.c
+++ b/drivers/leds/ledtrig-heartbeat.c
@@ -18,6 +18,7 @@
 #include <linux/timer.h>
 #include <linux/sched.h>
 #include <linux/leds.h>
+#include <linux/reboot.h>
 #include "leds.h"
 
 struct heartbeat_trig_data {
@@ -101,13 +102,37 @@ static struct led_trigger heartbeat_led_trigger = {
 	.deactivate = heartbeat_trig_deactivate,
 };
 
+static int heartbeat_reboot_notifier(struct notifier_block *nb,
+				     unsigned long code, void *unused)
+{
+	led_trigger_unregister(&heartbeat_led_trigger);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block heartbeat_reboot_nb = {
+	.notifier_call = heartbeat_reboot_notifier,
+};
+
+static struct notifier_block heartbeat_panic_nb = {
+	.notifier_call = heartbeat_reboot_notifier,
+};
+
 static int __init heartbeat_trig_init(void)
 {
-	return led_trigger_register(&heartbeat_led_trigger);
+	int rc = led_trigger_register(&heartbeat_led_trigger);
+	if( ! rc ) {
+		atomic_notifier_chain_register(&panic_notifier_list,
+					       &heartbeat_panic_nb);
+		register_reboot_notifier(&heartbeat_reboot_nb);
+	}
+	return rc;
 }
 
 static void __exit heartbeat_trig_exit(void)
 {
+	unregister_reboot_notifier(&heartbeat_reboot_nb);
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &heartbeat_panic_nb);
 	led_trigger_unregister(&heartbeat_led_trigger);
 }
 
-- 
1.7.6.5

[PATCH v4] leds: heartbeat: stop on shutdown, reboot or panic

[Alexander Holler]

A halted kernel should not show a heartbeat.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/leds/ledtrig-heartbeat.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c
index 759c0bb..31c234c 100644
--- a/drivers/leds/ledtrig-heartbeat.c
+++ b/drivers/leds/ledtrig-heartbeat.c
@@ -18,6 +18,7 @@
 #include <linux/timer.h>
 #include <linux/sched.h>
 #include <linux/leds.h>
+#include <linux/reboot.h>
 #include "leds.h"
 
 struct heartbeat_trig_data {
@@ -101,13 +102,37 @@ static struct led_trigger heartbeat_led_trigger = {
 	.deactivate = heartbeat_trig_deactivate,
 };
 
+static int heartbeat_reboot_notifier(struct notifier_block *nb,
+				     unsigned long code, void *unused)
+{
+	led_trigger_unregister(&heartbeat_led_trigger);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block heartbeat_reboot_nb = {
+	.notifier_call = heartbeat_reboot_notifier,
+};
+
+static struct notifier_block heartbeat_panic_nb = {
+	.notifier_call = heartbeat_reboot_notifier,
+};
+
 static int __init heartbeat_trig_init(void)
 {
-	return led_trigger_register(&heartbeat_led_trigger);
+	int rc = led_trigger_register(&heartbeat_led_trigger);
+	if (!rc) {
+		atomic_notifier_chain_register(&panic_notifier_list,
+					       &heartbeat_panic_nb);
+		register_reboot_notifier(&heartbeat_reboot_nb);
+	}
+	return rc;
 }
 
 static void __exit heartbeat_trig_exit(void)
 {
+	unregister_reboot_notifier(&heartbeat_reboot_nb);
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &heartbeat_panic_nb);
 	led_trigger_unregister(&heartbeat_led_trigger);
 }
 
-- 
1.7.6.5