Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Darren Hart]

On Thu, Dec 04, 2014 at 12:16:20AM +0100, Gabriele Mazzotta wrote:
> Currently dell-wmi reports keypresses for WMI events that are
> notifications of changes performed by the BIOS.
> This patch series make sure that no keypresses are sent for those
> events so that nothing is done from userspace. 
> 
> Gabriele Mazzotta (3):
>   dell-wmi: Use appropriate keycode for radio state changes
>   dell-wmi: Don't report keypresses for radio state changes

Merged into one patch, queued.

>   dell-wmi: Don't report keypresses on keybord illumination change

Queued.

Thanks Gabriele.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Darren Hart]

On Fri, Dec 05, 2014 at 10:07:35PM +0100, Pali Rohár wrote:
> On Friday 05 December 2014 21:41:22 Pavel Machek wrote:
> > On Fri 2014-12-05 21:31:34, Pali Rohár wrote:
> > > On Wednesday 03 December 2014 14:34:32 Darren Hart wrote:
> > > > On Thu, Dec 04, 2014 at 12:16:20AM +0100, Gabriele
> > > > Mazzotta
> > > 
> > > wrote:
> > > > > Currently dell-wmi reports keypresses for WMI events
> > > > > that are notifications of changes performed by the
> > > > > BIOS. This patch series make sure that no keypresses
> > > > > are sent for those events so that nothing is done from
> > > > > userspace.
> > > > > 
> > > > > Gabriele Mazzotta (3):
> > > > >   dell-wmi: Use appropriate keycode for radio state
> > > > >   changes dell-wmi: Don't report keypresses for radio
> > > > >   state changes
> > > > 
> > > > Merged into one patch, queued.
> > > > 
> > > > >   dell-wmi: Don't report keypresses on keybord
> > > > >   illumination change
> > > > 
> > > > Queued.
> > > > 
> > > > Thanks Gabriele.
> > > 
> > > Darren, what do you think about sending patch into stable
> > > kernel?
> > 
> > I'd suggest against that. -stable is for "serious" bugs, and
> > we don't want to change this kind of behaviour in -stable
> > kernel.
> > 
> > 								Pavel
> 
> Ok, I agree that it is subjective how serious it is...
> Just to remind that patch fixing problem described in
> 
> http://www.spinics.net/lists/platform-driver-x86/msg05922.html
> http://www.spinics.net/lists/platform-driver-x86/msg05924.html

I don't have any objection to sending this back to stable. Stable is for fixing
REAL bugs, as opposed to theorhetical races, etc. This is a "real" bug.

As to not chaning behavior, if it's OK for mainline, it's OK for stable. At
least that is my understanding of it. Folks are free to verify with Greg if they
disagree.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Gabriele Mazzotta]

Currently dell-wmi reports keypresses for WMI events that are
notifications of changes performed by the BIOS.
This patch series make sure that no keypresses are sent for those
events so that nothing is done from userspace. 

Gabriele Mazzotta (3):
  dell-wmi: Use appropriate keycode for radio state changes
  dell-wmi: Don't report keypresses for radio state changes
  dell-wmi: Don't report keypresses on keybord illumination change

 drivers/platform/x86/dell-wmi.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
2.1.3

[PATCH 1/3] dell-wmi: Use appropriate keycode for radio state changes

[Gabriele Mazzotta]

The WMI events associated to KEY_WLAN are for all the radio devices
available. Use KEY_RFKILL instead since it's more appropriate.

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/platform/x86/dell-wmi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 25721bf..1e31fab 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -65,10 +65,9 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	/* Battery health status button */
 	{ KE_KEY, 0xe007, { KEY_BATTERY } },
 
-	/* This is actually for all radios. Although physically a
-	 * switch, the notification does not provide an indication of
-	 * state and so it should be reported as a key */
-	{ KE_KEY, 0xe008, { KEY_WLAN } },
+	/* Although physically a switch, the notification does not provide
+	 * an indication of state and so it should be reported as a key */
+	{ KE_KEY, 0xe008, { KEY_RFKILL } },
 
 	/* The next device is at offset 6, the active devices are at
 	   offset 8 and the attached devices at offset 10 */
-- 
2.1.3

[PATCH 2/3] dell-wmi: Don't report keypresses for radio state changes

[Gabriele Mazzotta]

