] Revert "leds: handle suspend/resume in heartbeat trigger"

PATCH New Zhang Bo 6 participants 10 messages Jun 13, 2017 – Aug 27, 2017

Subsystems (2)

Stable branch LED

lore

Share & tools

lore

Zhang Bo

author

2017-06-13 02:39 UTC #0 lore raw

PATCH 1/1 ] Revert "leds: handle suspend/resume in heartbeat trigger"

This reverts commit 5ab92a7cb82c66bf30685583a38a18538e3807db.

System cannot enter suspend mode because of heartbeat led trigger. In autosleep_wq, try_to_suspend function will try to enter suspend mode in specific period. it will get wakeup_count then call pm_notifier chain callback function and freeze processes. Heartbeat_pm_notifier is called and it call led_trigger_unregister to change the trigger of led device to none. It will send uevent message and the wakeup source count changed. As wakeup_count changed, suspend will abort.

Signed-off-by Zhang Bo <[email protected]>

---
 drivers/leds/trigger/ledtrig-heartbeat.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

drivers/leds/trigger/ledtrig-heartbeat.c +0 ~~-31~~

index afa3b40..e95ea65 100644
@@ -20,7 +20,6 @@
 #include <linux/sched/loadavg.h>
 #include <linux/leds.h>
 #include <linux/reboot.h>
-#include <linux/suspend.h>
 #include "../leds.h"
 
 static int panic_heartbeats;

@@ -163,30 +162,6 @@ static void heartbeat_trig_deactivate(struct led_classdev *led_cdev)
 	.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:
-	case PM_HIBERNATION_PREPARE:
-	case PM_RESTORE_PREPARE:
-		led_trigger_unregister(&heartbeat_led_trigger);
-		break;
-	case PM_POST_SUSPEND:
-	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)
 {

@@ -201,10 +176,6 @@ 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,
 };

@@ -221,14 +192,12 @@ 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);

Signature · 1 line

1.9.1

Pavel Machek

2017-06-13 07:31 UTC #1 lore raw

PATCH 1/1 ] Revert "leds: handle suspend/resume in heartbeat trigger"

Quoted 11 lines · Zhang Bo

This reverts commit 5ab92a7cb82c66bf30685583a38a18538e3807db.
System cannot enter suspend mode because of heartbeat led trigger. In autosleep_wq, try_to_suspend function will try to enter suspend mode in specific period. it will get wakeup_count then call pm_notifier chain callback function and freeze processes. Heartbeat_pm_notifier is called and it call led_trigger_unregister to change the trigger of led device to none. It will send uevent message and the wakeup source count changed. As wakeup_count changed, suspend will abort.

Quoted 1 line

Signed-off-by Zhang Bo <[email protected]>

Yes, this makes sense. Turning led off should be done at driver level.

As this fixes a regression, it would be kind of nice to get it to v4.12...

Acked-by Pavel Machek <[email protected]>

Signature · 9 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170613/75079f2d/attachment.sig>

Linus Walleij

2017-06-13 09:35 UTC #2 lore raw

PATCH 1/1 ] Revert "leds: handle suspend/resume in heartbeat trigger"

In reply to Zhang Bo

Quoted 11 lines · Zhang Bo

This reverts commit 5ab92a7cb82c66bf30685583a38a18538e3807db.
System cannot enter suspend mode because of heartbeat led trigger. In autosleep_wq, try_to_suspend function will try to enter suspend mode in specific period. it will get wakeup_count then call pm_notifier chain callback function and freeze processes. Heartbeat_pm_notifier is called and it call led_trigger_unregister to change the trigger of led device to none. It will send uevent message and the wakeup source count changed. As wakeup_count changed, suspend will abort.

Quoted 1 line

Signed-off-by Zhang Bo <[email protected]>

OK I was stupid, lesson learned.

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Pavel Machek

2017-07-13 12:03 UTC #3 lore raw

PATCH 1/1 ] Revert "leds: handle suspend/resume in heartbeat trigger"

Quoted 13 lines · Linus Walleij

Quoted 11 lines · Zhang Bo
This reverts commit 5ab92a7cb82c66bf30685583a38a18538e3807db.
System cannot enter suspend mode because of heartbeat led trigger. In autosleep_wq, try_to_suspend function will try to enter suspend mode in specific period. it will get wakeup_count then call pm_notifier chain callback function and freeze processes. Heartbeat_pm_notifier is called and it call led_trigger_unregister to change the trigger of led device to none. It will send uevent message and the wakeup source count changed. As wakeup_count changed, suspend will abort.

Quoted 4 lines

Quoted 1 line
Signed-off-by Zhang Bo <[email protected]>
OK I was stupid, lesson learned.
Reviewed-by Linus Walleij <[email protected]>

You may still want to submit patch to turn the LED off over suspend for your platform...

Signature · 9 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170713/26983c1c/attachment-0001.sig>

Linus Walleij

2017-07-13 12:46 UTC #4 lore raw

