[PATCH] keymap: support for force_release quirk

[PATCH] keymap: support for force_release quirk

[Johannes Stezenbach]

Hi,

I got no response for my last patch so I just continued trying
to get it into a mergable state. I tested it and it works for me.

Changes from last version:
- put force-release key list into extras/keymap/keymaps/force-release/
- use relative path to find force-release key list file
  in the keyboard-force-release.sh script
- add script, rule and key list file in Makefile.am


Thanks
Johannes


From 501d9230390886afc7d98b2d1c33288aa6b1fa7e Mon Sep 17 00:00:00 2001
From: Johannes Stezenbach <js@sig21.net>
Date: Sun, 13 Dec 2009 23:54:05 +0100
Subject: [PATCH] keymap: handle atkbd force_release quirk

Some laptop keyboards don't generate release events
for some hotkeys.  Since linux-2.6.32 the list of scancodes
for which to enable the force_release quirk can be set
via sysfs.
Apply this to Samsung N130.

Signed-off-by: Johannes Stezenbach <js@sig21.net>
---
 Makefile.am                                       |    6 +++++
 extras/keymap/95-keyboard-force-release.rules     |   24 +++++++++++++++++++++
 extras/keymap/keyboard-force-release.sh           |   22 +++++++++++++++++++
 extras/keymap/keymaps/force-release/samsung-other |   10 ++++++++
 4 files changed, 62 insertions(+), 0 deletions(-)
 create mode 100644 extras/keymap/95-keyboard-force-release.rules
 create mode 100644 extras/keymap/keyboard-force-release.sh
 create mode 100644 extras/keymap/keymaps/force-release/samsung-other

diff --git a/Makefile.am b/Makefile.am
index 3e9713f..4435398 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -480,8 +480,10 @@ BUILT_SOURCES += $(nodist_extras_keymap_keymap_SOURCES)
 
 EXTRA_DIST += extras/keymap/check-keymaps.sh
 dist_udevrules_DATA += extras/keymap/95-keymap.rules
+dist_udevrules_DATA += extras/keymap/95-keyboard-force-release.rules
 libexec_PROGRAMS += extras/keymap/keymap
 dist_libexec_SCRIPTS += extras/keymap/findkeyboards
+dist_libexec_SCRIPTS += extras/keymap/keyboard-force-release.sh
 dist_doc_DATA = extras/keymap/README.keymap.txt
 
 CLEANFILES += \
@@ -535,6 +537,10 @@ dist_udevkeymap_DATA = \
 	extras/keymap/keymaps/toshiba-satellite_a110 \
 	extras/keymap/keymaps/zepto-znote
 
+udevkeymapforcereldir = $(libexecdir)/keymaps/force-release
+dist_udevkeymapforcerel_DATA = \
+	extras/keymap/keymaps/force-release/samsung-other
+
 extras/keymap/keys.txt: /usr/include/linux/input.h
 	$(AM_V_GEN)$(AWK) '/^#define.*KEY_[^ ]+[ \t]+[0-9]/ { if ($$2 != "KEY_MAX") { print $$2 } }' < $< | sed 's/^KEY_COFFEE$$/KEY_SCREENLOCK/' > $@
 
