Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

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

On Wednesday 22 February 2017 09:36:08 Hans de Goede wrote:
> Hi,
> 
> On 21-02-17 18:08, Pali Rohár wrote:
> >On Tuesday 21 February 2017 17:14:06 Hans de Goede wrote:
> >>Hi,
> >>
> >>On 21-02-17 16:13, Pali Rohár wrote:
> >>>On Tuesday 21 February 2017 15:56:43 Hans de Goede wrote:
> >>>>>So do we really need this code which prevents update?
> >>>>
> >>>>Yes, because the ABI specification for the new brightness_hw_changed says
> >>>>that poll() listeners will only be woken up if the brightness is changed
> >>>>outside of the kernel and not when the kernel changes it itself.
> >>>
> >>>So in case there are two applications in userspace which want to monitor
> >>>brightness change and both of those application could change brightness
> >>>(via sysfs) then these two applications would not know about every
> >>>brightness change and would be out-of-sync of reality what is really
> >>>configured by kernel.
> >>
> >>Yes, because with triggers and blinking etc. it is impossible for
> >>userspace to continuously monitor brigthness in a way which does not
> >>cause a high system load.
> >
> >Triggers and blinking features are out due to high cpu load. Fine.
> >
> >But why also manual writes to /sys/class/leds/... by userspace
> >applications is filtered and not reported via poll()?
> 
> I agree that having a way for interested userspace to detect those
> would be good, but that would need to be another API, may poll()
> on the brightness attribute itself while excluding triggers / blinking
> changes from wakeup ?
> 
> Anyways that is something to discuss in a thread specific to the
> LED subsystem and somewhat orthogonal to this patch.

Ok, lets start discussion about it in new separate thread. I was in
impression that this was already part of discussion and in proposed ABI.
Now I see that it was just in original cpu consuming ABI which was
rejected.

> One disadvantage of poll() is that it does not give the source of
> the change, so in retrospect I actually like the new brightness_hw_changed
> attribute as that does give the source, which is something which we need
> to know in userspace.

Do you really in userspace need to know source of change? And to
distinguish between change from hardware and change from userspace done
by echo > /sys/class/leds?

I though that this is for informing userspace application that led
status was changed and application should update some bar or number
which show current state of backlight level.

> In previous versions of the ABI I had to do
> the same brightness comparison I'm doing in the dell-laptop driver
> now in userspace, where it can never be done safely as userspace does
> not know about other userspace.
> 
> Since the Dell smbios events don't provide us with a source of the
> change, we need to compare the brightness to the last set brightness
> somewhere and IMHO the kernel is the best (least bad) place to do
> that.

Maybe kernel place is the least bad place, but still it is unreliable
for Dell machines.

You do not know latency and delay how long it will take Dell firmware to
deliver event to kernel. It also implies that there is race condition
between delivering event and reading new backlight level. Basically it
is different and similar race condition as you are fixing by another
patch in this series.

> >Due to fundamental reasons we ignore all race condition between
> >libsmbios and kernel (they are basically not possible to solve). I'm
> >fine with this.
> >
> >But why should setting keyboard backlight via smbios-keyboard-ctl and
> >via echo > /sys/class/leds/ behave differently?
> 
> Because we cannot solve the smbios-keyboard-ctl case, but we can solve
> the echo case, as said we could probably use a new kernel ABI to allow
> userspace to detect changes caused by the echo example, but that
> really is a whole new discussion.
> 
> Regards,
> 
> Hans
> 

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