[PATCH] leds: handle suspend/resume in heartbeat trigger

[PATCH] leds: handle suspend/resume in heartbeat trigger

[Linus Walleij]

The following phenomena was observed: when suspending the
system, sometimes the heartbeat LED was left on, glowing and
wasting power while the rest of the system is asleep, also
disturbing power dissapation measures on the odd suspend
cycle when it's left on.

Clearly this is not how we want the heartbeat trigger to
work: it should turn off and leave the LED off during
system suspend.

This removes the heartbeat trigger when preparing suspend and
restores it during resume. The trigger code will make sure all
LEDs are left in OFF state after removing the trigger, and
will re-enable the trigger on all LEDs after resuming.

Cc: linux-pm@vger.kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/leds/trigger/ledtrig-heartbeat.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
index 410c39c62dc7..c80e91152b6b 100644
--- a/drivers/leds/trigger/ledtrig-heartbeat.c
+++ b/drivers/leds/trigger/ledtrig-heartbeat.c
@@ -19,6 +19,7 @@
 #include <linux/sched.h>
 #include <linux/leds.h>
 #include <linux/reboot.h>
+#include <linux/suspend.h>
 #include "../leds.h"
 
 static int panic_heartbeats;
@@ -154,6 +155,26 @@ static struct led_trigger heartbeat_led_trigger = {
 	.deactivate = heartbeat_trig_deactivate,
 };
 
