On 06/09/2017 10:47 PM, Jacek Anaszewski wrote:

Hi Bruce,

On 06/09/2017 04:57 AM, Bruce Zhang wrote:

quoted

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

quoted

-----Original Message-----
From: Jacek Anaszewski [mailto:jacek.anaszewski at gmail.com] 
Sent: Friday, June 09, 2017 4:28 AM
To: Bruce Zhang <redacted>; Pavel Machek <redacted>; Linus Walleij <redacted>
Cc: Ulf Hansson <redacted>; linux-arm-kernel at lists.infradead.org; Richard Purdie <redacted>; 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:

quoted

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

quoted

-----Original Message-----
From: Pavel Machek [mailto:pavel at ucw.cz]
Sent: Wednesday, June 07, 2017 6:31 PM
To: Linus Walleij <redacted>
Cc: Bruce Zhang <redacted>; Ulf Hansson 
[off-list ref]; linux-arm-kernel at lists.infradead.org; Jacek 
Anaszewski [off-list ref]; Richard Purdie 
[off-list ref]; 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:

quoted

On Wed, Jun 7, 2017 at 9:12 AM, Pavel Machek [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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