[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Zhang Bo]

System cannot enter suspend mode because of heartbeat led trigger.
In autosleep_wq, try_to_suspend function will try to enter suspend
mode in specific period. it will get wakeup_count then call pm_notifier
chain callback function and freeze processes.
Heartbeat_pm_notifier is called and it call led_trigger_unregister to
change the trigger of led device to none. It will send uevent 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              | 30 +++++++++++++++++++++++++++
 drivers/leds/trigger/ledtrig-heartbeat.c | 35 ++++++++++++++++++++++++++------
 include/linux/leds.h                     |  6 ++++++
 3 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 431123b..b239907 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -333,6 +333,36 @@ 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);
+	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);
+	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

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Zhang Bo]

System cannot enter suspend mode because of heartbeat led trigger.
In autosleep_wq, try_to_suspend function will try to enter suspend
mode in specific period. it will get wakeup_count then call pm_notifier
chain callback function and freeze processes.
Heartbeat_pm_notifier is called and it call led_trigger_unregister to
change the trigger of led device to none. It will send uevent 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              | 30 +++++++++++++++++++++++++++
 drivers/leds/trigger/ledtrig-heartbeat.c | 35 ++++++++++++++++++++++++++------
 include/linux/leds.h                     |  6 ++++++
 3 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 431123b..b239907 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -333,6 +333,36 @@ 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);
+	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);
+	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

Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]

On Wed 2017-06-07 10:50:10, 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 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>

NAK.

led_trigger should not need special handling over suspend.

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

Yes, timers can and should be active over suspend.

Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over
the issue for one trigger, but we have more than one... Even if
special handling for heartbeat is warranted, that handling would be
"turn the led off" not "unregister the trigger", which is
userland-visible action.) That one should be reverted, then maybe the
driver should be fixed to turn the led off during suspend.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Pavel Machek]

On Wed 2017-06-07 10:50:10, 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 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>

NAK.

led_trigger should not need special handling over suspend.

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

Yes, timers can and should be active over suspend.

Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over
the issue for one trigger, but we have more than one... Even if
special handling for heartbeat is warranted, that handling would be
"turn the led off" not "unregister the trigger", which is
userland-visible action.) That one should be reverted, then maybe the
driver should be fixed to turn the led off during 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/20170607/7171a591/attachment.sig>

Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Ulf Hansson]

On 7 June 2017 at 09:12, Pavel Machek <pavel@ucw.cz> wrote:
> On Wed 2017-06-07 10:50:10, 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 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>
>
> NAK.
>
> led_trigger should not need special handling over suspend.
>
>> 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.
>
> Yes, timers can and should be active over suspend.
>
> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over
> the issue for one trigger, but we have more than one... Even if
> special handling for heartbeat is warranted, that handling would be
> "turn the led off" not "unregister the trigger", which is
> userland-visible action.) That one should be reverted, then maybe the
> driver should be fixed to turn the led off during suspend.

I fully agree! However, I haven't yet figured out exactly how to solve
it yet. :-)

FYI, the same problem exist to for example ledtrig-cpu, which uses a
sycore_ops do deal with suspend/resume.
I think that is way too late, especially since it's not uncommon that
an i2c transfer needs to be done to change the led. Then as i2c is
suspended far earlier than when entering the syscore stages in
suspend, it breaks.

Kind regards
Uffe

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Ulf Hansson]

On 7 June 2017 at 09:12, Pavel Machek <pavel@ucw.cz> wrote:
> On Wed 2017-06-07 10:50:10, 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 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>
>
> NAK.
>
> led_trigger should not need special handling over suspend.
>
>> 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.
>
> Yes, timers can and should be active over suspend.
>
> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over
> the issue for one trigger, but we have more than one... Even if
> special handling for heartbeat is warranted, that handling would be
> "turn the led off" not "unregister the trigger", which is
> userland-visible action.) That one should be reverted, then maybe the
> driver should be fixed to turn the led off during suspend.

I fully agree! However, I haven't yet figured out exactly how to solve
it yet. :-)

FYI, the same problem exist to for example ledtrig-cpu, which uses a
sycore_ops do deal with suspend/resume.
I think that is way too late, especially since it's not uncommon that
an i2c transfer needs to be done to change the led. Then as i2c is
suspended far earlier than when entering the syscore stages in
suspend, it breaks.

Kind regards
Uffe

Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Linus Walleij]

On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:

> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over
> the issue for one trigger, but we have more than one... Even if
> special handling for heartbeat is warranted, that handling would be
> "turn the led off" not "unregister the trigger", which is
> userland-visible action.)

OK yeah I guess you're right.

I just couldn't think about anything better and didn't get any
review at the time, so mistakes were made.

> That one should be reverted, then maybe the
> driver should be fixed to turn the led off during suspend.

Do you mean that the heartbeat trigger driver should be fixed to
turn off the LED during sleep?

That is essentially what I was trying to achieve.

The reason it is done as it is, is that the trigger sets up
a timer to do its job, and the timer may trigger between
the point you turn off the LED and the system really goes
to suspend, again maybe turning the LED on and
again leaving the system with a glowing LED at suspend.

The patch also solves the following phenomenon, sorry
for not writing in the commit:

- Turn off LED
- Suspend I2C hardware
- Timer trigger
- Trying to blink the LED using I2C
- Crap in the console about the failed I2C transaction
- Actual suspen happens
- System comes online
- Trying to blink the LED using I2C
- Crap in the console about the failed I2C transaction
- Resume I2C hardware

It's just very fragile this trigger, turning off the LED from
the PM notifier is obviously not enough, we also need to
disable the timer, and then take it back online after resume.

That is the tricky part, which is simplified by just removing
and adding back the trigger.

Yours,
Linus Walleij

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Linus Walleij]

On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:

> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over
> the issue for one trigger, but we have more than one... Even if
> special handling for heartbeat is warranted, that handling would be
> "turn the led off" not "unregister the trigger", which is
> userland-visible action.)

OK yeah I guess you're right.

I just couldn't think about anything better and didn't get any
review at the time, so mistakes were made.

> That one should be reverted, then maybe the
> driver should be fixed to turn the led off during suspend.

Do you mean that the heartbeat trigger driver should be fixed to
turn off the LED during sleep?

That is essentially what I was trying to achieve.

The reason it is done as it is, is that the trigger sets up
a timer to do its job, and the timer may trigger between
the point you turn off the LED and the system really goes
to suspend, again maybe turning the LED on and
again leaving the system with a glowing LED at suspend.

The patch also solves the following phenomenon, sorry
for not writing in the commit:

- Turn off LED
- Suspend I2C hardware
- Timer trigger
- Trying to blink the LED using I2C
- Crap in the console about the failed I2C transaction
- Actual suspen happens
- System comes online
- Trying to blink the LED using I2C
- Crap in the console about the failed I2C transaction
- Resume I2C hardware

It's just very fragile this trigger, turning off the LED from
the PM notifier is obviously not enough, we also need to
disable the timer, and then take it back online after resume.

That is the tricky part, which is simplified by just removing
and adding back the trigger.

Yours,
Linus Walleij

Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2539 bytes --]

Hi!

On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
> 
> > Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over
> > the issue for one trigger, but we have more than one... Even if
> > special handling for heartbeat is warranted, that handling would be
> > "turn the led off" not "unregister the trigger", which is
> > userland-visible action.)
> 
> OK yeah I guess you're right.
> 
> I just couldn't think about anything better and didn't get any
> review at the time, so mistakes were made.
> 
> > That one should be reverted, then maybe the
> > driver should be fixed to turn the led off during suspend.
> 
> Do you mean that the heartbeat trigger driver should be fixed to
> turn off the LED during sleep?
> 
> That is essentially what I was trying to achieve.

I don't think we should be fixing it at trigger level.

If userspace keeps blinking using "brightness" attribute, would we
like the LED to be off during suspend? I think so.

