| Message ID | b409d60963f81611e398b492a73ffd998414cd68.1448931589.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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 > >
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 platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
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 platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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?
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 platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 --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;
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(-)