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

> >(*) 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,

It is possible and also that no notebooks with backlight keyboard is
affected. I just wanted to show that there can be a real problem as we
got reports about such freezes in SMM.

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

Yes. But I think that we do not have to penalize users who are not using
new brightness_hw_changed attribute.

> Regards,
> 
> Hans

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