thinkpad_acpi generated EV_KEY events are mssing scancodes

This is probably an one-line patch in thinkpad_acpi, and would allow me to provide much better hotkey support for ThinkPads, so I take the liberty to assign this to the kernel team (Andy confirmed that we really want to fix this keind of bugs).

Thanks!

thinkpad_acpi does send scancodes, but only when it's reporting KEY_UNKNOWN, and then it sends the scancode after the keycode. keymap seems to expect that there's always a scancode and that it will always come before the keycode. Do you know if there's any requirement to send the scancode or for the order of the events? I do see a number of drivers that don't send scancodes, but of those that do all that I looked at except thinkpad_acpi send the scancode before the keycode.

Maybe keymap needs to wait for the sync event before printing make sure it receives both events?

> thinkpad_acpi does send scancodes, but only when it's reporting KEY_UNKNOWN

Ah, that would be part of the problem. In the reported cases, the faulty keys didn't have an unknown, but a wrong key code assigned.

To be honest I'm not quite sure what you mean by the ordering issue. From what I understand, the input_event struct has both the scan and the key code, and there's one event sent with both? But even if there are two separate events for this, keymap -i just prints any incoming EV_KEY event, it doesn't discard any of them.

On Fri, Jan 14, 2011 at 06:30:12PM -0000, Martin Pitt wrote:
> > thinkpad_acpi does send scancodes, but only when it's reporting
> KEY_UNKNOWN
>
> Ah, that would be part of the problem. In the reported cases, the faulty
> keys didn't have an unknown, but a wrong key code assigned.

Right, keymap just keeps the scan code in a static variable, so if the
driver doesn't send a scan code you just get whatever the last reported
scan code was.

> To be honest I'm not quite sure what you mean by the ordering issue.
> From what I understand, the input_event struct has both the scan and the
> key code, and there's one event sent with both? But even if there are
> two separate events for this, keymap -i just prints any incoming EV_KEY
> event, it doesn't discard any of them.

No, it's one event for the scan code (type = EV_MSC, code = MSC_SCAN,
value = scan code) and another for the key code (type = EV_KEY, code =
key code, value = state), and then a third that's a sync event (type =
EV_SYN, code = SYN_REPORT). The usual sequence is EV_MSC, EV_KEY,
EV_SYN, but thinkpad_acpi does either EV_KEY, EV_SYN or EV_KEY, EV_MSC,
EV_SYN, which are both problematic for keymap.

I'm getting ready to test a patch for thinkpad_acpi that will always
send the scan code, which I'll send upstream once I've verified it
works. This is what you need for the FN_F8 case you mentioned. It also
reorders the events to be consistent with what all the other drivers are
doing, so the current keymap should work properly.

I also hacked up a change to keymap that prints on the sync event
instead of the key event. Now I get output more like:

scan code: none key code: prog1
scan code: 0x1A key code: unknown
scan code: none key code: fn_f11
scan code: none key code: f24

I need to verify that drivers always send a sync event though; if not
keymap needs to print and reset the state for key events without
sync events in between.

Ah, thanks Seth! I was never really clear that I actually need for a series of events and a SYN. If some drivers don't send SYN events, then it becomes highly nontrivial to do a correct operation then, as you'd need to wait for a SYN, some events before, and time out if no SYN arrives?

Thanks Seth! I implemented the more robust state machine in http://git.kernel.org/?p=linux/hotplug/udev.git;a=commit;h=90f182c3d69a4451bb1ea6f79d8d7fe4895cdf89 now.