> The reason it is done as it is, is that the trigger sets up
> a timer to do its job, and the timer may trigger between
> the point you turn off the LED and the system really goes
> to suspend, again maybe turning the LED on and
> again leaving the system with a glowing LED at suspend.

> The patch also solves the following phenomenon, sorry
> for not writing in the commit:
> 
> - Turn off LED
> - Suspend I2C hardware
> - Timer trigger
> - Trying to blink the LED using I2C
> - Crap in the console about the failed I2C transaction
> - Actual suspen happens
> - System comes online
> - Trying to blink the LED using I2C
> - Crap in the console about the failed I2C transaction
> - Resume I2C hardware
> 
> It's just very fragile this trigger, turning off the LED from
> the PM notifier is obviously not enough, we also need to
> disable the timer, and then take it back online after resume.

No, leave the timer alone, and actually leave the trigger alone.

Simply make the driver turn the LED off in .suspend() callback, and
then ignore further requests until .resume(). That will get rid of
power drain _and_ "failed I2C transaction" messages, right?

(Actually, I tested with the heartbeat trigger, and it is not
re-installed after resume. Another reason to revert the patch).

Thanks,

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Pavel Machek]

Hi!

On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
> 
> > Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over
> > the issue for one trigger, but we have more than one... Even if
> > special handling for heartbeat is warranted, that handling would be
> > "turn the led off" not "unregister the trigger", which is
> > userland-visible action.)
> 
> OK yeah I guess you're right.
> 
> I just couldn't think about anything better and didn't get any
> review at the time, so mistakes were made.
> 
> > That one should be reverted, then maybe the
> > driver should be fixed to turn the led off during suspend.
> 
> Do you mean that the heartbeat trigger driver should be fixed to
> turn off the LED during sleep?
> 
> That is essentially what I was trying to achieve.

I don't think we should be fixing it at trigger level.

If userspace keeps blinking using "brightness" attribute, would we
like the LED to be off during suspend? I think so.

> The reason it is done as it is, is that the trigger sets up
> a timer to do its job, and the timer may trigger between
> the point you turn off the LED and the system really goes
> to suspend, again maybe turning the LED on and
> again leaving the system with a glowing LED at suspend.

> The patch also solves the following phenomenon, sorry
> for not writing in the commit:
> 
> - Turn off LED
> - Suspend I2C hardware
> - Timer trigger
> - Trying to blink the LED using I2C
> - Crap in the console about the failed I2C transaction
> - Actual suspen happens
> - System comes online
> - Trying to blink the LED using I2C
> - Crap in the console about the failed I2C transaction
> - Resume I2C hardware
> 
> It's just very fragile this trigger, turning off the LED from
> the PM notifier is obviously not enough, we also need to
> disable the timer, and then take it back online after resume.

No, leave the timer alone, and actually leave the trigger alone.

Simply make the driver turn the LED off in .suspend() callback, and
then ignore further requests until .resume(). That will get rid of
power drain _and_ "failed I2C transaction" messages, right?

(Actually, I tested with the heartbeat trigger, and it is not
re-installed after resume. Another reason to revert the patch).

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/20170607/2879506a/attachment.sig>

RE: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Bruce Zhang]

Hi,

As suspend/resume procedure is as below:
pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.

Best Regards,
Bo

-----Original Message-----
From: Pavel Machek [mailto:pavel@ucw.cz] 
Sent: Wednesday, June 07, 2017 6:31 PM
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Jacek Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

Hi!

On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
> 
> > Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
> > the issue for one trigger, but we have more than one... Even if 
> > special handling for heartbeat is warranted, that handling would be 
> > "turn the led off" not "unregister the trigger", which is 
> > userland-visible action.)
> 
> OK yeah I guess you're right.
> 
> I just couldn't think about anything better and didn't get any review 
> at the time, so mistakes were made.
> 
> > That one should be reverted, then maybe the driver should be fixed 
> > to turn the led off during suspend.
> 
> Do you mean that the heartbeat trigger driver should be fixed to turn 
> off the LED during sleep?
> 
> That is essentially what I was trying to achieve.

I don't think we should be fixing it at trigger level.

If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.

> The reason it is done as it is, is that the trigger sets up a timer to 
> do its job, and the timer may trigger between the point you turn off 
> the LED and the system really goes to suspend, again maybe turning the 
> LED on and again leaving the system with a glowing LED at suspend.

> The patch also solves the following phenomenon, sorry for not writing 
> in the commit:
> 
> - Turn off LED
> - Suspend I2C hardware
> - Timer trigger
> - Trying to blink the LED using I2C
> - Crap in the console about the failed I2C transaction
> - Actual suspen happens
> - System comes online
> - Trying to blink the LED using I2C
> - Crap in the console about the failed I2C transaction
> - Resume I2C hardware
> 
> It's just very fragile this trigger, turning off the LED from the PM 
> notifier is obviously not enough, we also need to disable the timer, 
> and then take it back online after resume.

No, leave the timer alone, and actually leave the trigger alone.

Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?

(Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).

Thanks,

									Pavel

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Bruce Zhang]

Hi,

As suspend/resume procedure is as below:
pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.

Best Regards,
Bo

-----Original Message-----
From: Pavel Machek [mailto:pavel at ucw.cz] 
Sent: Wednesday, June 07, 2017 6:31 PM
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel at lists.infradead.org; Jacek Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

Hi!

On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
> 
> > Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
> > the issue for one trigger, but we have more than one... Even if 
> > special handling for heartbeat is warranted, that handling would be 
> > "turn the led off" not "unregister the trigger", which is 
> > userland-visible action.)
> 
> OK yeah I guess you're right.
> 
> I just couldn't think about anything better and didn't get any review 
> at the time, so mistakes were made.
> 
> > That one should be reverted, then maybe the driver should be fixed 
> > to turn the led off during suspend.
> 
> Do you mean that the heartbeat trigger driver should be fixed to turn 
> off the LED during sleep?
> 
> That is essentially what I was trying to achieve.

I don't think we should be fixing it at trigger level.

If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.

> The reason it is done as it is, is that the trigger sets up a timer to 
> do its job, and the timer may trigger between the point you turn off 
> the LED and the system really goes to suspend, again maybe turning the 
> LED on and again leaving the system with a glowing LED at suspend.

> The patch also solves the following phenomenon, sorry for not writing 
> in the commit:
> 
> - Turn off LED
> - Suspend I2C hardware
> - Timer trigger
> - Trying to blink the LED using I2C
> - Crap in the console about the failed I2C transaction
> - Actual suspen happens
> - System comes online
> - Trying to blink the LED using I2C
> - Crap in the console about the failed I2C transaction
> - Resume I2C hardware
> 
> It's just very fragile this trigger, turning off the LED from the PM 
> notifier is obviously not enough, we also need to disable the timer, 
> and then take it back online after resume.

No, leave the timer alone, and actually leave the trigger alone.

Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?

(Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).

Thanks,

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

Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Jacek Anaszewski]

Hi Bruce,

On 06/08/2017 04:57 AM, Bruce Zhang wrote:
> Hi,
> 
> As suspend/resume procedure is as below:
> pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
> If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.

OK, so I agree that the most proper fix to the discussed problem
would be ignoring brightness setting requests after suspend, as Pavel
suggested.

led_classdev_suspend() already turns the LED off and
led_classdev_resume() brings the brightness back.

Also led_set_brightness_nosleep() and led_set_brightness_sync()
don't propagate brightness change to the driver if LED_SUSPENDED
is set.

There are two problems though:
1. __led_set_brightness() and __led_set_brightness_blocking() don't
   check the flag, and they can be called from set_brightness_delayed(),
   that was already queued at the time of setting LED_SUSPENDED flag.
2. LED_SUSPENDED flag is accessed in a non-atomic way.

The solution as I see it would be respectively:
1. Return from __led_set_brightness() and
   __led_set_brightness_blocking() if LED_SUSPENDED is set.