The state of radio devices is changed directly by the BIOS when hotkeys
are pressed, so no events should be reported.

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/platform/x86/dell-wmi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 1e31fab..9d3507c 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -65,9 +65,8 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	/* Battery health status button */
 	{ KE_KEY, 0xe007, { KEY_BATTERY } },
 
-	/* Although physically a switch, the notification does not provide
-	 * an indication of state and so it should be reported as a key */
-	{ KE_KEY, 0xe008, { KEY_RFKILL } },
+	/* Radio devices state change */
+	{ KE_IGNORE, 0xe008, { KEY_RFKILL } },
 
 	/* The next device is at offset 6, the active devices are at
 	   offset 8 and the attached devices at offset 10 */
-- 
2.1.3

[PATCH 3/3] dell-wmi: Don't report keypresses on keybord illumination change

[Gabriele Mazzotta]

Keyboard illumination level changes are performed by the BIOS, so no
events should be reported on keypress. This is already done on systems
using the legacy keymap, do it also for systems that don't use it.

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/platform/x86/dell-wmi.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 9d3507c..0344b89 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -211,11 +211,16 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
 	for (i = 0; i < hotkey_num; i++) {
 		const struct dell_bios_keymap_entry *bios_entry =
 					&dell_bios_hotkey_table->keymap[i];
-		keymap[i].type = KE_KEY;
-		keymap[i].code = bios_entry->scancode;
-		keymap[i].keycode = bios_entry->keycode < 256 ?
+		u16 keycode = bios_entry->keycode < 256 ?
 				    bios_to_linux_keycode[bios_entry->keycode] :
 				    KEY_RESERVED;
+
+		if (keycode == KEY_KBDILLUMTOGGLE)
+			keymap[i].type = KE_IGNORE;
+		else
+			keymap[i].type = KE_KEY;
+		keymap[i].code = bios_entry->scancode;
+		keymap[i].keycode = keycode;
 	}
 
 	keymap[hotkey_num].type = KE_END;
-- 
2.1.3

Re: [PATCH 3/3] dell-wmi: Don't report keypresses on keybord illumination change

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 798 bytes --]

On Thursday 04 December 2014 00:16:23 Gabriele Mazzotta wrote:
> Keyboard illumination level changes are performed by the BIOS,
> so no events should be reported on keypress. This is already
> done on systems using the legacy keymap, do it also for
> systems that don't use it.
> 
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> ---

Tested-by: Pali Rohár <pali.rohar@gmail.com>
CC: stable@vger.kernel.org

Tested on Latitude E6440 and now userspace does not receive 
KEY_KBDILLUMTOGGLE event, so does not try to change keyboard 
backlight (which is already done by BIOS when key is pressed).

I suggest to backport this patch to stable kernels because this 
problem is in all kernel versions which have dell-wmi.c driver.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 809 bytes --]

On Wednesday 03 December 2014 14:34:32 Darren Hart wrote:
> On Thu, Dec 04, 2014 at 12:16:20AM +0100, Gabriele Mazzotta 
wrote:
> > Currently dell-wmi reports keypresses for WMI events that
> > are notifications of changes performed by the BIOS.
> > This patch series make sure that no keypresses are sent for
> > those events so that nothing is done from userspace.
> > 
> > Gabriele Mazzotta (3):
> >   dell-wmi: Use appropriate keycode for radio state changes
> >   dell-wmi: Don't report keypresses for radio state changes
> 
> Merged into one patch, queued.
> 
> >   dell-wmi: Don't report keypresses on keybord illumination
> >   change
> 
> Queued.
> 
> Thanks Gabriele.

Darren, what do you think about sending patch into stable kernel?

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Pavel Machek]

On Fri 2014-12-05 21:31:34, Pali Rohár wrote:
> On Wednesday 03 December 2014 14:34:32 Darren Hart wrote:
> > On Thu, Dec 04, 2014 at 12:16:20AM +0100, Gabriele Mazzotta 
> wrote:
> > > Currently dell-wmi reports keypresses for WMI events that
> > > are notifications of changes performed by the BIOS.
> > > This patch series make sure that no keypresses are sent for
> > > those events so that nothing is done from userspace.
> > > 
> > > Gabriele Mazzotta (3):
> > >   dell-wmi: Use appropriate keycode for radio state changes
> > >   dell-wmi: Don't report keypresses for radio state changes
> > 
> > Merged into one patch, queued.
> > 
> > >   dell-wmi: Don't report keypresses on keybord illumination
> > >   change
> > 
> > Queued.
> > 
> > Thanks Gabriele.
> 
> Darren, what do you think about sending patch into stable kernel?