I confirm that in the case of known key codes, the thinkpad_acpi driver is not reporting scan codes, but it should; otherwise you can never fix broken key codes like KEY_FN_F8. (Note that using those at all is deprecated, and drivers should rather use UNKNOWN indeed -- see http://linux.derkeiler.com/Mailing-Lists/Kernel/2007-06/msg00012.html ff).

I've sent a patch for thinkpad_acpi to always produce scan codes upstream and received an ack from the maintainer. It also reverses the order of the codes so that versions of keymap without your new state machine will work with thinkpad_acpi as well.

Attaching the patch for thinkpad_acpi that was submitted upstream.

This bug was fixed in the package linux - 2.6.38-1.27

---------------
linux (2.6.38-1.27) natty; urgency=low

  [ Andy Whitcroft ]

  * ubuntu: AUFS -- update aufs-update to track new locations of headers
  * ubuntu: AUFS -- update to c5021514085a5d96364e096dbd34cadb2251abfd
  * SAUCE: ensure root is ready before running usermodehelpers in it
  * correct the Vcs linkage to point to natty
  * rebase to linux tip e78bf5e6cbe837daa6ab628a5f679548742994d3
  * [Config] update configs following rebase
    e78bf5e6cbe837daa6ab628a5f679548742994d3
  * SAUCE: Yama: follow changes to generic_permission
  * ubuntu: compcache -- follow changes to bd_claim/bd_release
  * ubuntu: iscsitarget -- follow changes to open_bdev_exclusive
  * ubuntu: ndiswrapper -- fix interaction between __packed and packed
  * ubuntu: AUFS -- update to 806051bcbeec27748aae2b7957726a4e63ff308e
  * update package version to match payload version
  * rebase to e6f597a1425b5af64917be3448b29e2d5a585ac8
  * rebase to v2.6.38-rc1
  * [Config] updateconfigs following rebase to v2.6.38-rc1
  * SAUCE: x86 fix up jiffies/jiffies_64 handling
  * rebase to linus tip 2b1caf6ed7b888c95a1909d343799672731651a5
  * [Config] updateconfigs following rebase to
    2b1caf6ed7b888c95a1909d343799672731651a5
  * [Config] disable CONFIG_TRANSPARENT_HUGEPAGE to fix i386 boot crashes
  * ubuntu: AUFS -- suppress benign plink warning messages
    - LP: #621195
  * [Config] CONFIG_NR_CPUS=256 for amd64 -server flavour
  * rebase to v2.6.38-rc2
  * rebase to mainline d315777b32a4696feb86f2a0c9e9f39c94683649
  * rebase to c723fdab8aa728dc2bf0da6a0de8bb9c3f588d84
  * [Config] update configs following rebase to
    c723fdab8aa728dc2bf0da6a0de8bb9c3f588d84
  * [Config] disable CONFIG_AD7152 to fix FTBS on armel versatile
  * [Config] disable CONFIG_AD7150 to fix FTBS on armel versatile
  * [Config] disable CONFIG_RTL8192CE to fix FTBS on armel omap
  * [Config] disable CONFIG_MANTIS_CORE to fix FTBS on armel versatile

  [ Kees Cook ]

  * SAUCE: kernel: make /proc/kallsyms mode 400 to reduce ease of attacking

  [ Stefan Bader ]

  * Temporarily disable RODATA for virtual i386
    - LP: #699828

  [ Tim Gardner ]

  * [Config] CONFIG_NLS_DEFAULT=utf8
    - LP: #683690
  * [Config] CONFIG_HIBERNATION=n
  * update bnx2 firmware files in d-i/firmware/nic-modules

  [ Upstream Kernel Changes ]

  * Revert "drm/radeon/bo: add some fallback placements for VRAM only
    objects."
  * packaging: make System.map mode 0600
  * thinkpad_acpi: Always report scancodes for hotkeys
    - LP: #702407
  * sched: tg->se->load should be initialised to tg->shares
  * Input: sysrq -- ensure sysrq_enabled and __sysrq_enabled are consistent
  * brcm80211: include linux/slab.h for kfree
  * pch_dma: add include/slab.h for kfree
  * i2c-eg20t: include linux/slab.h for kfree
  * gpio/ml_ioh_gpio: include linux/slab.h for kfree
  * tty: include linux/slab.h for kfree
  * winbond: include linux/delay.h for mdelay et al

  [ Upstream Kernel Changes ]

  * mark the start of v2.6.38 versioning
  * rebase v2.6.37 to v2.6.38-rc2 + c723fdab8aa728dc2bf0da6a0de8bb9c3f588d84
    - LP: #689886
    - LP: #702125
    - LP: #608775
    - LP: #215802
...

This bug was fixed in the package udev - 166-0ubuntu1

---------------
udev (166-0ubuntu1) natty; urgency=low

  * New upstream release:
    - 8 keymap fixes, including LP: #702426
    - Fix keymap tool to display scan codes in unexpected event order.
      (LP: #702407)
    - Bug fixes.
  * Merge from trunk:
    - Create by-id links with interface numbers for USB input devices with
      multiple interfaces. Thanks a7x! (LP: #626449)
    - Drop old v4l1 code, to build with current 2.6.38 kernels.
  * debian/rules prep: Drop gtk-doc.make seddery, it's not necessary any more
    with the full source tree copying that happens now.
 -- Martin Pitt <email address hidden> Tue, 15 Feb 2011 18:22:47 +0100