Re: [PATCH v5 1/6] leds: triggers: Add current_brightness trigger parameter

["Pali Rohár" <pali.rohar@gmail.com>]

[-- Attachment #1: Type: Text/Plain, Size: 3433 bytes --]

On Sunday 20 November 2016 19:45:40 Hans de Goede wrote:
> Hi,
> 
> On 20-11-16 15:59, Pali Rohár wrote:
> > On Thursday 17 November 2016 23:24:36 Hans de Goede wrote:
> >> In some cases it may be desirable for userspace to be notified
> >> when a trigger event happens. This commit adds support for a
> >> poll-able current_brightness trigger specific sysfs attribute
> >> which triggers may register:
> >> 
> >> What:		/sys/class/leds/<led>/current_brightness
> >> Date:		November 2016
> >> KernelVersion:	4.10
> >> 
> >> Description:
> >> 	Triggers which support it may register a current_brightness
> >> 	file. This file supports poll() to detect when the trigger
> >> 	modifies the brightness of the LED.
> >> 	Reading this file will always return the current brightness
> >> 	of the LED.
> >> 	Writing this file sets the current brightness of the LED,
> >> 	without influencing the trigger.
> > 
> > Personally I do not like this new sysfs attribute...
> > 
> > Now when somebody look at /sys/class/leds/<something>/, the first
> > thing which say would be:
> > 
> > "What the hell, why there are two files (brightness and
> > current_brightness) for changing LED level? And which should I
> > use?"
> > 
> > If I understood correctly we need to handle two things:
> > 
> > 1) Provide poll() for userspace when LED level is changed (either
> > by HW
> > 
> >    or other user call)
> > 
> > 2) Deal with fact that on _some_ hardware, special key is hardwired
> > to
> > 
> >    change LED level
> > 
> > So why for 1) we cannot use existing sysfs file "brightness"? I do
> > not see any problem with it.
> 
> That was our first attempt at this, but because the brightness may
> also be changed by triggers / blink-timers, we need to wakeup poll()
> in those cases too (anything else would be inconsistent) and doing
> such a wakeup in that case has turned out to cause too much overhead
> in some cases (even if userspace is not listening), specifically the
> idle power uses on some systems got multiplied by a factor of 5 or
> more.
> 
> So this approach was rejected.

But approach with exporting new sysfs file with name current_brightness 
with existence of old brightness sysfs file is not good and in my 
opinion it is even worst as current situation (= without poll support).

What happen in next 5 years? Somebody point that sysfs file "brightness" 
and sysfs file "current_brightness" is still not good and invent 
"really_current_brightness" sysfs with new logic? No this is really not 
a way...

I understand that extending current "brightness" sysfs is complicated, 
but it is not a reason to not doing it and inventing new crippling sysfs 
file which duplicate existing one (with some modifications).

Anyway, I cannot believe that power usage is increased by factor 5 with 
exporting poll support. If we are going to change brightness level 
(either by trigger or timer) we still need to do wakeup.

And we can do some optimizations, e.g. do not send poll event when 
nobody is listening or postpone event so we do not send too many events 
in 1s interval (or choose longer interval).

Polling support is not in mainline kernel yet, so we can change its 
behaviour without breaking ABI. And we can define (if it help us!) that 
evens can be send to userspace with some delay (e.g. 1-3s).

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]