I'd suggest against that. -stable is for "serious" bugs, and we don't
want to change this kind of behaviour in -stable kernel.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1445 bytes --]

On Friday 05 December 2014 21:41:22 Pavel Machek wrote:
> On Fri 2014-12-05 21:31:34, Pali Rohár wrote:
> > On Wednesday 03 December 2014 14:34:32 Darren Hart wrote:
> > > On Thu, Dec 04, 2014 at 12:16:20AM +0100, Gabriele
> > > Mazzotta
> > 
> > wrote:
> > > > Currently dell-wmi reports keypresses for WMI events
> > > > that are notifications of changes performed by the
> > > > BIOS. This patch series make sure that no keypresses
> > > > are sent for those events so that nothing is done from
> > > > userspace.
> > > > 
> > > > Gabriele Mazzotta (3):
> > > >   dell-wmi: Use appropriate keycode for radio state
> > > >   changes dell-wmi: Don't report keypresses for radio
> > > >   state changes
> > > 
> > > Merged into one patch, queued.
> > > 
> > > >   dell-wmi: Don't report keypresses on keybord
> > > >   illumination change
> > > 
> > > Queued.
> > > 
> > > Thanks Gabriele.
> > 
> > Darren, what do you think about sending patch into stable
> > kernel?
> 
> I'd suggest against that. -stable is for "serious" bugs, and
> we don't want to change this kind of behaviour in -stable
> kernel.
> 
> 								Pavel

Ok, I agree that it is subjective how serious it is...
Just to remind that patch fixing problem described in

http://www.spinics.net/lists/platform-driver-x86/msg05922.html
http://www.spinics.net/lists/platform-driver-x86/msg05924.html

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2201 bytes --]

On Wednesday 03 December 2014 19:03:37 Darren Hart wrote:
> On Fri, Dec 05, 2014 at 10:07:35PM +0100, Pali Rohár wrote:
> > On Friday 05 December 2014 21:41:22 Pavel Machek wrote:
> > > On Fri 2014-12-05 21:31:34, Pali Rohár wrote:
> > > > On Wednesday 03 December 2014 14:34:32 Darren Hart wrote:
> > > > > On Thu, Dec 04, 2014 at 12:16:20AM +0100, Gabriele
> > > > > Mazzotta
> > > > 
> > > > wrote:
> > > > > > Currently dell-wmi reports keypresses for WMI events
> > > > > > that are notifications of changes performed by the
> > > > > > BIOS. This patch series make sure that no keypresses
> > > > > > are sent for those events so that nothing is done
> > > > > > from userspace.
> > > > > > 
> > > > > > Gabriele Mazzotta (3):
> > > > > >   dell-wmi: Use appropriate keycode for radio state
> > > > > >   changes dell-wmi: Don't report keypresses for
> > > > > >   radio state changes
> > > > > 
> > > > > Merged into one patch, queued.
> > > > > 
> > > > > >   dell-wmi: Don't report keypresses on keybord
> > > > > >   illumination change
> > > > > 
> > > > > Queued.
> > > > > 
> > > > > Thanks Gabriele.
> > > > 
> > > > Darren, what do you think about sending patch into
> > > > stable kernel?
> > > 
> > > I'd suggest against that. -stable is for "serious" bugs,
> > > and we don't want to change this kind of behaviour in
> > > -stable kernel.
> > > 
> > > 								Pavel
> > 
> > Ok, I agree that it is subjective how serious it is...
> > Just to remind that patch fixing problem described in
> > 
> > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht
> > ml
> > http://www.spinics.net/lists/platform-driver-x86/msg05924.h
> > tml
> 
> I don't have any objection to sending this back to stable.
> Stable is for fixing REAL bugs, as opposed to theorhetical
> races, etc. This is a "real" bug.
> 
> As to not chaning behavior, if it's OK for mainline, it's OK
> for stable. At least that is my understanding of it. Folks
> are free to verify with Greg if they disagree.

Darren, so how you decided? Now when patches are in linus tree, 
are you going to send them to stable tree?

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Pavel Machek]

Hi!