2. Switch to accessing struct led_classdev's flags with use of
   bitops. Fortunately those flags are mainly set prior LED class
   device registration and only LED_SYSFS_DISABLE, LED_SUSPENDED
   and LED_UNREGISTERING are accessed afterwards, and only in the LED
   core, so only those places would have to be modified.

Best regards,
Jacek Anaszewski

> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz] 
> Sent: Wednesday, June 07, 2017 6:31 PM
> To: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Jacek Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend
> 
> Hi!
> 
> On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
>> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>
>>> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
>>> the issue for one trigger, but we have more than one... Even if 
>>> special handling for heartbeat is warranted, that handling would be 
>>> "turn the led off" not "unregister the trigger", which is 
>>> userland-visible action.)
>>
>> OK yeah I guess you're right.
>>
>> I just couldn't think about anything better and didn't get any review 
>> at the time, so mistakes were made.
>>
>>> That one should be reverted, then maybe the driver should be fixed 
>>> to turn the led off during suspend.
>>
>> Do you mean that the heartbeat trigger driver should be fixed to turn 
>> off the LED during sleep?
>>
>> That is essentially what I was trying to achieve.
> 
> I don't think we should be fixing it at trigger level.
> 
> If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.
> 
>> The reason it is done as it is, is that the trigger sets up a timer to 
>> do its job, and the timer may trigger between the point you turn off 
>> the LED and the system really goes to suspend, again maybe turning the 
>> LED on and again leaving the system with a glowing LED at suspend.
> 
>> The patch also solves the following phenomenon, sorry for not writing 
>> in the commit:
>>
>> - Turn off LED
>> - Suspend I2C hardware
>> - Timer trigger
>> - Trying to blink the LED using I2C
>> - Crap in the console about the failed I2C transaction
>> - Actual suspen happens
>> - System comes online
>> - Trying to blink the LED using I2C
>> - Crap in the console about the failed I2C transaction
>> - Resume I2C hardware
>>
>> It's just very fragile this trigger, turning off the LED from the PM 
>> notifier is obviously not enough, we also need to disable the timer, 
>> and then take it back online after resume.
> 
> No, leave the timer alone, and actually leave the trigger alone.
> 
> Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?
> 
> (Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).
> 
> Thanks,
> 
> 									Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Jacek Anaszewski]

Hi Bruce,

On 06/08/2017 04:57 AM, Bruce Zhang wrote:
> Hi,
> 
> As suspend/resume procedure is as below:
> pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
> If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.

OK, so I agree that the most proper fix to the discussed problem
would be ignoring brightness setting requests after suspend, as Pavel
suggested.

led_classdev_suspend() already turns the LED off and
led_classdev_resume() brings the brightness back.

Also led_set_brightness_nosleep() and led_set_brightness_sync()
don't propagate brightness change to the driver if LED_SUSPENDED
is set.

There are two problems though:
1. __led_set_brightness() and __led_set_brightness_blocking() don't
   check the flag, and they can be called from set_brightness_delayed(),
   that was already queued at the time of setting LED_SUSPENDED flag.
2. LED_SUSPENDED flag is accessed in a non-atomic way.

The solution as I see it would be respectively:
1. Return from __led_set_brightness() and
   __led_set_brightness_blocking() if LED_SUSPENDED is set.
2. Switch to accessing struct led_classdev's flags with use of
   bitops. Fortunately those flags are mainly set prior LED class
   device registration and only LED_SYSFS_DISABLE, LED_SUSPENDED
   and LED_UNREGISTERING are accessed afterwards, and only in the LED
   core, so only those places would have to be modified.

Best regards,
Jacek Anaszewski

> -----Original Message-----
> From: Pavel Machek [mailto:pavel at ucw.cz] 
> Sent: Wednesday, June 07, 2017 6:31 PM
> To: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel at lists.infradead.org; Jacek Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend
> 
> Hi!
> 
> On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
>> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>
>>> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
>>> the issue for one trigger, but we have more than one... Even if 
>>> special handling for heartbeat is warranted, that handling would be 
>>> "turn the led off" not "unregister the trigger", which is 
>>> userland-visible action.)
>>
>> OK yeah I guess you're right.
>>
>> I just couldn't think about anything better and didn't get any review 
>> at the time, so mistakes were made.
>>
>>> That one should be reverted, then maybe the driver should be fixed 
>>> to turn the led off during suspend.
>>
>> Do you mean that the heartbeat trigger driver should be fixed to turn 
>> off the LED during sleep?
>>
>> That is essentially what I was trying to achieve.
> 
> I don't think we should be fixing it at trigger level.
> 
> If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.
> 
>> The reason it is done as it is, is that the trigger sets up a timer to 
>> do its job, and the timer may trigger between the point you turn off 
>> the LED and the system really goes to suspend, again maybe turning the 
>> LED on and again leaving the system with a glowing LED at suspend.
> 
>> The patch also solves the following phenomenon, sorry for not writing 
>> in the commit:
>>
>> - Turn off LED
>> - Suspend I2C hardware
>> - Timer trigger
>> - Trying to blink the LED using I2C
>> - Crap in the console about the failed I2C transaction
>> - Actual suspen happens
>> - System comes online
>> - Trying to blink the LED using I2C
>> - Crap in the console about the failed I2C transaction
>> - Resume I2C hardware
>>
>> It's just very fragile this trigger, turning off the LED from the PM 
>> notifier is obviously not enough, we also need to disable the timer, 
>> and then take it back online after resume.
> 
> No, leave the timer alone, and actually leave the trigger alone.
> 
> Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?
> 
> (Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).
> 
> Thanks,
> 
> 									Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 

RE: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Bruce Zhang]

Hi Jacek,

I think that no change need to make for __led_set_brightness() and __led_set_brightness_blocking() function. Because led_classdev_suspend() need to turn off LED with these API. set_brightness_delayed() is the same level API as __led_set_brightness(). No need to add LED_SUSPENDED checking as well.

Best Regards,
Bo

-----Original Message-----
From: Jacek Anaszewski [mailto:jacek.anaszewski@gmail.com] 
Sent: Friday, June 09, 2017 4:28 AM
To: Bruce Zhang <bo.zhang@nxp.com>; Pavel Machek <pavel@ucw.cz>; Linus Walleij <linus.walleij@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Richard Purdie <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

Hi Bruce,

On 06/08/2017 04:57 AM, Bruce Zhang wrote:
> Hi,
> 
> As suspend/resume procedure is as below:
> pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
> If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.

OK, so I agree that the most proper fix to the discussed problem would be ignoring brightness setting requests after suspend, as Pavel suggested.

led_classdev_suspend() already turns the LED off and
led_classdev_resume() brings the brightness back.

Also led_set_brightness_nosleep() and led_set_brightness_sync() don't propagate brightness change to the driver if LED_SUSPENDED is set.

There are two problems though:
1. __led_set_brightness() and __led_set_brightness_blocking() don't
   check the flag, and they can be called from set_brightness_delayed(),
   that was already queued at the time of setting LED_SUSPENDED flag.
2. LED_SUSPENDED flag is accessed in a non-atomic way.

The solution as I see it would be respectively:
1. Return from __led_set_brightness() and
   __led_set_brightness_blocking() if LED_SUSPENDED is set.
2. Switch to accessing struct led_classdev's flags with use of
   bitops. Fortunately those flags are mainly set prior LED class
   device registration and only LED_SYSFS_DISABLE, LED_SUSPENDED
   and LED_UNREGISTERING are accessed afterwards, and only in the LED
   core, so only those places would have to be modified.

Best regards,
Jacek Anaszewski

> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> Sent: Wednesday, June 07, 2017 6:31 PM
> To: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson 
> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Jacek 
> Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie 
> <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter 
> suspend
> 
> Hi!
> 
> On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
>> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>
>>> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
>>> the issue for one trigger, but we have more than one... Even if 
>>> special handling for heartbeat is warranted, that handling would be 
>>> "turn the led off" not "unregister the trigger", which is 
>>> userland-visible action.)
>>
>> OK yeah I guess you're right.
>>
>> I just couldn't think about anything better and didn't get any review 
>> at the time, so mistakes were made.
>>
>>> That one should be reverted, then maybe the driver should be fixed 
>>> to turn the led off during suspend.
>>
>> Do you mean that the heartbeat trigger driver should be fixed to turn 
>> off the LED during sleep?
>>
>> That is essentially what I was trying to achieve.
> 
> I don't think we should be fixing it at trigger level.
> 
> If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.
> 
>> The reason it is done as it is, is that the trigger sets up a timer 
>> to do its job, and the timer may trigger between the point you turn 
>> off the LED and the system really goes to suspend, again maybe 
>> turning the LED on and again leaving the system with a glowing LED at suspend.
> 
>> The patch also solves the following phenomenon, sorry for not writing 
>> in the commit:
>>
>> - Turn off LED
>> - Suspend I2C hardware
>> - Timer trigger
>> - Trying to blink the LED using I2C
>> - Crap in the console about the failed I2C transaction
>> - Actual suspen happens
>> - System comes online
>> - Trying to blink the LED using I2C
>> - Crap in the console about the failed I2C transaction
>> - Resume I2C hardware
>>
>> It's just very fragile this trigger, turning off the LED from the PM 
>> notifier is obviously not enough, we also need to disable the timer, 
>> and then take it back online after resume.
> 
> No, leave the timer alone, and actually leave the trigger alone.
> 
> Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?
> 
> (Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).
> 
> Thanks,
> 
> 									Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Bruce Zhang]

Hi Jacek,

I think that no change need to make for __led_set_brightness() and __led_set_brightness_blocking() function. Because led_classdev_suspend() need to turn off LED with these API. set_brightness_delayed() is the same level API as __led_set_brightness(). No need to add LED_SUSPENDED checking as well.

Best Regards,
Bo

-----Original Message-----
From: Jacek Anaszewski [mailto:jacek.anaszewski at gmail.com] 
Sent: Friday, June 09, 2017 4:28 AM
To: Bruce Zhang <bo.zhang@nxp.com>; Pavel Machek <pavel@ucw.cz>; Linus Walleij <linus.walleij@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel at lists.infradead.org; Richard Purdie <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

Hi Bruce,

On 06/08/2017 04:57 AM, Bruce Zhang wrote:
> Hi,
> 
> As suspend/resume procedure is as below:
> pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
> If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.

OK, so I agree that the most proper fix to the discussed problem would be ignoring brightness setting requests after suspend, as Pavel suggested.

led_classdev_suspend() already turns the LED off and
led_classdev_resume() brings the brightness back.

Also led_set_brightness_nosleep() and led_set_brightness_sync() don't propagate brightness change to the driver if LED_SUSPENDED is set.

There are two problems though:
1. __led_set_brightness() and __led_set_brightness_blocking() don't
   check the flag, and they can be called from set_brightness_delayed(),
   that was already queued at the time of setting LED_SUSPENDED flag.
2. LED_SUSPENDED flag is accessed in a non-atomic way.

The solution as I see it would be respectively:
1. Return from __led_set_brightness() and
   __led_set_brightness_blocking() if LED_SUSPENDED is set.
2. Switch to accessing struct led_classdev's flags with use of
   bitops. Fortunately those flags are mainly set prior LED class
   device registration and only LED_SYSFS_DISABLE, LED_SUSPENDED
   and LED_UNREGISTERING are accessed afterwards, and only in the LED
   core, so only those places would have to be modified.

Best regards,
Jacek Anaszewski

> -----Original Message-----
> From: Pavel Machek [mailto:pavel at ucw.cz]
> Sent: Wednesday, June 07, 2017 6:31 PM
> To: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson 
> <ulf.hansson@linaro.org>; linux-arm-kernel at lists.infradead.org; Jacek 
> Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie 
> <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter 
> suspend
> 
> Hi!
> 
> On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
>> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>
>>> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
>>> the issue for one trigger, but we have more than one... Even if 
>>> special handling for heartbeat is warranted, that handling would be 
>>> "turn the led off" not "unregister the trigger", which is 
>>> userland-visible action.)
>>
>> OK yeah I guess you're right.
>>
>> I just couldn't think about anything better and didn't get any review 
>> at the time, so mistakes were made.
>>
>>> That one should be reverted, then maybe the driver should be fixed 
>>> to turn the led off during suspend.
>>
>> Do you mean that the heartbeat trigger driver should be fixed to turn 
>> off the LED during sleep?
>>
>> That is essentially what I was trying to achieve.
> 
> I don't think we should be fixing it at trigger level.
> 
> If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.
> 
>> The reason it is done as it is, is that the trigger sets up a timer 
>> to do its job, and the timer may trigger between the point you turn 
>> off the LED and the system really goes to suspend, again maybe 
>> turning the LED on and again leaving the system with a glowing LED at suspend.
> 
>> The patch also solves the following phenomenon, sorry for not writing 
>> in the commit:
>>
>> - Turn off LED
>> - Suspend I2C hardware
>> - Timer trigger
>> - Trying to blink the LED using I2C
>> - Crap in the console about the failed I2C transaction
>> - Actual suspen happens
>> - System comes online
>> - Trying to blink the LED using I2C
>> - Crap in the console about the failed I2C transaction
>> - Resume I2C hardware
>>
>> It's just very fragile this trigger, turning off the LED from the PM 
>> notifier is obviously not enough, we also need to disable the timer, 
>> and then take it back online after resume.
> 
> No, leave the timer alone, and actually leave the trigger alone.
> 
> Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?
> 
> (Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).
> 
> Thanks,
> 
> 									Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 

Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Jacek Anaszewski]

Hi Bruce,

On 06/09/2017 04:57 AM, Bruce Zhang wrote:
> Hi Jacek,
> 
> I think that no change need to make for __led_set_brightness() and __led_set_brightness_blocking() function. Because led_classdev_suspend() need to turn off LED with these API. set_brightness_delayed() is the same level API as __led_set_brightness(). No need to add LED_SUSPENDED checking as well.

Indeed, it would be even harmful with the current sequence of
instructions in the led_classdev_suspend().

So, the only action to take is to revert the commit 5ab92a7cb8.

Would you mind submitting the patch so that everyone interested
could give their ack?

Best regards,
Jacek Anaszewski