PATCH 1/1 ] Revert "leds: handle suspend/resume in heartbeat trigger"

Quoted 14 lines · Pavel Machek

Quoted 13 lines · Linus Walleij
Quoted 11 lines · Zhang Bo
This reverts commit 5ab92a7cb82c66bf30685583a38a18538e3807db.
System cannot enter suspend mode because of heartbeat led trigger. In autosleep_wq, try_to_suspend function will try to enter suspend mode in specific period. it will get wakeup_count then call pm_notifier chain callback function and freeze processes. Heartbeat_pm_notifier is called and it call led_trigger_unregister to change the trigger of led device to none. It will send uevent message and the wakeup source count changed. As wakeup_count changed, suspend will abort.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Signed-off-by Zhang Bo <[email protected]>
OK I was stupid, lesson learned.
Reviewed-by Linus Walleij <[email protected]>
You may still want to submit patch to turn the LED off over suspend for your platform...

I think it would make more sense to have the LED subsystem handle that. Given that it already keeps track of LED_SUSPENDED in a special flag.

Probably optionally, like LEDs can opt in to be turned off during suspend.

Yours,
Linus Walleij

Jacek Anaszewski

2017-06-13 18:44 UTC #5 lore raw

PATCH 1/1 ] Revert "leds: handle suspend/resume in heartbeat trigger"

In reply to Zhang Bo

Hi Bo,

Quoted 76 lines · Zhang Bo

This reverts commit 5ab92a7cb82c66bf30685583a38a18538e3807db.

Signed-off-by Zhang Bo <[email protected]>

---
 drivers/leds/trigger/ledtrig-heartbeat.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

drivers/leds/trigger/ledtrig-heartbeat.c +0 ~~-30~~

index afa3b40..e95ea65 100644
@@ -20,7 +20,6 @@
 #include <linux/sched/loadavg.h>
 #include <linux/leds.h>
 #include <linux/reboot.h>
-#include <linux/suspend.h>
 #include "../leds.h"
 
 static int panic_heartbeats;

@@ -163,30 +162,6 @@ static void heartbeat_trig_deactivate(struct led_classdev *led_cdev)
 	.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:
-	case PM_HIBERNATION_PREPARE:
-	case PM_RESTORE_PREPARE:
-		led_trigger_unregister(&heartbeat_led_trigger);
-		break;
-	case PM_POST_SUSPEND:
-	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)
 {

@@ -201,10 +176,6 @@ 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,
 };

@@ -221,14 +192,12 @@ 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);

drivers/leds/trigger/ledtrig-heartbeat.c static int __init heartbeat_trig_init(void) · line 203

Quoted 10 lines

drivers/leds/trigger/ledtrig-heartbeat.c +0 -1

 	}
 	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);

Thanks for the patch.

Added tag:

Fixes 5ab92a7cb82c ("leds: handle suspend/resume in heartbeat trigger")

and applied to the fixes-for-4.12-rc6 branch of linux-leds.git.

Signature · 2 lines

Best regards,
Jacek Anaszewski

Geert Uytterhoeven

2017-08-23 14:02 UTC #6 lore raw

PATCH 1/1 ] Revert "leds: handle suspend/resume in heartbeat trigger"

In reply to Zhang Bo

Hi Zhang,

Quoted 11 lines · Zhang Bo

This reverts commit 5ab92a7cb82c66bf30685583a38a18538e3807db.
System cannot enter suspend mode because of heartbeat led trigger. In autosleep_wq, try_to_suspend function will try to enter suspend mode in specific period. it will get wakeup_count then call pm_notifier chain callback function and freeze processes. Heartbeat_pm_notifier is called and it call led_trigger_unregister to change the trigger of led device to none. It will send uevent message and the wakeup source count changed. As wakeup_count changed, suspend will abort.

Quoted 1 line

Signed-off-by Zhang Bo <[email protected]>

While investigating an heartbeat LED issue in v4.9.40-ltsi, I stumpled on this revert (commit 436c4c45b5b9562b ("Revert "leds: handle suspend/resume in heartbeat trigger"") in v4.12-rc6), which was not backported to v4.9 (yet).

Interestingly, while running v4.9.x on r8a7791/koelsch with an LED configured for heartbeat, I can actually suspend the system using s2ram. However, after system resume the heartbeat LED no longer flashes. /sys/class/leds/LED6/trigger shows the trigger is still "none", and thus wasn't restored to "heartbeat" by the notifier.
There's no "could not re-register heartbeat trigger" message printed.

Backporting the revert fixes this.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Pavel Machek

2017-08-23 19:34 UTC #7 lore raw

PATCH 1/1 ] Revert "leds: handle suspend/resume in heartbeat trigger"

Hi!

Quoted 18 lines · Zhang Bo

