| Message ID | 1496648191-134355-1-git-send-email-bo.zhang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
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
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@ucw.cz] Sent: Tuesday, June 06, 2017 4:06 AM To: Bruce Zhang <bo.zhang@nxp.com> Cc: linux-arm-kernel@lists.infradead.org; jacek.anaszewski@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
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; > -- Best regards, Jacek Anaszewski
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 >
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
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
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;
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