> -----Original Message-----
> From: Jacek Anaszewski [mailto:jacek.anaszewski@gmail.com] 
> Sent: Friday, June 09, 2017 4:28 AM
> To: Bruce Zhang <bo.zhang@nxp.com>; Pavel Machek <pavel@ucw.cz>; Linus Walleij <linus.walleij@linaro.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Richard Purdie <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend
> 
> Hi Bruce,
> 
> On 06/08/2017 04:57 AM, Bruce Zhang wrote:
>> Hi,
>>
>> As suspend/resume procedure is as below:
>> pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
>> If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.
> 
> OK, so I agree that the most proper fix to the discussed problem would be ignoring brightness setting requests after suspend, as Pavel suggested.
> 
> led_classdev_suspend() already turns the LED off and
> led_classdev_resume() brings the brightness back.
> 
> Also led_set_brightness_nosleep() and led_set_brightness_sync() don't propagate brightness change to the driver if LED_SUSPENDED is set.
> 
> There are two problems though:
> 1. __led_set_brightness() and __led_set_brightness_blocking() don't
>    check the flag, and they can be called from set_brightness_delayed(),
>    that was already queued at the time of setting LED_SUSPENDED flag.
> 2. LED_SUSPENDED flag is accessed in a non-atomic way.
> 
> The solution as I see it would be respectively:
> 1. Return from __led_set_brightness() and
>    __led_set_brightness_blocking() if LED_SUSPENDED is set.
> 2. Switch to accessing struct led_classdev's flags with use of
>    bitops. Fortunately those flags are mainly set prior LED class
>    device registration and only LED_SYSFS_DISABLE, LED_SUSPENDED
>    and LED_UNREGISTERING are accessed afterwards, and only in the LED
>    core, so only those places would have to be modified.
> 
> Best regards,
> Jacek Anaszewski
> 
>> -----Original Message-----
>> From: Pavel Machek [mailto:pavel@ucw.cz]
>> Sent: Wednesday, June 07, 2017 6:31 PM
>> To: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson 
>> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Jacek 
>> Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie 
>> <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
>> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter 
>> suspend
>>
>> Hi!
>>
>> On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
>>> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>>
>>>> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
>>>> the issue for one trigger, but we have more than one... Even if 
>>>> special handling for heartbeat is warranted, that handling would be 
>>>> "turn the led off" not "unregister the trigger", which is 
>>>> userland-visible action.)
>>>
>>> OK yeah I guess you're right.
>>>
>>> I just couldn't think about anything better and didn't get any review 
>>> at the time, so mistakes were made.
>>>
>>>> That one should be reverted, then maybe the driver should be fixed 
>>>> to turn the led off during suspend.
>>>
>>> Do you mean that the heartbeat trigger driver should be fixed to turn 
>>> off the LED during sleep?
>>>
>>> That is essentially what I was trying to achieve.
>>
>> I don't think we should be fixing it at trigger level.
>>
>> If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.
>>
>>> The reason it is done as it is, is that the trigger sets up a timer 
>>> to do its job, and the timer may trigger between the point you turn 
>>> off the LED and the system really goes to suspend, again maybe 
>>> turning the LED on and again leaving the system with a glowing LED at suspend.
>>
>>> The patch also solves the following phenomenon, sorry for not writing 
>>> in the commit:
>>>
>>> - Turn off LED
>>> - Suspend I2C hardware
>>> - Timer trigger
>>> - Trying to blink the LED using I2C
>>> - Crap in the console about the failed I2C transaction
>>> - Actual suspen happens
>>> - System comes online
>>> - Trying to blink the LED using I2C
>>> - Crap in the console about the failed I2C transaction
>>> - Resume I2C hardware
>>>
>>> It's just very fragile this trigger, turning off the LED from the PM 
>>> notifier is obviously not enough, we also need to disable the timer, 
>>> and then take it back online after resume.
>>
>> No, leave the timer alone, and actually leave the trigger alone.
>>
>> Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?
>>
>> (Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).
>>
>> Thanks,
>>
>> 									Pavel
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) 
>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>>
> 
> 

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Jacek Anaszewski]

Hi Bruce,

On 06/09/2017 04:57 AM, Bruce Zhang wrote:
> Hi Jacek,
> 
> I think that no change need to make for __led_set_brightness() and __led_set_brightness_blocking() function. Because led_classdev_suspend() need to turn off LED with these API. set_brightness_delayed() is the same level API as __led_set_brightness(). No need to add LED_SUSPENDED checking as well.

Indeed, it would be even harmful with the current sequence of
instructions in the led_classdev_suspend().

So, the only action to take is to revert the commit 5ab92a7cb8.

Would you mind submitting the patch so that everyone interested
could give their ack?

Best regards,
Jacek Anaszewski

> -----Original Message-----
> From: Jacek Anaszewski [mailto:jacek.anaszewski at gmail.com] 
> Sent: Friday, June 09, 2017 4:28 AM
> To: Bruce Zhang <bo.zhang@nxp.com>; Pavel Machek <pavel@ucw.cz>; Linus Walleij <linus.walleij@linaro.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel at lists.infradead.org; Richard Purdie <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend
> 
> Hi Bruce,
> 
> On 06/08/2017 04:57 AM, Bruce Zhang wrote:
>> Hi,
>>
>> As suspend/resume procedure is as below:
>> pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
>> If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.
> 
> OK, so I agree that the most proper fix to the discussed problem would be ignoring brightness setting requests after suspend, as Pavel suggested.
> 
> led_classdev_suspend() already turns the LED off and
> led_classdev_resume() brings the brightness back.
> 
> Also led_set_brightness_nosleep() and led_set_brightness_sync() don't propagate brightness change to the driver if LED_SUSPENDED is set.
> 
> There are two problems though:
> 1. __led_set_brightness() and __led_set_brightness_blocking() don't
>    check the flag, and they can be called from set_brightness_delayed(),
>    that was already queued at the time of setting LED_SUSPENDED flag.
> 2. LED_SUSPENDED flag is accessed in a non-atomic way.
> 
> The solution as I see it would be respectively:
> 1. Return from __led_set_brightness() and
>    __led_set_brightness_blocking() if LED_SUSPENDED is set.
> 2. Switch to accessing struct led_classdev's flags with use of
>    bitops. Fortunately those flags are mainly set prior LED class
>    device registration and only LED_SYSFS_DISABLE, LED_SUSPENDED
>    and LED_UNREGISTERING are accessed afterwards, and only in the LED
>    core, so only those places would have to be modified.
> 
> Best regards,
> Jacek Anaszewski
> 
>> -----Original Message-----
>> From: Pavel Machek [mailto:pavel at ucw.cz]
>> Sent: Wednesday, June 07, 2017 6:31 PM
>> To: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson 
>> <ulf.hansson@linaro.org>; linux-arm-kernel at lists.infradead.org; Jacek 
>> Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie 
>> <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
>> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter 
>> suspend
>>
>> Hi!
>>
>> On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
>>> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>>
>>>> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
>>>> the issue for one trigger, but we have more than one... Even if 
>>>> special handling for heartbeat is warranted, that handling would be 
>>>> "turn the led off" not "unregister the trigger", which is 
>>>> userland-visible action.)
>>>
>>> OK yeah I guess you're right.
>>>
>>> I just couldn't think about anything better and didn't get any review 
>>> at the time, so mistakes were made.
>>>
>>>> That one should be reverted, then maybe the driver should be fixed 
>>>> to turn the led off during suspend.
>>>
>>> Do you mean that the heartbeat trigger driver should be fixed to turn 
>>> off the LED during sleep?
>>>
>>> That is essentially what I was trying to achieve.
>>
>> I don't think we should be fixing it at trigger level.
>>
>> If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.
>>
>>> The reason it is done as it is, is that the trigger sets up a timer 
>>> to do its job, and the timer may trigger between the point you turn 
>>> off the LED and the system really goes to suspend, again maybe 
>>> turning the LED on and again leaving the system with a glowing LED at suspend.
>>
>>> The patch also solves the following phenomenon, sorry for not writing 
>>> in the commit:
>>>
>>> - Turn off LED
>>> - Suspend I2C hardware
>>> - Timer trigger
>>> - Trying to blink the LED using I2C
>>> - Crap in the console about the failed I2C transaction
>>> - Actual suspen happens
>>> - System comes online
>>> - Trying to blink the LED using I2C
>>> - Crap in the console about the failed I2C transaction
>>> - Resume I2C hardware
>>>
>>> It's just very fragile this trigger, turning off the LED from the PM 
>>> notifier is obviously not enough, we also need to disable the timer, 
>>> and then take it back online after resume.
>>
>> No, leave the timer alone, and actually leave the trigger alone.
>>
>> Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?
>>
>> (Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).
>>
>> Thanks,
>>
>> 									Pavel
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) 
>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>>
> 
> 

Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Jacek Anaszewski]

On 06/09/2017 10:47 PM, Jacek Anaszewski wrote:
> Hi Bruce,
> 
> On 06/09/2017 04:57 AM, Bruce Zhang wrote:
>> Hi Jacek,
>>
>> I think that no change need to make for __led_set_brightness() and __led_set_brightness_blocking() function. Because led_classdev_suspend() need to turn off LED with these API. set_brightness_delayed() is the same level API as __led_set_brightness(). No need to add LED_SUSPENDED checking as well.
> 
> Indeed, it would be even harmful with the current sequence of
> instructions in the led_classdev_suspend().
> 
> So, the only action to take is to revert the commit 5ab92a7cb8.