> > > Ok, I agree that it is subjective how serious it is...
> > > Just to remind that patch fixing problem described in
> > > 
> > > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht
> > > ml
> > > http://www.spinics.net/lists/platform-driver-x86/msg05924.h
> > > tml
> > 
> > I don't have any objection to sending this back to stable.
> > Stable is for fixing REAL bugs, as opposed to theorhetical
> > races, etc. This is a "real" bug.
> > 
> > As to not chaning behavior, if it's OK for mainline, it's OK
> > for stable. At least that is my understanding of it. Folks
> > are free to verify with Greg if they disagree.
> 
> Darren, so how you decided? Now when patches are in linus tree, 
> are you going to send them to stable tree?

Please don't. -stable is for serious mainline bugs people are actually
hitting. Null pointer dereference counts, if people actually hit
it. This is more behaviour change, and yes, the new behaviour is
better, but it is really different class.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Darren Hart]

On Sat, Dec 20, 2014 at 04:11:08PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > Ok, I agree that it is subjective how serious it is...
> > > > Just to remind that patch fixing problem described in
> > > > 
> > > > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht
> > > > ml
> > > > http://www.spinics.net/lists/platform-driver-x86/msg05924.h
> > > > tml
> > > 
> > > I don't have any objection to sending this back to stable.
> > > Stable is for fixing REAL bugs, as opposed to theorhetical
> > > races, etc. This is a "real" bug.
> > > 
> > > As to not chaning behavior, if it's OK for mainline, it's OK
> > > for stable. At least that is my understanding of it. Folks
> > > are free to verify with Greg if they disagree.
> > 
> > Darren, so how you decided? Now when patches are in linus tree, 
> > are you going to send them to stable tree?
> 
> Please don't. -stable is for serious mainline bugs people are actually
> hitting. Null pointer dereference counts, if people actually hit
> it. This is more behaviour change, and yes, the new behaviour is
> better, but it is really different class.

In this case I agree with Pavel. While the patches are small enough, fix one
thing each, etc, it isn't clear from the description exactly how these patches
affect users.

If you can articulate how they are "real bugs that bother people" (see
stable_kernel_rules.txt) we can reconsider. I should have pushed for better
commit messages on these it appears as this should be obvious from those, but it
isn't - at least not to me at 8:15am ;-)

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Henrique de Moraes Holschuh]

On Sat, 20 Dec 2014, Pavel Machek wrote:
> > > > Ok, I agree that it is subjective how serious it is...
> > > > Just to remind that patch fixing problem described in
> > > > 
> > > > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht
> > > > ml
> > > > http://www.spinics.net/lists/platform-driver-x86/msg05924.h
> > > > tml
> > > 
> > > I don't have any objection to sending this back to stable.
> > > Stable is for fixing REAL bugs, as opposed to theorhetical
> > > races, etc. This is a "real" bug.
> > > 
> > > As to not chaning behavior, if it's OK for mainline, it's OK
> > > for stable. At least that is my understanding of it. Folks
> > > are free to verify with Greg if they disagree.
> > 
> > Darren, so how you decided? Now when patches are in linus tree, 
> > are you going to send them to stable tree?
> 
> Please don't. -stable is for serious mainline bugs people are actually
> hitting. Null pointer dereference counts, if people actually hit
> it. This is more behaviour change, and yes, the new behaviour is
> better, but it is really different class.

Sometimes the old behavior is something that is a major pain for users and
userspace.  In that case, where the new behavior fixes really annoying
usecase bugs, the fix belongs in -stable IMHO.

Broken behavior hits, by definition, every user of the feature after all.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Darren Hart]

On December 20, 2014 8:28:04 AM PST, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
>On Sat, 20 Dec 2014, Pavel Machek wrote:
>> > > > Ok, I agree that it is subjective how serious it is...
>> > > > Just to remind that patch fixing problem described in
>> > > > 
>> > > > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht
>> > > > ml
>> > > > http://www.spinics.net/lists/platform-driver-x86/msg05924.h
>> > > > tml
>> > > 
>> > > I don't have any objection to sending this back to stable.
>> > > Stable is for fixing REAL bugs, as opposed to theorhetical
>> > > races, etc. This is a "real" bug.
>> > > 
>> > > As to not chaning behavior, if it's OK for mainline, it's OK
>> > > for stable. At least that is my understanding of it. Folks
>> > > are free to verify with Greg if they disagree.
>> > 
>> > Darren, so how you decided? Now when patches are in linus tree, 
>> > are you going to send them to stable tree?
>> 
>> Please don't. -stable is for serious mainline bugs people are
>actually
>> hitting. Null pointer dereference counts, if people actually hit
>> it. This is more behaviour change, and yes, the new behaviour is
>> better, but it is really different class.
>
>Sometimes the old behavior is something that is a major pain for users
>and
>userspace.  In that case, where the new behavior fixes really annoying
>usecase bugs, the fix belongs in -stable IMHO.
>
>Broken behavior hits, by definition, every user of the feature after
>all.

