linux-kernel - Re: LEDs that change brightness "itself" -- that's a trigger. 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]

Message-ID: <2f04d861-d2b9-8f53-6ebf-fe39bf73463a@redhat.com>
Date:   Tue, 15 Nov 2016 12:17:00 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Jacek Anaszewski <j.anaszewski@...sung.com>,
        Pavel Machek <pavel@....cz>
Cc:     Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Tony Lindgren <tony@...mide.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: LEDs that change brightness "itself" -- that's a trigger. Re: PM
 regression with LED changes in next-20161109

HI,

On 15-11-16 11:58, Jacek Anaszewski wrote:
> On 11/15/2016 11:31 AM, Pavel Machek wrote:
>> Hi!
>>
>>>> Hmm, v4 still calls led_notify_brightness_change(led_cdev)
>>> >from both __led_set_brightness() and __led_set_brightness_blocking().
>>>
>>> Ugh, I see I accidentally send a v4 twice, instead of
>>> calling the version which dropped those called v5 as
>>> I should have, sorry.
>>>
>>> The v4 which I would like to see merged, the one with
>>> those calls dropped, is here:
>>>
>>> https://patchwork.kernel.org/patch/9423093/
>>
>> Please, lets fix this properly.
>>
>> The LED you are talking about _has_ a trigger, implemented in
>> hardware. That trigger can change LED brightness behind kernel's (and
>> userspace's) back. Don't pretend the trigger does not exist, it does.
>>
>> And when you do that, you'll have nice place to report changes to
>> userspace -- trigger can now export that information, and offer poll()
>> interface.
>
> Well, that sounds interesting. It is logically justifiable.
> I initially proposed exactly this solution, with recently
> added userspace LED being a trigger listener. It seems a bit
> awkward though. How would you listen to the trigger events?

We could make the trigger sysfs attribute poll()-able, but only
for select triggers, e.g.:

Documentation/ABI/testing/sysfs-class-led

What:           /sys/class/leds/<led>/trigger
Date:           March 2006
KernelVersion:  2.6.17
Contact:        Richard Purdie <rpurdie@...ys.net>
Description:
                 Set the trigger for this LED. A trigger is a kernel based source
                 of led events.
                 You can change triggers in a similar manner to the way an IO
                 scheduler is chosen. Trigger specific parameters can appear in
                 /sys/class/leds/<led> once a given trigger is selected. For
                 their documentation see sysfs-class-led-trigger-*.
+
+		For some triggers userspace my poll() this file, watching for
+		POLL_PRI to detect when the trigger triggers. This is only
+		supported if this is explicitly mentioned as supported in
+		sysfs-class-led-trigger-* for the selected trigger.

The reason for making this only supported for select triggers is to
avoid getting the whole power-consumption issue from triggers which fire
frequently again.

And then we could add a new:

Documentation/ABI/testing/sysfs-class-led-trigger-kbd-backlight-change

File which documents that the new to be added kbd-backlight-change
trigger is poll-able.

We would also need:

--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -47,6 +47,7 @@ struct led_classdev {
  #define LED_DEV_CAP_FLASH      (1 << 18)
  #define LED_HW_PLUGGABLE       (1 << 19)
  #define LED_PANIC_INDICATOR    (1 << 20)
+#define LED_TRIGGER_READ_ONLY   (1 << 21)

         /* set_brightness_work / blink_timer flags, atomic, private. */
         unsigned long           work_flags;

To allow led drivers to indicate that there trigger is hardwired.

Regards,

Hans