Ugh, of course it would fix your problem with uevent, but it would
also bring back the problem the commit 5ab92a7cb8 was solving.

The question is why the problem existed at all, despite that
led_classdev_suspend() seems to do what's needed.

There are three options:
- races between led_heartbeat_function and LED core
- the LED class driver for which the phenomenon was observed
  doesn't set LED_CORE_SUSPENDRESUME flag
- if the driver uses brightness_set_blocking op, then maybe
  it is possible that the work queue task scheduled from
  led_classdev_suspend() that sets brightness to 0 isn't handled
  before the device enters suspend

We could narrow down the possible options if we knew the name of the
LED class driver in question.

Best regards,
Jacek Anaszewski


>> -----Original Message-----
>> From: Jacek Anaszewski [mailto:jacek.anaszewski@gmail.com] 
>> Sent: Friday, June 09, 2017 4:28 AM
>> To: Bruce Zhang <bo.zhang@nxp.com>; Pavel Machek <pavel@ucw.cz>; Linus Walleij <linus.walleij@linaro.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Richard Purdie <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
>> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend
>>
>> Hi Bruce,
>>
>> On 06/08/2017 04:57 AM, Bruce Zhang wrote:
>>> Hi,
>>>
>>> As suspend/resume procedure is as below:
>>> pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
>>> If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.
>>
>> OK, so I agree that the most proper fix to the discussed problem would be ignoring brightness setting requests after suspend, as Pavel suggested.
>>
>> led_classdev_suspend() already turns the LED off and
>> led_classdev_resume() brings the brightness back.
>>
>> Also led_set_brightness_nosleep() and led_set_brightness_sync() don't propagate brightness change to the driver if LED_SUSPENDED is set.
>>
>> There are two problems though:
>> 1. __led_set_brightness() and __led_set_brightness_blocking() don't
>>    check the flag, and they can be called from set_brightness_delayed(),
>>    that was already queued at the time of setting LED_SUSPENDED flag.
>> 2. LED_SUSPENDED flag is accessed in a non-atomic way.
>>
>> The solution as I see it would be respectively:
>> 1. Return from __led_set_brightness() and
>>    __led_set_brightness_blocking() if LED_SUSPENDED is set.
>> 2. Switch to accessing struct led_classdev's flags with use of
>>    bitops. Fortunately those flags are mainly set prior LED class
>>    device registration and only LED_SYSFS_DISABLE, LED_SUSPENDED
>>    and LED_UNREGISTERING are accessed afterwards, and only in the LED
>>    core, so only those places would have to be modified.
>>
>> Best regards,
>> Jacek Anaszewski
>>
>>> -----Original Message-----
>>> From: Pavel Machek [mailto:pavel@ucw.cz]
>>> Sent: Wednesday, June 07, 2017 6:31 PM
>>> To: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson 
>>> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Jacek 
>>> Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie 
>>> <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
>>> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter 
>>> suspend
>>>
>>> Hi!
>>>
>>> On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
>>>> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>>>
>>>>> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
>>>>> the issue for one trigger, but we have more than one... Even if 
>>>>> special handling for heartbeat is warranted, that handling would be 
>>>>> "turn the led off" not "unregister the trigger", which is 
>>>>> userland-visible action.)
>>>>
>>>> OK yeah I guess you're right.
>>>>
>>>> I just couldn't think about anything better and didn't get any review 
>>>> at the time, so mistakes were made.
>>>>
>>>>> That one should be reverted, then maybe the driver should be fixed 
>>>>> to turn the led off during suspend.
>>>>
>>>> Do you mean that the heartbeat trigger driver should be fixed to turn 
>>>> off the LED during sleep?
>>>>
>>>> That is essentially what I was trying to achieve.
>>>
>>> I don't think we should be fixing it at trigger level.
>>>
>>> If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.
>>>
>>>> The reason it is done as it is, is that the trigger sets up a timer 
>>>> to do its job, and the timer may trigger between the point you turn 
>>>> off the LED and the system really goes to suspend, again maybe 
>>>> turning the LED on and again leaving the system with a glowing LED at suspend.
>>>
>>>> The patch also solves the following phenomenon, sorry for not writing 
>>>> in the commit:
>>>>
>>>> - Turn off LED
>>>> - Suspend I2C hardware
>>>> - Timer trigger
>>>> - Trying to blink the LED using I2C
>>>> - Crap in the console about the failed I2C transaction
>>>> - Actual suspen happens
>>>> - System comes online
>>>> - Trying to blink the LED using I2C
>>>> - Crap in the console about the failed I2C transaction
>>>> - Resume I2C hardware
>>>>
>>>> It's just very fragile this trigger, turning off the LED from the PM 
>>>> notifier is obviously not enough, we also need to disable the timer, 
>>>> and then take it back online after resume.
>>>
>>> No, leave the timer alone, and actually leave the trigger alone.
>>>
>>> Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?
>>>
>>> (Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).
>>>
>>> Thanks,
>>>
>>> 									Pavel
>>> --
>>> (english) http://www.livejournal.com/~pavelmachek
>>> (cesky, pictures) 
>>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>>>
>>
>>

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Jacek Anaszewski]

On 06/09/2017 10:47 PM, Jacek Anaszewski wrote:
> Hi Bruce,
> 
> On 06/09/2017 04:57 AM, Bruce Zhang wrote:
>> Hi Jacek,
>>
>> I think that no change need to make for __led_set_brightness() and __led_set_brightness_blocking() function. Because led_classdev_suspend() need to turn off LED with these API. set_brightness_delayed() is the same level API as __led_set_brightness(). No need to add LED_SUSPENDED checking as well.
> 
> Indeed, it would be even harmful with the current sequence of
> instructions in the led_classdev_suspend().
> 
> So, the only action to take is to revert the commit 5ab92a7cb8.

Ugh, of course it would fix your problem with uevent, but it would
also bring back the problem the commit 5ab92a7cb8 was solving.

The question is why the problem existed at all, despite that
led_classdev_suspend() seems to do what's needed.

There are three options:
- races between led_heartbeat_function and LED core
- the LED class driver for which the phenomenon was observed
  doesn't set LED_CORE_SUSPENDRESUME flag
- if the driver uses brightness_set_blocking op, then maybe
  it is possible that the work queue task scheduled from
  led_classdev_suspend() that sets brightness to 0 isn't handled
  before the device enters suspend

We could narrow down the possible options if we knew the name of the
LED class driver in question.

Best regards,
Jacek Anaszewski


