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@xxxxxx] > Sent: Wednesday, June 07, 2017 6:31 PM > To: Linus Walleij <linus.walleij@xxxxxxxxxx> > Cc: Bruce Zhang <bo.zhang@xxxxxxx>; Ulf Hansson <ulf.hansson@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>; Richard Purdie <rpurdie@xxxxxxxxx>; linux-leds@xxxxxxxxxxxxxxx > 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@xxxxxx> 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 >
