] drivers: leds/trigger: system cannot enter suspend

PATCH New Zhang Bo 4 participants 9 messages Jun 6, 2017 – Jun 9, 2017 LED

Share & tools

Zhang Bo

author

2017-06-06 02:36 UTC #0 lore raw

PATCH 1/1 ] drivers: leds/trigger: system cannot enter suspend

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.

Add suspend/resume ops to the struct led_trigger, implement these two ops for ledtrig-heartbeat. Add led_trigger_suspend/led_trigger_resume funcitons to iterate through all LED class devices registered on given trigger and call their suspend/resume ops and disable/enable sysfs at the same time.

Call led_trigger_suspend{resuem} API in pm_notifier function to suspend/resume all led devices listed in given trigger.

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

---
 drivers/leds/led-triggers.c              | 34 +++++++++++++++++++++++++++++++
 drivers/leds/trigger/ledtrig-heartbeat.c | 35 ++++++++++++++++++++++++++------
 include/linux/leds.h                     |  6 ++++++
 3 files changed, 69 insertions(+), 6 deletions(-)

drivers/leds/led-triggers.c +34 -0

index 431123b..1cb1674 100644
@@ -333,6 +333,40 @@ void led_trigger_blink_oneshot(struct led_trigger *trig,
 }
 EXPORT_SYMBOL_GPL(led_trigger_blink_oneshot);
 
+void led_trigger_suspend(struct led_trigger *trig)
+{
+	struct led_classdev *led_cdev;
+
+	if (!trig)
+		return;
+
+	read_lock(&trig->leddev_list_lock);
+	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
+		if (led_cdev->trigger->suspend)
+			led_cdev->trigger->suspend(led_cdev);
+		led_sysfs_disable(led_cdev);
+	}
+	read_unlock(&trig->leddev_list_lock);
+}
+EXPORT_SYMBOL_GPL(led_trigger_suspend);
+
+void led_trigger_resume(struct led_trigger *trig)
+{
+	struct led_classdev *led_cdev;
+
+	if (!trig)
+		return;
+
+	read_lock(&trig->leddev_list_lock);
+	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
+		if (led_cdev->trigger->resume)
+			led_cdev->trigger->resume(led_cdev);
+		led_sysfs_enable(led_cdev);
+	}
+	read_unlock(&trig->leddev_list_lock);
+}
+EXPORT_SYMBOL_GPL(led_trigger_resume);
+
 void led_trigger_register_simple(const char *name, struct led_trigger **tp)
 {
 	struct led_trigger *trig;

drivers/leds/trigger/ledtrig-heartbeat.c +29 -6

index afa3b40..8f779dd 100644
@@ -142,6 +142,7 @@ static void heartbeat_trig_activate(struct led_classdev *led_cdev)
 	led_heartbeat_function(heartbeat_data->timer.data);
 	set_bit(LED_BLINK_SW, &led_cdev->work_flags);
 	led_cdev->activated = true;
+	led_cdev->suspended = false;
 }
 
 static void heartbeat_trig_deactivate(struct led_classdev *led_cdev)

@@ -154,6 +155,30 @@ static void heartbeat_trig_deactivate(struct led_classdev *led_cdev)
 		kfree(heartbeat_data);
 		clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
 		led_cdev->activated = false;
+		led_cdev->suspended = false;
+	}
+}
+
+static void heartbeat_trig_resume(struct led_classdev *led_cdev)
+{
+	struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
+
+	if (led_cdev->suspended) {
+		setup_timer(&heartbeat_data->timer,
+			    led_heartbeat_function, (unsigned long) led_cdev);
+		heartbeat_data->phase = 0;
+		led_heartbeat_function(heartbeat_data->timer.data);
+		led_cdev->suspended = false;
+	}
+}
+
+static void heartbeat_trig_suspend(struct led_classdev *led_cdev)
+{
+	struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
+
+	if (led_cdev->activated && !led_cdev->suspended) {
+		del_timer_sync(&heartbeat_data->timer);
+		led_cdev->suspended = true;
 	}
 }

@@ -161,25 +186,23 @@ static void heartbeat_trig_deactivate(struct led_classdev *led_cdev)
 	.name     = "heartbeat",
 	.activate = heartbeat_trig_activate,
 	.deactivate = heartbeat_trig_deactivate,
+	.suspend = heartbeat_trig_suspend,
+	.resume = heartbeat_trig_resume,
 };
 
 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);