>> -----Original Message-----
>> From: Jacek Anaszewski [mailto:jacek.anaszewski at gmail.com] 
>> Sent: Friday, June 09, 2017 4:28 AM
>> To: Bruce Zhang <bo.zhang@nxp.com>; Pavel Machek <pavel@ucw.cz>; Linus Walleij <linus.walleij@linaro.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel at lists.infradead.org; Richard Purdie <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
>> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend
>>
>> Hi Bruce,
>>
>> On 06/08/2017 04:57 AM, Bruce Zhang wrote:
>>> Hi,
>>>
>>> As suspend/resume procedure is as below:
>>> pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
>>> If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.
>>
>> OK, so I agree that the most proper fix to the discussed problem would be ignoring brightness setting requests after suspend, as Pavel suggested.
>>
>> led_classdev_suspend() already turns the LED off and
>> led_classdev_resume() brings the brightness back.
>>
>> Also led_set_brightness_nosleep() and led_set_brightness_sync() don't propagate brightness change to the driver if LED_SUSPENDED is set.
>>
>> There are two problems though:
>> 1. __led_set_brightness() and __led_set_brightness_blocking() don't
>>    check the flag, and they can be called from set_brightness_delayed(),
>>    that was already queued at the time of setting LED_SUSPENDED flag.
>> 2. LED_SUSPENDED flag is accessed in a non-atomic way.
>>
>> The solution as I see it would be respectively:
>> 1. Return from __led_set_brightness() and
>>    __led_set_brightness_blocking() if LED_SUSPENDED is set.
>> 2. Switch to accessing struct led_classdev's flags with use of
>>    bitops. Fortunately those flags are mainly set prior LED class
>>    device registration and only LED_SYSFS_DISABLE, LED_SUSPENDED
>>    and LED_UNREGISTERING are accessed afterwards, and only in the LED
>>    core, so only those places would have to be modified.
>>
>> Best regards,
>> Jacek Anaszewski
>>
>>> -----Original Message-----
>>> From: Pavel Machek [mailto:pavel at ucw.cz]
>>> Sent: Wednesday, June 07, 2017 6:31 PM
>>> To: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson 
>>> <ulf.hansson@linaro.org>; linux-arm-kernel at lists.infradead.org; Jacek 
>>> Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie 
>>> <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
>>> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter 
>>> suspend
>>>
>>> Hi!
>>>
>>> On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
>>>> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>>>
>>>>> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
>>>>> the issue for one trigger, but we have more than one... Even if 
>>>>> special handling for heartbeat is warranted, that handling would be 
>>>>> "turn the led off" not "unregister the trigger", which is 
>>>>> userland-visible action.)
>>>>
>>>> OK yeah I guess you're right.
>>>>
>>>> I just couldn't think about anything better and didn't get any review 
>>>> at the time, so mistakes were made.
>>>>
>>>>> That one should be reverted, then maybe the driver should be fixed 
>>>>> to turn the led off during suspend.
>>>>
>>>> Do you mean that the heartbeat trigger driver should be fixed to turn 
>>>> off the LED during sleep?
>>>>
>>>> That is essentially what I was trying to achieve.
>>>
>>> I don't think we should be fixing it at trigger level.
>>>
>>> If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.
>>>
>>>> The reason it is done as it is, is that the trigger sets up a timer 
>>>> to do its job, and the timer may trigger between the point you turn 
>>>> off the LED and the system really goes to suspend, again maybe 
>>>> turning the LED on and again leaving the system with a glowing LED at suspend.
>>>
>>>> The patch also solves the following phenomenon, sorry for not writing 
>>>> in the commit:
>>>>
>>>> - Turn off LED
>>>> - Suspend I2C hardware
>>>> - Timer trigger
>>>> - Trying to blink the LED using I2C
>>>> - Crap in the console about the failed I2C transaction
>>>> - Actual suspen happens
>>>> - System comes online
>>>> - Trying to blink the LED using I2C
>>>> - Crap in the console about the failed I2C transaction
>>>> - Resume I2C hardware
>>>>
>>>> It's just very fragile this trigger, turning off the LED from the PM 
>>>> notifier is obviously not enough, we also need to disable the timer, 
>>>> and then take it back online after resume.
>>>
>>> No, leave the timer alone, and actually leave the trigger alone.
>>>
>>> Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?
>>>
>>> (Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).
>>>
>>> Thanks,
>>>
>>> 									Pavel
>>> --
>>> (english) http://www.livejournal.com/~pavelmachek
>>> (cesky, pictures) 
>>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>>>
>>
>>

RE: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Bruce Zhang]

Hi Jacek,

Of course I can submit the patch. But we need to confirm that it does not bring back the problem the commit 5ab92a7cb8 was solving. Maybe we can use reference counter to record if some device used by other device. When some device suspend, it should decrease the reference count of the other device that it depends on. Only when reference count of some device decrease to zero, it can enter suspend mode. By the way, we should avoid devices call loop.

Best Regards,
Bo

-----Original Message-----
From: Jacek Anaszewski [mailto:jacek.anaszewski@gmail.com] 
Sent: Saturday, June 10, 2017 4:47 AM
To: Bruce Zhang <bo.zhang@nxp.com>; Pavel Machek <pavel@ucw.cz>; Linus Walleij <linus.walleij@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Richard Purdie <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

Hi Bruce,

On 06/09/2017 04:57 AM, Bruce Zhang wrote:
> Hi Jacek,
> 
> I think that no change need to make for __led_set_brightness() and __led_set_brightness_blocking() function. Because led_classdev_suspend() need to turn off LED with these API. set_brightness_delayed() is the same level API as __led_set_brightness(). No need to add LED_SUSPENDED checking as well.

Indeed, it would be even harmful with the current sequence of instructions in the led_classdev_suspend().

So, the only action to take is to revert the commit 5ab92a7cb8.

Would you mind submitting the patch so that everyone interested could give their ack?

Best regards,
Jacek Anaszewski

> -----Original Message-----
> From: Jacek Anaszewski [mailto:jacek.anaszewski@gmail.com]
> Sent: Friday, June 09, 2017 4:28 AM
> To: Bruce Zhang <bo.zhang@nxp.com>; Pavel Machek <pavel@ucw.cz>; Linus 
> Walleij <linus.walleij@linaro.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>; 
> linux-arm-kernel@lists.infradead.org; Richard Purdie 
> <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter 
> suspend
> 
> Hi Bruce,
> 
> On 06/08/2017 04:57 AM, Bruce Zhang wrote:
>> Hi,
>>
>> As suspend/resume procedure is as below:
>> pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
>> If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.
> 
> OK, so I agree that the most proper fix to the discussed problem would be ignoring brightness setting requests after suspend, as Pavel suggested.
> 
> led_classdev_suspend() already turns the LED off and
> led_classdev_resume() brings the brightness back.
> 
> Also led_set_brightness_nosleep() and led_set_brightness_sync() don't propagate brightness change to the driver if LED_SUSPENDED is set.
> 
> There are two problems though:
> 1. __led_set_brightness() and __led_set_brightness_blocking() don't
>    check the flag, and they can be called from set_brightness_delayed(),
>    that was already queued at the time of setting LED_SUSPENDED flag.
> 2. LED_SUSPENDED flag is accessed in a non-atomic way.
> 
> The solution as I see it would be respectively:
> 1. Return from __led_set_brightness() and
>    __led_set_brightness_blocking() if LED_SUSPENDED is set.
> 2. Switch to accessing struct led_classdev's flags with use of
>    bitops. Fortunately those flags are mainly set prior LED class
>    device registration and only LED_SYSFS_DISABLE, LED_SUSPENDED
>    and LED_UNREGISTERING are accessed afterwards, and only in the LED
>    core, so only those places would have to be modified.
> 
> Best regards,
> Jacek Anaszewski
> 
>> -----Original Message-----
>> From: Pavel Machek [mailto:pavel@ucw.cz]
>> Sent: Wednesday, June 07, 2017 6:31 PM
>> To: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson 
>> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Jacek 
>> Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie 
>> <rpurdie@rpsys.net>; linux-leds@vger.kernel.org
>> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter 
>> suspend
>>
>> Hi!
>>
>> On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
>>> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>>
>>>> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
>>>> the issue for one trigger, but we have more than one... Even if 
>>>> special handling for heartbeat is warranted, that handling would be 
>>>> "turn the led off" not "unregister the trigger", which is 
>>>> userland-visible action.)
>>>
>>> OK yeah I guess you're right.
>>>
>>> I just couldn't think about anything better and didn't get any 
>>> review at the time, so mistakes were made.
>>>
>>>> That one should be reverted, then maybe the driver should be fixed 
>>>> to turn the led off during suspend.
>>>
>>> Do you mean that the heartbeat trigger driver should be fixed to 
>>> turn off the LED during sleep?
>>>
>>> That is essentially what I was trying to achieve.
>>
>> I don't think we should be fixing it at trigger level.
>>
>> If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.
>>
>>> The reason it is done as it is, is that the trigger sets up a timer 
>>> to do its job, and the timer may trigger between the point you turn 
>>> off the LED and the system really goes to suspend, again maybe 
>>> turning the LED on and again leaving the system with a glowing LED at suspend.
>>
>>> The patch also solves the following phenomenon, sorry for not 
>>> writing in the commit:
>>>
>>> - Turn off LED
>>> - Suspend I2C hardware
>>> - Timer trigger
>>> - Trying to blink the LED using I2C
>>> - Crap in the console about the failed I2C transaction
>>> - Actual suspen happens
>>> - System comes online
>>> - Trying to blink the LED using I2C
>>> - Crap in the console about the failed I2C transaction
>>> - Resume I2C hardware
>>>
>>> It's just very fragile this trigger, turning off the LED from the PM 
>>> notifier is obviously not enough, we also need to disable the timer, 
>>> and then take it back online after resume.
>>
>> No, leave the timer alone, and actually leave the trigger alone.
>>
>> Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?
>>
>> (Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).
>>
>> Thanks,
>>
>> 									Pavel
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures)
>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>>
> 
> 

