thinkpad_acpi generated EV_KEY events are mssing scancodes

Bug #702407 reported by Martin Pitt on 2011-01-13
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
linux (Ubuntu)
Undecided
Seth Forshee
udev (Ubuntu)
Medium
Martin Pitt

Bug Description

Being the udev keymap maintainer, I get a lot of bug reports about wrong scancode -> keycode maps on a lot of laptops (see [1], [2] for details). For fixing these, the user needs to start the udev keymap tool in interactive mode, press the faulty keys, note the scan code, and tell me what the desired key code is.

In about 90% of the bug reports this works fine, but there are some cases where the evdev key events have null scan codes, such as on current ThinkPads:

$ /lib/udev/findkeyboards
AT keyboard: input/event3
module: input/event4

$ sudo ./keymap -i input/event4
Press ESC to finish
scan code: 0x00 key code: fn_f8
scan code: 0x00 key code: fn_f9

I. e. when pressing Fn+F8, I get the key code KEY_FN_F8 which is not particularly useful; I want something like KEY_VOLUMEDOWN. But as the scan code is zero, I have no way of fixing the faulty key.

I confirmed this on a few other thinkpads. Apparently the thinkpad_acpi driver doesn't copy the scan code of keys to the evdev struct, only the converted keycode.

The original ACPI events for the same keys (Fn+F8 and Fn+F9):

$ acpi_listen
ibm/hotkey HKEY 00000080 00001008
ibm/hotkey HKEY 00000080 00001009

[1] http://git.kernel.org/?p=linux/hotplug/udev.git;a=blob;f=extras/keymap/README.keymap.txt;hb=HEAD
[2] https://wiki.ubuntu.com/Hotkeys/Troubleshooting

Martin Pitt (pitti) wrote :

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!

Changed in linux (Ubuntu):
assignee: nobody → Canonical Kernel Team (canonical-kernel-team)
status: New → Triaged
Seth Forshee (sforshee) wrote :

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?

Martin Pitt (pitti) 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.

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.

Martin Pitt (pitti) wrote :

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?

Martin Pitt (pitti) on 2011-01-16
Changed in udev (Ubuntu):
assignee: nobody → Martin Pitt (pitti)
importance: Undecided → Medium
status: New → In Progress
Martin Pitt (pitti) wrote :

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).

Changed in udev (Ubuntu):
status: In Progress → Fix Committed
Seth Forshee (sforshee) wrote :

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.

Changed in linux (Ubuntu):
assignee: Canonical Kernel Team (canonical-kernel-team) → Seth Forshee (sforshee)
Seth Forshee (sforshee) wrote :

Attaching the patch for thinkpad_acpi that was submitted upstream.

Andy Whitcroft (apw) on 2011-01-19
Changed in linux (Ubuntu):
status: Triaged → Fix Committed
Launchpad Janitor (janitor) wrote :
Download full text (3.2 KiB)

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
...

Read more...

Changed in linux (Ubuntu):
status: Fix Committed → Fix Released
Launchpad Janitor (janitor) wrote :

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

Changed in udev (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers