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 06-03-17 14:39, Hans de Goede wrote:
> Hi,
>
> On 03-03-17 13:00, Pali Rohár wrote:
>> On Wednesday 01 March 2017 14:58:45 Hans de Goede wrote:
>>>> In my idea I do not add any additional information to event. Which means
>>>> all received events are same in current driver state and I can swap
>>>> them. So I can decide to drop one event when driver is changing
>>>> backlight level. And because I can swap received events it does not
>>>> matter if I drop "correct" one (that which was really generated by my
>>>> backlight level change) or any other before or after (as they are same).
>>>> The only important is that kernel use correct number of firmware events.
>>>>
>>>> Idea for time interval of 2-3 seconds is that if event is delivered
>>>> later as after 3 seconds it is probably useless as driver application
>>>> code can be already out-of-sync. But this time interval is just another
>>>> idea and we do not need to use it.
>>>
>>> I really don't like the 2-3 seconds stuff, but I'm fine with going
>>> with your idea of just swallowing one event after a sysfs-write
>>> without the timeout stuff.
>>
>> Ok. I'm fine with just dropping exactly one event after setting new
>> value via sysfs. It is OK for you?
>
> Yes that will work for me. I will prepare a new version. Note I probably
> will not have time for this till the end of this week.
>
> One thing I was wondering, is what will happen if we write the
> same value to the sysfs brightness file twice, do we get events
> for both writes?  One way to avoid this problem would be to compare
> the written value to the last known value and discard the write
> (avoiding an expensive SMM call) if they are equal.
>
> Discarding double writes does introduce a race between hotkey changes
> vs sysfs writes, if a hotkey change has been made but the event has
> not yet been handled then a sysfs write may be seen as being double
> and get discarded even though it is not double. OTOH if the hotkey
> press wins the race then the result would have been the write being
> discarded anyways, so I don't think this is a real problem.
>
> You're input on this would be appreciated. If writing the same value
> twice does not generate events then we must filter out the double
> writes. If it does drop events we can choose, the simplest would be
> to not filter in that case, avoiding any races (and userspace normally
> should not write the same value twice, but we cannot assume that).

Ok, I've spend some time on this today. As I was afraid no new wmi
events are generated when writing the same value to sysfs twice,
making the ignore one event after write approach tricky.

But luckily I've found a better fix, the kbd led related wmi events
with a type of 0x0010 are only send when using the hotkeys.

So if we ignore the events with a type of 0x0011, by dropping the
part of the patch which was mapping those to KEY_KBDILLUMTOGGLE
instead of to KEY_RESERVED, we only end up calling
dell_laptop_call_notifier from dell-wmi.c on hotkey presses and
we can drop the current vs new brightness comparison which
you disliked from dell-laptop.c.

So we can fix this by making the patch smaller :)

I will prepare a v9 with these changes and send that out later
today.

Regards,

Hans