* Re: [PATCH] keymap: support for force_release quirk
2009-12-14 0:12 [PATCH] keymap: support for force_release quirk Johannes Stezenbach
@ 2009-12-14 9:14 ` Martin Pitt
2009-12-14 10:28 ` Johannes Stezenbach
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Martin Pitt @ 2009-12-14 9:14 UTC (permalink / raw)
To: linux-hotplug
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] keymap: support for force_release quirk
2009-12-14 0:12 [PATCH] keymap: support for force_release quirk Johannes Stezenbach
2009-12-14 9:14 ` Martin Pitt
@ 2009-12-14 10:28 ` Johannes Stezenbach
2009-12-14 10:34 ` Kay Sievers
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Johannes Stezenbach @ 2009-12-14 10:28 UTC (permalink / raw)
To: linux-hotplug
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] keymap: support for force_release quirk
2009-12-14 0:12 [PATCH] keymap: support for force_release quirk Johannes Stezenbach
2009-12-14 9:14 ` Martin Pitt
2009-12-14 10:28 ` Johannes Stezenbach
@ 2009-12-14 10:34 ` Kay Sievers
2009-12-14 12:37 ` Johannes Stezenbach
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Kay Sievers @ 2009-12-14 10:34 UTC (permalink / raw)
To: linux-hotplug
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] keymap: support for force_release quirk
2009-12-14 0:12 [PATCH] keymap: support for force_release quirk Johannes Stezenbach
` (2 preceding siblings ...)
2009-12-14 10:34 ` Kay Sievers
@ 2009-12-14 12:37 ` Johannes Stezenbach
2009-12-14 12:45 ` Martin Pitt
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Johannes Stezenbach @ 2009-12-14 12:37 UTC (permalink / raw)
To: linux-hotplug
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] keymap: support for force_release quirk
2009-12-14 0:12 [PATCH] keymap: support for force_release quirk Johannes Stezenbach
` (3 preceding siblings ...)
2009-12-14 12:37 ` Johannes Stezenbach
@ 2009-12-14 12:45 ` Martin Pitt
2009-12-14 12:50 ` Kay Sievers
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Martin Pitt @ 2009-12-14 12:45 UTC (permalink / raw)
To: linux-hotplug
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] keymap: support for force_release quirk
2009-12-14 0:12 [PATCH] keymap: support for force_release quirk Johannes Stezenbach
` (4 preceding siblings ...)
2009-12-14 12:45 ` Martin Pitt
@ 2009-12-14 12:50 ` Kay Sievers
2009-12-14 12:55 ` Johannes Stezenbach
2009-12-14 13:32 ` Martin Pitt
7 siblings, 0 replies; 9+ messages in thread
From: Kay Sievers @ 2009-12-14 12:50 UTC (permalink / raw)
To: linux-hotplug
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] keymap: support for force_release quirk
2009-12-14 0:12 [PATCH] keymap: support for force_release quirk Johannes Stezenbach
` (5 preceding siblings ...)
2009-12-14 12:50 ` Kay Sievers
@ 2009-12-14 12:55 ` Johannes Stezenbach
2009-12-14 13:32 ` Martin Pitt
7 siblings, 0 replies; 9+ messages in thread
From: Johannes Stezenbach @ 2009-12-14 12:55 UTC (permalink / raw)
To: linux-hotplug
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] keymap: support for force_release quirk
2009-12-14 0:12 [PATCH] keymap: support for force_release quirk Johannes Stezenbach
` (6 preceding siblings ...)
2009-12-14 12:55 ` Johannes Stezenbach
@ 2009-12-14 13:32 ` Martin Pitt
7 siblings, 0 replies; 9+ messages in thread
From: Martin Pitt @ 2009-12-14 13:32 UTC (permalink / raw)
To: linux-hotplug
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)
^ permalink raw reply [flat|nested] 9+ messages in thread