+		led_trigger_suspend(&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");
+		led_trigger_resume(&heartbeat_led_trigger);
 		break;
 	default:
 		break;

include/linux/leds.h +6 -0

index 64c56d4..b792950 100644
@@ -111,6 +111,7 @@ struct led_classdev {
 	void			*trigger_data;
 	/* true if activated - deactivate routine uses it to do cleanup */
 	bool			activated;
+	bool			suspended;
 #endif
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED

@@ -254,6 +255,8 @@ struct led_trigger {
 	const char	 *name;
 	void		(*activate)(struct led_classdev *led_cdev);
 	void		(*deactivate)(struct led_classdev *led_cdev);
+	void		(*suspend)(struct led_classdev *led_cdev);
+	void		(*resume)(struct led_classdev *led_cdev);
 
 	/* LEDs under control by this trigger (for simple triggers) */
 	rwlock_t	  leddev_list_lock;

@@ -291,6 +294,9 @@ extern void led_trigger_set(struct led_classdev *led_cdev,
 			struct led_trigger *trigger);
 extern void led_trigger_remove(struct led_classdev *led_cdev);
 
+extern void led_trigger_suspend(struct led_trigger *trig);
+extern void led_trigger_resume(struct led_trigger *trig);
+
 static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 {
 	return led_cdev->trigger_data;

Signature · 1 line

1.9.1

Pavel Machek

2017-06-06 19:25 UTC #1 lore raw

PATCH 1/1 ] drivers: leds/trigger: system cannot enter suspend

Quoted 6 lines · Zhang Bo

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

Why is heartbeat_pm_notifier calling led_trigger_unregister? That sounds like a bug.
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/20170606/22382c8b/attachment.sig>

Jacek Anaszewski

2017-06-06 20:05 UTC #2 lore raw

PATCH 1/1 ] drivers: leds/trigger: system cannot enter suspend

Quoted 10 lines · Pavel Machek

Quoted 6 lines · Zhang Bo
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
Why is heartbeat_pm_notifier calling led_trigger_unregister? That sounds like a bug.

I suggest using git blame. The commit message adding this code is pretty informative.

Signature · 2 lines

Best regards,
Jacek Anaszewski

Grygorii Strashko

2017-06-06 20:19 UTC #3 lore raw

PATCH 1/1 ] drivers: leds/trigger: system cannot enter suspend

Quoted 9 lines · Jacek Anaszewski

Quoted 8 lines · Pavel Machek
Quoted 6 lines · Zhang Bo
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

Quoted 6 lines

Quoted 2 lines
Why is heartbeat_pm_notifier calling led_trigger_unregister? That sounds like a bug.
I suggest using git blame. The commit message adding this code is pretty informative.

In my opinion original commit do not contain ehough info about root cause of the problem (commit 5ab92a7cb "leds: handle suspend/resume in heartbeat trigger")

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

But why is this happen? In general, leds expected to be switched off during suspend from led_classdev_suspend() which sets LED_SUSPENDED and so blocks led_set_brightness_nosleep() and led_set_brightness_sync().

I think, there was some race between led_classdev_suspend() and set_brightness_work and led_heartbeat_function() which is timer callback.

So, In my opinion, real solution is to remove heartbeat_pm_notifier() and ensure that LED_SUSPENDED flag is processed correctly during suspend. Another option could be to use syscore_ops as it's done in ledtrig-cpu.c.

Signature · 2 lines

regards,
-grygorii

Pavel Machek

2017-06-06 20:34 UTC #4 lore raw

PATCH 1/1 ] drivers: leds/trigger: system cannot enter suspend

Quoted 33 lines · Grygorii Strashko

Quoted 9 lines · Jacek Anaszewski
Quoted 8 lines · Pavel Machek
Quoted 6 lines · Zhang Bo
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
Quoted 6 lines
Quoted 2 lines
Why is heartbeat_pm_notifier calling led_trigger_unregister? That sounds like a bug.
I suggest using git blame. The commit message adding this code is pretty informative.
In my opinion original commit do not contain ehough info about root cause of the problem (commit 5ab92a7cb "leds: handle suspend/resume in heartbeat trigger")
"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.
"
But why is this happen? In general, leds expected to be switched off during suspend from led_classdev_suspend() which sets LED_SUSPENDED and so blocks led_set_brightness_nosleep() and led_set_brightness_sync().

Quoted 6 lines

I think, there was some race between led_classdev_suspend() and set_brightness_work and led_heartbeat_function() which is timer callback.
So, In my opinion, real solution is to remove heartbeat_pm_notifier() and ensure that LED_SUSPENDED flag is processed correctly during suspend. Another option could be to use syscore_ops as it's done in ledtrig-cpu.c.

Agreed. LED staying on is not good enough reason to do led_trigger_unregister() and then add more suspend/resume code to make sure it does not break suspend.
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/20170606/c3778c9a/attachment.sig>

Pavel Machek

2017-06-06 20:38 UTC #5 lore raw

PATCH 1/1 ] drivers: leds/trigger: system cannot enter suspend

