] drivers: leds/trigger: system cannot enter suspend

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

Share & tools

Zhang Bo

author

2017-06-05 07: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. Heartbeat_pm_notifier is called when system prepare to enter suspend mode, then 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(-)
 mode change 100644 => 100755 drivers/leds/led-triggers.c
 mode change 100644 => 100755 drivers/leds/trigger/ledtrig-heartbeat.c
 mode change 100644 => 100755 include/linux/leds.h

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

old mode 100644
new mode 100755
index 431123b..1cb1674
@@ -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

old mode 100644
new mode 100755
index afa3b40..8f779dd
@@ -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

old mode 100644
new mode 100755
index 64c56d4..b792950
@@ -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

Jacek Anaszewski

2017-06-05 19:27 UTC #1 lore raw

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

Hi Zhang,

Thanks for the patch. It seems to be almost ready to merge. Just two issues below.

Please also always cc linux-leds at vger.kernel.org when modifying LED subsystem.

Quoted 14 lines · Zhang Bo

System cannot enter suspend mode because of heartbeat led trigger. Heartbeat_pm_notifier is called when system prepare to enter suspend mode, then 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]>

Quoted 8 lines

---
 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(-)
 mode change 100644 => 100755 drivers/leds/led-triggers.c
 mode change 100644 => 100755 drivers/leds/trigger/ledtrig-heartbeat.c
 mode change 100644 => 100755 include/linux/leds.h

You accidentally changed file permissions.

Quoted 12 lines

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