Quoted 11 lines
This reverts commit 5ab92a7cb82c66bf30685583a38a18538e3807db.
System cannot enter suspend mode because of heartbeat led trigger. In autosleep_wq, try_to_suspend function will try to enter suspend mode in specific period. it will get wakeup_count then call pm_notifier chain callback function and freeze processes. Heartbeat_pm_notifier is called and it call led_trigger_unregister to change the trigger of led device to none. It will send uevent message and the wakeup source count changed. As wakeup_count changed, suspend will abort.
Quoted 1 line
Signed-off-by Zhang Bo <[email protected]>
While investigating an heartbeat LED issue in v4.9.40-ltsi, I stumpled on this revert (commit 436c4c45b5b9562b ("Revert "leds: handle suspend/resume in heartbeat trigger"") in v4.12-rc6), which was not backported to v4.9 (yet).

Quoted 8 lines

Interestingly, while running v4.9.x on r8a7791/koelsch with an LED configured for heartbeat, I can actually suspend the system using s2ram. However, after system resume the heartbeat LED no longer flashes. /sys/class/leds/LED6/trigger shows the trigger is still "none", and thus wasn't restored to "heartbeat" by the notifier.
There's no "could not re-register heartbeat trigger" message printed.
Backporting the revert fixes this.

You have my Acked-by: if you want to submit this to some -stable kernels...

Pavel

Signature · 9 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170823/12e18239/attachment.sig>

Geert Uytterhoeven

2017-08-25 08:23 UTC #8 lore raw

PATCH 1/1 ] Revert "leds: handle suspend/resume in heartbeat trigger"

Quoted 25 lines · Pavel Machek

Quoted 25 lines · Zhang Bo
Quoted 11 lines
This reverts commit 5ab92a7cb82c66bf30685583a38a18538e3807db.
System cannot enter suspend mode because of heartbeat led trigger. In autosleep_wq, try_to_suspend function will try to enter suspend mode in specific period. it will get wakeup_count then call pm_notifier chain callback function and freeze processes. Heartbeat_pm_notifier is called and it call led_trigger_unregister to change the trigger of led device to none. It will send uevent message and the wakeup source count changed. As wakeup_count changed, suspend will abort.
Quoted 1 line
Signed-off-by Zhang Bo <[email protected]>
While investigating an heartbeat LED issue in v4.9.40-ltsi, I stumpled on this revert (commit 436c4c45b5b9562b ("Revert "leds: handle suspend/resume in heartbeat trigger"") in v4.12-rc6), which was not backported to v4.9 (yet).
Interestingly, while running v4.9.x on r8a7791/koelsch with an LED configured for heartbeat, I can actually suspend the system using s2ram. However, after system resume the heartbeat LED no longer flashes. /sys/class/leds/LED6/trigger shows the trigger is still "none", and thus wasn't restored to "heartbeat" by the notifier.
There's no "could not re-register heartbeat trigger" message printed.

Quoted 4 lines

Quoted 1 line
Backporting the revert fixes this.
You have my Acked-by: if you want to submit this to some -stable kernels...

Thank you!

Greg: Can you please queue 436c4c45b5b9562b ("Revert "leds: handle suspend/resume in heartbeat trigger"") for v4.9.x or v4.9-ltsi? Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Greg KH

2017-08-27 12:46 UTC #9 lore raw

PATCH 1/1 ] Revert "leds: handle suspend/resume in heartbeat trigger"

Quoted 28 lines · Geert Uytterhoeven

Quoted 27 lines · Pavel Machek
Quoted 26 lines · Zhang Bo
Quoted 11 lines
This reverts commit 5ab92a7cb82c66bf30685583a38a18538e3807db.
System cannot enter suspend mode because of heartbeat led trigger. In autosleep_wq, try_to_suspend function will try to enter suspend mode in specific period. it will get wakeup_count then call pm_notifier chain callback function and freeze processes. Heartbeat_pm_notifier is called and it call led_trigger_unregister to change the trigger of led device to none. It will send uevent message and the wakeup source count changed. As wakeup_count changed, suspend will abort.
Quoted 1 line
Signed-off-by Zhang Bo <[email protected]>
While investigating an heartbeat LED issue in v4.9.40-ltsi, I stumpled on this revert (commit 436c4c45b5b9562b ("Revert "leds: handle suspend/resume in heartbeat trigger"") in v4.12-rc6), which was not backported to v4.9 (yet).
Interestingly, while running v4.9.x on r8a7791/koelsch with an LED configured for heartbeat, I can actually suspend the system using s2ram. However, after system resume the heartbeat LED no longer flashes. /sys/class/leds/LED6/trigger shows the trigger is still "none", and thus wasn't restored to "heartbeat" by the notifier.
There's no "could not re-register heartbeat trigger" message printed.
Backporting the revert fixes this.

Quoted 8 lines

Quoted 2 lines
You have my Acked-by: if you want to submit this to some -stable kernels...
Thank you!
Greg: Can you please queue 436c4c45b5b9562b ("Revert "leds: handle suspend/resume in heartbeat trigger"") for v4.9.x or v4.9-ltsi? Thanks!

Now applied, thanks.

greg k-h