* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
@ 2017-06-05 7:36 Zhang Bo
2017-06-05 19:27 ` Jacek Anaszewski
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Zhang Bo @ 2017-06-05 7:36 UTC (permalink / raw)
To: linux-arm-kernel
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 <bo.zhang@nxp.com>
---
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
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
old mode 100644
new mode 100755
index 431123b..1cb1674
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -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;
diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
old mode 100644
new mode 100755
index afa3b40..8f779dd
--- a/drivers/leds/trigger/ledtrig-heartbeat.c
+++ b/drivers/leds/trigger/ledtrig-heartbeat.c
@@ -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;
diff --git a/include/linux/leds.h b/include/linux/leds.h
old mode 100644
new mode 100755
index 64c56d4..b792950
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -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;
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-05 7:36 [[PATCH]] drivers: leds/trigger: system cannot enter suspend Zhang Bo
@ 2017-06-05 19:27 ` Jacek Anaszewski
2017-06-05 20:05 ` Pavel Machek
2017-06-09 11:25 ` Linus Walleij
2 siblings, 0 replies; 21+ messages in thread
From: Jacek Anaszewski @ 2017-06-05 19:27 UTC (permalink / raw)
To: Zhang Bo, linux-arm-kernel; +Cc: pavel, Linux LED Subsystem
Hi Zhang,
Thanks for the patch. It seems to be almost ready to merge.
Just two issues below.
Please also always cc linux-leds@vger.kernel.org when modifying
LED subsystem.
On 06/05/2017 09:36 AM, Zhang Bo wrote:
> 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 <bo.zhang@nxp.com>
> ---
> 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.
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> old mode 100644
> new mode 100755
> index 431123b..1cb1674
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -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);
Could you share what kind of problem this call to led_syfs_disable()
solves? Did you come across one?
> + }
> + 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;
> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
> old mode 100644
> new mode 100755
> index afa3b40..8f779dd
> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> @@ -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;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> old mode 100644
> new mode 100755
> index 64c56d4..b792950
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -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;
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
@ 2017-06-05 19:27 ` Jacek Anaszewski
0 siblings, 0 replies; 21+ messages in thread
From: Jacek Anaszewski @ 2017-06-05 19:27 UTC (permalink / raw)
To: linux-arm-kernel
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.
On 06/05/2017 09:36 AM, Zhang Bo wrote:
> 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 <bo.zhang@nxp.com>
> ---
> 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.
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> old mode 100644
> new mode 100755
> index 431123b..1cb1674
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -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);
Could you share what kind of problem this call to led_syfs_disable()
solves? Did you come across one?
> + }
> + 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;
> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
> old mode 100644
> new mode 100755
> index afa3b40..8f779dd
> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> @@ -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;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> old mode 100644
> new mode 100755
> index 64c56d4..b792950
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -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;
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-05 7:36 [[PATCH]] drivers: leds/trigger: system cannot enter suspend Zhang Bo
2017-06-05 19:27 ` Jacek Anaszewski
@ 2017-06-05 20:05 ` Pavel Machek
2017-06-06 2:11 ` Bruce Zhang
2017-06-09 11:25 ` Linus Walleij
2 siblings, 1 reply; 21+ messages in thread
From: Pavel Machek @ 2017-06-05 20:05 UTC (permalink / raw)
To: linux-arm-kernel
On Mon 2017-06-05 15:36:31, Zhang Bo wrote:
> 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
--
(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>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-05 20:05 ` Pavel Machek
@ 2017-06-06 2:11 ` Bruce Zhang
0 siblings, 0 replies; 21+ messages in thread
From: Bruce Zhang @ 2017-06-06 2:11 UTC (permalink / raw)
To: linux-arm-kernel
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 <bo.zhang@nxp.com>
Cc: linux-arm-kernel at lists.infradead.org; jacek.anaszewski at gmail.com
Subject: Re: [[PATCH]] drivers: leds/trigger: system cannot enter suspend
On Mon 2017-06-05 15:36:31, Zhang Bo wrote:
> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-05 19:27 ` Jacek Anaszewski
@ 2017-06-06 2:47 ` Bruce Zhang
-1 siblings, 0 replies; 21+ messages in thread
From: Bruce Zhang @ 2017-06-06 2:47 UTC (permalink / raw)
To: Jacek Anaszewski, linux-arm-kernel; +Cc: pavel, Linux LED Subsystem
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@gmail.com]
Sent: Tuesday, June 06, 2017 3:27 AM
To: Bruce Zhang <bo.zhang@nxp.com>; linux-arm-kernel@lists.infradead.org
Cc: pavel@ucw.cz; Linux LED Subsystem <linux-leds@vger.kernel.org>
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@vger.kernel.org when modifying LED subsystem.
On 06/05/2017 09:36 AM, Zhang Bo wrote:
> 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 <bo.zhang@nxp.com>
> ---
> 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.
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> old mode 100644 new mode 100755 index 431123b..1cb1674
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -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);
Could you share what kind of problem this call to led_syfs_disable() solves? Did you come across one?
> + }
> + 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;
> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c
> b/drivers/leds/trigger/ledtrig-heartbeat.c
> old mode 100644
> new mode 100755
> index afa3b40..8f779dd
> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> @@ -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;
> diff --git a/include/linux/leds.h b/include/linux/leds.h old mode
> 100644 new mode 100755 index 64c56d4..b792950
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -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;
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
@ 2017-06-06 2:47 ` Bruce Zhang
0 siblings, 0 replies; 21+ messages in thread
From: Bruce Zhang @ 2017-06-06 2:47 UTC (permalink / raw)
To: linux-arm-kernel
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 <bo.zhang@nxp.com>; linux-arm-kernel at lists.infradead.org
Cc: pavel at ucw.cz; Linux LED Subsystem <linux-leds@vger.kernel.org>
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.
On 06/05/2017 09:36 AM, Zhang Bo wrote:
> 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 <bo.zhang@nxp.com>
> ---
> 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.
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> old mode 100644 new mode 100755 index 431123b..1cb1674
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -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);
Could you share what kind of problem this call to led_syfs_disable() solves? Did you come across one?
> + }
> + 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;
> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c
> b/drivers/leds/trigger/ledtrig-heartbeat.c
> old mode 100644
> new mode 100755
> index afa3b40..8f779dd
> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> @@ -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;
> diff --git a/include/linux/leds.h b/include/linux/leds.h old mode
> 100644 new mode 100755 index 64c56d4..b792950
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -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;
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-06 2:47 ` Bruce Zhang
@ 2017-06-06 20:00 ` Jacek Anaszewski
-1 siblings, 0 replies; 21+ messages in thread
From: Jacek Anaszewski @ 2017-06-06 20:00 UTC (permalink / raw)
To: Bruce Zhang, linux-arm-kernel; +Cc: pavel, Linux LED Subsystem
Hi Bo,
On 06/06/2017 04:47 AM, Bruce Zhang wrote:
> 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.
> -----Original Message-----
> From: Jacek Anaszewski [mailto:jacek.anaszewski@gmail.com]
> Sent: Tuesday, June 06, 2017 3:27 AM
> To: Bruce Zhang <bo.zhang@nxp.com>; linux-arm-kernel@lists.infradead.org
> Cc: pavel@ucw.cz; Linux LED Subsystem <linux-leds@vger.kernel.org>
> 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@vger.kernel.org when modifying LED subsystem.
>
> On 06/05/2017 09:36 AM, Zhang Bo wrote:
>> 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 <bo.zhang@nxp.com>
>> ---
>> 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.
>
>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>> old mode 100644 new mode 100755 index 431123b..1cb1674
>> --- a/drivers/leds/led-triggers.c
>> +++ b/drivers/leds/led-triggers.c
>> @@ -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);
>
> Could you share what kind of problem this call to led_syfs_disable() solves? Did you come across one?
>
>> + }
>> + 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;
>> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c
>> b/drivers/leds/trigger/ledtrig-heartbeat.c
>> old mode 100644
>> new mode 100755
>> index afa3b40..8f779dd
>> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
>> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
>> @@ -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;
>> diff --git a/include/linux/leds.h b/include/linux/leds.h old mode
>> 100644 new mode 100755 index 64c56d4..b792950
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -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;
>>
>
> --
> Best regards,
> Jacek Anaszewski
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
@ 2017-06-06 20:00 ` Jacek Anaszewski
0 siblings, 0 replies; 21+ messages in thread
From: Jacek Anaszewski @ 2017-06-06 20:00 UTC (permalink / raw)
To: linux-arm-kernel
Hi Bo,
On 06/06/2017 04:47 AM, Bruce Zhang wrote:
> 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.
> -----Original Message-----
> From: Jacek Anaszewski [mailto:jacek.anaszewski at gmail.com]
> Sent: Tuesday, June 06, 2017 3:27 AM
> To: Bruce Zhang <bo.zhang@nxp.com>; linux-arm-kernel at lists.infradead.org
> Cc: pavel at ucw.cz; Linux LED Subsystem <linux-leds@vger.kernel.org>
> 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.
>
> On 06/05/2017 09:36 AM, Zhang Bo wrote:
>> 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 <bo.zhang@nxp.com>
>> ---
>> 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.
>
>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>> old mode 100644 new mode 100755 index 431123b..1cb1674
>> --- a/drivers/leds/led-triggers.c
>> +++ b/drivers/leds/led-triggers.c
>> @@ -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);
>
> Could you share what kind of problem this call to led_syfs_disable() solves? Did you come across one?
>
>> + }
>> + 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;
>> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c
>> b/drivers/leds/trigger/ledtrig-heartbeat.c
>> old mode 100644
>> new mode 100755
>> index afa3b40..8f779dd
>> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
>> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
>> @@ -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;
>> diff --git a/include/linux/leds.h b/include/linux/leds.h old mode
>> 100644 new mode 100755 index 64c56d4..b792950
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -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;
>>
>
> --
> Best regards,
> Jacek Anaszewski
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-05 7:36 [[PATCH]] drivers: leds/trigger: system cannot enter suspend Zhang Bo
2017-06-05 19:27 ` Jacek Anaszewski
2017-06-05 20:05 ` Pavel Machek
@ 2017-06-09 11:25 ` Linus Walleij
2017-06-09 13:01 ` Bruce Zhang
2 siblings, 1 reply; 21+ messages in thread
From: Linus Walleij @ 2017-06-09 11:25 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 5, 2017 at 9:36 AM, Zhang Bo <bo.zhang@nxp.com> wrote:
> System cannot enter suspend mode because of heartbeat led trigger.
As mentioned this should not be a problem.
> 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 <bo.zhang@nxp.com>
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")
(...)
> +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;
> }
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.
> @@ -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,
(...)
> + 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-09 11:25 ` Linus Walleij
@ 2017-06-09 13:01 ` Bruce Zhang
0 siblings, 0 replies; 21+ messages in thread
From: Bruce Zhang @ 2017-06-09 13:01 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-kernel, Jacek Anaszewski, Pavel Machek, Ulf Hansson,
Richard Purdie, linux-leds
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@linaro.org]
Sent: Friday, June 09, 2017 7:25 PM
To: Bruce Zhang <bo.zhang@nxp.com>
Cc: linux-arm-kernel@lists.infradead.org; Jacek Anaszewski <jacek.anaszewski@gmail.com>; Pavel Machek <pavel@ucw.cz>
Subject: Re: [[PATCH]] drivers: leds/trigger: system cannot enter suspend
On Mon, Jun 5, 2017 at 9:36 AM, Zhang Bo <bo.zhang@nxp.com> wrote:
> System cannot enter suspend mode because of heartbeat led trigger.
As mentioned this should not be a problem.
> 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 <bo.zhang@nxp.com>
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")
(...)
> +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;
> }
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.
> @@ -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,
(...)
> + 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
@ 2017-06-09 13:01 ` Bruce Zhang
0 siblings, 0 replies; 21+ messages in thread
From: Bruce Zhang @ 2017-06-09 13:01 UTC (permalink / raw)
To: linux-arm-kernel
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 <bo.zhang@nxp.com>
Cc: linux-arm-kernel at lists.infradead.org; Jacek Anaszewski <jacek.anaszewski@gmail.com>; Pavel Machek <pavel@ucw.cz>
Subject: Re: [[PATCH]] drivers: leds/trigger: system cannot enter suspend
On Mon, Jun 5, 2017 at 9:36 AM, Zhang Bo <bo.zhang@nxp.com> wrote:
> System cannot enter suspend mode because of heartbeat led trigger.
As mentioned this should not be a problem.
> 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 <bo.zhang@nxp.com>
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")
(...)
> +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;
> }
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.
> @@ -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,
(...)
> + 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-07 15:38 ` Grygorii Strashko
@ 2017-06-09 22:16 ` Pavel Machek
0 siblings, 0 replies; 21+ messages in thread
From: Pavel Machek @ 2017-06-09 22:16 UTC (permalink / raw)
To: linux-arm-kernel
On Wed 2017-06-07 10:38:47, Grygorii Strashko wrote:
>
>
> On 06/06/2017 11:24 PM, Bruce Zhang wrote:
> > 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.
Yes, trigger keeps running. It should not be a problem... right?
Pavel
--
(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>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-07 4:24 ` Bruce Zhang
@ 2017-06-07 15:38 ` Grygorii Strashko
2017-06-09 22:16 ` Pavel Machek
0 siblings, 1 reply; 21+ messages in thread
From: Grygorii Strashko @ 2017-06-07 15:38 UTC (permalink / raw)
To: linux-arm-kernel
On 06/06/2017 11:24 PM, Bruce Zhang wrote:
> 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.
>
> On Tue 2017-06-06 15:19:23, Grygorii Strashko wrote:
>>
>>
>> On 06/06/2017 03:05 PM, Jacek Anaszewski wrote:
>>> On 06/06/2017 09:25 PM, Pavel Machek wrote:
>>>> On Tue 2017-06-06 10:36:36, Zhang Bo wrote:
>>>>> 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.
>>>
>>
>> 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
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-06 20:38 ` Pavel Machek
@ 2017-06-07 4:24 ` Bruce Zhang
2017-06-07 15:38 ` Grygorii Strashko
0 siblings, 1 reply; 21+ messages in thread
From: Bruce Zhang @ 2017-06-07 4:24 UTC (permalink / raw)
To: linux-arm-kernel
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 <grygorii.strashko@ti.com>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>; Bruce Zhang <bo.zhang@nxp.com>; Linus Walleij <linus.walleij@linaro.org>; linux-arm-kernel at lists.infradead.org
Subject: Re: [[PATCH]] drivers: leds/trigger: system cannot enter suspend
On Tue 2017-06-06 15:19:23, Grygorii Strashko wrote:
>
>
> On 06/06/2017 03:05 PM, Jacek Anaszewski wrote:
> > On 06/06/2017 09:25 PM, Pavel Machek wrote:
> >> On Tue 2017-06-06 10:36:36, Zhang Bo wrote:
> >>> 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.
> >
>
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-06 20:19 ` Grygorii Strashko
2017-06-06 20:34 ` Pavel Machek
@ 2017-06-06 20:38 ` Pavel Machek
2017-06-07 4:24 ` Bruce Zhang
1 sibling, 1 reply; 21+ messages in thread
From: Pavel Machek @ 2017-06-06 20:38 UTC (permalink / raw)
To: linux-arm-kernel
On Tue 2017-06-06 15:19:23, Grygorii Strashko wrote:
>
>
> On 06/06/2017 03:05 PM, Jacek Anaszewski wrote:
> > On 06/06/2017 09:25 PM, Pavel Machek wrote:
> >> On Tue 2017-06-06 10:36:36, Zhang Bo wrote:
> >>> 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.
> >
>
> 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
-------------- 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>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-06 20:19 ` Grygorii Strashko
@ 2017-06-06 20:34 ` Pavel Machek
2017-06-06 20:38 ` Pavel Machek
1 sibling, 0 replies; 21+ messages in thread
From: Pavel Machek @ 2017-06-06 20:34 UTC (permalink / raw)
To: linux-arm-kernel
On Tue 2017-06-06 15:19:23, Grygorii Strashko wrote:
>
>
> On 06/06/2017 03:05 PM, Jacek Anaszewski wrote:
> > On 06/06/2017 09:25 PM, Pavel Machek wrote:
> >> On Tue 2017-06-06 10:36:36, Zhang Bo wrote:
> >>> 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.
> >
>
> 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.
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
--
(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>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-06 20:05 ` Jacek Anaszewski
@ 2017-06-06 20:19 ` Grygorii Strashko
2017-06-06 20:34 ` Pavel Machek
2017-06-06 20:38 ` Pavel Machek
0 siblings, 2 replies; 21+ messages in thread
From: Grygorii Strashko @ 2017-06-06 20:19 UTC (permalink / raw)
To: linux-arm-kernel
On 06/06/2017 03:05 PM, Jacek Anaszewski wrote:
> On 06/06/2017 09:25 PM, Pavel Machek wrote:
>> On Tue 2017-06-06 10:36:36, Zhang Bo wrote:
>>> 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.
>
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.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-06 19:25 ` Pavel Machek
@ 2017-06-06 20:05 ` Jacek Anaszewski
2017-06-06 20:19 ` Grygorii Strashko
0 siblings, 1 reply; 21+ messages in thread
From: Jacek Anaszewski @ 2017-06-06 20:05 UTC (permalink / raw)
To: linux-arm-kernel
On 06/06/2017 09:25 PM, Pavel Machek wrote:
> On Tue 2017-06-06 10:36:36, Zhang Bo wrote:
>> 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.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
2017-06-06 2:36 Zhang Bo
@ 2017-06-06 19:25 ` Pavel Machek
2017-06-06 20:05 ` Jacek Anaszewski
0 siblings, 1 reply; 21+ messages in thread
From: Pavel Machek @ 2017-06-06 19:25 UTC (permalink / raw)
To: linux-arm-kernel
On Tue 2017-06-06 10:36:36, Zhang Bo wrote:
> 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
--
(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>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [[PATCH]] drivers: leds/trigger: system cannot enter suspend
@ 2017-06-06 2:36 Zhang Bo
2017-06-06 19:25 ` Pavel Machek
0 siblings, 1 reply; 21+ messages in thread
From: Zhang Bo @ 2017-06-06 2:36 UTC (permalink / raw)
To: linux-arm-kernel
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 <bo.zhang@nxp.com>
---
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(-)
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 431123b..1cb1674 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -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;
diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
index afa3b40..8f779dd 100644
--- a/drivers/leds/trigger/ledtrig-heartbeat.c
+++ b/drivers/leds/trigger/ledtrig-heartbeat.c
@@ -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;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 64c56d4..b792950 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -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;
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-06-09 22:16 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-05 7:36 [[PATCH]] drivers: leds/trigger: system cannot enter suspend Zhang Bo
2017-06-05 19:27 ` Jacek Anaszewski
2017-06-05 19:27 ` Jacek Anaszewski
2017-06-06 2:47 ` Bruce Zhang
2017-06-06 2:47 ` Bruce Zhang
2017-06-06 20:00 ` Jacek Anaszewski
2017-06-06 20:00 ` Jacek Anaszewski
2017-06-05 20:05 ` Pavel Machek
2017-06-06 2:11 ` Bruce Zhang
2017-06-09 11:25 ` Linus Walleij
2017-06-09 13:01 ` Bruce Zhang
2017-06-09 13:01 ` Bruce Zhang
2017-06-06 2:36 Zhang Bo
2017-06-06 19:25 ` Pavel Machek
2017-06-06 20:05 ` Jacek Anaszewski
2017-06-06 20:19 ` Grygorii Strashko
2017-06-06 20:34 ` Pavel Machek
2017-06-06 20:38 ` Pavel Machek
2017-06-07 4:24 ` Bruce Zhang
2017-06-07 15:38 ` Grygorii Strashko
2017-06-09 22:16 ` Pavel Machek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.