From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v8 7/7] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change Date: Thu, 16 Mar 2017 11:11:01 +0100 Message-ID: <707e2a4e-ab96-a8a8-e2ba-07c3521108c3@redhat.com> References: <20170221170838.GO9795@pali> <7bb7c48f-789e-8b4c-b76b-1389d7fbbfb7@redhat.com> <20170222084935.GA21558@pali> <20170222120109.GC21558@pali> <035b56e6-b606-c632-35cf-00f880abcbf5@redhat.com> <20170301111558.GC3185@pali> <8c181bbc-e786-16c7-39f7-80d4be502066@redhat.com> <20170301125532.GF3185@pali> <20170303120020.GD25757@pali> <595f88da-a0ee-005e-b22d-ee04cfe20628@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:58580 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751279AbdCPKLg (ORCPT ); Thu, 16 Mar 2017 06:11:36 -0400 In-Reply-To: <595f88da-a0ee-005e-b22d-ee04cfe20628@redhat.com> Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: =?UTF-8?Q?Pali_Roh=c3=a1r?= Cc: Darren Hart , Andy Shevchenko , Henrique de Moraes Holschuh , Jacek Anaszewski , Pavel Machek , platform-driver-x86@vger.kernel.org, linux-leds@vger.kernel.org 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