old mode 100644
new mode 100755
index 431123b..1cb1674
@@ -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)
+{

drivers/leds/led-triggers.c void led_trigger_blink_oneshot(struct led_trigger *trig, · line 347

Quoted 10 lines

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

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

Could you share what kind of problem this call to led_syfs_disable() solves? Did you come across one?

Quoted 134 lines

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

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

old mode 100644
new mode 100755
index afa3b40..8f779dd
@@ -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

old mode 100644
new mode 100755
index 64c56d4..b792950
@@ -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 · 2 lines

Best regards,
Jacek Anaszewski

Bruce Zhang

author

2017-06-06 02:47 UTC #2 lore raw

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

Hi Jacek,

The led_syfs_disable() is used to avoid heartbeat_trig_activate/ heartbeat_trig_deactivate called when this led device is suspended. I am not encounter this issue, but I think it is a risk.

Best Regards,
Bo

-----Original Message-----
From: Jacek Anaszewski [mailto:jacek.anaszewski at gmail.com]
Sent: Tuesday, June 06, 2017 3:27 AM
To: Bruce Zhang <[email protected]>; linux-arm-kernel at lists.infradead.org
Cc: pavel at ucw.cz; Linux LED Subsystem <[email protected]>
Subject: Re: [[PATCH]] drivers: leds/trigger: system cannot enter suspend

Hi Zhang,

Thanks for the patch. It seems to be almost ready to merge. Just two issues below.

Please also always cc linux-leds at vger.kernel.org when modifying LED subsystem.

Quoted 14 lines · Zhang Bo

System cannot enter suspend mode because of heartbeat led trigger. Heartbeat_pm_notifier is called when system prepare to enter suspend mode, then 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]>

Quoted 8 lines

---
 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(-)  mode change 100644 
=> 100755 drivers/leds/led-triggers.c  mode change 100644 => 100755 
drivers/leds/trigger/ledtrig-heartbeat.c
 mode change 100644 => 100755 include/linux/leds.h

You accidentally changed file permissions.

Quoted 8 lines

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

old mode 100644 new mode 100755 index 431123b..1cb1674
@@ -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) {

drivers/leds/led-triggers.c void led_trigger_blink_oneshot(struct led_trigger · line 345

Quoted 10 lines

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

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

Could you share what kind of problem this call to led_syfs_disable() solves? Did you come across one?

Quoted 120 lines

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

+	}
+	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 +27 -6

old mode 100644
new mode 100755
index afa3b40..8f779dd
@@ -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 old mode renamed from include/linux/leds.h +0 -0

100644 new mode 100755 index 64c56d4..b792950

include/linux/leds.h +1 -0

@@ -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 {

include/linux/leds.h +2 -0

 	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;

include/linux/leds.h extern void led_trigger_set(struct led_classdev *led_cdev, · line 302

Quoted 9 lines

include/linux/leds.h +3 -0

@@ -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;

--
Best regards,
Jacek Anaszewski

Jacek Anaszewski

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

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

Hi Bo,

Quoted 4 lines · Bruce Zhang

Hi Jacek,
The led_syfs_disable() is used to avoid heartbeat_trig_activate/ heartbeat_trig_deactivate called when this led device is suspended. I am not encounter this issue, but I think it is a risk.

The led_sysfs_enable{disable} have been designed mainly to allow for disabling an access to a LED flash class device, when it is under v4l2-flash sub-device control. While it seems that calling it here shouldn't do any harm, since vl42_flash sub-dev removes any trigger on open, it would be semantically inconsistent, since suspending/resuming a trigger shouldn't influence any LED class device sysfs availability.

Moreover, it will not eliminate the possibility that heartbeat trigger is set in the meantime on some other LED class device, which will also send an uevent. This is however under userspace control.

Please don't use this API here.

Quoted 195 lines

Hi Zhang,

Thanks for the patch. It seems to be almost ready to merge. Just two issues below.

Please also always cc linux-leds at vger.kernel.org when modifying LED subsystem.

Quoted 14 lines · Zhang Bo

System cannot enter suspend mode because of heartbeat led trigger. Heartbeat_pm_notifier is called when system prepare to enter suspend mode, then 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]>

Quoted 8 lines

---
 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(-)  mode change 100644 
=> 100755 drivers/leds/led-triggers.c  mode change 100644 => 100755 
drivers/leds/trigger/ledtrig-heartbeat.c
 mode change 100644 => 100755 include/linux/leds.h

You accidentally changed file permissions.

Quoted 8 lines

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

old mode 100644 new mode 100755 index 431123b..1cb1674
@@ -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) {

drivers/leds/led-triggers.c void led_trigger_blink_oneshot(struct led_trigger · line 345

Quoted 10 lines

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

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

Could you share what kind of problem this call to led_syfs_disable() solves? Did you come across one?

Quoted 120 lines

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

+	}
+	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 +27 -6

old mode 100644
new mode 100755
index afa3b40..8f779dd
@@ -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 old mode renamed from include/linux/leds.h +0 -0

100644 new mode 100755 index 64c56d4..b792950

include/linux/leds.h +1 -0

@@ -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 {

include/linux/leds.h +2 -0

 	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;

include/linux/leds.h extern void led_trigger_set(struct led_classdev *led_cdev, · line 302

Quoted 9 lines

include/linux/leds.h +3 -0

@@ -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;

--
Best regards,
Jacek Anaszewski

Signature · 2 lines

Best regards,
Jacek Anaszewski

Pavel Machek

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

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

In reply to Zhang Bo

Quoted 1 line · Zhang Bo

System cannot enter suspend mode because of heartbeat led trigger.

Can you tell us _why_ the heartbeat trigger prevents suspend/resume?

Because it does not:

# echo heartbeat > /sys/class/leds/tpacpi\:\:standby/trigger
# echo mem > /sys/power/state
(system suspends, as expected).
#

Linux amd 4.12.0-rc2+ #400 SMP Mon May 22 22:44:02 CEST 2017 i686...

If it does for you, you may want to find out why, then fix that.

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/20170605/2886f31f/attachment.sig>

Bruce Zhang

author

2017-06-06 02:11 UTC #5 lore raw

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

In autosleep_wq, try_to_suspend function will try to enter suspend
mode in specific period. it will get wakeup_count as initial value 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 count changed.
When try to freeze processes, system will get wakeup_count again and
compare it with initial value. When not equal, suspend will fail.

While "# echo mem > /sys/power/state" does not record the initial wakeup_count. So it can enter suspend mode suscessfully.

Best Regards,
Bo

-----Original Message-----
From: Pavel Machek [mailto:pavel at ucw.cz]
Sent: Tuesday, June 06, 2017 4:06 AM
To: Bruce Zhang <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org; jacek.anaszewski at gmail.com
Subject: Re: [[PATCH]] drivers: leds/trigger: system cannot enter suspend

Quoted 1 line · Zhang Bo

System cannot enter suspend mode because of heartbeat led trigger.

Can you tell us _why_ the heartbeat trigger prevents suspend/resume?

Because it does not:

# echo heartbeat > /sys/class/leds/tpacpi\:\:standby/trigger
# echo mem > /sys/power/state
(system suspends, as expected).
#

Linux amd 4.12.0-rc2+ #400 SMP Mon May 22 22:44:02 CEST 2017 i686...

If it does for you, you may want to find out why, then fix that.

Thanks,
Pavel

Signature · 2 lines

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

Linus Walleij

2017-06-09 11:25 UTC #6 lore raw

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

In reply to Zhang Bo

Quoted 1 line · Zhang Bo

System cannot enter suspend mode because of heartbeat led trigger.

As mentioned this should not be a problem.

Quoted 9 lines

Heartbeat_pm_notifier is called when system prepare to enter suspend mode, then 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.

Quoted 4 lines

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

The patch is nice because it fixes my sloppy approach of just adding/removing the trigger made earlier.

It stops unsolicited I2C traffic and other nastines from happening during the suspend/resume cycle.

I would even add:

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

(...)

Quoted 11 lines

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

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

drivers/leds/trigger/ledtrig-heartbeat.c static void heartbeat_trig_deactivate(struct led_classdev *led_cdev) · line 182

Quoted 10 lines

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

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

Are we sure that the LED is *off* at this point so you don't suspend/resume with a glowing LED?

Else insert a call to make sure that it's like so, and maybe even a variable to hold the current state (off/on) across the suspend/resume cycle.

Quoted 6 lines

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

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

(...)

include/linux/leds.h struct led_trigger { · line 259

Quoted 2 lines

include/linux/leds.h +2 -0

+       void            (*suspend)(struct led_classdev *led_cdev);
+       void            (*resume)(struct led_classdev *led_cdev);

These names do not correspons to the actual PM calls they are utilized for.

Name them
.pm_prepare_suspend()
.pm_post_suspend()

Instead, this indicates better the place when they get called.

Yours,
Linus Walleij

Bruce Zhang

author

2017-06-09 13:01 UTC #7 lore raw

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

At first we should check that whether exist the use case that LED turn on/off is based on other device(such as I2C). If true, how to guarantee the other devices(such as I2C) are available when turn off LED. Maybe pm_notifier is accepted method to process this case, because pm_notifier callback function is executed before other devices enter suspend mode.

If we don't need to care such case or not exist such use case in application, we can revert the patch 5ab92a7cb.

Best Regards,
Bruce

-----Original Message-----
From: Linus Walleij [mailto:linus.walleij at linaro.org]
Sent: Friday, June 09, 2017 7:25 PM
To: Bruce Zhang <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org; Jacek Anaszewski <[email protected]>; Pavel Machek <[email protected]>
Subject: Re: [[PATCH]] drivers: leds/trigger: system cannot enter suspend

Quoted 1 line · Zhang Bo

System cannot enter suspend mode because of heartbeat led trigger.

As mentioned this should not be a problem.

Quoted 9 lines

Heartbeat_pm_notifier is called when system prepare to enter suspend mode, then 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.

Quoted 4 lines

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

The patch is nice because it fixes my sloppy approach of just adding/removing the trigger made earlier.

It stops unsolicited I2C traffic and other nastines from happening during the suspend/resume cycle.

I would even add:

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

(...)

Quoted 11 lines

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

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

drivers/leds/trigger/ledtrig-heartbeat.c static void heartbeat_trig_deactivate(struct led_classdev *led_cdev) · line 182

Quoted 10 lines

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

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

Are we sure that the LED is *off* at this point so you don't suspend/resume with a glowing LED?

Else insert a call to make sure that it's like so, and maybe even a variable to hold the current state (off/on) across the suspend/resume cycle.

Quoted 6 lines

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

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

(...)

include/linux/leds.h struct led_trigger { · line 259

Quoted 2 lines

include/linux/leds.h +2 -0

+       void            (*suspend)(struct led_classdev *led_cdev);
+       void            (*resume)(struct led_classdev *led_cdev);

These names do not correspons to the actual PM calls they are utilized for.

Name them
.pm_prepare_suspend()
.pm_post_suspend()

Instead, this indicates better the place when they get called.

Yours,
Linus Walleij