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

[Hans de Goede <hdegoede@redhat.com>]

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.

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

> 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