[PATCH v2] dell-wmi: Improve unknown hotkey handling

[PATCH v2] dell-wmi: Improve unknown hotkey handling

[Andy Lutomirski]

If DMI lists a hotkey that we don't recognize, log and ignore it
instead of trying to map it to keycode 0.  I haven't seen this happen,
but it will help maintain the key map in the future and it will help
avoid sending bogus events.

This also improves the message that we log when we get an unknown key
event.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Changes from v1:
 - Use KEY_RESERVED instead of zero and document why that's okay
 - Fix scancode vs keycode confusion in the log message (whoops!)
 - Switch from hardcoded 256 to ARRAY_SIZE

 drivers/platform/x86/dell-wmi.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index d2daf5417cd7..cb96ef03fa79 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -118,6 +118,7 @@ struct dell_bios_hotkey_table {
 
 static const struct dell_bios_hotkey_table *dell_bios_hotkey_table;
 
+/* Uninitialized entries here are KEY_RESERVED == 0. */
 static const u16 bios_to_linux_keycode[256] __initconst = {
 	[0]	= KEY_MEDIA,
 	[1]	= KEY_NEXTSONG,
@@ -180,7 +181,8 @@ static void dell_wmi_process_key(int reported_key)
 	key = sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
 						reported_key);
 	if (!key) {
-		pr_info("Unknown key %x pressed\n", reported_key);
+		pr_info("Unknown key with scancode 0x%x pressed\n",
+			reported_key);
 		return;
 	}
 
@@ -339,9 +341,24 @@ 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];
-		u16 keycode = bios_entry->keycode < 256 ?
-				    bios_to_linux_keycode[bios_entry->keycode] :
-				    KEY_RESERVED;
+
+		/* Uninitialized entries are 0 aka KEY_RESERVED. */
+		BUILD_BUG_ON(KEY_RESERVED != 0);
+		u16 keycode = (bios_entry->keycode <
+			       ARRAY_SIZE(bios_to_linux_keycode)) ?
+			bios_to_linux_keycode[bios_entry->keycode] :
+			KEY_RESERVED;
+
+		/*
+		 * Log if we find an entry in the DMI table that we don't
+		 * understand.  If this happens, we should figure out what
+		 * the entry means and add it to bios_to_linux_keycode.
+		 */
+		if (keycode == KEY_RESERVED) {
+			pr_info("firmware scancode %d maps to unrecognized keycode %d\n",
+				bios_entry->scancode, bios_entry->keycode);
+			continue;
+		}
 
 		if (keycode == KEY_KBDILLUMTOGGLE)
 			keymap[pos].type = KE_IGNORE;
-- 
2.4.3

Re: [PATCH v2] dell-wmi: Improve unknown hotkey handling

[Andy Lutomirski]

On Fri, Nov 20, 2015 at 5:27 PM, Andy Lutomirski <luto@kernel.org> wrote:
> If DMI lists a hotkey that we don't recognize, log and ignore it
> instead of trying to map it to keycode 0.  I haven't seen this happen,
> but it will help maintain the key map in the future and it will help
> avoid sending bogus events.
>
> This also improves the message that we log when we get an unknown key
> event.
>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>
> Changes from v1:
>  - Use KEY_RESERVED instead of zero and document why that's okay
>  - Fix scancode vs keycode confusion in the log message (whoops!)
>  - Switch from hardcoded 256 to ARRAY_SIZE
>
>  drivers/platform/x86/dell-wmi.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index d2daf5417cd7..cb96ef03fa79 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c


> +               /* Uninitialized entries are 0 aka KEY_RESERVED. */
> +               BUILD_BUG_ON(KEY_RESERVED != 0);
> +               u16 keycode = (bios_entry->keycode <
> +                              ARRAY_SIZE(bios_to_linux_keycode)) ?
> +                       bios_to_linux_keycode[bios_entry->keycode] :
> +                       KEY_RESERVED;

Oops.  BUILD_BUG_ON should be below u16 keycode = ... to avoid a
warning.  Feel free to fix it up.  I can also send a v3.

--Andy

Re: [PATCH v2] dell-wmi: Improve unknown hotkey handling

[Pali Rohár]

