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()? > >This is one part which I did not liked in proposed ABI as it force > >userspace to choose and use only one brightness monitoring application > >at same time. > > Oh please, we are not going to have this whole ABI discussion again. > I already spend months on this, this ship has long sailed. Ok, I just want to know reason for above one question. > >Plus if ABI was specified that poll() could be used only for hardware > >change (and not by software e.g. kernel/userspace) then such > >functionality is not possible to implement for Dell machines. As it > >looks like Dell firmware send event about every change of brightness and > >we cannot distinguish if change was done by software (kernel) or by > >hardware itself. > > It is possible just fine on Dell machines, except that libsmbios changes > will get seen as hardware triggered changes, which in a way they are > as libsmbios is making hardware changes behind the kernels back. Tool smbios-keyboard-ctl is valid and will be there. If you like it or not we need to deal with fact that libsmbios exists and is used... 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? Only just because we say that some userspace application should not be used and because we invented new kernel ABI which is not fully designed for this case for existing Dell machines? For me it looks like a bad design somewhere... And also I do not like implementation in this dell driver that for every one received event we need to query smbios with another SMM call to get current brightness level. And after few seconds userspace ask again for current level (because it received event that brightness was changed) and we need to do another SMM call. > >I do not know now if you already accepted that ABI > > Yes the ABI is in current next, and given that the 4.11 merge-window has > opened this ship has really sailed, also you're making a problem where > there really is none. Now can we please move forward with this ? > > Regards, > > Hans -- Pali Rohár pali.rohar@xxxxxxxxx
