Re: PM regression with LED changes in next-20161109

[Pavel Machek <pavel@ucw.cz>]

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

On Thu 2016-11-10 22:34:07, Jacek Anaszewski wrote:
> Hi,
> 
> On 11/10/2016 09:29 PM, Pavel Machek wrote:
> >On Thu 2016-11-10 10:55:37, Tony Lindgren wrote:
> >>* Pavel Machek <pavel@ucw.cz> [161110 09:29]:
> >>>Hi!
> >>>
> >>>>>>>Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> >>>>>>>the sysfs brightness attr for changes.") breaks runtime PM for me.
> >>>>>>>
> >>>>>>>On my omap dm3730 based test system, idle power consumption is over 70
> >>>>>>>times higher now with this patch! It goes from about 6mW for the core
> >>>>>>>system to over 440mW during idle meaning there's some busy timer now
> >>>>>>>active.
> >>>>>>>
> >>>>>>>Reverting this patch fixes the issue. Any ideas?
> >>>
> >>>Are you using any LED that toggles with high frequency? Like perhaps
> >>>LED that is lit when CPU is active?
> >>
> >>Yeah one of them seems to have cpu0 as the default trigger.
> >
> >Aha. Its quite obvious we don't want to notify sysfs each time that
> >one is toggled, right?
> >
> >IMO brightness should display max brightness for the trigger, as Hans
> >suggested, anything else is madness for trigger such as cpu activity.
> 
> Are you suggesting that we should revert changes introduced
> by below patch?
> 
> commit 29d76dfa29fe22583aefddccda0bc56aa81035dc
> Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Date:   Tue Mar 18 09:47:48 2008 +0000
> 
>     leds: Add support to leds with readable status
> 
>     Some led hardware allows drivers to query the led state, and this patch
>     adds a hook to let the led class take advantage of that information when
>     available.
> 
>     Without this functionality, when access to the led hardware is not
>     exclusive (i.e. firmware or hardware might change its state behind the
>     kernel's back), reality goes out of sync with the led class' idea of
> what
>     the led is doing, which is annoying at best.

Hmm. So userland can read the LED state, and it can get _some_ value
back, but it can not know if it is current state or not.

I don't think that's a good interface. I see it is from 2008... is
someone using it? Maybe it is too late for revert.

But I'd certainly not extend it with poll.

IMO reading/polling should only be available with some triggers. It
does not make sense with "CPU load" trigger. It makes sense with
"keyboard light changeable by hardware" trigger.

Best regards,
									
									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 --]