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@xxxxxxxxx] > Sent: Friday, June 09, 2017 4:28 AM > To: Bruce Zhang <bo.zhang@xxxxxxx>; Pavel Machek <pavel@xxxxxx>; Linus Walleij <linus.walleij@xxxxxxxxxx> > Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Richard Purdie <rpurdie@xxxxxxxxx>; linux-leds@xxxxxxxxxxxxxxx > 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@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 >> > >