diff --git a/extras/keymap/95-keyboard-force-release.rules b/extras/keymap/95-keyboard-force-release.rules
new file mode 100644
index 0000000..8771a1e
--- /dev/null
+++ b/extras/keymap/95-keyboard-force-release.rules
@@ -0,0 +1,24 @@
+# Set model specific atkbd force_release quirk
+#
+# Serveral laptops have hotkeys which don't generate release events,
+# which can cause problems with software key repeat.
+# The atkbd driver has a quirk handler for generating synthetic
+# release events, which can be configured via sysfs since 2.6.32.
+# Simply add a file with a list of scancodes for your laptop model
+# in /lib/udev/keymaps, and add a rule here.
+# If the hotkeys also need a keymap assignment you can copy the
+# scancodes from the keymap file, otherwise you can run
+# /lib/udev/keymap -i /dev/input/eventX
+# on a Linux vt to find out.
+
+ACTION!="add", GOTO="force_release_end"
+SUBSYSTEM!="serio", GOTO="force_release_end"
+KERNEL!="serio*", GOTO="force_release_end"
+DRIVER!="atkbd", GOTO="force_release_end"
+
+ENV{DMI_VENDOR}="$attr{[dmi/id]sys_vendor}"
+
+
+ENV{DMI_VENDOR}="[sS][aA][mM][sS][uU][nN][gG]*", ATTR{[dmi/id]product_name}="*N130*", RUN+="keyboard-force-release.sh $sys$devpath samsung-other"
+
+LABEL="force_release_end"
diff --git a/extras/keymap/keyboard-force-release.sh b/extras/keymap/keyboard-force-release.sh
new file mode 100644
index 0000000..5abb016
--- /dev/null
+++ b/extras/keymap/keyboard-force-release.sh
@@ -0,0 +1,22 @@
+#!/bin/sh -e
+# read list of scancodes, convert hex to decimal and
+# append to the atkbd force_release sysfs attribute
+# $1 sysfs devpath for serioX
+# $2 file with scancode list (hex or dec)
+
+case "$2" in
+	/*) scf="$2" ;;
+	*)  scf="${0%keyboard-force-release.sh}/keymaps/force-release/$2" ;;
+esac
+
+attr=`cat "$1/force_release"`
+while read scancode dummy; do
+	case "$scancode" in
+		\#*) ;;
+		*)
+			scancode=$(($scancode))
+			[ -n "$attr" ] && attr="$attr,$scancode" || attr="$scancode"
+			;;
+	esac
+done <"$scf"
+echo "$attr" >"$1/force_release"
diff --git a/extras/keymap/keymaps/force-release/samsung-other b/extras/keymap/keymaps/force-release/samsung-other
new file mode 100644
index 0000000..c51123a
--- /dev/null
+++ b/extras/keymap/keymaps/force-release/samsung-other
@@ -0,0 +1,10 @@
+# list of scancodes (hex or decimal), optional comment
+0x82 # Fn+F4 CRT/LCD
+0x83 # Fn+F2 battery
+0x84 # Fn+F5 backlight on/off
+0x86 # Fn+F9 WLAN
+0x88 # Fn-Up brightness up
+0x89 # Fn-Down brightness down
+0xB3 # Fn+F8 switch power mode (battery/dynamic/performance)
+0xF7 # Fn+F10 Touchpad on
+0xF9 # Fn+F10 Touchpad off
-- 
1.6.5.4

Re: [PATCH] keymap: support for force_release quirk

[Martin Pitt]

[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]

Hello Johannes,

thanks for working on this, and sorry for the late response (had a
busy week..)

Some remarks:

> --- /dev/null
> +++ b/extras/keymap/95-keyboard-force-release.rules
> @@ -0,0 +1,24 @@
> +# Set model specific atkbd force_release quirk
> +#
> +# Serveral laptops have hotkeys which don't generate release events,

Typo → "Several"

> +ACTION!="add", GOTO="force_release_end"

Can we please also do this on "change", so that rule changes can be
picked up on package upgrades? Packages which ship udev rules should
usually do something like

  udevadm trigger --action=change --subsystem-match=...

> +ENV{DMI_VENDOR}=="[sS][aA][mM][sS][uU][nN][gG]*", ATTR{[dmi/id]product_name}=="*N130*", RUN+="keyboard-force-release.sh $sys$devpath samsung-other"

For consistency, can we just use $devpath here, and care for
prepending /sys in the callout?

> --- /dev/null
> +++ b/extras/keymap/keyboard-force-release.sh
> @@ -0,0 +1,22 @@
> +#!/bin/sh -e
> +# read list of scancodes, convert hex to decimal and
> +# append to the atkbd force_release sysfs attribute
> +# $1 sysfs devpath for serioX
> +# $2 file with scancode list (hex or dec)
> +
> +case "$2" in
> +	/*) scf="$2" ;;
> +	*)  scf="${0%keyboard-force-release.sh}/keymaps/force-release/$2" ;;

I don't think we should really tie the callout path to the map
directory. Just hardcoding /lib/udev/keymaps/force-release/ seems
cleaner to me.

> +attr=`cat "$1/force_release"`

Please use "read" to avoid calling an external program and a subshell.
The script is -e already (as it should be), so I think the error
message if the file doesn't exist (older kernels) will be okay.

> +while read scancode dummy; do
> +	case "$scancode" in
> +		\#*) ;;
> +		*)
> +			scancode=$(($scancode))
> +			[ -n "$attr" ] && attr="$attr,$scancode" || attr="$scancode"
> +			;;
> +	esac
> +done <"$scf"
> +echo "$attr" >"$1/force_release"

Corresponding to comment above, please use /sys/$1 here.

A gotcha that I see here is that the force_release only ever gets
appended to. I. e. whenever we run udevtrigger, we'd append the same
keys again. POSIX shell doesn't have elaborate substring matching
capabilities like bash's ${x/pattern/string}, so it might not be too
easy to check if we already have a key in POSIX sh. (But please don't
call grep in a loop; let's rather rewrite this bit in C). There might
be some trickery with splitting by IFS=, into an array or so, if you
want to keep using sh?

Thanks!

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] keymap: support for force_release quirk

[Johannes Stezenbach]

Hi Martin,

On Mon, Dec 14, 2009 at 10:14:50AM +0100, Martin Pitt wrote:
> 
> A gotcha that I see here is that the force_release only ever gets
> appended to. I. e. whenever we run udevtrigger, we'd append the same
> keys again. POSIX shell doesn't have elaborate substring matching
> capabilities like bash's ${x/pattern/string}, so it might not be too
> easy to check if we already have a key in POSIX sh. (But please don't
> call grep in a loop; let's rather rewrite this bit in C). There might
> be some trickery with splitting by IFS=, into an array or so, if you
> want to keep using sh?

The problem is that force_release is preset by the kernel, so
in order to do what you want we need either
a) pass both old and new key list on "change"
b) save initial force_release value to a file on "add"
   and use the file on "change"

I'm not sure how a) could work on a package upgrade. But b)
sounds simple enough, the question is where to put the file.
How about /dev/.udev/?


Thanks for your other comments, I'll follow up with a new
patch asap.


Johannes

Re: [PATCH] keymap: support for force_release quirk

[Kay Sievers]

On Mon, Dec 14, 2009 at 11:28, Johannes Stezenbach <js@sig21.net> wrote:
> On Mon, Dec 14, 2009 at 10:14:50AM +0100, Martin Pitt wrote:
>>
>> A gotcha that I see here is that the force_release only ever gets
>> appended to. I. e. whenever we run udevtrigger, we'd append the same
>> keys again. POSIX shell doesn't have elaborate substring matching
>> capabilities like bash's ${x/pattern/string}, so it might not be too
>> easy to check if we already have a key in POSIX sh. (But please don't
>> call grep in a loop; let's rather rewrite this bit in C). There might
>> be some trickery with splitting by IFS=, into an array or so, if you
>> want to keep using sh?
>
> The problem is that force_release is preset by the kernel, so
> in order to do what you want we need either
> a) pass both old and new key list on "change"
> b) save initial force_release value to a file on "add"
>   and use the file on "change"

It should just not mangle the sysfs string if the value is already
contained. Why would you need to store anything between a "change" and
"add" event? Both events are handled the same way, and the two
independent events run one after each other.

I guess the logic to append a value to the sysfs string only if needed
should be done in a tine C program, or a sed script with a RUN+= call
directly to sed maybe can be used ...

Thanks,
Kay

Re: [PATCH] keymap: support for force_release quirk

[Johannes Stezenbach]

On Mon, Dec 14, 2009 at 11:34:31AM +0100, Kay Sievers wrote:
> On Mon, Dec 14, 2009 at 11:28, Johannes Stezenbach <js@sig21.net> wrote:
> > On Mon, Dec 14, 2009 at 10:14:50AM +0100, Martin Pitt wrote:
> >>
> >> A gotcha that I see here is that the force_release only ever gets
> >> appended to. I. e. whenever we run udevtrigger, we'd append the same
> >> keys again. POSIX shell doesn't have elaborate substring matching
> >> capabilities like bash's ${x/pattern/string}, so it might not be too
> >> easy to check if we already have a key in POSIX sh. (But please don't
> >> call grep in a loop; let's rather rewrite this bit in C). There might
> >> be some trickery with splitting by IFS=, into an array or so, if you
> >> want to keep using sh?
> >
> > The problem is that force_release is preset by the kernel, so
> > in order to do what you want we need either
> > a) pass both old and new key list on "change"
> > b) save initial force_release value to a file on "add"
> >   and use the file on "change"
> 
> It should just not mangle the sysfs string if the value is already
> contained. Why would you need to store anything between a "change" and
> "add" event? Both events are handled the same way, and the two
> independent events run one after each other.

I don't understand.  I only append to the force_release attribute
because the kernel already removes duplicates.
However, what does not work this way is to remove scancodes from
force_release. I thought that was what Martin wanted to accomplish
on the "change" event.

> I guess the logic to append a value to the sysfs string only if needed
> should be done in a tine C program, or a sed script with a RUN+= call
> directly to sed maybe can be used ...

sed cannot do hex to decimal conversion, but awk could be used.


Thanks,
Johannes

Re: [PATCH] keymap: support for force_release quirk

[Martin Pitt]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

Hello Johannes,

Johannes Stezenbach [2009-12-14 13:37 +0100]:
> I don't understand.  I only append to the force_release attribute
> because the kernel already removes duplicates.

Oh, indeed it does:

$ cat /sys/devices/platform/i8042/serio0/force_release
133-139,143,147,369-370

$ echo '133-139,143,147,369-370,143,147,370,371,147,371' | sudo tee /sys/devices/platform/i8042/serio0/force_release
$ cat /sys/devices/platform/i8042/serio0/force_release
133-139,143,147,369-371

So indeed we then don't need to worry about sending duplicate keys.
Nice!

> However, what does not work this way is to remove scancodes from
> force_release. I thought that was what Martin wanted to accomplish
> on the "change" event.

Hm, it seems to work just fine here:

$ echo 133 | sudo tee /sys/devices/platform/i8042/serio0/force_release133
$ cat /sys/devices/platform/i8042/serio0/force_release
133

So it seems every time you write the file you have to do it completely
(or read the original one).

So I think your approach is fine after all, and we just need the other
small cleanups.

Thanks,

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] keymap: support for force_release quirk

[Kay Sievers]

On Mon, Dec 14, 2009 at 13:37, Johannes Stezenbach <js@sig21.net> wrote:
> On Mon, Dec 14, 2009 at 11:34:31AM +0100, Kay Sievers wrote:
>> On Mon, Dec 14, 2009 at 11:28, Johannes Stezenbach <js@sig21.net> wrote:
>> > On Mon, Dec 14, 2009 at 10:14:50AM +0100, Martin Pitt wrote:
>> >>
>> >> A gotcha that I see here is that the force_release only ever gets
>> >> appended to. I. e. whenever we run udevtrigger, we'd append the same
>> >> keys again. POSIX shell doesn't have elaborate substring matching
>> >> capabilities like bash's ${x/pattern/string}, so it might not be too
>> >> easy to check if we already have a key in POSIX sh. (But please don't
>> >> call grep in a loop; let's rather rewrite this bit in C). There might
>> >> be some trickery with splitting by IFS=, into an array or so, if you
>> >> want to keep using sh?
>> >
>> > The problem is that force_release is preset by the kernel, so
>> > in order to do what you want we need either
>> > a) pass both old and new key list on "change"
>> > b) save initial force_release value to a file on "add"
>> >   and use the file on "change"
>>
>> It should just not mangle the sysfs string if the value is already
>> contained. Why would you need to store anything between a "change" and
>> "add" event? Both events are handled the same way, and the two
>> independent events run one after each other.
>
> I don't understand.  I only append to the force_release attribute
> because the kernel already removes duplicates.

Ah, cool.

> However, what does not work this way is to remove scancodes from
> force_release. I thought that was what Martin wanted to accomplish
> on the "change" event.

I don't think we really need to care about that. It thought it was
only the pattern ACTION="add|change", which is pretty common.

>> I guess the logic to append a value to the sysfs string only if needed
>> should be done in a tine C program, or a sed script with a RUN+= call
>> directly to sed maybe can be used ...
>
> sed cannot do hex to decimal conversion, but awk could be used.

Ah, that may be fine, if that works with a single commandline passed by RUN.

Thanks,
Kay

Re: [PATCH] keymap: support for force_release quirk

[Johannes Stezenbach]

Hello Martin,

On Mon, Dec 14, 2009 at 01:45:43PM +0100, Martin Pitt wrote:
> Johannes Stezenbach [2009-12-14 13:37 +0100]:
> 
> > However, what does not work this way is to remove scancodes from
> > force_release. I thought that was what Martin wanted to accomplish
> > on the "change" event.
> 
> Hm, it seems to work just fine here:
> 
> $ echo 133 | sudo tee /sys/devices/platform/i8042/serio0/force_release133
> $ cat /sys/devices/platform/i8042/serio0/force_release
> 133
> 
> So it seems every time you write the file you have to do it completely
> (or read the original one).

Yes, that was my question: Can I just save the original at boot
(first "add" event) into a file (like /dev/.udev/force-release/serioX)?

> So I think your approach is fine after all, and we just need the other
> small cleanups.

Yes, will resend a cleaned up patch asap.


BTW, 95-keymap.rules also doesn't handle "change". Does it need updating
or is there a difference in handling "change" for force_release?


Thanks,
Johannes

Re: [PATCH] keymap: support for force_release quirk

[Martin Pitt]

Johannes Stezenbach [2009-12-14 13:55 +0100]:
> Yes, that was my question: Can I just save the original at boot
> (first "add" event) into a file (like /dev/.udev/force-release/serioX)?

No, please don't; such external state keeping is very ugly and error
prone IMHO, and not what udev is designed to do. I rather think we
should do one of:

(1) Demand to have complete force-release tables, and completely
    reset the value with each invocation of the udev rule.

    Pro: Can remove values
    Con: Hard to just fix a single key

(2) Only ever add new values by reading the current value, appending
    our tables, and writing it back.

    Pro: Can fix single keys and potentially simplifies rules
    Con: Needs reboot for bad tables which set quirk erroneously

I suppose that on computers which need these quirks, all Fn keys are
affected equally, so the "fix single key" case is probably irrelevant.
OTOH, I don't think we'll get too many errors like "erroneous quirk",
so both approaches should work well in practice.

Personally I tend to prefer (2).

> BTW, 95-keymap.rules also doesn't handle "change". Does it need
> updating or is there a difference in handling "change" for
> force_release?

Fixed in git, thanks for pointing out.

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)