diff mbox

[v2,3/3] dell-wmi: Improve unknown hotkey handling

Message ID b409d60963f81611e398b492a73ffd998414cd68.1448931589.git.luto@kernel.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Andy Lutomirski Dec. 1, 2015, 1:02 a.m. UTC
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>
---
 drivers/platform/x86/dell-wmi.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

Darren Hart Dec. 3, 2015, 11:38 p.m. UTC | #1
On Mon, Nov 30, 2015 at 05:02:01PM -0800, Andy Lutomirski 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>
> ---

Please include the individual patch changelogs here under --- in the patch
itself - this helps me build confidence that I am indeed looking at the right
patch and the expected changes are here.

Pali, this appears to have the one change you asked for (0x%x instead of %d).
I've added the Reviewed-by: Pali Rohár <pali.rohar@gmail.com> you provided
previously pending this change.

Queued to testing, thanks.

>  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 baff658a3621..7c3ebda811ca 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,
> @@ -191,7 +192,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;
>  	}
>  
> @@ -350,9 +352,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. */
> +		u16 keycode = (bios_entry->keycode <
> +			       ARRAY_SIZE(bios_to_linux_keycode)) ?
> +			bios_to_linux_keycode[bios_entry->keycode] :
> +			KEY_RESERVED;
> +		BUILD_BUG_ON(KEY_RESERVED != 0);
> +
> +		/*
> +		 * 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 0x%x maps to unrecognized keycode 0x%x\n",
> +				bios_entry->scancode, bios_entry->keycode);
> +			continue;
> +		}
>  
>  		if (keycode == KEY_KBDILLUMTOGGLE)
>  			keymap[pos].type = KE_IGNORE;
> -- 
> 2.5.0
> 
>
Andy Lutomirski Dec. 3, 2015, 11:45 p.m. UTC | #2
On Thu, Dec 3, 2015 at 3:38 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Mon, Nov 30, 2015 at 05:02:01PM -0800, Andy Lutomirski 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>
>> ---
>
> Please include the individual patch changelogs here under --- in the patch
> itself - this helps me build confidence that I am indeed looking at the right
> patch and the expected changes are here.

Will do in the future -- different maintainers have different
preferences here, I think.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darren Hart Dec. 4, 2015, 12:07 a.m. UTC | #3
On Thu, Dec 03, 2015 at 03:45:11PM -0800, Andy Lutomirski wrote:
> On Thu, Dec 3, 2015 at 3:38 PM, Darren Hart <dvhart@infradead.org> wrote:
> > On Mon, Nov 30, 2015 at 05:02:01PM -0800, Andy Lutomirski 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>
> >> ---
> >
> > Please include the individual patch changelogs here under --- in the patch
> > itself - this helps me build confidence that I am indeed looking at the right
> > patch and the expected changes are here.
> 
> Will do in the future -- different maintainers have different
> preferences here, I think.

Oh really?

This is the documented process in Documentation/SubmittingPatches Section 14)
The canonical patch format...
Andy Lutomirski Dec. 4, 2015, 12:10 a.m. UTC | #4
On Thu, Dec 3, 2015 at 4:07 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Thu, Dec 03, 2015 at 03:45:11PM -0800, Andy Lutomirski wrote:
>> On Thu, Dec 3, 2015 at 3:38 PM, Darren Hart <dvhart@infradead.org> wrote:
>> > On Mon, Nov 30, 2015 at 05:02:01PM -0800, Andy Lutomirski 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>
>> >> ---
>> >
>> > Please include the individual patch changelogs here under --- in the patch
>> > itself - this helps me build confidence that I am indeed looking at the right
>> > patch and the expected changes are here.
>>
>> Will do in the future -- different maintainers have different
>> preferences here, I think.
>
> Oh really?
>
> This is the documented process in Documentation/SubmittingPatches Section 14)
> The canonical patch format...
>

Ingo and the other -tip maintainers seem to like all the comments up
front on the cover letter.  Or at least they haven't complained yet.

Anyway, I'm giving 'git notes' a try to generate nice change comments for v3.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darren Hart Dec. 4, 2015, 12:21 a.m. UTC | #5
On Thu, Dec 03, 2015 at 04:10:49PM -0800, Andy Lutomirski wrote:
> On Thu, Dec 3, 2015 at 4:07 PM, Darren Hart <dvhart@infradead.org> wrote:
> > On Thu, Dec 03, 2015 at 03:45:11PM -0800, Andy Lutomirski wrote:
> >> On Thu, Dec 3, 2015 at 3:38 PM, Darren Hart <dvhart@infradead.org> wrote:
> >> > On Mon, Nov 30, 2015 at 05:02:01PM -0800, Andy Lutomirski 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>
> >> >> ---
> >> >
> >> > Please include the individual patch changelogs here under --- in the patch
> >> > itself - this helps me build confidence that I am indeed looking at the right
> >> > patch and the expected changes are here.
> >>
> >> Will do in the future -- different maintainers have different
> >> preferences here, I think.
> >
> > Oh really?
> >
> > This is the documented process in Documentation/SubmittingPatches Section 14)
> > The canonical patch format...
> >
> 
> Ingo and the other -tip maintainers seem to like all the comments up
> front on the cover letter.  Or at least they haven't complained yet.
> 

Ugh, well, I don't like to ask contributors to treat this subsystem special by
sticking to the documentation as much as possible. I really do find the
per-patch changelog to be very useful. Thanks for working with me on that.

> Anyway, I'm giving 'git notes' a try to generate nice change comments for v3.

If you get a good solution, that would be nice to add to the documentation to
help automate the process. Anything we can do to avoid mechanical errors here is
goodness.
Pali Rohár Dec. 4, 2015, 8:39 a.m. UTC | #6
On Thursday 03 December 2015 15:38:56 Darren Hart wrote:
> Pali, this appears to have the one change you asked for (0x%x instead of %d).
> I've added the Reviewed-by: Pali Rohár <pali.rohar@gmail.com> you provided
> previously pending this change.

Ok.

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

Anyway, Darren what do you think is that BUILD_BUG_ON check needed?
Andy Lutomirski Dec. 4, 2015, 4:15 p.m. UTC | #7
On Fri, Dec 4, 2015 at 12:39 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Thursday 03 December 2015 15:38:56 Darren Hart wrote:
>> Pali, this appears to have the one change you asked for (0x%x instead of %d).
>> I've added the Reviewed-by: Pali Rohár <pali.rohar@gmail.com> you provided
>> previously pending this change.
>
> Ok.
>
>> > +           /* Uninitialized entries are 0 aka KEY_RESERVED. */
>> > +           u16 keycode = (bios_entry->keycode <
>> > +                          ARRAY_SIZE(bios_to_linux_keycode)) ?
>> > +                   bios_to_linux_keycode[bios_entry->keycode] :
>> > +                   KEY_RESERVED;
>> > +           BUILD_BUG_ON(KEY_RESERVED != 0);
>
> Anyway, Darren what do you think is that BUILD_BUG_ON check needed?
>

Sorry, my bad.  IIRC Darren made that change, I made the other change,
and then I resent and we lost this one.

Darren, want to fix it in your tree?  If not, I'll include the
BUILD_BUG_ON deletion in v3.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darren Hart Dec. 9, 2015, 12:33 a.m. UTC | #8
On Fri, Dec 04, 2015 at 08:15:04AM -0800, Andy Lutomirski wrote:
> On Fri, Dec 4, 2015 at 12:39 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Thursday 03 December 2015 15:38:56 Darren Hart wrote:
> >> Pali, this appears to have the one change you asked for (0x%x instead of %d).
> >> I've added the Reviewed-by: Pali Rohár <pali.rohar@gmail.com> you provided
> >> previously pending this change.
> >
> > Ok.
> >
> >> > +           /* Uninitialized entries are 0 aka KEY_RESERVED. */
> >> > +           u16 keycode = (bios_entry->keycode <
> >> > +                          ARRAY_SIZE(bios_to_linux_keycode)) ?
> >> > +                   bios_to_linux_keycode[bios_entry->keycode] :
> >> > +                   KEY_RESERVED;
> >> > +           BUILD_BUG_ON(KEY_RESERVED != 0);
> >
> > Anyway, Darren what do you think is that BUILD_BUG_ON check needed?
> >
> 
> Sorry, my bad.  IIRC Darren made that change, I made the other change,
> and then I resent and we lost this one.
> 
> Darren, want to fix it in your tree?  If not, I'll include the
> BUILD_BUG_ON deletion in v3.

Thanks for the catch Pali. I've removed this in my tree. No need to resend.
diff mbox

Patch

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index baff658a3621..7c3ebda811ca 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,
@@ -191,7 +192,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;
 	}
 
@@ -350,9 +352,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. */
+		u16 keycode = (bios_entry->keycode <
+			       ARRAY_SIZE(bios_to_linux_keycode)) ?
+			bios_to_linux_keycode[bios_entry->keycode] :
+			KEY_RESERVED;
+		BUILD_BUG_ON(KEY_RESERVED != 0);
+
+		/*
+		 * 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 0x%x maps to unrecognized keycode 0x%x\n",
+				bios_entry->scancode, bios_entry->keycode);
+			continue;
+		}
 
 		if (keycode == KEY_KBDILLUMTOGGLE)
 			keymap[pos].type = KE_IGNORE;