On Friday 20 November 2015 17:30:13 Andy Lutomirski wrote:
> On Fri, Nov 20, 2015 at 5:27 PM, Andy Lutomirski <luto@kernel.org> wrote:
> > If DMI lists a hotkey that we don't recognize, log and ignore it
> > instead of trying to map it to keycode 0.  I haven't seen this happen,
> > but it will help maintain the key map in the future and it will help
> > avoid sending bogus events.
> >
> > This also improves the message that we log when we get an unknown key
> > event.
> >
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > ---
> >
> > Changes from v1:
> >  - Use KEY_RESERVED instead of zero and document why that's okay
> >  - Fix scancode vs keycode confusion in the log message (whoops!)
> >  - Switch from hardcoded 256 to ARRAY_SIZE
> >
> >  drivers/platform/x86/dell-wmi.c | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index d2daf5417cd7..cb96ef03fa79 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> 
> 
> > +               /* Uninitialized entries are 0 aka KEY_RESERVED. */
> > +               BUILD_BUG_ON(KEY_RESERVED != 0);
> > +               u16 keycode = (bios_entry->keycode <
> > +                              ARRAY_SIZE(bios_to_linux_keycode)) ?
> > +                       bios_to_linux_keycode[bios_entry->keycode] :
> > +                       KEY_RESERVED;
> 
> Oops.  BUILD_BUG_ON should be below u16 keycode = ... to avoid a
> warning.  Feel free to fix it up.  I can also send a v3.

KEY_RESERVED is zero by definition and exported to user space. So this
should not be redefined otherwise Linux ABI will be broken too.

So I think BUILD_BUG_ON is not needed there.

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

Re: [PATCH v2] dell-wmi: Improve unknown hotkey handling

[Darren Hart]

On Mon, Nov 23, 2015 at 03:56:43PM +0100, Pali Rohár wrote:
> On Friday 20 November 2015 17:30:13 Andy Lutomirski wrote:
> > On Fri, Nov 20, 2015 at 5:27 PM, Andy Lutomirski <luto@kernel.org> wrote:
> > > If DMI lists a hotkey that we don't recognize, log and ignore it
> > > instead of trying to map it to keycode 0.  I haven't seen this happen,
> > > but it will help maintain the key map in the future and it will help
> > > avoid sending bogus events.
> > >
> > > This also improves the message that we log when we get an unknown key
> > > event.
> > >
> > > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > > ---
> > >
> > > Changes from v1:
> > >  - Use KEY_RESERVED instead of zero and document why that's okay
> > >  - Fix scancode vs keycode confusion in the log message (whoops!)
> > >  - Switch from hardcoded 256 to ARRAY_SIZE
> > >
> > >  drivers/platform/x86/dell-wmi.c | 25 +++++++++++++++++++++----
> > >  1 file changed, 21 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > > index d2daf5417cd7..cb96ef03fa79 100644
> > > --- a/drivers/platform/x86/dell-wmi.c
> > > +++ b/drivers/platform/x86/dell-wmi.c
> > 
> > 
> > > +               /* Uninitialized entries are 0 aka KEY_RESERVED. */
> > > +               BUILD_BUG_ON(KEY_RESERVED != 0);
> > > +               u16 keycode = (bios_entry->keycode <
> > > +                              ARRAY_SIZE(bios_to_linux_keycode)) ?
> > > +                       bios_to_linux_keycode[bios_entry->keycode] :
> > > +                       KEY_RESERVED;
> > 
> > Oops.  BUILD_BUG_ON should be below u16 keycode = ... to avoid a
> > warning.  Feel free to fix it up.  I can also send a v3.
> 
> KEY_RESERVED is zero by definition and exported to user space. So this
> should not be redefined otherwise Linux ABI will be broken too.
> 
> So I think BUILD_BUG_ON is not needed there.

Queued to testing sans BUILD_BUG_ON. Pali, any further concerns? If not, please
provide a reviewed-by.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] dell-wmi: Improve unknown hotkey handling

[Andy Lutomirski]