+static int heartbeat_pm_notifier(struct notifier_block *nb,
+				 unsigned long pm_event, void *unused)
+{
+	int rc;
+
+	switch (pm_event) {
+	case PM_SUSPEND_PREPARE:
+		led_trigger_unregister(&heartbeat_led_trigger);
+		break;
+	case PM_POST_SUSPEND:
+		rc = led_trigger_register(&heartbeat_led_trigger);
+		if (rc)
+			pr_err("could not re-register heartbeat trigger\n");
+		break;
+	default:
+		break;
+	}
+	return NOTIFY_DONE;
+}
+
 static int heartbeat_reboot_notifier(struct notifier_block *nb,
 				     unsigned long code, void *unused)
 {
@@ -168,6 +189,10 @@ static int heartbeat_panic_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static struct notifier_block heartbeat_pm_nb = {
+	.notifier_call = heartbeat_pm_notifier,
+};
+
 static struct notifier_block heartbeat_reboot_nb = {
 	.notifier_call = heartbeat_reboot_notifier,
 };
@@ -184,12 +209,14 @@ static int __init heartbeat_trig_init(void)
 		atomic_notifier_chain_register(&panic_notifier_list,
 					       &heartbeat_panic_nb);
 		register_reboot_notifier(&heartbeat_reboot_nb);
+		register_pm_notifier(&heartbeat_pm_nb);
 	}
 	return rc;
 }
 
 static void __exit heartbeat_trig_exit(void)
 {
+	unregister_pm_notifier(&heartbeat_pm_nb);
 	unregister_reboot_notifier(&heartbeat_reboot_nb);
 	atomic_notifier_chain_unregister(&panic_notifier_list,
 					 &heartbeat_panic_nb);
-- 
2.4.11

Re: [PATCH] leds: handle suspend/resume in heartbeat trigger

[Jacek Anaszewski]

Hi Linus,

Thanks for the patch, applied.

Best regards,
Jacek Anaszewski

On 06/02/2016 03:41 PM, Linus Walleij wrote:
> The following phenomena was observed: when suspending the
> system, sometimes the heartbeat LED was left on, glowing and
> wasting power while the rest of the system is asleep, also
> disturbing power dissapation measures on the odd suspend
> cycle when it's left on.
>
> Clearly this is not how we want the heartbeat trigger to
> work: it should turn off and leave the LED off during
> system suspend.
>
> This removes the heartbeat trigger when preparing suspend and
> restores it during resume. The trigger code will make sure all
> LEDs are left in OFF state after removing the trigger, and
> will re-enable the trigger on all LEDs after resuming.
>
> Cc: linux-pm@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>   drivers/leds/trigger/ledtrig-heartbeat.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
> index 410c39c62dc7..c80e91152b6b 100644
> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> @@ -19,6 +19,7 @@
>   #include <linux/sched.h>
>   #include <linux/leds.h>
>   #include <linux/reboot.h>
> +#include <linux/suspend.h>
>   #include "../leds.h"
>
>   static int panic_heartbeats;
> @@ -154,6 +155,26 @@ static struct led_trigger heartbeat_led_trigger = {
>   	.deactivate = heartbeat_trig_deactivate,
>   };
>
> +static int heartbeat_pm_notifier(struct notifier_block *nb,
> +				 unsigned long pm_event, void *unused)
> +{
> +	int rc;
> +
> +	switch (pm_event) {
> +	case PM_SUSPEND_PREPARE:
> +		led_trigger_unregister(&heartbeat_led_trigger);
> +		break;
> +	case PM_POST_SUSPEND:
> +		rc = led_trigger_register(&heartbeat_led_trigger);
> +		if (rc)
> +			pr_err("could not re-register heartbeat trigger\n");
> +		break;
> +	default:
> +		break;
> +	}
> +	return NOTIFY_DONE;
> +}
> +
>   static int heartbeat_reboot_notifier(struct notifier_block *nb,
>   				     unsigned long code, void *unused)
>   {
> @@ -168,6 +189,10 @@ static int heartbeat_panic_notifier(struct notifier_block *nb,
>   	return NOTIFY_DONE;
>   }
>
> +static struct notifier_block heartbeat_pm_nb = {
> +	.notifier_call = heartbeat_pm_notifier,
> +};
> +
>   static struct notifier_block heartbeat_reboot_nb = {
>   	.notifier_call = heartbeat_reboot_notifier,
>   };
> @@ -184,12 +209,14 @@ static int __init heartbeat_trig_init(void)
>   		atomic_notifier_chain_register(&panic_notifier_list,
>   					       &heartbeat_panic_nb);
>   		register_reboot_notifier(&heartbeat_reboot_nb);
> +		register_pm_notifier(&heartbeat_pm_nb);
>   	}
>   	return rc;
>   }
>
>   static void __exit heartbeat_trig_exit(void)
>   {
> +	unregister_pm_notifier(&heartbeat_pm_nb);
>   	unregister_reboot_notifier(&heartbeat_reboot_nb);
>   	atomic_notifier_chain_unregister(&panic_notifier_list,
>   					 &heartbeat_panic_nb);
>

Re: [PATCH] leds: handle suspend/resume in heartbeat trigger

[Ulf Hansson]

On 2 June 2016 at 15:41, Linus Walleij <linus.walleij@linaro.org> wrote:
> The following phenomena was observed: when suspending the
> system, sometimes the heartbeat LED was left on, glowing and
> wasting power while the rest of the system is asleep, also
> disturbing power dissapation measures on the odd suspend
> cycle when it's left on.
>
> Clearly this is not how we want the heartbeat trigger to
> work: it should turn off and leave the LED off during
> system suspend.

Agree!

>
> This removes the heartbeat trigger when preparing suspend and
> restores it during resume. The trigger code will make sure all
> LEDs are left in OFF state after removing the trigger, and
> will re-enable the trigger on all LEDs after resuming.

I believe most other led-trigger types that should also be "suspended"
in the similar fashion as the heartbeat. Perhaps not all, but least
some more (timer, cpu, etc).

I looked at the cpu trigger, which currently registers a syscore_ops
to deal with suspend/resume. That means the trigger being suspended
far later in system PM suspend phase, and I wonder if that is really
necessary!?
Does people really care about the cpu trigger being active that late
in the system PM suspend phase!?

More importantly, I think it could be problematic to allow it that
late in system PM phase, at least for for those ARM SoC I have been
working on which might use an i2c transfer to control the led.

Okay, my point is, perhaps we should do this in a more generic manner
and manage the suspend/resume of triggers in
drivers/leds/led-triggers.c?

If we need the suspend/resume trigger to be optional, we can add that
as a configuration flag in struct led_trigger, to inform the
led-triggers.c about the wanted behaviour. In that way, each
led-trigger type don't have to register their own set of pm_notifiers.

Some core comments below...

>
> Cc: linux-pm@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/leds/trigger/ledtrig-heartbeat.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
> index 410c39c62dc7..c80e91152b6b 100644
> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> @@ -19,6 +19,7 @@
>  #include <linux/sched.h>
>  #include <linux/leds.h>
>  #include <linux/reboot.h>
> +#include <linux/suspend.h>
>  #include "../leds.h"
>
>  static int panic_heartbeats;
> @@ -154,6 +155,26 @@ static struct led_trigger heartbeat_led_trigger = {
>         .deactivate = heartbeat_trig_deactivate,
>  };
>
> +static int heartbeat_pm_notifier(struct notifier_block *nb,
> +                                unsigned long pm_event, void *unused)
> +{
> +       int rc;
> +
> +       switch (pm_event) {
> +       case PM_SUSPEND_PREPARE:

I think you should add:
case PM_HIBERNATION_PREPARE:
case PM_RESTORE_PREPARE:

> +               led_trigger_unregister(&heartbeat_led_trigger);
> +               break;
> +       case PM_POST_SUSPEND:

I think you should add:
case PM_POST_HIBERNATION:
case PM_POST_RESTORE:

> +               rc = led_trigger_register(&heartbeat_led_trigger);
> +               if (rc)
> +                       pr_err("could not re-register heartbeat trigger\n");
> +               break;
> +       default:
> +               break;
> +       }
> +       return NOTIFY_DONE;
> +}
> +
>  static int heartbeat_reboot_notifier(struct notifier_block *nb,
>                                      unsigned long code, void *unused)
>  {
> @@ -168,6 +189,10 @@ static int heartbeat_panic_notifier(struct notifier_block *nb,
>         return NOTIFY_DONE;
>  }
>
> +static struct notifier_block heartbeat_pm_nb = {
> +       .notifier_call = heartbeat_pm_notifier,
> +};
> +
>  static struct notifier_block heartbeat_reboot_nb = {
>         .notifier_call = heartbeat_reboot_notifier,
>  };
> @@ -184,12 +209,14 @@ static int __init heartbeat_trig_init(void)
>                 atomic_notifier_chain_register(&panic_notifier_list,
>                                                &heartbeat_panic_nb);
>                 register_reboot_notifier(&heartbeat_reboot_nb);
> +               register_pm_notifier(&heartbeat_pm_nb);
>         }
>         return rc;
>  }
>
>  static void __exit heartbeat_trig_exit(void)
>  {
> +       unregister_pm_notifier(&heartbeat_pm_nb);
>         unregister_reboot_notifier(&heartbeat_reboot_nb);
>         atomic_notifier_chain_unregister(&panic_notifier_list,
>                                          &heartbeat_panic_nb);
> --
> 2.4.11
>

Besides my upper comments, this looks like a good approach!

Kind regards
Uffe

Re: [PATCH] leds: handle suspend/resume in heartbeat trigger

[Jacek Anaszewski]

On 06/03/2016 11:32 AM, Ulf Hansson wrote:
> On 2 June 2016 at 15:41, Linus Walleij <linus.walleij@linaro.org> wrote:
>> The following phenomena was observed: when suspending the
>> system, sometimes the heartbeat LED was left on, glowing and
>> wasting power while the rest of the system is asleep, also
>> disturbing power dissapation measures on the odd suspend
>> cycle when it's left on.
>>
>> Clearly this is not how we want the heartbeat trigger to
>> work: it should turn off and leave the LED off during
>> system suspend.
>
> Agree!
>
>>
>> This removes the heartbeat trigger when preparing suspend and
>> restores it during resume. The trigger code will make sure all
>> LEDs are left in OFF state after removing the trigger, and
>> will re-enable the trigger on all LEDs after resuming.
>
> I believe most other led-trigger types that should also be "suspended"
> in the similar fashion as the heartbeat. Perhaps not all, but least
> some more (timer, cpu, etc).
>
> I looked at the cpu trigger, which currently registers a syscore_ops
> to deal with suspend/resume. That means the trigger being suspended
> far later in system PM suspend phase, and I wonder if that is really
> necessary!?
> Does people really care about the cpu trigger being active that late
> in the system PM suspend phase!?
>
> More importantly, I think it could be problematic to allow it that
> late in system PM phase, at least for for those ARM SoC I have been
> working on which might use an i2c transfer to control the led.
>
> Okay, my point is, perhaps we should do this in a more generic manner
> and manage the suspend/resume of triggers in
> drivers/leds/led-triggers.c?

We'd have to consider it in connection with pm_ops in led-class.c, which
set brightness to LED_OFF on suspend and set it back to the previous
value on resume. Setting brightness to LED_OFF results also in disabling
blinking. In case of drivers that implement blink_set op this results
in disabling hardware blinking, but it is not reactivated upon resume,
where only brightness_set{_blocking} op is called.

> If we need the suspend/resume trigger to be optional, we can add that
> as a configuration flag in struct led_trigger, to inform the
> led-triggers.c about the wanted behaviour. In that way, each
> led-trigger type don't have to register their own set of pm_notifiers.

> Some core comments below...
>
>>
>> Cc: linux-pm@vger.kernel.org
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>   drivers/leds/trigger/ledtrig-heartbeat.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
>> index 410c39c62dc7..c80e91152b6b 100644
>> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
>> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/sched.h>
>>   #include <linux/leds.h>
>>   #include <linux/reboot.h>
>> +#include <linux/suspend.h>
>>   #include "../leds.h"
>>
>>   static int panic_heartbeats;
>> @@ -154,6 +155,26 @@ static struct led_trigger heartbeat_led_trigger = {
>>          .deactivate = heartbeat_trig_deactivate,
>>   };
>>
>> +static int heartbeat_pm_notifier(struct notifier_block *nb,
>> +                                unsigned long pm_event, void *unused)
>> +{
>> +       int rc;
>> +
>> +       switch (pm_event) {
>> +       case PM_SUSPEND_PREPARE:
>
> I think you should add:
> case PM_HIBERNATION_PREPARE:
> case PM_RESTORE_PREPARE:
>
>> +               led_trigger_unregister(&heartbeat_led_trigger);
>> +               break;
>> +       case PM_POST_SUSPEND:
>
> I think you should add:
> case PM_POST_HIBERNATION:
> case PM_POST_RESTORE:
>
>> +               rc = led_trigger_register(&heartbeat_led_trigger);
>> +               if (rc)
>> +                       pr_err("could not re-register heartbeat trigger\n");
>> +               break;
>> +       default:
>> +               break;
>> +       }
>> +       return NOTIFY_DONE;
>> +}
>> +
>>   static int heartbeat_reboot_notifier(struct notifier_block *nb,
>>                                       unsigned long code, void *unused)
>>   {
>> @@ -168,6 +189,10 @@ static int heartbeat_panic_notifier(struct notifier_block *nb,
>>          return NOTIFY_DONE;
>>   }
>>
>> +static struct notifier_block heartbeat_pm_nb = {
>> +       .notifier_call = heartbeat_pm_notifier,
>> +};
>> +
>>   static struct notifier_block heartbeat_reboot_nb = {
>>          .notifier_call = heartbeat_reboot_notifier,
>>   };
>> @@ -184,12 +209,14 @@ static int __init heartbeat_trig_init(void)
>>                  atomic_notifier_chain_register(&panic_notifier_list,
>>                                                 &heartbeat_panic_nb);
>>                  register_reboot_notifier(&heartbeat_reboot_nb);
>> +               register_pm_notifier(&heartbeat_pm_nb);
>>          }
>>          return rc;
>>   }
>>
>>   static void __exit heartbeat_trig_exit(void)
>>   {
>> +       unregister_pm_notifier(&heartbeat_pm_nb);
>>          unregister_reboot_notifier(&heartbeat_reboot_nb);
>>          atomic_notifier_chain_unregister(&panic_notifier_list,
>>                                           &heartbeat_panic_nb);
>> --
>> 2.4.11
>>
>
> Besides my upper comments, this looks like a good approach!
>
> Kind regards
> Uffe
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: handle suspend/resume in heartbeat trigger

[Linus Walleij]

On Fri, Jun 3, 2016 at 11:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> I believe most other led-trigger types that should also be "suspended"
> in the similar fashion as the heartbeat. Perhaps not all, but least
> some more (timer, cpu, etc).

I'm uncertain about those ... I guess they need separate patches
at least.

> Okay, my point is, perhaps we should do this in a more generic manner
> and manage the suspend/resume of triggers in
> drivers/leds/led-triggers.c?

That must be done by someone familar with that code I'm afraid.
I'm not sure, the other triggers have their own special hooks (like
the CPU trigger hooking into the CPU suspend/resume ops) so
I'm worried about breaking them.

>> +       switch (pm_event) {
>> +       case PM_SUSPEND_PREPARE:
>
> I think you should add:
> case PM_HIBERNATION_PREPARE:
> case PM_RESTORE_PREPARE:
>
>> +               led_trigger_unregister(&heartbeat_led_trigger);
>> +               break;
>> +       case PM_POST_SUSPEND:
>
> I think you should add:
> case PM_POST_HIBERNATION:
> case PM_POST_RESTORE:

Fixing this and resending.

Yours,
Linus Walleij