How is this behavior affecting users with this hardware today? How does it manifest? Does it make for a bad or non-functional user experience? Again, I should have pressed for more complete commit messages, apologies for that.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Gabriele Mazzotta]

On Saturday 20 December 2014 08:16:54 Darren Hart wrote:
> On Sat, Dec 20, 2014 at 04:11:08PM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > > > Ok, I agree that it is subjective how serious it is...
> > > > > Just to remind that patch fixing problem described in
> > > > > 
> > > > > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht
> > > > > ml
> > > > > http://www.spinics.net/lists/platform-driver-x86/msg05924.h
> > > > > tml
> > > > 
> > > > I don't have any objection to sending this back to stable.
> > > > Stable is for fixing REAL bugs, as opposed to theorhetical
> > > > races, etc. This is a "real" bug.
> > > > 
> > > > As to not chaning behavior, if it's OK for mainline, it's OK
> > > > for stable. At least that is my understanding of it. Folks
> > > > are free to verify with Greg if they disagree.
> > > 
> > > Darren, so how you decided? Now when patches are in linus tree, 
> > > are you going to send them to stable tree?
> > 
> > Please don't. -stable is for serious mainline bugs people are actually
> > hitting. Null pointer dereference counts, if people actually hit
> > it. This is more behaviour change, and yes, the new behaviour is
> > better, but it is really different class.
> 
> In this case I agree with Pavel. While the patches are small enough, fix one
> thing each, etc, it isn't clear from the description exactly how these patches
> affect users.
> 
> If you can articulate how they are "real bugs that bother people" (see
> stable_kernel_rules.txt) we can reconsider. I should have pushed for better
> commit messages on these it appears as this should be obvious from those, but it
> isn't - at least not to me at 8:15am ;-)

The problem is that userspace programs responds to those keypresses when
they shouldn't.

In case of KEY_KBDILLUMTOGGLE, the illumination level is changed by the
BIOS, so if the keypress is reported, userspace programs will try to
toggle the keyboard illumination after the BIOS changed the level.
This is even more problematic if you consider that there could be
multiple illumination levels that are not taken into account if a
KEY_KBDILLUMTOGGLE is sent. Userspace will simply turn ON/OFF the
illumination, interfering with the BIOS.
This is shouldn't be a major problem since dell-laptop can control the
keyboard illumination only now and I can't see what userspace
application can misbehave without this change.

In the case of KEY_WLAN/KEY_RFKILL, the BIOS already takes care of
everything when the key is pressed, so sending keypresses as if
userspace programs have to do something is wrong. If for instance the
WiFi rfkill is soft blocked and I press the Fn key twice, I want it
to be soft blocked at the end. However, this is not the case.

Sorry for the brief commit messages.

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Pavel Machek]

On Sat 2014-12-20 18:02:49, Pali Rohár wrote:
> 2014-12-20 17:28 GMT+01:00 Henrique de Moraes Holschuh <hmh@hmh.eng.br>:
> 
> > On Sat, 20 Dec 2014, Pavel Machek wrote:
> > > > > > Ok, I agree that it is subjective how serious it is...
> > > > > > Just to remind that patch fixing problem described in
> > > > > >
> > > > > > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht
> > > > > > ml
> > > > > > http://www.spinics.net/lists/platform-driver-x86/msg05924.h
> > > > > > tml
> > > > >
> > > > > I don't have any objection to sending this back to stable.
> > > > > Stable is for fixing REAL bugs, as opposed to theorhetical
> > > > > races, etc. This is a "real" bug.
> > > > >
> > > > > As to not chaning behavior, if it's OK for mainline, it's OK
> > > > > for stable. At least that is my understanding of it. Folks
> > > > > are free to verify with Greg if they disagree.
> > > >
> > > > Darren, so how you decided? Now when patches are in linus tree,
> > > > are you going to send them to stable tree?
> > >
> > > Please don't. -stable is for serious mainline bugs people are actually
> > > hitting. Null pointer dereference counts, if people actually hit
> > > it. This is more behaviour change, and yes, the new behaviour is
> > > better, but it is really different class.
> >
> > Sometimes the old behavior is something that is a major pain for users and
> > userspace.  In that case, where the new behavior fixes really annoying
> > usecase bugs, the fix belongs in -stable IMHO.
> >
> > Broken behavior hits, by definition, every user of the feature after all.
> >
> > --
> >   "One disk to rule them all, One disk to find them. One disk to bring
> >   them all and in the darkness grind them. In the Land of Redmond
> >   where the shadows lie." -- The Silicon Valley Tarot
> >   Henrique Holschuh
> >
> 
> Ok, I'm asking. It's up to you how you decide. Problem with keyboard
> illumination button is that BIOS change keyboard backlight plus it send
> keycode. So if you have userspace application which listening for
> illumination button and then do something with that event (e.g change
> keyboard backlight) you will get duplicate actions (or cyclic events, etc).
> But if you think that this change should not go to stable tree, its ok. I'm
> just ask how you decide...