On Mon, Nov 23, 2015 at 10:47 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Mon, Nov 23, 2015 at 03:56:43PM +0100, Pali Rohár wrote:
>> On Friday 20 November 2015 17:30:13 Andy Lutomirski wrote:
>> > On Fri, Nov 20, 2015 at 5:27 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> > > If DMI lists a hotkey that we don't recognize, log and ignore it
>> > > instead of trying to map it to keycode 0.  I haven't seen this happen,
>> > > but it will help maintain the key map in the future and it will help
>> > > avoid sending bogus events.
>> > >
>> > > This also improves the message that we log when we get an unknown key
>> > > event.
>> > >
>> > > Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> > > ---
>> > >
>> > > Changes from v1:
>> > >  - Use KEY_RESERVED instead of zero and document why that's okay
>> > >  - Fix scancode vs keycode confusion in the log message (whoops!)
>> > >  - Switch from hardcoded 256 to ARRAY_SIZE
>> > >
>> > >  drivers/platform/x86/dell-wmi.c | 25 +++++++++++++++++++++----
>> > >  1 file changed, 21 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> > > index d2daf5417cd7..cb96ef03fa79 100644
>> > > --- a/drivers/platform/x86/dell-wmi.c
>> > > +++ b/drivers/platform/x86/dell-wmi.c
>> >
>> >
>> > > +               /* Uninitialized entries are 0 aka KEY_RESERVED. */
>> > > +               BUILD_BUG_ON(KEY_RESERVED != 0);
>> > > +               u16 keycode = (bios_entry->keycode <
>> > > +                              ARRAY_SIZE(bios_to_linux_keycode)) ?
>> > > +                       bios_to_linux_keycode[bios_entry->keycode] :
>> > > +                       KEY_RESERVED;
>> >
>> > Oops.  BUILD_BUG_ON should be below u16 keycode = ... to avoid a
>> > warning.  Feel free to fix it up.  I can also send a v3.
>>
>> KEY_RESERVED is zero by definition and exported to user space. So this
>> should not be redefined otherwise Linux ABI will be broken too.
>>
>> So I think BUILD_BUG_ON is not needed there.
>
> Queued to testing sans BUILD_BUG_ON. Pali, any further concerns? If not, please
> provide a reviewed-by.
>

Removing the BUILD_BUG_ON is fine with me.

--Andy

Re: [PATCH v2] dell-wmi: Improve unknown hotkey handling

[Pali Rohár]

On Friday 20 November 2015 17:27:00 Andy Lutomirski wrote:
> +		if (keycode == KEY_RESERVED) {
> +			pr_info("firmware scancode %d maps to unrecognized keycode %d\n",
> +				bios_entry->scancode, bios_entry->keycode);
> +			continue;
> +		}

Please use "0x%x" instead "%d" for those scan and key codes. Other parts
in driver use "0x%x" so that new info message could be more confused if
it has decimal codes...

With that fix, you can add my:
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

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

Re: [PATCH v2] dell-wmi: Improve unknown hotkey handling

[Darren Hart]

On Mon, Nov 23, 2015 at 08:47:10PM +0100, Pali Rohár wrote:
> On Friday 20 November 2015 17:27:00 Andy Lutomirski wrote:
> > +		if (keycode == KEY_RESERVED) {
> > +			pr_info("firmware scancode %d maps to unrecognized keycode %d\n",
> > +				bios_entry->scancode, bios_entry->keycode);
> > +			continue;
> > +		}
> 
> Please use "0x%x" instead "%d" for those scan and key codes. Other parts
> in driver use "0x%x" so that new info message could be more confused if
> it has decimal codes...
> 
> With that fix, you can add my:
> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

Andy, will you be resending with this change?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] dell-wmi: Improve unknown hotkey handling

[Andy Lutomirski]

On Mon, Nov 30, 2015 at 10:27 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Mon, Nov 23, 2015 at 08:47:10PM +0100, Pali Rohár wrote:
>> On Friday 20 November 2015 17:27:00 Andy Lutomirski wrote:
>> > +           if (keycode == KEY_RESERVED) {
>> > +                   pr_info("firmware scancode %d maps to unrecognized keycode %d\n",
>> > +                           bios_entry->scancode, bios_entry->keycode);
>> > +                   continue;
>> > +           }
>>
>> Please use "0x%x" instead "%d" for those scan and key codes. Other parts
>> in driver use "0x%x" so that new info message could be more confused if
>> it has decimal codes...
>>
>> With that fix, you can add my:
>> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
>
> Andy, will you be resending with this change?

Sure.

--Andy