In reply to Grygorii Strashko

Quoted 15 lines · Grygorii Strashko

Quoted 12 lines · Jacek Anaszewski
Quoted 10 lines · Pavel Machek
Quoted 6 lines · Zhang Bo
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
Why is heartbeat_pm_notifier calling led_trigger_unregister? That sounds like a bug.

Quoted 7 lines

Quoted 3 lines
I suggest using git blame. The commit message adding this code is pretty informative.
In my opinion original commit do not contain ehough info about root cause of the problem (commit 5ab92a7cb "leds: handle suspend/resume in heartbeat trigger")

Zhang: Can you try to revert commit 5ab92a7cb in your tree, to see if it fixes your problem?

Thanks,
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/20170606/ba7cafed/attachment.sig>

Bruce Zhang

author

2017-06-07 04:24 UTC #6 lore raw

PATCH 1/1 ] drivers: leds/trigger: system cannot enter suspend

Hi Pavel,

It can also fix my issue by reverting the commit 5ab92a7cb. But this action only does not make led_set_brightness_nosleep function to set brightness. The heartbeat trigger timer is still running even though it is not harmful.

Best Regards,
Bo

-----Original Message-----
From: Pavel Machek [mailto:pavel at ucw.cz]
Sent: Wednesday, June 07, 2017 4:38 AM
To: Grygorii Strashko <[email protected]>
Cc: Jacek Anaszewski <[email protected]>; Bruce Zhang <[email protected]>; Linus Walleij <[email protected]>; linux-arm-kernel at lists.infradead.org
Subject: Re: [[PATCH]] drivers: leds/trigger: system cannot enter suspend

Quoted 15 lines · Grygorii Strashko

Quoted 12 lines · Jacek Anaszewski
Quoted 10 lines · Pavel Machek
Quoted 6 lines · Zhang Bo
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
Why is heartbeat_pm_notifier calling led_trigger_unregister? That sounds like a bug.

Quoted 7 lines

Quoted 3 lines
I suggest using git blame. The commit message adding this code is pretty informative.
In my opinion original commit do not contain ehough info about root cause of the problem (commit 5ab92a7cb "leds: handle suspend/resume in heartbeat trigger")

Zhang: Can you try to revert commit 5ab92a7cb in your tree, to see if it fixes your problem?

Thanks,
Pavel

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

Grygorii Strashko

2017-06-07 15:38 UTC #7 lore raw

PATCH 1/1 ] drivers: leds/trigger: system cannot enter suspend

Quoted 3 lines · Bruce Zhang

Hi Pavel,
It can also fix my issue by reverting the commit 5ab92a7cb. But this action only does not make led_set_brightness_nosleep function to set brightness. The heartbeat trigger timer is still running even though it is not harmful.

Unfortunately, this is sort of design issue of leds triggers framework as it doesn't have Suspend related interfaces, so triggers continue running even if led, by it self, is suspended from led_classdev_suspend(). For example, heartbeat trigger timer can be source of spurious wake-ups in some cases.

I've had to deal with such kind of issues but related to ledtrig-cpu.c connected to gpio.

Quoted 29 lines · Grygorii Strashko

Quoted 15 lines · Jacek Anaszewski
Quoted 12 lines · Pavel Machek
Quoted 10 lines · Zhang Bo
Quoted 6 lines
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
Why is heartbeat_pm_notifier calling led_trigger_unregister? That sounds like a bug.
Quoted 7 lines
Quoted 3 lines
I suggest using git blame. The commit message adding this code is pretty informative.
In my opinion original commit do not contain ehough info about root cause of the problem (commit 5ab92a7cb "leds: handle suspend/resume in heartbeat trigger")
Zhang: Can you try to revert commit 5ab92a7cb in your tree, to see if it fixes your problem?
Thanks,
Pavel

Signature · 2 lines

regards,
-grygorii

Pavel Machek

2017-06-09 22:16 UTC #8 lore raw

PATCH 1/1 ] drivers: leds/trigger: system cannot enter suspend

Quoted 5 lines · Grygorii Strashko

Quoted 2 lines · Bruce Zhang
Hi Pavel,

Quoted 7 lines

Quoted 1 line
It can also fix my issue by reverting the commit 5ab92a7cb. But this action only does not make led_set_brightness_nosleep function to set brightness. The heartbeat trigger timer is still running even though it is not harmful.
Unfortunately, this is sort of design issue of leds triggers framework as it doesn't have Suspend related interfaces, so triggers continue running even if led, by it self, is suspended from led_classdev_suspend(). For example, heartbeat trigger timer can be source of spurious wake-ups in some cases.

Yes, trigger keeps running. It should not be a problem... right?

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/20170610/bcd6e3f4/attachment.sig>