Well, user report "keyboard illumination does not work in GNOME
3.14159 on Mandriva 1.732 and this patch fixes it" would certainly help.

But I'm afraid that Fedora 1.4142 already knows and expects those
duplicate events.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses

[Darren Hart]

On Sat, Dec 20, 2014 at 06:03:54PM +0100, Gabriele Mazzotta wrote:
> On Saturday 20 December 2014 08:16:54 Darren Hart wrote:
> > On Sat, Dec 20, 2014 at 04:11:08PM +0100, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > > Ok, I agree that it is subjective how serious it is...
> > > > > > Just to remind that patch fixing problem described in
> > > > > > 
> > > > > > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht
> > > > > > ml
> > > > > > http://www.spinics.net/lists/platform-driver-x86/msg05924.h
> > > > > > tml
> > > > > 
> > > > > I don't have any objection to sending this back to stable.
> > > > > Stable is for fixing REAL bugs, as opposed to theorhetical
> > > > > races, etc. This is a "real" bug.
> > > > > 
> > > > > As to not chaning behavior, if it's OK for mainline, it's OK
> > > > > for stable. At least that is my understanding of it. Folks
> > > > > are free to verify with Greg if they disagree.
> > > > 
> > > > Darren, so how you decided? Now when patches are in linus tree, 
> > > > are you going to send them to stable tree?
> > > 
> > > Please don't. -stable is for serious mainline bugs people are actually
> > > hitting. Null pointer dereference counts, if people actually hit
> > > it. This is more behaviour change, and yes, the new behaviour is
> > > better, but it is really different class.
> > 
> > In this case I agree with Pavel. While the patches are small enough, fix one
> > thing each, etc, it isn't clear from the description exactly how these patches
> > affect users.
> > 
> > If you can articulate how they are "real bugs that bother people" (see
> > stable_kernel_rules.txt) we can reconsider. I should have pushed for better
> > commit messages on these it appears as this should be obvious from those, but it
> > isn't - at least not to me at 8:15am ;-)
> 
> The problem is that userspace programs responds to those keypresses when
> they shouldn't.
> 
> In case of KEY_KBDILLUMTOGGLE, the illumination level is changed by the
> BIOS, so if the keypress is reported, userspace programs will try to
> toggle the keyboard illumination after the BIOS changed the level.
> This is even more problematic if you consider that there could be
> multiple illumination levels that are not taken into account if a
> KEY_KBDILLUMTOGGLE is sent. Userspace will simply turn ON/OFF the
> illumination, interfering with the BIOS.
> This is shouldn't be a major problem since dell-laptop can control the
> keyboard illumination only now and I can't see what userspace
> application can misbehave without this change.

Agreed, this one should not go back to stable.

> 
> In the case of KEY_WLAN/KEY_RFKILL, the BIOS already takes care of
> everything when the key is pressed, so sending keypresses as if
> userspace programs have to do something is wrong. If for instance the
> WiFi rfkill is soft blocked and I press the Fn key twice, I want it
> to be soft blocked at the end. However, this is not the case.

These sound like good candidates, real bugs that bother people. I would support
sending them back to stable.

Since we didn't have this discussion before they went mainline, sorry it's been
a bad month for me :-/, these need to be sent manually. Pali, Gabriele, please
have a look at stable_kernel_rules, determine how far back these should go, and
submit the patches to the stable list.

> Sorry for the brief commit messages.
> 

They didn't bother me at the time as I saw the improvement, but they weren't
enough to make the stable decision and I need to watch out for that in the
future. Lesson learned :-)

Thanks everyone,

-- 
Darren Hart
Intel Open Source Technology Center