Hi,
On 01-03-17 13:55, Pali Rohár wrote:
On Wednesday 01 March 2017 13:02:58 Hans de Goede wrote:
Hi,
On 01-03-17 12:15, Pali Rohár wrote:
On Wednesday 22 February 2017 13:20:46 Hans de Goede wrote:
1) These events do not happen 100s of times per second, they happen
almost never
100 events per second is probably not happening, but on my Latitude I
see that sometimes more events are delayed and delivered at once.
2) This is the best we can do given the firmware interface we have
What about just dropping upcoming one event in interval of next 2-3
second? Instead of trying to remember last state in kernel and then
trying to mach if received event was that which was caused by change?
That is way more racy then the solution with the kernel remembering
the brightness. With my proposed solution there is basically no race,
Is is racy. When you change keyboard backlight via LED sysfs and also in
that time kernel receive hardware event about changed backlight, kernel
do not know if that event comes as reaction to change via LED sysfs or
as autonomous firmware decision or as reaction to pressing key for
changing keyboard backlight (managed by firmware).
In this case there are only a limited number of scenarios:
1. The hotkey event gets the mutex before the sysfs write
In this case there is no race as the event is processed before the
sysfs write can be proceed.
2. The sysfs write gets the mutex before the hotkey event:
There are 2 sub scenarios here:
2a) The sysfs writes the same value as the hotkey press just set:
In this case the cached brightness will get updated to this new
value before processing either event can be processed, when both
events get processed they will fail the new_brightness != brightness
check and no event will be generated. Arguably this is a bug but
I believe if the user just set the brightness to the same value,
making the hotkey change a nop that this behavior is fine. The sysfs
write won the race after all, so from a kernel pov it really was
first and the hotkey event really is a nop.
2b) The sysfs write writes a different value then the hotkey,
in this case a question becomes which of the 2 we actually get,
but this is up to the BIOS not up to the kernel, basically this
depends on who won the race from the BIOS pov. If the sysfs
write was seen last from the BIOS pov then our SMM call to get
the current value will report the already cached value for both
events and no events get generated. This make sense since the
hotkey change got cancelled by the sysfs write. If otoh the hotkey
gets handled last, then the first event will read back
a different value, after which an events gets send to userspace
and the cached value gets updated, changing the second event
into a NOP. So in this case things just work as if there was
a significant amount of time between the sysfs write and the
hotkey press.
TL;DR: the very worst then can happen is loosing a hotkey
event because the exact same value was written to sysfs after
the press was handled by the BIOS but before we get to process
the event.
I'm not talking here about userspace libsmbios. All is just by
autonomous firmware, keyboard usage and LED sysfs API.
And how is my idea about dropping exactly one event for one request for
changing backlight via LED sysfs API more racy as yours?
You wrote: "What about just dropping upcoming one event in interval
of next 2-3 second?"
The time window you added makes things more razy. If we just increment
an atomic_t on sysfs_write, and decrement_and_test on an event then
that would be fine and would fix your sysfs write vs hotkey press write
as we would get 2 events in that case.
Received event from firmware has only the timestamp, no more information
yet (*). Therefore state of events does is not ordered and any two
events are basically same.
In your provides patch you are adding additional information to event.
It is currently read brightness level in time when was event delivered,
Correct, that is part of the ABI definition of the brightness_hw_changed
sysfs attribute, we must pass the brightness at the time of the hardware
initiated change.
not state of time when event was created. Which means you cannot swap
any two received events anymore as they are part of current driver
state.
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.
(*) Looks like for some machines in dell-wmi.c we know also which
brightness level was set by firmware, but it is not used now and we
still have machines without this information.
the only downside is the driver seeing a brightness change caused by
libsmbios as one caused by the hotkeys, but it will do so 100%
reliably and that is something which I believe we can live with
just fine.
This would allow us to reduce doing one SMM call for each received
event and I think it would be same reliable as your approach.
As I explained *already* we already have only one SMM call for reach
received event, we pass the read-back brightness into
led_classdev_notify_brightness_hw_changed and when userspace
reads the new brightness_hw_changed event in response to the wakeup
the led-core will use the value passed through
led_classdev_notify_brightness_hw_changed so only 1 SMM call happens
per event.
Also again this does not happen 100 times per second you're really
trying to optimize something which does not need any optimization
at all here.
We have reports that on some Dell notebooks is issuing SMM call causing
freezing kernel for about 1-3 seconds (as firmware stay in SMM mode for
a longer time...).
I doubt those are notebooks with backlight keyboards, but anyways if
we swallow 1 event per sysfs write we are only doing SMM calls when
actually reporting a change to userspace at which point we need to
make that smm call sooner or later anyways...
Regards,
Hans