[[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

[Bruce Zhang]

Hi Jacek,

Of course I can submit the patch. But we need to confirm that it does not bring back the problem the commit 5ab92a7cb8 was solving. Maybe we can use reference counter to record if some device used by other device. When some device suspend, it should decrease the reference count of the other device that it depends on. Only when reference count of some device decrease to zero, it can enter suspend mode. By the way, we should avoid devices call loop.

Best Regards,
Bo

-----Original Message-----
From: Jacek Anaszewski [mailto:jacek.anaszewski at gmail.com] 
Sent: Saturday, June 10, 2017 4:47 AM
To: Bruce Zhang <bo.zhang@nxp.com>; Pavel Machek <pavel@ucw.cz>; Linus Walleij <linus.walleij@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>; linux-arm-kernel at lists.infradead.org; Richard Purdie <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter suspend

Hi Bruce,

On 06/09/2017 04:57 AM, Bruce Zhang wrote:
> Hi Jacek,
> 
> I think that no change need to make for __led_set_brightness() and __led_set_brightness_blocking() function. Because led_classdev_suspend() need to turn off LED with these API. set_brightness_delayed() is the same level API as __led_set_brightness(). No need to add LED_SUSPENDED checking as well.

Indeed, it would be even harmful with the current sequence of instructions in the led_classdev_suspend().

So, the only action to take is to revert the commit 5ab92a7cb8.

Would you mind submitting the patch so that everyone interested could give their ack?

Best regards,
Jacek Anaszewski

> -----Original Message-----
> From: Jacek Anaszewski [mailto:jacek.anaszewski at gmail.com]
> Sent: Friday, June 09, 2017 4:28 AM
> To: Bruce Zhang <bo.zhang@nxp.com>; Pavel Machek <pavel@ucw.cz>; Linus 
> Walleij <linus.walleij@linaro.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>; 
> linux-arm-kernel at lists.infradead.org; Richard Purdie 
> <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter 
> suspend
> 
> Hi Bruce,
> 
> On 06/08/2017 04:57 AM, Bruce Zhang wrote:
>> Hi,
>>
>> As suspend/resume procedure is as below:
>> pm_notifier_call_chain(PM_SUSPEND_PREPARE)->device suspend -> syscore suspend -> enter suspend mode -> syscore resume -> device resume -> pm_notifier_call_chain(PM_POST_SUSPEND).
>> If led brightness control depends on other device/hardware (such as I2C), the LED turn off action should be earlier than device suspend. pm_notifier may be a suitable method to turn off LED.
> 
> OK, so I agree that the most proper fix to the discussed problem would be ignoring brightness setting requests after suspend, as Pavel suggested.
> 
> led_classdev_suspend() already turns the LED off and
> led_classdev_resume() brings the brightness back.
> 
> Also led_set_brightness_nosleep() and led_set_brightness_sync() don't propagate brightness change to the driver if LED_SUSPENDED is set.
> 
> There are two problems though:
> 1. __led_set_brightness() and __led_set_brightness_blocking() don't
>    check the flag, and they can be called from set_brightness_delayed(),
>    that was already queued at the time of setting LED_SUSPENDED flag.
> 2. LED_SUSPENDED flag is accessed in a non-atomic way.
> 
> The solution as I see it would be respectively:
> 1. Return from __led_set_brightness() and
>    __led_set_brightness_blocking() if LED_SUSPENDED is set.
> 2. Switch to accessing struct led_classdev's flags with use of
>    bitops. Fortunately those flags are mainly set prior LED class
>    device registration and only LED_SYSFS_DISABLE, LED_SUSPENDED
>    and LED_UNREGISTERING are accessed afterwards, and only in the LED
>    core, so only those places would have to be modified.
> 
> Best regards,
> Jacek Anaszewski
> 
>> -----Original Message-----
>> From: Pavel Machek [mailto:pavel at ucw.cz]
>> Sent: Wednesday, June 07, 2017 6:31 PM
>> To: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Bruce Zhang <bo.zhang@nxp.com>; Ulf Hansson 
>> <ulf.hansson@linaro.org>; linux-arm-kernel at lists.infradead.org; Jacek 
>> Anaszewski <jacek.anaszewski@gmail.com>; Richard Purdie 
>> <rpurdie@rpsys.net>; linux-leds at vger.kernel.org
>> Subject: Re: [[PATCH v2]] drivers: leds/trigger: system cannot enter 
>> suspend
>>
>> Hi!
>>
>> On Wed 2017-06-07 11:46:10, Linus Walleij wrote:
>>> On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>>
>>>> Core reason is that 5ab92a7cb is wrong. (Very wrong; it papers over 
>>>> the issue for one trigger, but we have more than one... Even if 
>>>> special handling for heartbeat is warranted, that handling would be 
>>>> "turn the led off" not "unregister the trigger", which is 
>>>> userland-visible action.)
>>>
>>> OK yeah I guess you're right.
>>>
>>> I just couldn't think about anything better and didn't get any 
>>> review at the time, so mistakes were made.
>>>
>>>> That one should be reverted, then maybe the driver should be fixed 
>>>> to turn the led off during suspend.
>>>
>>> Do you mean that the heartbeat trigger driver should be fixed to 
>>> turn off the LED during sleep?
>>>
>>> That is essentially what I was trying to achieve.
>>
>> I don't think we should be fixing it at trigger level.
>>
>> If userspace keeps blinking using "brightness" attribute, would we like the LED to be off during suspend? I think so.
>>
>>> The reason it is done as it is, is that the trigger sets up a timer 
>>> to do its job, and the timer may trigger between the point you turn 
>>> off the LED and the system really goes to suspend, again maybe 
>>> turning the LED on and again leaving the system with a glowing LED at suspend.
>>
>>> The patch also solves the following phenomenon, sorry for not 
>>> writing in the commit:
>>>
>>> - Turn off LED
>>> - Suspend I2C hardware
>>> - Timer trigger
>>> - Trying to blink the LED using I2C
>>> - Crap in the console about the failed I2C transaction
>>> - Actual suspen happens
>>> - System comes online
>>> - Trying to blink the LED using I2C
>>> - Crap in the console about the failed I2C transaction
>>> - Resume I2C hardware
>>>
>>> It's just very fragile this trigger, turning off the LED from the PM 
>>> notifier is obviously not enough, we also need to disable the timer, 
>>> and then take it back online after resume.
>>
>> No, leave the timer alone, and actually leave the trigger alone.
>>
>> Simply make the driver turn the LED off in .suspend() callback, and then ignore further requests until .resume(). That will get rid of power drain _and_ "failed I2C transaction" messages, right?
>>
>> (Actually, I tested with the heartbeat trigger, and it is not re-installed after resume. Another reason to revert the patch).
>>
>> Thanks,
>>
>> 									Pavel
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures)
>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>>
> 
>