linux-kernel - Re: PM regression with LED changes in next-20161109

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Nov 2016 13:01:14 +0100
From:   Pavel Machek <pavel@....cz>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>
Cc:     Tony Lindgren <tony@...mide.com>,
        Jacek Anaszewski <j.anaszewski@...sung.com>,
        Hans de Goede <hdegoede@...hat.com>,
        linux-leds@...r.kernel.org, linux-omap@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Darren Hart <dvhart@...radead.org>
Subject: Re: PM regression with LED changes in next-20161109

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

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)