From: Mark Lord <hidden> Date: 2011-01-23 17:03:53
As of the 2.6.36 kernel, the userspace commands lsinput and input-kbd
no longer work. And if I grab newer/patched versions of those from the latest
Ubuntu 10.10, then those newer/patched versions do not work with kernels
*before* 2.6.36.
At first glance, this looks like a visible regression.
Is there a version of input-utils that works with both
old and new kernels ?
Thanks
Hi Mark,
On Sun, Jan 23, 2011 at 12:03:47PM -0500, Mark Lord wrote:
As of the 2.6.36 kernel, the userspace commands lsinput and input-kbd
no longer work. And if I grab newer/patched versions of those from the latest
Ubuntu 10.10, then those newer/patched versions do not work with kernels
*before* 2.6.36.
At first glance, this looks like a visible regression.
Is there a version of input-utils that works with both
old and new kernels ?
The event protocol number was updated to reflect support of large
scancodes, unfortunately some of the utilities expected exact version
and refuse to work with updated one. Ubuntu's fix was simply recompile
the code using the new define, this make it work on newer kernels but of
course broke the old ones...
--
Dmitry
From: Mark Lord <hidden> Date: 2011-01-25 00:32:12
On 11-01-24 12:54 PM, Dmitry Torokhov wrote:
Hi Mark,
On Sun, Jan 23, 2011 at 12:03:47PM -0500, Mark Lord wrote:
quoted
As of the 2.6.36 kernel, the userspace commands lsinput and input-kbd
no longer work. And if I grab newer/patched versions of those from the latest
Ubuntu 10.10, then those newer/patched versions do not work with kernels
*before* 2.6.36.
At first glance, this looks like a visible regression.
Is there a version of input-utils that works with both
old and new kernels ?
The event protocol number was updated to reflect support of large
scancodes, unfortunately some of the utilities expected exact version
and refuse to work with updated one.
So is there a danger of memory corruption if running a binary
that doesn't check the version number?
In other words, did the size and/or format of returned data
change for an ioctl() or something here?
If so, then that is a user-visible regression, and shouldn't happen.
One correct way to handle that, would be to create a new ioctl(),
and mark the old one as deprecated, for removal a few years later perhaps.
???
Thanks
On Mon, Jan 24, 2011 at 07:32:08PM -0500, Mark Lord wrote:
On 11-01-24 12:54 PM, Dmitry Torokhov wrote:
quoted
Hi Mark,
On Sun, Jan 23, 2011 at 12:03:47PM -0500, Mark Lord wrote:
quoted
As of the 2.6.36 kernel, the userspace commands lsinput and input-kbd
no longer work. And if I grab newer/patched versions of those from the latest
Ubuntu 10.10, then those newer/patched versions do not work with kernels
*before* 2.6.36.
At first glance, this looks like a visible regression.
Is there a version of input-utils that works with both
old and new kernels ?
The event protocol number was updated to reflect support of large
scancodes, unfortunately some of the utilities expected exact version
and refuse to work with updated one.
So is there a danger of memory corruption if running a binary
that doesn't check the version number?
No, as far as I know we kept ABI intact.
In other words, did the size and/or format of returned data
change for an ioctl() or something here?
Yes, we introduced new ioctls (keeping old ones and their ABI intact).
The change is that EVIOCGVERSION ioctl now returns 0x10001 instead of
0x10000.
If so, then that is a user-visible regression, and shouldn't happen.
One correct way to handle that, would be to create a new ioctl(),
and mark the old one as deprecated, for removal a few years later perhaps.
???
Right.
However a few input utilities insist that they will only work with event
protocol version 0x10000. It is purely their choice, however misguided
it might be.
--
Dmitry
From: Mark Lord <hidden> Date: 2011-01-25 04:13:11
On 11-01-24 07:55 PM, Dmitry Torokhov wrote:
No, as far as I know we kept ABI intact.
Okay, I hacked lsinput and input-kbd to ignore the protocol number.
input-kbd is still broken: it thinks my remote control (Hauppauge)
has only ten buttons, and won't allow me to remap codes larger than 10.
I've now hacked around that too, but without determining exactly
where the interface got broken.
Ugh.
Thanks.
On Mon, Jan 24, 2011 at 11:13:05PM -0500, Mark Lord wrote:
On 11-01-24 07:55 PM, Dmitry Torokhov wrote:
quoted
No, as far as I know we kept ABI intact.
Okay, I hacked lsinput and input-kbd to ignore the protocol number.
input-kbd is still broken: it thinks my remote control (Hauppauge)
has only ten buttons, and won't allow me to remap codes larger than 10.
I've now hacked around that too, but without determining exactly
where the interface got broken.
Ugh.
Where are the sources? I can take a look...
--
Dmitry
From: Mark Lord <hidden> Date: 2011-01-25 04:37:09
On 11-01-24 11:20 PM, Dmitry Torokhov wrote:
On Mon, Jan 24, 2011 at 11:13:05PM -0500, Mark Lord wrote:
quoted
On 11-01-24 07:55 PM, Dmitry Torokhov wrote:
quoted
No, as far as I know we kept ABI intact.
Okay, I hacked lsinput and input-kbd to ignore the protocol number.
input-kbd is still broken: it thinks my remote control (Hauppauge)
has only ten buttons, and won't allow me to remap codes larger than 10.
I've now hacked around that too, but without determining exactly
where the interface got broken.
Ugh.
Where are the sources? I can take a look...
I used "apt-get source input-utils" under Ubuntu-10.10.
The problem seems to be here somewhere:
static struct kbd_map* kbd_map_read(int fd)
{
struct kbd_entry entry;
struct kbd_map *map;
int rc;
map = malloc(sizeof(*map));
memset(map,0,sizeof(*map));
for (map->size = 0; map->size < 65536; map->size++) {
entry.scancode = map->size;
entry.keycode = KEY_RESERVED;
rc = ioctl(fd, EVIOCGKEYCODE, &entry);
if (rc < 0) {
break;
}
if (map->size >= map->alloc) {
map->alloc += 64;
map->map = realloc(map->map, map->alloc * sizeof(entry));
}
map->map[map->size] = entry;
if (KEY_RESERVED != entry.keycode)
map->keys++;
}
if (map->keys) {
printf("map: %d keys, size: %d/%d\n",
map->keys, map->size, map->alloc);
return map;
} else {
free(map);
return NULL;
}
}
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
From: Mark Lord <hidden> Date: 2011-01-25 04:43:41
On 11-01-24 11:37 PM, Mark Lord wrote:
On 11-01-24 11:20 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:13:05PM -0500, Mark Lord wrote:
quoted
On 11-01-24 07:55 PM, Dmitry Torokhov wrote:
quoted
No, as far as I know we kept ABI intact.
Okay, I hacked lsinput and input-kbd to ignore the protocol number.
input-kbd is still broken: it thinks my remote control (Hauppauge)
has only ten buttons, and won't allow me to remap codes larger than 10.
I've now hacked around that too, but without determining exactly
where the interface got broken.
Ugh.
Where are the sources? I can take a look...
I used "apt-get source input-utils" under Ubuntu-10.10.
The problem seems to be here somewhere:
static struct kbd_map* kbd_map_read(int fd)
{
struct kbd_entry entry;
struct kbd_map *map;
int rc;
map = malloc(sizeof(*map));
memset(map,0,sizeof(*map));
for (map->size = 0; map->size < 65536; map->size++) {
entry.scancode = map->size;
entry.keycode = KEY_RESERVED;
rc = ioctl(fd, EVIOCGKEYCODE, &entry);
if (rc < 0) {
break;
...
}
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
I hacked input-kbd to ignore the map->size calculated above
when writing a new map.. seems to work. Weird that the old
method stopped working with 2.6.36, though.
I'm using this with ir-kbd-i2c.c as the hardware driver
for the hauppauge R/C interface on a PVR-250 card.
Hey.. perhaps you may also know where in the code this thing
is being forced to a repeat rate of about 4 times/sec max?
I'd like the remote to be slightly faster that this,
but my 2.5.35 (and earlier) hacks to ir-kbd-i2c now don't
work for repeat intervals less than approx 220msecs.
Cheers!
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
On 11-01-24 11:20 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:13:05PM -0500, Mark Lord wrote:
quoted
On 11-01-24 07:55 PM, Dmitry Torokhov wrote:
quoted
No, as far as I know we kept ABI intact.
Okay, I hacked lsinput and input-kbd to ignore the protocol number.
input-kbd is still broken: it thinks my remote control (Hauppauge)
has only ten buttons, and won't allow me to remap codes larger than 10.
I've now hacked around that too, but without determining exactly
where the interface got broken.
Ugh.
Where are the sources? I can take a look...
I used "apt-get source input-utils" under Ubuntu-10.10.
The problem seems to be here somewhere:
static struct kbd_map* kbd_map_read(int fd)
{
struct kbd_entry entry;
struct kbd_map *map;
int rc;
map = malloc(sizeof(*map));
memset(map,0,sizeof(*map));
for (map->size = 0; map->size < 65536; map->size++) {
entry.scancode = map->size;
entry.keycode = KEY_RESERVED;
rc = ioctl(fd, EVIOCGKEYCODE, &entry);
if (rc < 0) {
break;
}
if (map->size >= map->alloc) {
map->alloc += 64;
map->map = realloc(map->map, map->alloc * sizeof(entry));
}
map->map[map->size] = entry;
if (KEY_RESERVED != entry.keycode)
map->keys++;
}
if (map->keys) {
printf("map: %d keys, size: %d/%d\n",
map->keys, map->size, map->alloc);
return map;
} else {
free(map);
return NULL;
}
}
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
Thanks.
--
Dmitry
From: Mark Lord <hidden> Date: 2011-01-25 05:04:13
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
Thanks.
From: Mark Lord <hidden> Date: 2011-01-25 05:07:33
On 11-01-25 12:04 AM, Mark Lord wrote:
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
quoted
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
Note that this device DOES have "flat scancode space",
and the kernel is now incorrectly signalling an error (-EINVAL)
in response to a perfectly valid query of a VALID (and mappable)
keycode on the remote control
The code really is a valid button, it just doesn't have a default mapping
set by the kernel (I can set a mapping for that code from userspace and it works).
This is a BUG. Returning -EINVAL here is entirely wrong.
Cheers
From: Mark Lord <hidden> Date: 2011-01-25 05:16:05
On 11-01-25 12:07 AM, Mark Lord wrote:
On 11-01-25 12:04 AM, Mark Lord wrote:
..
quoted
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
Actually, what changed could be something different:
it's possible that this bug was always there,
and older kernels had a more complete default keymap
for the remote than that in 2.6.36+, thereby never
triggering the bug. But I don't know that, and my best
efforts to-date to locate any of this in the kernel
have been futile. Oh well. :)
quoted
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
Note that this device DOES have "flat scancode space",
and the kernel is now incorrectly signalling an error (-EINVAL)
in response to a perfectly valid query of a VALID (and mappable)
keycode on the remote control
The code really is a valid button, it just doesn't have a default mapping
set by the kernel (I can set a mapping for that code from userspace and it works).
This is a BUG. Returning -EINVAL here is entirely wrong.
On Tue, Jan 25, 2011 at 12:04:10AM -0500, Mark Lord wrote:
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
quoted
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
You are inventing rules. You are requesting a scancode->keycode
mapping. If scancode is unknown/invalid for the device ioctl returns
-EINVAL.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
For unmapped - yes, either KEY_RESERVED or KEY_UNKNOWN should be
returned. For invalid scancodes -EINVAL shoudl be returned. Scancodes
are not guaranteed to be continuous (and never have been for all devices
although there are still plenty of devices with continuous scancodes,
like atkbd).
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote:
On 11-01-25 12:04 AM, Mark Lord wrote:
quoted
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
quoted
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
Note that this device DOES have "flat scancode space",
and the kernel is now incorrectly signalling an error (-EINVAL)
in response to a perfectly valid query of a VALID (and mappable)
keycode on the remote control
The code really is a valid button, it just doesn't have a default mapping
set by the kernel (I can set a mapping for that code from userspace and it works).
OK, in this case let's ping Mauro - I think he done the adjustments to
IR keymap hanlding.
Thanks.
--
Dmitry
On Mon, Jan 24, 2011 at 09:31:17PM -0800, Dmitry Torokhov wrote:
On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote:
quoted
On 11-01-25 12:04 AM, Mark Lord wrote:
quoted
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
quoted
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
Note that this device DOES have "flat scancode space",
and the kernel is now incorrectly signalling an error (-EINVAL)
in response to a perfectly valid query of a VALID (and mappable)
keycode on the remote control
The code really is a valid button, it just doesn't have a default mapping
set by the kernel (I can set a mapping for that code from userspace and it works).
OK, in this case let's ping Mauro - I think he done the adjustments to
IR keymap hanlding.
Thanks.
BTW, could you please try the following patch (it assumes that
EVIOCGVERSION in input.c is alreday relaxed).
Thanks!
--
Dmitry
From c22c85c0b675422a23e3d853ed06fedc36805774 Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Mon, 24 Jan 2011 22:49:59 -0800
Subject: [PATCH] input-kbd - switch to using EVIOCGKEYCODE2 when available
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
input-kbd.c | 118 ++++++++++++++++++++++++++++++++++++++++-------------------
1 files changed, 80 insertions(+), 38 deletions(-)
@@ -223,8 +246,10 @@ static int usage(char *prog, int error)intmain(intargc,char*argv[]){-intc,devnr;+intc,devnr,fd;char*mapfile=NULL;+unsignedintprotocol_version;+intrc=EXIT_FAILURE;for(;;){if(-1==(c=getopt(argc,argv,"hf:")))
@@ -244,12 +269,29 @@ int main(int argc, char *argv[])usage(argv[0],1);devnr=atoi(argv[optind]);-if(mapfile){-set_kbd(devnr,mapfile);-}else{-show_kbd(devnr);++fd=device_open(devnr,1);+if(fd<0)+gotoout;++if(ioctl(fd,EVIOCGVERSION,&protocol_version)<0){+fprintf(stderr,+"Unable to query evdev protocol version: %s\n",+strerror(errno));+gotoout_close;}-return0;++if(mapfile)+set_kbd(fd,protocol_version,mapfile);+else+show_kbd(fd,protocol_version);++rc=EXIT_SUCCESS;++out_close:+close(fd);+out:+returnrc;}/* ---------------------------------------------------------------------
On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote:
quoted
On 11-01-25 12:04 AM, Mark Lord wrote:
quoted
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
quoted
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
Note that this device DOES have "flat scancode space",
and the kernel is now incorrectly signalling an error (-EINVAL)
in response to a perfectly valid query of a VALID (and mappable)
keycode on the remote control
The code really is a valid button, it just doesn't have a default mapping
set by the kernel (I can set a mapping for that code from userspace and it works).
OK, in this case let's ping Mauro - I think he done the adjustments to
IR keymap hanlding.
I lost part of the thread, but a quick search via the Internet showed that you're using
the input tools to work with a Remote Controller, right? Are you using a vanilla
kernel, or are you using the media_build backports? There are some distros that are
using those backports also like Fedora 14.
In the latter case, I found the reason why the backports were not working and I fixed
it a couple days ago:
http://git.linuxtv.org/media_build.git?a=commit;h=b83dc3e49d90527d8e1016d09e06f4842a6a847a
The issue is simple, and it is related on how the input.c used to handle EVIOSGKEYCODE.
Basically, before allowing you to change a key, it used to call EVIOCGKEYCODE to check
it that key exists. However, when you're creating a new association, the key didn't
exist, and, to be strict with input rules, EVIOCGKEYCODE should return -EINVAL.
To circumvent that behaviour, old versions were returning 0, and associating unmapped
scancodes to KEY_RESERVED. We used this workaround for a few kernel versions, while
we were discussing the improvements so support larger scancodes.
Yet, on all vanilla kernels, changing the keycode association works fine.
However, the backport patch at media_build were not taking this workaround into account,
and were just returning -EINVAL. So, the backported media drivers stopped allowing
some keytable changes. The patch above fixes it.
Cheers,
Mauro
From: Mark Lord <hidden> Date: 2011-01-25 14:28:28
On 11-01-25 12:29 AM, Dmitry Torokhov wrote:
On Tue, Jan 25, 2011 at 12:04:10AM -0500, Mark Lord wrote:
quoted
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
quoted
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
You are inventing rules. You are requesting a scancode->keycode
mapping. If scancode is unknown/invalid for the device ioctl returns
-EINVAL.
-EINVAL signals bad/invalid parameters.
That's NOT what is happening here.
For unmapped - yes, either KEY_RESERVED or KEY_UNKNOWN should be
returned. For invalid scancodes -EINVAL shoudl be returned.
Exactly my point. The scancode in question is 100% valid and mapable,
yet the kernel is rejecting it as -EINVAL. Incorrect.
BUG. Regression. Breaks userspace. Must get fixed.
Cheers
From: Mark Lord <hidden> Date: 2011-01-25 14:32:56
On 11-01-25 06:42 AM, Mauro Carvalho Chehab wrote:
I lost part of the thread, but a quick search via the Internet showed that you're using
the input tools to work with a Remote Controller, right? Are you using a vanilla
kernel, or are you using the media_build backports? There are some distros that are
using those backports also like Fedora 14.
I use kernel.org kernels exclusively.
The issue is simple, and it is related on how the input.c used to handle EVIOSGKEYCODE.
Basically, before allowing you to change a key, it used to call EVIOCGKEYCODE to check
it that key exists. However, when you're creating a new association, the key didn't
exist, and, to be strict with input rules, EVIOCGKEYCODE should return -EINVAL.
No, if the parameters are a valid key, then -EINVAL is never the correct
thing for a kernel to return. -EINVAL means "bad parameters",
and that's not an accurate description of a valid yet unmapped key.
To circumvent that behaviour, old versions were returning 0, and associating unmapped
scancodes to KEY_RESERVED. We used this workaround for a few kernel versions, while
we were discussing the improvements so support larger scancodes.
And now we're stuck with it. If that is how it works,
and userspace depends upon it (it does), then consider
it cast in stone. Immutable by Linus's Law: don't break userspace.
Create a new ioctl() number for the new behaviour,
but preserve the old behaviour in exact form for
a suitable (multi-year) overlap period.
Cheers
On Mon, Jan 24, 2011 at 09:31:17PM -0800, Dmitry Torokhov wrote:
quoted
On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote:
quoted
On 11-01-25 12:04 AM, Mark Lord wrote:
quoted
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
quoted
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
Note that this device DOES have "flat scancode space",
and the kernel is now incorrectly signalling an error (-EINVAL)
in response to a perfectly valid query of a VALID (and mappable)
keycode on the remote control
The code really is a valid button, it just doesn't have a default mapping
set by the kernel (I can set a mapping for that code from userspace and it works).
OK, in this case let's ping Mauro - I think he done the adjustments to
IR keymap hanlding.
Thanks.
BTW, could you please try the following patch (it assumes that
EVIOCGVERSION in input.c is alreday relaxed).
Dmitry,
Thanks for your patch. I used part of his logic to improve the ir-keytable
tool at v4l-utils:
http://git.linuxtv.org/v4l-utils.git
The ir-keytable is a tool that just handles Remote Controller input devices,
and do it well, allowing all sorts of operations related to it, and using the
sysfs /sys/class/rc stuff to help its operation. Without any arguments, it
lists the existing RC devices. Arguments are there to allow enabling/disabling
RC protocols, reading/writing/cleaning keycode tables and to test if the
remote is generating events (EV_MSC/EV_KEY/EV_REP/EV_SYN).
Now, it will be using V2 for reads and keycode cleanups, but will still use
V1 for writes, as, currently with 32 bits scancodes, there's no gain to use
V2 for it. Also, changing the tool to use more bits will require to rewrite
part of the code.
Also, writing a rc-core code that can work with an arbitrary large scancode
is still on our TODO list.
I'm not entirely sure how to extend the scancode size, as there are a
few options:
1) Core would always work internally with 32 bytes (1024 bits). Some
logic will be required to accept entries with .len < 32;
2) Drivers will define the code lengtht, and core will use it,
returning -EINVAL if userspace uses a len grater than used internally by
the core. In this case, we'll need a sysfs node to tell userspace what's
the maximum allowed size;
3) Drivers will define the max number of bits, and core will use it,
truncating the number to the max size if userspace tries to write more bits
than the internal representation;
4) Drivers will define the max number of bits, and core will use it,
returning an error if the number is bigger than the max scancode that can be
represented internally.
I think that (2) is the best way for doing it, but I'm not yet entirely sure.
So, it is good to hear some comments about that.
Cheers,
Mauro
On Tue, Jan 25, 2011 at 09:42:44AM -0200, Mauro Carvalho Chehab wrote:
Em 25-01-2011 03:31, Dmitry Torokhov escreveu:
quoted
On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote:
quoted
On 11-01-25 12:04 AM, Mark Lord wrote:
quoted
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
quoted
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
Note that this device DOES have "flat scancode space",
and the kernel is now incorrectly signalling an error (-EINVAL)
in response to a perfectly valid query of a VALID (and mappable)
keycode on the remote control
The code really is a valid button, it just doesn't have a default mapping
set by the kernel (I can set a mapping for that code from userspace and it works).
OK, in this case let's ping Mauro - I think he done the adjustments to
IR keymap hanlding.
I lost part of the thread, but a quick search via the Internet showed that you're using
the input tools to work with a Remote Controller, right? Are you using a vanilla
kernel, or are you using the media_build backports? There are some distros that are
using those backports also like Fedora 14.
In the latter case, I found the reason why the backports were not working and I fixed
it a couple days ago:
http://git.linuxtv.org/media_build.git?a=commit;h=b83dc3e49d90527d8e1016d09e06f4842a6a847a
The issue is simple, and it is related on how the input.c used to handle EVIOSGKEYCODE.
Basically, before allowing you to change a key, it used to call EVIOCGKEYCODE to check
it that key exists. However, when you're creating a new association, the key didn't
exist, and, to be strict with input rules, EVIOCGKEYCODE should return -EINVAL.
We should be able to handle the case where scancode is valid even though
it might be unmapped yet. This is regardless of what version of
EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
Is it possible to validate the scancode by driver?
--
Dmitry
On Tue, Jan 25, 2011 at 12:42:57PM -0200, Mauro Carvalho Chehab wrote:
Em 25-01-2011 04:52, Dmitry Torokhov escreveu:
quoted
On Mon, Jan 24, 2011 at 09:31:17PM -0800, Dmitry Torokhov wrote:
quoted
On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote:
quoted
On 11-01-25 12:04 AM, Mark Lord wrote:
quoted
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
quoted
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
Note that this device DOES have "flat scancode space",
and the kernel is now incorrectly signalling an error (-EINVAL)
in response to a perfectly valid query of a VALID (and mappable)
keycode on the remote control
The code really is a valid button, it just doesn't have a default mapping
set by the kernel (I can set a mapping for that code from userspace and it works).
OK, in this case let's ping Mauro - I think he done the adjustments to
IR keymap hanlding.
Thanks.
BTW, could you please try the following patch (it assumes that
EVIOCGVERSION in input.c is alreday relaxed).
Dmitry,
Thanks for your patch. I used part of his logic to improve the ir-keytable
tool at v4l-utils:
http://git.linuxtv.org/v4l-utils.git
The ir-keytable is a tool that just handles Remote Controller input devices,
and do it well, allowing all sorts of operations related to it, and using the
sysfs /sys/class/rc stuff to help its operation. Without any arguments, it
lists the existing RC devices. Arguments are there to allow enabling/disabling
RC protocols, reading/writing/cleaning keycode tables and to test if the
remote is generating events (EV_MSC/EV_KEY/EV_REP/EV_SYN).
Now, it will be using V2 for reads and keycode cleanups, but will still use
V1 for writes, as, currently with 32 bits scancodes, there's no gain to use
V2 for it. Also, changing the tool to use more bits will require to rewrite
part of the code.
Also, writing a rc-core code that can work with an arbitrary large scancode
is still on our TODO list.
I'm not entirely sure how to extend the scancode size, as there are a
few options:
1) Core would always work internally with 32 bytes (1024 bits). Some
logic will be required to accept entries with .len < 32;
2) Drivers will define the code lengtht, and core will use it,
returning -EINVAL if userspace uses a len grater than used internally by
the core. In this case, we'll need a sysfs node to tell userspace what's
the maximum allowed size;
3) Drivers will define the max number of bits, and core will use it,
truncating the number to the max size if userspace tries to write more bits
than the internal representation;
4) Drivers will define the max number of bits, and core will use it,
returning an error if the number is bigger than the max scancode that can be
represented internally.
I think that (2) is the best way for doing it, but I'm not yet entirely sure.
So, it is good to hear some comments about that.
I'd say 4 and userspace utility should normalize scancodes packing them into the
least number of bits possible. Since keymap should be device specific
data in the keymap will not exceed what the driver expects, right?
--
Dmitry
On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
[off-list ref] wrote:
We should be able to handle the case where scancode is valid even though
it might be unmapped yet. This is regardless of what version of
EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
Is it possible to validate the scancode by driver?
More appropriately, why not just revert the thing? The version change
and the buggy EINVAL return both.
As Mark said, breaking user space simply isn't acceptable. And since
breaking user space isn't acceptable, then incrementing the version is
stupid too.
The way we add new ioctl's is not by incrementing some "ABI version"
crap. It's by adding new ioctl's or system calls or whatever that
simply used to return -ENOSYS or other error before, while preserving
the old ABI. That way old binaries don't break (for _ANY_ reason), and
new binaries can see "oh, this doesn't support the new thing".
Linus
On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
We should be able to handle the case where scancode is valid even though
it might be unmapped yet. This is regardless of what version of
EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
Is it possible to validate the scancode by driver?
More appropriately, why not just revert the thing? The version change
Well, then we'll break Ubuntu again as they recompiled their input-utils
package (without fixing the check). And the rest of distros do not seem
to be using that package...
and the buggy EINVAL return both.
I believe that -EINVAL thing only affects RC devices that Mauro switched
to the new rc-core; input core in itself should be ABI compatible. Thus
I'll leave the decision to him whether he wants to revert or fix
compatibility issue.
As Mark said, breaking user space simply isn't acceptable. And since
breaking user space isn't acceptable, then incrementing the version is
stupid too.
It might not have been the best idea to increment, however I maintain
that if there exists version is can be changed. Otherwise there is no
point in having version at all.
As I said, reverting the version bump will cause yet another wave of
breakages so I propose leaving version as is.
The way we add new ioctl's is not by incrementing some "ABI version"
crap. It's by adding new ioctl's or system calls or whatever that
simply used to return -ENOSYS or other error before, while preserving
the old ABI. That way old binaries don't break (for _ANY_ reason), and
new binaries can see "oh, this doesn't support the new thing".
That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
--
Dmitry
On Tue, Jan 25, 2011 at 12:54:53PM -0800, Dmitry Torokhov wrote:
On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
quoted
On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
We should be able to handle the case where scancode is valid even though
it might be unmapped yet. This is regardless of what version of
EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
Is it possible to validate the scancode by driver?
More appropriately, why not just revert the thing? The version change
Well, then we'll break Ubuntu again as they recompiled their input-utils
package (without fixing the check). And the rest of distros do not seem
to be using that package...
quoted
and the buggy EINVAL return both.
I believe that -EINVAL thing only affects RC devices that Mauro switched
to the new rc-core; input core in itself should be ABI compatible. Thus
I'll leave the decision to him whether he wants to revert or fix
compatibility issue.
quoted
As Mark said, breaking user space simply isn't acceptable. And since
breaking user space isn't acceptable, then incrementing the version is
stupid too.
It might not have been the best idea to increment, however I maintain
that if there exists version is can be changed. Otherwise there is no
point in having version at all.
As I said, reverting the version bump will cause yet another wave of
breakages so I propose leaving version as is.
quoted
The way we add new ioctl's is not by incrementing some "ABI version"
crap. It's by adding new ioctl's or system calls or whatever that
simply used to return -ENOSYS or other error before, while preserving
the old ABI. That way old binaries don't break (for _ANY_ reason), and
new binaries can see "oh, this doesn't support the new thing".
That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
BTW, another issue is that evdev's ioctl returns -EINVAL for unknown
ioctls so applications would have hard time figuring out whether error
returned because of kernel being too old or because they are trying to
retrieve/establish invalid mapping if they had to go only by the error
code.
As far as I can see EINVAL is a proper error for unknown ioctls:
[dtor@hammer work]$ man 2 ioctl | grep EINVAL
EINVAL Request or argp is not valid.
[dtor@hammer work]$
--
Dmitry
On Wed, Jan 26, 2011 at 7:01 AM, Dmitry Torokhov
[off-list ref] wrote:
BTW, another issue is that evdev's ioctl returns -EINVAL for unknown
ioctls so applications would have hard time figuring out whether error
returned because of kernel being too old or because they are trying to
retrieve/establish invalid mapping if they had to go only by the error
code.
So that's just another evdev interface bug.
As far as I can see EINVAL is a proper error for unknown ioctls:
[dtor@hammer work]$ man 2 ioctl | grep EINVAL
EINVAL Request or argp is not valid.
Yeah, there's some confusion there.
The "unknown ioctl" error code is (for traditional reasons) ENOTTY,
but yes, the EINVAL thing admittedly has a lot of legacy use too.
Inside the kernel, the preferred way to say "I don't recognize that
ioctl number" is actually ENOIOCTLCMD. That's exactly so that various
nested ioctl handlers can then tell the difference between "I didn't
recognize that ioctl" and "I understand what you asked me to do, but
your arguments were crap".
vfs_ioctl() will then turn ENOIOCTLCMD to EINVAL to return to user space.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 26, 2011 at 07:20:07AM +1000, Linus Torvalds wrote:
On Wed, Jan 26, 2011 at 7:01 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
BTW, another issue is that evdev's ioctl returns -EINVAL for unknown
ioctls so applications would have hard time figuring out whether error
returned because of kernel being too old or because they are trying to
retrieve/establish invalid mapping if they had to go only by the error
code.
So that's just another evdev interface bug.
Huh? I do not have lot of options here as far as error codes go. Invalid
request, invalid data in request - all goes to EINVAL.
quoted
As far as I can see EINVAL is a proper error for unknown ioctls:
[dtor@hammer work]$ man 2 ioctl | grep EINVAL
EINVAL Request or argp is not valid.
Yeah, there's some confusion there.
The "unknown ioctl" error code is (for traditional reasons) ENOTTY,
but yes, the EINVAL thing admittedly has a lot of legacy use too.
Inside the kernel, the preferred way to say "I don't recognize that
ioctl number" is actually ENOIOCTLCMD. That's exactly so that various
nested ioctl handlers can then tell the difference between "I didn't
recognize that ioctl" and "I understand what you asked me to do, but
your arguments were crap".
vfs_ioctl() will then turn ENOIOCTLCMD to EINVAL to return to user space.
OK, so I can change evdev to employ ENOIOCTLCMD where needed, bit that
will not change older kernels where such distinction is needed (as never
kernels do support newer ioctl). And even if I could go back it would
not help since userspace still sees EINVAL only.
--
Dmitry
On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
quoted
On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
We should be able to handle the case where scancode is valid even though
it might be unmapped yet. This is regardless of what version of
EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
Is it possible to validate the scancode by driver?
More appropriately, why not just revert the thing? The version change
Reverting the version increment is a bad thing. I agree with Dmitry that
an application that fails just because the API version were incremented
is buggy.
Well, then we'll break Ubuntu again as they recompiled their input-utils
package (without fixing the check). And the rest of distros do not seem
to be using that package...
Reverting it will also break the ir-keytable userspace program that it is
meant to be used by the Remote Controller devices, and uses it to adjust
its behaviour to support RC's with more than 16 bits of scancodes.
I agree that it is bad that the ABI broke, but reverting it will cause even
more damage.
quoted
and the buggy EINVAL return both.
I believe that -EINVAL thing only affects RC devices that Mauro switched
to the new rc-core; input core in itself should be ABI compatible. Thus
I'll leave the decision to him whether he wants to revert or fix
compatibility issue.
The Remote Controller keycode tables are very sparse. In general,
they contain up to 100 entries, and the scan codes typically have 16
bits. Some newer devices have 24 or 32 bits. With version 1, as the table
index is the scancode, in order to read all keytables with EVIOCGKEYCODE,
the userspace needs to do 2^16 reads (or 2^32 for RC-6 remotes).
I don't need to say that this is highly ineffective. So, using V1
doesn't work fine anyway for Remote Controllers.
Btw, ir-keycodestool don't work with V1 and more than 16 bits, because it
doesn't scale. I didn't actually checked, but based on Dmitry's patch
for input-kbd, it is clear to me that the old version only supports 16
bits scancodes:
Em 25-01-2011 04:52, Dmitry Torokhov escreveu:
quoted hunk
From c22c85c0b675422a23e3d853ed06fedc36805774 Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Mon, 24 Jan 2011 22:49:59 -0800
Subject: [PATCH] input-kbd - switch to using EVIOCGKEYCODE2 when available
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
input-kbd.c | 118 ++++++++++++++++++++++++++++++++++++++++-------------------
1 files changed, 80 insertions(+), 38 deletions(-)
See, it will only look into the 16-bits scancode space. There are several remote
controllers with 24 bits and 32 bits, so the tool is already broken anyway.
On the tests I did here with an ir-keytable version made before such change,
with a Fedora rawhide kernel (2.6.37), I didn't notice any breakage at
EVIOCGKEYCODE. I'll do more tests tomorrow with a vanilla Kernel. I'll
compile a vanilla 2.6.37 kernel tomorrow and, if needed, write a patch.
quoted
As Mark said, breaking user space simply isn't acceptable. And since
breaking user space isn't acceptable, then incrementing the version is
stupid too.
It might not have been the best idea to increment, however I maintain
that if there exists version is can be changed. Otherwise there is no
point in having version at all.
Not arguing in favor of the version numbering, but it is easy to read
the version increment at the beginning of the application, and adjust
if the code will use EVIOCGKEYCODE or EVIOCGKEYCODE_V2 of the ioctl's,
depending on what kernel provides.
Ok, we might be just calling the new ioctl and check for -ENOSYS at
the beginning, using some fake arguments.
As I said, reverting the version bump will cause yet another wave of
breakages so I propose leaving version as is.
quoted
The way we add new ioctl's is not by incrementing some "ABI version"
crap. It's by adding new ioctl's or system calls or whatever that
simply used to return -ENOSYS or other error before, while preserving
the old ABI. That way old binaries don't break (for _ANY_ reason), and
new binaries can see "oh, this doesn't support the new thing".
That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
From: Mark Lord <hidden> Date: 2011-01-25 22:22:15
On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote:
Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
quoted
On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
quoted
On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
We should be able to handle the case where scancode is valid even though
it might be unmapped yet. This is regardless of what version of
EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
Is it possible to validate the scancode by driver?
More appropriately, why not just revert the thing? The version change
Reverting the version increment is a bad thing. I agree with Dmitry that
an application that fails just because the API version were incremented
is buggy.
quoted
Well, then we'll break Ubuntu again as they recompiled their input-utils
package (without fixing the check). And the rest of distros do not seem
to be using that package...
Reverting it will also break the ir-keytable userspace program that it is
meant to be used by the Remote Controller devices, and uses it to adjust
its behaviour to support RC's with more than 16 bits of scancodes.
I agree that it is bad that the ABI broke, but reverting it will cause even
more damage.
There we disagree. Sure it's a very poorly thought out interface,
but the way to fix it is to put a new one along side the old,
and put the old back the way it was before it got broken.
I'm not making a fuss here for myself -- I'm more than capable of working
around new kernel bugs like these, but for every person like me there are
likely hundreds of others who simply get frustrated and give up.
If you're worried about Ubuntu's adaptation to the buggy regression,
then email their developers (kernel and input-utils packagers) explaining
the revert, and they can coordination their kernel and input-utils updates
to do the Right Thing.
But for all of the rest of us, our systems are broken by this change.
...
quoted
quoted
As Mark said, breaking user space simply isn't acceptable. And since
breaking user space isn't acceptable, then incrementing the version is
stupid too.
It might not have been the best idea to increment, however I maintain
that if there exists version is can be changed. Otherwise there is no
point in having version at all.
Not arguing in favor of the version numbering, but it is easy to read
the version increment at the beginning of the application, and adjust
if the code will use EVIOCGKEYCODE or EVIOCGKEYCODE_V2 of the ioctl's,
depending on what kernel provides.
Ok, we might be just calling the new ioctl and check for -ENOSYS at
the beginning, using some fake arguments.
quoted
As I said, reverting the version bump will cause yet another wave of
breakages so I propose leaving version as is.
quoted
The way we add new ioctl's is not by incrementing some "ABI version"
crap. It's by adding new ioctl's or system calls or whatever that
simply used to return -ENOSYS or other error before, while preserving
the old ABI. That way old binaries don't break (for _ANY_ reason), and
new binaries can see "oh, this doesn't support the new thing".
That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
That's the problem: you did NOT keep the two old ioctls().
Those got changed too.. so now we have four NEW ioctls(),
none of which backward compatible with userspace.
On Wed, Jan 26, 2011 at 8:00 AM, Mauro Carvalho Chehab
[off-list ref] wrote:
See, it will only look into the 16-bits scancode space. There are several remote
controllers with 24 bits and 32 bits, so the tool is already broken anyway.
Mauro, stop blathering.
The problem is that the tool used to work with OLD DEVICES AND SETUPS
that used to work. That broke. It needs to get fixed.
We do not change user-land ABI. Not now, not ever. And no, "the tool
is broken" is NOT an excuse. Bringing it up as one is unacceptable.
The fact that there are devices that didn't use to be supported at all
that don't work with the old tool has absolutely ZERO relevance,
because that's not a regression.
No regressions. No excuses. No "user-land is broken", however much you
disagree with it.
Linus
On Tue, Jan 25, 2011 at 05:22:09PM -0500, Mark Lord wrote:
On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote:
quoted
Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
quoted
On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
quoted
On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
We should be able to handle the case where scancode is valid even though
it might be unmapped yet. This is regardless of what version of
EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
Is it possible to validate the scancode by driver?
More appropriately, why not just revert the thing? The version change
Reverting the version increment is a bad thing. I agree with Dmitry that
an application that fails just because the API version were incremented
is buggy.
quoted
Well, then we'll break Ubuntu again as they recompiled their input-utils
package (without fixing the check). And the rest of distros do not seem
to be using that package...
Reverting it will also break the ir-keytable userspace program that it is
meant to be used by the Remote Controller devices, and uses it to adjust
its behaviour to support RC's with more than 16 bits of scancodes.
I agree that it is bad that the ABI broke, but reverting it will cause even
more damage.
There we disagree. Sure it's a very poorly thought out interface,
but the way to fix it is to put a new one along side the old,
and put the old back the way it was before it got broken.
I'm not making a fuss here for myself -- I'm more than capable of working
around new kernel bugs like these, but for every person like me there are
likely hundreds of others who simply get frustrated and give up.
If you're worried about Ubuntu's adaptation to the buggy regression,
then email their developers (kernel and input-utils packagers) explaining
the revert, and they can coordination their kernel and input-utils updates
to do the Right Thing.
But for all of the rest of us, our systems are broken by this change.
...
quoted
quoted
quoted
As Mark said, breaking user space simply isn't acceptable. And since
breaking user space isn't acceptable, then incrementing the version is
stupid too.
It might not have been the best idea to increment, however I maintain
that if there exists version is can be changed. Otherwise there is no
point in having version at all.
Not arguing in favor of the version numbering, but it is easy to read
the version increment at the beginning of the application, and adjust
if the code will use EVIOCGKEYCODE or EVIOCGKEYCODE_V2 of the ioctl's,
depending on what kernel provides.
Ok, we might be just calling the new ioctl and check for -ENOSYS at
the beginning, using some fake arguments.
quoted
As I said, reverting the version bump will cause yet another wave of
breakages so I propose leaving version as is.
quoted
The way we add new ioctl's is not by incrementing some "ABI version"
crap. It's by adding new ioctl's or system calls or whatever that
simply used to return -ENOSYS or other error before, while preserving
the old ABI. That way old binaries don't break (for _ANY_ reason), and
new binaries can see "oh, this doesn't support the new thing".
That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
That's the problem: you did NOT keep the two old ioctls().
Those got changed too.. so now we have four NEW ioctls(),
none of which backward compatible with userspace.
Please calm down. This, in fact, is not new vs old ioctl problem but
rather particular driver (or rather set of drivers) implementation
issue. Even if we drop the new ioctls and convert the RC code to use the
old ones you'd be observing the same breakage as RC code responds with
-EINVAL to not-yet-established mappings.
I'll see what can be done for these drivers; I guess we could supply a
fake KEY_RESERVED entry for not mapped scancodes if there are mapped
scancodes "above" current one. That should result in the same behavior
for RCs as before.
--
Dmitry
On Tue, Jan 25, 2011 at 03:29:14PM -0800, Dmitry Torokhov wrote:
On Tue, Jan 25, 2011 at 05:22:09PM -0500, Mark Lord wrote:
quoted
On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote:
quoted
Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
quoted
On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
quoted
On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
We should be able to handle the case where scancode is valid even though
it might be unmapped yet. This is regardless of what version of
EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
Is it possible to validate the scancode by driver?
More appropriately, why not just revert the thing? The version change
Reverting the version increment is a bad thing. I agree with Dmitry that
an application that fails just because the API version were incremented
is buggy.
quoted
Well, then we'll break Ubuntu again as they recompiled their input-utils
package (without fixing the check). And the rest of distros do not seem
to be using that package...
Reverting it will also break the ir-keytable userspace program that it is
meant to be used by the Remote Controller devices, and uses it to adjust
its behaviour to support RC's with more than 16 bits of scancodes.
I agree that it is bad that the ABI broke, but reverting it will cause even
more damage.
There we disagree. Sure it's a very poorly thought out interface,
but the way to fix it is to put a new one along side the old,
and put the old back the way it was before it got broken.
I'm not making a fuss here for myself -- I'm more than capable of working
around new kernel bugs like these, but for every person like me there are
likely hundreds of others who simply get frustrated and give up.
If you're worried about Ubuntu's adaptation to the buggy regression,
then email their developers (kernel and input-utils packagers) explaining
the revert, and they can coordination their kernel and input-utils updates
to do the Right Thing.
But for all of the rest of us, our systems are broken by this change.
...
quoted
quoted
quoted
As Mark said, breaking user space simply isn't acceptable. And since
breaking user space isn't acceptable, then incrementing the version is
stupid too.
It might not have been the best idea to increment, however I maintain
that if there exists version is can be changed. Otherwise there is no
point in having version at all.
Not arguing in favor of the version numbering, but it is easy to read
the version increment at the beginning of the application, and adjust
if the code will use EVIOCGKEYCODE or EVIOCGKEYCODE_V2 of the ioctl's,
depending on what kernel provides.
Ok, we might be just calling the new ioctl and check for -ENOSYS at
the beginning, using some fake arguments.
quoted
As I said, reverting the version bump will cause yet another wave of
breakages so I propose leaving version as is.
quoted
The way we add new ioctl's is not by incrementing some "ABI version"
crap. It's by adding new ioctl's or system calls or whatever that
simply used to return -ENOSYS or other error before, while preserving
the old ABI. That way old binaries don't break (for _ANY_ reason), and
new binaries can see "oh, this doesn't support the new thing".
That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
That's the problem: you did NOT keep the two old ioctls().
Those got changed too.. so now we have four NEW ioctls(),
none of which backward compatible with userspace.
Please calm down. This, in fact, is not new vs old ioctl problem but
rather particular driver (or rather set of drivers) implementation
issue. Even if we drop the new ioctls and convert the RC code to use the
old ones you'd be observing the same breakage as RC code responds with
-EINVAL to not-yet-established mappings.
I'll see what can be done for these drivers; I guess we could supply a
fake KEY_RESERVED entry for not mapped scancodes if there are mapped
scancodes "above" current one. That should result in the same behavior
for RCs as before.
I wonder if the patch below is all that is needed...
Thanks!
--
Dmitry
Input: ir-keymap - return KEY_RESERVED for unknown mappings
Do not respond with -EINVAL to EVIOCGKEYCODE for not-yet-mapped scancodes,
but rather return KEY_RESERVED.
This fixes breakage with Ubuntu's input-kbd utility that stopped returning
full keymaps for remote controls.
Signed-off-by: Dmitry Torokhov <redacted>
---
drivers/media/IR/ir-keytable.c | 28 +++++++++++++++++-----------
1 files changed, 17 insertions(+), 11 deletions(-)
On Tue, Jan 25, 2011 at 12:42:57PM -0200, Mauro Carvalho Chehab wrote:
quoted
Em 25-01-2011 04:52, Dmitry Torokhov escreveu:
quoted
On Mon, Jan 24, 2011 at 09:31:17PM -0800, Dmitry Torokhov wrote:
quoted
On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote:
quoted
On 11-01-25 12:04 AM, Mark Lord wrote:
quoted
On 11-01-24 11:55 PM, Dmitry Torokhov wrote:
quoted
On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote:
..
quoted
quoted
This results in (map->size==10) for 2.6.36+ (wrong),
and a much larger map->size for 2.6.35 and earlier.
So perhaps EVIOCGKEYCODE has changed?
So the utility expects that all devices have flat scancode space and
driver might have changed so it does not recognize scancode 10 as valid
scancode anymore.
The options are:
1. Convert to EVIOCGKEYCODE2
2. Ignore errors from EVIOCGKEYCODE and go through all 65536 iterations.
or 3. Revert/fix the in-kernel regression.
The EVIOCGKEYCODE ioctl is supposed to return KEY_RESERVED for unmapped
(but value) keycodes, and only return -EINVAL when the keycode itself
is out of range.
That's how it worked in all kernels prior to 2.6.36,
and now it is broken. It now returns -EINVAL for any unmapped keycode,
even though keycodes higher than that still have mappings.
This is a bug, a regression, and breaks userspace.
I haven't identified *where* in the kernel the breakage happened,
though.. that code confuses me. :)
Note that this device DOES have "flat scancode space",
and the kernel is now incorrectly signalling an error (-EINVAL)
in response to a perfectly valid query of a VALID (and mappable)
keycode on the remote control
The code really is a valid button, it just doesn't have a default mapping
set by the kernel (I can set a mapping for that code from userspace and it works).
OK, in this case let's ping Mauro - I think he done the adjustments to
IR keymap hanlding.
Thanks.
BTW, could you please try the following patch (it assumes that
EVIOCGVERSION in input.c is alreday relaxed).
Dmitry,
Thanks for your patch. I used part of his logic to improve the ir-keytable
tool at v4l-utils:
http://git.linuxtv.org/v4l-utils.git
The ir-keytable is a tool that just handles Remote Controller input devices,
and do it well, allowing all sorts of operations related to it, and using the
sysfs /sys/class/rc stuff to help its operation. Without any arguments, it
lists the existing RC devices. Arguments are there to allow enabling/disabling
RC protocols, reading/writing/cleaning keycode tables and to test if the
remote is generating events (EV_MSC/EV_KEY/EV_REP/EV_SYN).
Now, it will be using V2 for reads and keycode cleanups, but will still use
V1 for writes, as, currently with 32 bits scancodes, there's no gain to use
V2 for it. Also, changing the tool to use more bits will require to rewrite
part of the code.
Also, writing a rc-core code that can work with an arbitrary large scancode
is still on our TODO list.
I'm not entirely sure how to extend the scancode size, as there are a
few options:
1) Core would always work internally with 32 bytes (1024 bits). Some
logic will be required to accept entries with .len < 32;
2) Drivers will define the code lengtht, and core will use it,
returning -EINVAL if userspace uses a len grater than used internally by
the core. In this case, we'll need a sysfs node to tell userspace what's
the maximum allowed size;
3) Drivers will define the max number of bits, and core will use it,
truncating the number to the max size if userspace tries to write more bits
than the internal representation;
4) Drivers will define the max number of bits, and core will use it,
returning an error if the number is bigger than the max scancode that can be
represented internally.
I think that (2) is the best way for doing it, but I'm not yet entirely sure.
So, it is good to hear some comments about that.
I'd say 4 and userspace utility should normalize scancodes packing them into the
least number of bits possible. Since keymap should be device specific
data in the keymap will not exceed what the driver expects, right?
Well, it depends on what you name "device" ;) If you call it the Linux device that
will handle the Remote Controller, then, the keymap is not driver-specific data.
They will follow one of the protocol standards, like RC-5 (14 bits), NEC (16 bits),
NEC EXTENDED (24 bits), some NEC variants with 32 bits, RC6 (there are several modes,
and the key length depends on what mode is used, ranging from 16 to 64 bits).
The problem is that some drivers support the NEC protocol, with 16 bits, plus 16
bits of checksum, but doesn't support the variants that (ab)use the checksum bits
to extend it to 24 or 32 bits. So, if we do (4), the userspace program will clean
the keytable but will fail on the new keytable programming.
Currently, the driver exports the supported protocols, but it doesn't export the
protocol variants. Maybe the better would be to also export the supported protocol
variants via sysfs.
Cheers,
Mauro
Hi Dmitry,
Em 26-01-2011 00:00, Dmitry Torokhov escreveu:
On Tue, Jan 25, 2011 at 03:29:14PM -0800, Dmitry Torokhov wrote:
quoted
On Tue, Jan 25, 2011 at 05:22:09PM -0500, Mark Lord wrote:
quoted
On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote:
quoted
Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
quoted
On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:
quoted
On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
We should be able to handle the case where scancode is valid even though
it might be unmapped yet. This is regardless of what version of
EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not.
Is it possible to validate the scancode by driver?
More appropriately, why not just revert the thing? The version change
Reverting the version increment is a bad thing. I agree with Dmitry that
an application that fails just because the API version were incremented
is buggy.
quoted
Well, then we'll break Ubuntu again as they recompiled their input-utils
package (without fixing the check). And the rest of distros do not seem
to be using that package...
Reverting it will also break the ir-keytable userspace program that it is
meant to be used by the Remote Controller devices, and uses it to adjust
its behaviour to support RC's with more than 16 bits of scancodes.
I agree that it is bad that the ABI broke, but reverting it will cause even
more damage.
There we disagree. Sure it's a very poorly thought out interface,
but the way to fix it is to put a new one along side the old,
and put the old back the way it was before it got broken.
I'm not making a fuss here for myself -- I'm more than capable of working
around new kernel bugs like these, but for every person like me there are
likely hundreds of others who simply get frustrated and give up.
If you're worried about Ubuntu's adaptation to the buggy regression,
then email their developers (kernel and input-utils packagers) explaining
the revert, and they can coordination their kernel and input-utils updates
to do the Right Thing.
But for all of the rest of us, our systems are broken by this change.
...
quoted
quoted
quoted
As Mark said, breaking user space simply isn't acceptable. And since
breaking user space isn't acceptable, then incrementing the version is
stupid too.
It might not have been the best idea to increment, however I maintain
that if there exists version is can be changed. Otherwise there is no
point in having version at all.
Not arguing in favor of the version numbering, but it is easy to read
the version increment at the beginning of the application, and adjust
if the code will use EVIOCGKEYCODE or EVIOCGKEYCODE_V2 of the ioctl's,
depending on what kernel provides.
Ok, we might be just calling the new ioctl and check for -ENOSYS at
the beginning, using some fake arguments.
quoted
As I said, reverting the version bump will cause yet another wave of
breakages so I propose leaving version as is.
quoted
The way we add new ioctl's is not by incrementing some "ABI version"
crap. It's by adding new ioctl's or system calls or whatever that
simply used to return -ENOSYS or other error before, while preserving
the old ABI. That way old binaries don't break (for _ANY_ reason), and
new binaries can see "oh, this doesn't support the new thing".
That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
That's the problem: you did NOT keep the two old ioctls().
Those got changed too.. so now we have four NEW ioctls(),
none of which backward compatible with userspace.
Please calm down. This, in fact, is not new vs old ioctl problem but
rather particular driver (or rather set of drivers) implementation
issue. Even if we drop the new ioctls and convert the RC code to use the
old ones you'd be observing the same breakage as RC code responds with
-EINVAL to not-yet-established mappings.
I'll see what can be done for these drivers; I guess we could supply a
fake KEY_RESERVED entry for not mapped scancodes if there are mapped
scancodes "above" current one. That should result in the same behavior
for RCs as before.
I wonder if the patch below is all that is needed...
Input: ir-keymap - return KEY_RESERVED for unknown mappings
Do not respond with -EINVAL to EVIOCGKEYCODE for not-yet-mapped scancodes,
but rather return KEY_RESERVED.
This fixes breakage with Ubuntu's input-kbd utility that stopped returning
full keymaps for remote controls.
Signed-off-by: Dmitry Torokhov <redacted>
I tested your patch with both ir-keytable and the input-kbd (with the
version check fixed) on the top of a vanilla 2.6.37 kernel. It works
with both tools.
Feel free to add:
Tested-by: Mauro Carvalho Chehab <redacted>
-
Btw, I took some time to take analyse the input-kbd stuff.
As said at the README:
This is a small collection of input layer utilities. I wrote them
mainly for testing and debugging, but maybe others find them useful
too :-)
...
Gerd Knorr [off-list ref] [SUSE Labs]
This is an old testing tool written by Gerd Hoffmann probably used for him
to test the V4L early Remote Controller implementations. I think he never
meant to use it for anything else. So that's probably the reason why he
added a check for a specific input version.
The last "official" version seems to be this one:
http://dl.bytesex.org/cvs-snapshots/input-20081014-101501.tar.gz
I don't think that Gerd is still maintaining it. On a quick search,
it seems that most distros don't ship it (Debian being an exception).
Gerd, if you're still maintaining it, it is a good idea to apply Dmitry's
patch:
http://www.spinics.net/lists/linux-input/msg13728.html
Otherwise, the tool won't work for NEC-extended and RC-6 tables.
Most of the tools found on Gerd's input package can be replaced with gains
by ir-keytable [1][2].
[1] http://git.linuxtv.org/v4l-utils.git
[2] http://linuxtv.org/downloads/v4l-utils/v4l-utils-0.8.2.tar.bz2
The output format for input-kbd is compatible with the input format of
ir-keytable.
So, people that are using input-kbd to work with a remote controller at
/dev/input/event2 can read the existent RC tables with:
# input-kbd 2 >input_kbd_rc_map
or:
# ir-keytable -r >ir_keytable_rc_map
In order to replace the table, people can use:
# input-kbd -f some_rc_map 2
or:
# ir-keytable -cw input_kbd_rc_map
# ir-keytable -cw ir_keytable_rc_map
To list Remote Controller input devices:
$ ir-keytable
(somewhat similar to what lsinput does)
To test input events:
# input-events 2
# ir-keytable -t
However, as said previously in this thread, input-kbd won't work with any
RC table that uses NEC extended (and there are several devices on the
current Kernels with those tables), since it only reads up to 16 bits.
ir-keytable works with all RC tables, if you use a kernel equal or upper to
2.6.36, due to the usage of the new ioctl's.
ir-keytable also gets some additional data from the remote controllers,
available via sysfs (/sys/class/rc).
Thanks,
Mauro
Btw, I took some time to take analyse the input-kbd stuff.
As said at the README:
This is a small collection of input layer utilities. I wrote them
mainly for testing and debugging, but maybe others find them useful
too :-)
...
Gerd Knorr[off-list ref] [SUSE Labs]
This is an old testing tool written by Gerd Hoffmann probably used for him
to test the V4L early Remote Controller implementations.
Btw, I took some time to take analyse the input-kbd stuff.
As said at the README:
This is a small collection of input layer utilities. I wrote them
mainly for testing and debugging, but maybe others find them useful
too :-)
...
Gerd Knorr[off-list ref] [SUSE Labs]
This is an old testing tool written by Gerd Hoffmann probably used for him
to test the V4L early Remote Controller implementations.
I suspect that Dmitry did the patch against the Debian package, based on a 2007
version of it, as it seems that Debian is using an older version of the package.
Anyway, I've ported his patch to be applied over your -git tree, and tested
on it, with vanilla 2.6.37.
That's the incorrect output result of the old version, with an existing
NEC extended map:
$ sudo /tmp/input/input-kbd 2
/dev/input/event2
bustype : BUS_I2C
vendor : 0x0
product : 0x0
version : 0
name : "i2c IR (i2c IR (EM2820 Winfast "
phys : "i2c-0/0-0030/ir0"
bits ev : EV_SYN EV_KEY EV_MSC EV_REP
bits: KEY_1
bits: KEY_2
bits: KEY_3
bits: KEY_4
...
And that's the output after applying the patch:
$ sudo ./input-kbd 2
/dev/input/event2
bustype : BUS_I2C
vendor : 0x0
product : 0x0
version : 0
name : "i2c IR (i2c IR (EM2820 Winfast "
phys : "i2c-0/0-0030/ir0"
bits ev : EV_SYN EV_KEY EV_MSC EV_REP
map: 31 keys, size: 31/64
0x866b00 = 393 # KEY_VIDEO
0x866b01 = 2 # KEY_1
0x866b02 = 11 # KEY_0
0x866b03 = 386 # KEY_TUNER
-
[PATCH] input-kbd - switch to using EVIOCGKEYCODE2 when available
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
[mchehab@redhat.com: Ported it to the -git version]
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <redacted>
@@ -156,37 +187,25 @@ static void kbd_print_bits(int fd)}}-staticvoidshow_kbd(intnr)+staticvoidshow_kbd(intfd,unsignedintprotocol_version){structkbd_map*map;-intfd;-fd=device_open(nr,1);-if(-1==fd)-return;device_info(fd);-map=kbd_map_read(fd);-if(NULL!=map){-kbd_map_print(stdout,map,0);-}else{+map=kbd_map_read(fd,protocol_version);+if(map)+kbd_map_print(stdout,map,0);+elsekbd_print_bits(fd);-}--close(fd);}-staticintset_kbd(intnr,char*mapfile)+staticintset_kbd(intfd,unsignedintprotocol_version,char*mapfile){structkbd_map*map;FILE*fp;-intfd;-fd=device_open(nr,1);-if(-1==fd)-return-1;--map=kbd_map_read(fd);+map=kbd_map_read(fd,protocol_version);if(NULL==map){fprintf(stderr,"device has no map\n");close(fd);
@@ -203,14 +222,12 @@ static int set_kbd(int nr, char *mapfile)return-1;}}-+if(0!=kbd_map_parse(fp,map)||0!=kbd_map_write(fd,map)){-close(fd);return-1;}-close(fd);return0;}
@@ -224,8 +241,10 @@ static int usage(char *prog, int error)intmain(intargc,char*argv[]){-intc,devnr;+intc,devnr,fd;char*mapfile=NULL;+unsignedintprotocol_version;+intrc=EXIT_FAILURE;for(;;){if(-1==(c=getopt(argc,argv,"hf:")))
@@ -245,12 +264,29 @@ int main(int argc, char *argv[])usage(argv[0],1);devnr=atoi(argv[optind]);-if(mapfile){-set_kbd(devnr,mapfile);-}else{-show_kbd(devnr);++fd=device_open(devnr,1);+if(fd<0)+gotoout;++if(ioctl(fd,EVIOCGVERSION,&protocol_version)<0){+fprintf(stderr,+"Unable to query evdev protocol version: %s\n",+strerror(errno));+gotoout_close;}-return0;++if(mapfile)+set_kbd(fd,protocol_version,mapfile);+else+show_kbd(fd,protocol_version);++rc=EXIT_SUCCESS;++out_close:+close(fd);+out:+returnrc;}/* ---------------------------------------------------------------------
I suspect that Dmitry did the patch against the Debian package, based on a 2007
version of it, as it seems that Debian is using an older version of the package.
From: Mark Lord <hidden> Date: 2011-01-26 14:58:32
On 11-01-26 06:26 AM, Mauro Carvalho Chehab wrote:
..
However, as said previously in this thread, input-kbd won't work with any
RC table that uses NEC extended (and there are several devices on the
current Kernels with those tables), since it only reads up to 16 bits.
ir-keytable works with all RC tables, if you use a kernel equal or upper to
2.6.36, due to the usage of the new ioctl's.
Is there a way to control the key repeat rate for a device
controlled by ir-kbd-i2c ?
It appears to be limited to a max of between 4 and 5 repeats/sec somewhere,
and I'd like to fix that.
???
From: Mark Lord <hidden> Date: 2011-01-26 15:06:02
On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
On Tue, Jan 25, 2011 at 03:29:14PM -0800, Dmitry Torokhov wrote:
quoted
On Tue, Jan 25, 2011 at 05:22:09PM -0500, Mark Lord wrote:
quoted
On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote:
quoted
Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
..
quoted
quoted
quoted
quoted
That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
That's the problem: you did NOT keep the two old ioctls().
Those got changed too.. so now we have four NEW ioctls(),
none of which backward compatible with userspace.
Please calm down. This, in fact, is not new vs old ioctl problem but
rather particular driver (or rather set of drivers) implementation
issue. Even if we drop the new ioctls and convert the RC code to use the
old ones you'd be observing the same breakage as RC code responds with
-EINVAL to not-yet-established mappings.
I'll see what can be done for these drivers; I guess we could supply a
fake KEY_RESERVED entry for not mapped scancodes if there are mapped
scancodes "above" current one. That should result in the same behavior
for RCs as before.
I wonder if the patch below is all that is needed...
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
quoted hunk
Input: ir-keymap - return KEY_RESERVED for unknown mappings
Do not respond with -EINVAL to EVIOCGKEYCODE for not-yet-mapped scancodes,
but rather return KEY_RESERVED.
This fixes breakage with Ubuntu's input-kbd utility that stopped returning
full keymaps for remote controls.
Signed-off-by: Dmitry Torokhov <redacted>
---
drivers/media/IR/ir-keytable.c | 28 +++++++++++++++++-----------
1 files changed, 17 insertions(+), 11 deletions(-)
On Wed, Jan 26, 2011 at 10:05:57AM -0500, Mark Lord wrote:
On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
quoted
On Tue, Jan 25, 2011 at 03:29:14PM -0800, Dmitry Torokhov wrote:
quoted
On Tue, Jan 25, 2011 at 05:22:09PM -0500, Mark Lord wrote:
quoted
On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote:
quoted
Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
..
quoted
quoted
quoted
quoted
quoted
That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
That's the problem: you did NOT keep the two old ioctls().
Those got changed too.. so now we have four NEW ioctls(),
none of which backward compatible with userspace.
Please calm down. This, in fact, is not new vs old ioctl problem but
rather particular driver (or rather set of drivers) implementation
issue. Even if we drop the new ioctls and convert the RC code to use the
old ones you'd be observing the same breakage as RC code responds with
-EINVAL to not-yet-established mappings.
I'll see what can be done for these drivers; I guess we could supply a
fake KEY_RESERVED entry for not mapped scancodes if there are mapped
scancodes "above" current one. That should result in the same behavior
for RCs as before.
I wonder if the patch below is all that is needed...
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
It would be much more helpful if you tried to test what has been fixed
(hint: version change wasn't it).
--
Dmitry
On Wed, Jan 26, 2011 at 12:18:29PM -0200, Mauro Carvalho Chehab wrote:
Em 26-01-2011 11:08, Gerd Hoffmann escreveu:
quoted
Hi,
quoted
Btw, I took some time to take analyse the input-kbd stuff.
As said at the README:
This is a small collection of input layer utilities. I wrote them
mainly for testing and debugging, but maybe others find them useful
too :-)
...
Gerd Knorr[off-list ref] [SUSE Labs]
This is an old testing tool written by Gerd Hoffmann probably used for him
to test the V4L early Remote Controller implementations.
I suspect that Dmitry did the patch against the Debian package, based on a 2007
version of it, as it seems that Debian is using an older version of the package.
@@ -101,8 +101,8 @@ int device_open(int nr, int verbose)close(fd);return-1;}-if(EV_VERSION!=version){-fprintf(stderr,"protocol version mismatch (expected %d, got %d)\n",+if(EV_VERSION>version){+fprintf(stderr,"protocol version mismatch (expected >= %d, got %d)\n",EV_VERSION,version);
Please do not do this. It causes check to "float" depending on the
version of kernel headers it was compiled against.
The check should be against concrete version (0x10000 in this case).
Thanks.
--
Dmitry
On 11-01-26 06:26 AM, Mauro Carvalho Chehab wrote:
..
quoted
However, as said previously in this thread, input-kbd won't work with any
RC table that uses NEC extended (and there are several devices on the
current Kernels with those tables), since it only reads up to 16 bits.
ir-keytable works with all RC tables, if you use a kernel equal or upper to
2.6.36, due to the usage of the new ioctl's.
Is there a way to control the key repeat rate for a device
controlled by ir-kbd-i2c ?
It appears to be limited to a max of between 4 and 5 repeats/sec somewhere,
and I'd like to fix that.
It depends on what device do you have. Several I2C chips have the repeat
logic inside the I2C microcontroller PROM firmware. or at the remote
controller itself. So, there's nothing we can do to change it.
I have even one device here (I think it is a saa7134-based Kworld device)
that doesn't send any repeat event at all for most keys (I think it only
sends repeat events for volume - Can't remember the specific details anymore -
too many devices!).
The devices that produce repeat events can be adjusted via the normal
input layer tools like kbdrate.
Thanks,
Mauro
On Wed, Jan 26, 2011 at 03:41:01PM -0200, Mauro Carvalho Chehab wrote:
Em 26-01-2011 12:58, Mark Lord escreveu:
quoted
On 11-01-26 06:26 AM, Mauro Carvalho Chehab wrote:
..
quoted
However, as said previously in this thread, input-kbd won't work with any
RC table that uses NEC extended (and there are several devices on the
current Kernels with those tables), since it only reads up to 16 bits.
ir-keytable works with all RC tables, if you use a kernel equal or upper to
2.6.36, due to the usage of the new ioctl's.
Is there a way to control the key repeat rate for a device
controlled by ir-kbd-i2c ?
It appears to be limited to a max of between 4 and 5 repeats/sec somewhere,
and I'd like to fix that.
It depends on what device do you have. Several I2C chips have the repeat
logic inside the I2C microcontroller PROM firmware. or at the remote
controller itself. So, there's nothing we can do to change it.
I have even one device here (I think it is a saa7134-based Kworld device)
that doesn't send any repeat event at all for most keys (I think it only
sends repeat events for volume - Can't remember the specific details anymore -
too many devices!).
The devices that produce repeat events can be adjusted via the normal
input layer tools like kbdrate.
Unfortunately kbdrate affects all connected devices and I am not sure if
there is a utility allowing to set rate on individual devices. But here
is the main part:
static int input_set_rate(int fd,
unsigned int delay, unsigned int period)
{
unsigned int rep[2] = { delay, period };
if (ioctl(fd, EVIOCSREP, rep) < 0) {
perror("evdev ioctl");
return -1;
}
return 0;
}
--
Dmitry
On Tue, Jan 25, 2011 at 03:29:14PM -0800, Dmitry Torokhov wrote:
quoted
On Tue, Jan 25, 2011 at 05:22:09PM -0500, Mark Lord wrote:
quoted
On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote:
quoted
Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
..
quoted
quoted
quoted
quoted
quoted
That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
That's the problem: you did NOT keep the two old ioctls().
Those got changed too.. so now we have four NEW ioctls(),
none of which backward compatible with userspace.
Please calm down. This, in fact, is not new vs old ioctl problem but
rather particular driver (or rather set of drivers) implementation
issue. Even if we drop the new ioctls and convert the RC code to use the
old ones you'd be observing the same breakage as RC code responds with
-EINVAL to not-yet-established mappings.
I'll see what can be done for these drivers; I guess we could supply a
fake KEY_RESERVED entry for not mapped scancodes if there are mapped
scancodes "above" current one. That should result in the same behavior
for RCs as before.
I wonder if the patch below is all that is needed...
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
You need to relax the version test at the tree. As I said before, this is
a development tool from the early RC days, bound to work with one specific
version of the API, and programmed by purpose to fail if there would by any
updates at the Input layer.
Cheers,
Mauro.
@@ -101,8 +101,8 @@ int device_open(int nr, int verbose)close(fd);return-1;}-if(EV_VERSION!=version){-fprintf(stderr,"protocol version mismatch (expected %d, got %d)\n",+if(EV_VERSION>version){+fprintf(stderr,"protocol version mismatch (expected >= %d, got %d)\n",EV_VERSION,version);
Please do not do this. It causes check to "float" depending on the
version of kernel headers it was compiled against.
The check should be against concrete version (0x10000 in this case).
The idea here is to not prevent it to load if version is 0x10001.
This is actually the only change that it is really needed (after applying
your KEY_RESERVED patch to 2.6.37) for the tool to work. Reverting it causes
the error:
$ sudo ./input-kbd 2
/dev/input/event2
protocol version mismatch (expected >= 65536, got 65537)
Just applying this diff to the previous version:
$ git diff 442bc4e7697a3f20ce9a24df630324d94cd22ba6
@@ -101,8 +101,8 @@ int device_open(int nr, int verbose)close(fd);return-1;}-if(EV_VERSION!=version){-fprintf(stderr,"protocol version mismatch (expected %d, got %d)\n",+if(EV_VERSION>version){+fprintf(stderr,"protocol version mismatch (expected >= %d, got %d)\n",EV_VERSION,version);
Please do not do this. It causes check to "float" depending on the
version of kernel headers it was compiled against.
The check should be against concrete version (0x10000 in this case).
The idea here is to not prevent it to load if version is 0x10001.
This is actually the only change that it is really needed (after applying
your KEY_RESERVED patch to 2.6.37) for the tool to work. Reverting it causes
the error:
You did not understand. When comparing against EV_VERSION, if you
compile on 2.6.32 you are comparing with 0x10000. If you are compiling
on 2.6.37 you are comparing with 0x10001 as EV_VERSION value changes
(not the value returned by EVIOCGVERSION, the value of the _define_
itself).
The proper check is:
#define EVDEV_MIN_VERSION 0x10000
if (version < EVDEV_MIN_VERSION) {
fprintf(stderr,
"protocol version mismatch (need at least %d, got %d)\n",
EVDEV_MIN_VERSION, version);
...
}
--
Dmitry
The check should be against concrete version (0x10000 in this case).
Stepping back: what does the version mean?
0x10000 == 1.0 ?
0x10001 == 1.1 ?
Can I expect the interface stay backward compatible if only the minor
revision changes, i.e. makes it sense to accept 1.x?
Will the major revision ever change? Does it make sense to check the
version at all?
thanks,
Gerd
@@ -101,8 +101,8 @@ int device_open(int nr, int verbose)close(fd);return-1;}-if(EV_VERSION!=version){-fprintf(stderr,"protocol version mismatch (expected %d, got %d)\n",+if(EV_VERSION>version){+fprintf(stderr,"protocol version mismatch (expected >= %d, got %d)\n",EV_VERSION,version);
Please do not do this. It causes check to "float" depending on the
version of kernel headers it was compiled against.
The check should be against concrete version (0x10000 in this case).
The idea here is to not prevent it to load if version is 0x10001.
This is actually the only change that it is really needed (after applying
your KEY_RESERVED patch to 2.6.37) for the tool to work. Reverting it causes
the error:
You did not understand. When comparing against EV_VERSION, if you
compile on 2.6.32 you are comparing with 0x10000. If you are compiling
on 2.6.37 you are comparing with 0x10001 as EV_VERSION value changes
(not the value returned by EVIOCGVERSION, the value of the _define_
itself).
The proper check is:
#define EVDEV_MIN_VERSION 0x10000
if (version < EVDEV_MIN_VERSION) {
fprintf(stderr,
"protocol version mismatch (need at least %d, got %d)\n",
EVDEV_MIN_VERSION, version);
...
}
Guys, NO!
The proper check is actually to remove all of that silly VERSION testing
from the userspace binary. And then have it try EVIOCGKEYCODE_V2 first.
If EVIOCGKEYCODE_V2 fails (-ENOTTY, -EINVAL, or -ENOSYS), then
have it fall back to trying to use EVIOCGKEYCODE.
Of course this does assume that the new EVIOCGKEYCODE_V2 interface uses
correct ioctl return values..
Cheers
From: Mark Lord <hidden> Date: 2011-01-26 19:28:16
On 11-01-26 02:16 PM, Gerd Hoffmann wrote:
Hi,
quoted
quoted
quoted
The check should be against concrete version (0x10000 in this case).
Stepping back: what does the version mean?
0x10000 == 1.0 ?
0x10001 == 1.1 ?
Can I expect the interface stay backward compatible if only the minor revision
changes, i.e. makes it sense to accept 1.x?
Will the major revision ever change? Does it make sense to check the version at
all?
As already established earlier in this thread,
by Linus Torvalds as well as by myself,
NO!
That whole "version" concept is broken and inappropriate.
Userspace should simply ignore it completely.
Cheers
The check should be against concrete version (0x10000 in this case).
Dmitry,
Ok, now I see what you're meaning. Yeah, an absolute version check like
what you've proposed is better than a relative version check.
Stepping back: what does the version mean?
0x10000 == 1.0 ?
0x10001 == 1.1 ?
Can I expect the interface stay backward compatible if only the minor revision changes, i.e. makes it sense to accept 1.x?
Will the major revision ever change? Does it make sense to check the version at all?
Gerd,
Dmitry will likely have a better answer for me, but I think you should
just remove the test. By principle, the interface should always be
backward compatible (if it isn't, then we have a regression to fix).
You may expect newer features on newer versions, so I understand
that the version check is there to just allow userspace to enable
new code for newer evdev protocol revisions.
Thanks,
Mauro
From: Mark Lord <hidden> Date: 2011-01-26 19:30:22
On 11-01-26 12:59 PM, Dmitry Torokhov wrote:
On Wed, Jan 26, 2011 at 03:41:01PM -0200, Mauro Carvalho Chehab wrote:
quoted
Em 26-01-2011 12:58, Mark Lord escreveu:
quoted
On 11-01-26 06:26 AM, Mauro Carvalho Chehab wrote:
..
quoted
However, as said previously in this thread, input-kbd won't work with any
RC table that uses NEC extended (and there are several devices on the
current Kernels with those tables), since it only reads up to 16 bits.
ir-keytable works with all RC tables, if you use a kernel equal or upper to
2.6.36, due to the usage of the new ioctl's.
Is there a way to control the key repeat rate for a device
controlled by ir-kbd-i2c ?
It appears to be limited to a max of between 4 and 5 repeats/sec somewhere,
and I'd like to fix that.
It depends on what device do you have. Several I2C chips have the repeat
logic inside the I2C microcontroller PROM firmware. or at the remote
controller itself. So, there's nothing we can do to change it.
I have even one device here (I think it is a saa7134-based Kworld device)
that doesn't send any repeat event at all for most keys (I think it only
sends repeat events for volume - Can't remember the specific details anymore -
too many devices!).
The devices that produce repeat events can be adjusted via the normal
input layer tools like kbdrate.
Unfortunately kbdrate affects all connected devices and I am not sure if
there is a utility allowing to set rate on individual devices. But here
is the main part:
static int input_set_rate(int fd,
unsigned int delay, unsigned int period)
{
unsigned int rep[2] = { delay, period };
if (ioctl(fd, EVIOCSREP, rep) < 0) {
perror("evdev ioctl");
return -1;
}
return 0;
}
Okay, if that's still a global in this day and age,
then I suppose I'll just have to special-case it here in my copy.
The hardware itself is capable of much faster repeat rates,
and prior to 2.6.36 I used to patch it for intelligent ramp-up
on repeats inside ir-kbd-i2c.
As of 2.6.36 that stopped working, and is now limited somewhere
to no more than one repeat every 210msecs.
Cheers
From: Mark Lord <hidden> Date: 2011-01-26 19:31:47
On 11-01-26 11:44 AM, Dmitry Torokhov wrote:
On Wed, Jan 26, 2011 at 10:05:57AM -0500, Mark Lord wrote:
..
quoted
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
It would be much more helpful if you tried to test what has been fixed
(hint: version change wasn't it).
It would be much more helpful if you would revert that which was broken
in 2.6.36. (hint: version was part of it).
The other part does indeed appear to work with the old binary for input-kbd,
but the binary for lsinput still fails as above.
Cheers
On Wed, Jan 26, 2011 at 08:16:09PM +0100, Gerd Hoffmann wrote: > Hi,
quoted
quoted
quoted
The check should be against concrete version (0x10000 in this case).
Stepping back: what does the version mean?
Nothing, it is just a number.
0x10000 == 1.0 ?
0x10001 == 1.1 ?
No, not really.
Can I expect the interface stay backward compatible if only the
minor revision changes, i.e. makes it sense to accept 1.x?
I am not planning on breaking backward compatibility.
Will the major revision ever change? Does it make sense to check
the version at all?
It depends. We do not have a clear way to see if new ioctls are
supported (and I do not consider "try new ioctl and see if data sticks"
being a good way) so that facilitated protocol version rev-up. So keymap
manipulating tools might be forced to check protocol version. For the
rest I think doing EVIOCGVERSION just to check that ioctl is supported
is an OK way to validate that we are dealing with an event device, but
that's it.
BTW, maybe we should move lsinput and input-kbd into linuxconsole
package, together with evtest, fftest, etc?
--
Dmitry
From: Mark Lord <hidden> Date: 2011-01-26 19:33:21
On 11-01-26 12:32 PM, Mauro Carvalho Chehab wrote:
Em 26-01-2011 13:05, Mark Lord escreveu:
..
quoted
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
You need to relax the version test at the tree. As I said before, this is
a development tool from the early RC days, bound to work with one specific
version of the API, and programmed by purpose to fail if there would by any
updates at the Input layer.
..
As I said before, I personally have done that on my copy here.
But that's not what this thread is about.
This thread is about broken userspace courtesy of these changes.
So I am testing with the original userspace binary here,
and it still fails. And will continue to fail until that regression is fixed.
Cheers
On Wed, Jan 26, 2011 at 02:31:44PM -0500, Mark Lord wrote:
On 11-01-26 11:44 AM, Dmitry Torokhov wrote:
quoted
On Wed, Jan 26, 2011 at 10:05:57AM -0500, Mark Lord wrote:
..
quoted
quoted
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
It would be much more helpful if you tried to test what has been fixed
(hint: version change wasn't it).
It would be much more helpful if you would revert that which was broken
in 2.6.36. (hint: version was part of it).
No, version change will not be reverted as we do not have a way to
validate whether new ioctls are supported. The older kernels are
returning -EINVAL for unknown evdev ioctls so userspace can't know
if ioctl failed because it is unsupported or because arguments are
wrong/not applicable for the underlying device.
The other part does indeed appear to work with the old binary for input-kbd,
but the binary for lsinput still fails as above.
Great, then I'' include the fix for RC keytables in my next pull
request. I guess it should go to stable as well.
Thanks.
--
Dmitry
On Wed, Jan 26, 2011 at 02:33:17PM -0500, Mark Lord wrote:
On 11-01-26 12:32 PM, Mauro Carvalho Chehab wrote:
quoted
Em 26-01-2011 13:05, Mark Lord escreveu:
..
quoted
quoted
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
You need to relax the version test at the tree. As I said before, this is
a development tool from the early RC days, bound to work with one specific
version of the API, and programmed by purpose to fail if there would by any
updates at the Input layer.
..
As I said before, I personally have done that on my copy here.
But that's not what this thread is about.
This thread is about broken userspace courtesy of these changes.
So I am testing with the original userspace binary here,
and it still fails. And will continue to fail until that regression is fixed.
I do not consider lsinput refusing to work a regression. The tool
claims to work with particular protocol version and it is tool's choice.
Shall I write a utility that checks kernel version and only works with
2.6.37 and yell when we release 2.6.38?
--
Dmitry
From: Mark Lord <hidden> Date: 2011-01-26 19:47:21
On 11-01-26 02:41 PM, Dmitry Torokhov wrote:
I do not consider lsinput refusing to work a regression.
Obviously, since you don't use that tool.
Those of us who do use it see this as broken userspace compatibility.
Who the hell reviews this crap, anyway?
Code like that should never have made it upstream in the first place.
Cheers
On Wed, Jan 26, 2011 at 02:47:18PM -0500, Mark Lord wrote:
On 11-01-26 02:41 PM, Dmitry Torokhov wrote:
quoted
I do not consider lsinput refusing to work a regression.
Obviously, since you don't use that tool.
Those of us who do use it see this as broken userspace compatibility.
Who the hell reviews this crap, anyway?
Code like that should never have made it upstream in the first place.
You are more than welcome spend more time on reviews.
Thanks.
--
Dmitry
It depends. We do not have a clear way to see if new ioctls are
supported (and I do not consider "try new ioctl and see if data sticks"
being a good way) so that facilitated protocol version rev-up.
Yea, EVIOCGKEYCODE_V2 on a old kernel returns EINVAL. Not good. There
is another one which should have been used to signal "unknown ioctl",
ENOTTY IIRC (a bit silly for historical reasons), so you can figure
whenever your input data is invalid or whenever the ioctl isn't
supported in the first place (in which case you could just fallback to
the old version).
So keymap
manipulating tools might be forced to check protocol version.
From: Mark Lord <hidden> Date: 2011-01-26 21:41:12
On 11-01-26 02:50 PM, Dmitry Torokhov wrote:
On Wed, Jan 26, 2011 at 02:47:18PM -0500, Mark Lord wrote:
quoted
On 11-01-26 02:41 PM, Dmitry Torokhov wrote:
quoted
I do not consider lsinput refusing to work a regression.
Obviously, since you don't use that tool.
Those of us who do use it see this as broken userspace compatibility.
Who the hell reviews this crap, anyway?
Code like that should never have made it upstream in the first place.
You are more than welcome spend more time on reviews.
Somehow I detect a totally lack of sincerity there.
But thanks for fixing the worst of this regression, at least.
Perhaps you might think about eventually fixing the bad use of -EINVAL
in future revisions. One way perhaps to approach that, would be to begin
fixing it internally, but still returning the same things from the actual
f_ops->ioctl() routine.
Then eventually provide new ioctl numbers which return the correct -ENOTTY
(or whatever is best there), rather than converting to -EVINAL at the interface.
Then a nice multi-year overlap, with a scheduled removal of the old codes some day.
Then the input subsystem would work more like most other subsystems,
and make userspace programming simpler and easier to "get correct".
Cheers
From: Mark Lord <hidden> Date: 2011-01-26 21:49:18
Or perhaps get rid of that unworkable "version number" thing
(just freeze it in time with the 2.6.35 value returned),
and implement a "get_feature_flags" ioctl or something for going forward.
Then you can just turn on new bits in the flags as new features are added.
It's a kludge (to get around the poor use of -EINVAL everywhere),
but at least it's a design that's workable.
Cheers
On Wed, Jan 26, 2011 at 04:41:07PM -0500, Mark Lord wrote:
On 11-01-26 02:50 PM, Dmitry Torokhov wrote:
quoted
On Wed, Jan 26, 2011 at 02:47:18PM -0500, Mark Lord wrote:
quoted
On 11-01-26 02:41 PM, Dmitry Torokhov wrote:
quoted
I do not consider lsinput refusing to work a regression.
Obviously, since you don't use that tool.
Those of us who do use it see this as broken userspace compatibility.
Who the hell reviews this crap, anyway?
Code like that should never have made it upstream in the first place.
You are more than welcome spend more time on reviews.
Somehow I detect a totally lack of sincerity there.
No, not really. If we known about Ubuntu's employ of such utility before
we'd try to come up with workaround or updated the utility proactively
so user-visible changes would be limited.
But thanks for fixing the worst of this regression, at least.
Perhaps you might think about eventually fixing the bad use of -EINVAL
in future revisions. One way perhaps to approach that, would be to begin
fixing it internally,
Yes, that is on my lest (unless somebody beats me to it). Won't help
with the older kernels though, unfortunately.
but still returning the same things from the actual
f_ops->ioctl() routine.
Not sure if this is needed.
Then eventually provide new ioctl numbers which return the correct -ENOTTY
(or whatever is best there), rather than converting to -EVINAL at the interface.
Then a nice multi-year overlap, with a scheduled removal of the old codes some day.
Then the input subsystem would work more like most other subsystems,
and make userspace programming simpler and easier to "get correct".
I do not believe that such characterization is called for. We did fix
the breakage that was ABI breakage. The version issue is different. If
we go by what you say _none_ of the versions anywhere can be changed
ever because there might be a program that does not expect new version.
--
Dmitry
On Wed, Jan 26, 2011 at 04:49:14PM -0500, Mark Lord wrote:
Or perhaps get rid of that unworkable "version number" thing
(just freeze it in time with the 2.6.35 value returned),
and implement a "get_feature_flags" ioctl or something for going forward.
Then you can just turn on new bits in the flags as new features are added.
That could be done but I do not expect retire features so far so version
is about the same. Plus, what guarantees that someone in the future
won't write a utility that compares exact capability bitmap and refuse
to work when new ones will be added?
--
Dmitry
From: Mark Lord <hidden> Date: 2011-01-27 01:01:16
On 11-01-26 10:05 AM, Mark Lord wrote:
On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
..
quoted
I wonder if the patch below is all that is needed...
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
Heh.. I just noticed something *new* in the bootlogs on my system:
kernel: Registered IR keymap rc-rc5-tv
udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
kernel: ir-keytable[6439]: segfault at 8 ip 00000000004012d2 sp 00007fff6d43ca60
error 4 in ir-keytable[400000+7000]
kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv i2c
driver #0]
That's udev invoking ir-keyboard when the ir-kbd-i2c kernel module is loaded,
and that is also ir-keyboard (userspace) segfaulting when run.
That behaviour is new, with the proposed "fix" patch from this thread.
So the "fix" itself appears to also break userspace.
The ir-keyboard program reports: IR keytable control version 0.8.2
Cheers
From: Mark Lord <hidden> Date: 2011-01-27 01:07:33
On 11-01-26 08:01 PM, Mark Lord wrote:
On 11-01-26 10:05 AM, Mark Lord wrote:
quoted
On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
..
quoted
quoted
I wonder if the patch below is all that is needed...
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
Heh.. I just noticed something *new* in the bootlogs on my system:
kernel: Registered IR keymap rc-rc5-tv
udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
kernel: ir-keytable[6439]: segfault at 8 ip 00000000004012d2 sp 00007fff6d43ca60
error 4 in ir-keytable[400000+7000]
kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv i2c
driver #0]
That's udev invoking ir-keyboard when the ir-kbd-i2c kernel module is loaded,
and that is also ir-keyboard (userspace) segfaulting when run.
Note: I tried to capture an strace of ir-keyboard segfaulting during boot
(as above), but doing so kills the system (hangs on boot).
The command from udev was: /usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0
On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote:
On 11-01-26 08:01 PM, Mark Lord wrote:
quoted
On 11-01-26 10:05 AM, Mark Lord wrote:
quoted
On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
..
quoted
quoted
I wonder if the patch below is all that is needed...
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
Heh.. I just noticed something *new* in the bootlogs on my system:
kernel: Registered IR keymap rc-rc5-tv
udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
kernel: ir-keytable[6439]: segfault at 8 ip 00000000004012d2 sp 00007fff6d43ca60
error 4 in ir-keytable[400000+7000]
kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv i2c
driver #0]
That's udev invoking ir-keyboard when the ir-kbd-i2c kernel module is loaded,
and that is also ir-keyboard (userspace) segfaulting when run.
Note: I tried to capture an strace of ir-keyboard segfaulting during boot
(as above), but doing so kills the system (hangs on boot).
The command from udev was: /usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0
Does it die when you try to invoke the command by hand? Can you see
where?
--
Dmitry
From: Mark Lord <hidden> Date: 2011-01-27 03:18:58
On 11-01-26 09:12 PM, Dmitry Torokhov wrote:
On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote:
quoted
On 11-01-26 08:01 PM, Mark Lord wrote:
quoted
On 11-01-26 10:05 AM, Mark Lord wrote:
quoted
On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
..
quoted
quoted
I wonder if the patch below is all that is needed...
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
Heh.. I just noticed something *new* in the bootlogs on my system:
kernel: Registered IR keymap rc-rc5-tv
udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
kernel: ir-keytable[6439]: segfault at 8 ip 00000000004012d2 sp 00007fff6d43ca60
error 4 in ir-keytable[400000+7000]
kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv i2c
driver #0]
That's udev invoking ir-keyboard when the ir-kbd-i2c kernel module is loaded,
and that is also ir-keyboard (userspace) segfaulting when run.
Note: I tried to capture an strace of ir-keyboard segfaulting during boot
(as above), but doing so kills the system (hangs on boot).
The command from udev was: /usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0
Does it die when you try to invoke the command by hand? Can you see where?
No, it does not seem to segfault when I unload/reload ir-kbd-i2c
and then invoke it by hand with the same parameters.
Quite possibly the environment is different when udev invokes it,
and my strace attempt with udev killed the system, so no info there.
It does NOT segfault on the stock 2.6.37 kernel, without the patch.
-ml
On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote:
On 11-01-26 09:12 PM, Dmitry Torokhov wrote:
quoted
On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote:
quoted
On 11-01-26 08:01 PM, Mark Lord wrote:
quoted
On 11-01-26 10:05 AM, Mark Lord wrote:
quoted
On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
..
quoted
quoted
I wonder if the patch below is all that is needed...
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
Heh.. I just noticed something *new* in the bootlogs on my system:
kernel: Registered IR keymap rc-rc5-tv
udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
kernel: ir-keytable[6439]: segfault at 8 ip 00000000004012d2 sp 00007fff6d43ca60
error 4 in ir-keytable[400000+7000]
kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv i2c
driver #0]
That's udev invoking ir-keyboard when the ir-kbd-i2c kernel module is loaded,
and that is also ir-keyboard (userspace) segfaulting when run.
Note: I tried to capture an strace of ir-keyboard segfaulting during boot
(as above), but doing so kills the system (hangs on boot).
The command from udev was: /usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0
Does it die when you try to invoke the command by hand? Can you see where?
No, it does not seem to segfault when I unload/reload ir-kbd-i2c
and then invoke it by hand with the same parameters.
Quite possibly the environment is different when udev invokes it,
and my strace attempt with udev killed the system, so no info there.
It does NOT segfault on the stock 2.6.37 kernel, without the patch.
I must admit I am baffled. The patch in question only affects the
EVIOCGKEYCODE path whereas '-a' means "automatically load appropriate
keymap" and as far as I can see it does not call EVIOCGKEYCODE, only
EVIOCSKEYCODE...
Mauro, any ideas?
BTW, I wonder what package ir-keytable is coming from? Ubuntu seems to
have v4l-utils at 0.8.1-2 and you say yours is 0.8.2...
Thanks.
--
Dmitry
On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote:
quoted
On 11-01-26 09:12 PM, Dmitry Torokhov wrote:
quoted
On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote:
quoted
On 11-01-26 08:01 PM, Mark Lord wrote:
quoted
On 11-01-26 10:05 AM, Mark Lord wrote:
quoted
On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
..
quoted
quoted
I wonder if the patch below is all that is needed...
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
Heh.. I just noticed something *new* in the bootlogs on my system:
kernel: Registered IR keymap rc-rc5-tv
udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
kernel: ir-keytable[6439]: segfault at 8 ip 00000000004012d2 sp 00007fff6d43ca60
error 4 in ir-keytable[400000+7000]
kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv i2c
driver #0]
That's udev invoking ir-keyboard when the ir-kbd-i2c kernel module is loaded,
and that is also ir-keyboard (userspace) segfaulting when run.
Note: I tried to capture an strace of ir-keyboard segfaulting during boot
(as above), but doing so kills the system (hangs on boot).
The command from udev was: /usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0
Does it die when you try to invoke the command by hand? Can you see where?
No, it does not seem to segfault when I unload/reload ir-kbd-i2c
and then invoke it by hand with the same parameters.
Quite possibly the environment is different when udev invokes it,
and my strace attempt with udev killed the system, so no info there.
It does NOT segfault on the stock 2.6.37 kernel, without the patch.
I must admit I am baffled. The patch in question only affects the
EVIOCGKEYCODE path whereas '-a' means "automatically load appropriate
keymap" and as far as I can see it does not call EVIOCGKEYCODE, only
EVIOCSKEYCODE...
Mauro, any ideas?
BTW, I wonder what package ir-keytable is coming from? Ubuntu seems to
have v4l-utils at 0.8.1-2 and you say yours is 0.8.2...
0.8.2 is the new version that was released in Jan, 25. One of the major
differences is that it now installs the udev rules, with make install.
This is there in order to prepare to the removal of all those in-kernel
Remote Controller tables.
On my tests here, this is working fine, with Fedora and RHEL 6, on my
usual test devices, so I don't believe that the tool itself is broken,
nor I think that the issue is due to the fix patch.
I remember that when Kay added a persistence utility tool that opens a V4L
device in order to read some capabilities, this caused a race condition
into a number of drivers that use to register the video device too early.
The result is that udev were opening the device before the end of the
register process, causing OOPS and other problems.
I suspect that Mark may be experiencing a similar issue.
I don't think that most of the c/c will be able to help with this issue.
So, I think that the better is if Mark could either open a Buzgilla or
send me and linux-media the dmesg logs and other information that he may
have about what's happening. It would be interesting if he can remove the
udev rule, boot it and run the udev command manually, to see if this is
really a race condition or something else. If it fails manually, the
better is to activate ftrace logs for rc-core and for the driver functions
and send us the trace logs.
Thanks,
Mauro
From: Mark Lord <hidden> Date: 2011-01-27 15:00:14
On 11-01-27 05:30 AM, Mauro Carvalho Chehab wrote:
..
0.8.2 is the new version that was released in Jan, 25. One of the major
differences is that it now installs the udev rules, with make install.
Oh, and there's no "make uninstall" option in the Makefile, either.
Where does it put those tentacles, so that I can delete them again ?
On my tests here, this is working fine, with Fedora and RHEL 6, on my
usual test devices, so I don't believe that the tool itself is broken,
nor I think that the issue is due to the fix patch.
Well, all I know is that it does NOT segfault without the patch,
and now it does. At this point I should refer you back to Linus's
posts earlier in this thread for the definition of "breaks userspace".
I remember that when Kay added a persistence utility tool that opens a V4L
device in order to read some capabilities, this caused a race condition
into a number of drivers that use to register the video device too early.
The result is that udev were opening the device before the end of the
register process, causing OOPS and other problems.
I suspect that Mark may be experiencing a similar issue.
Could be. I really don't know.
Again, I could not care less about ir-keyboard,
as I don't use it here at all.
But also again, this thread isn't about what I need fixed,
but rather about broken userspace from 2.6.36 onward.
And the patch to "fix" it seems to possibly cause more breakage.
Cheers
On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote:
On 11-01-26 09:12 PM, Dmitry Torokhov wrote:
quoted
On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote:
quoted
On 11-01-26 08:01 PM, Mark Lord wrote:
quoted
On 11-01-26 10:05 AM, Mark Lord wrote:
quoted
On 11-01-25 09:00 PM, Dmitry Torokhov wrote:
..
quoted
quoted
I wonder if the patch below is all that is needed...
Nope. Does not work here:
$ lsinput
protocol version mismatch (expected 65536, got 65537)
Heh.. I just noticed something *new* in the bootlogs on my system:
kernel: Registered IR keymap rc-rc5-tv
udevd-event[6438]: run_program: '/usr/bin/ir-keytable' abnormal exit
kernel: input: i2c IR (Hauppauge) as /devices/virtual/rc/rc0/input7
kernel: ir-keytable[6439]: segfault at 8 ip 00000000004012d2 sp 00007fff6d43ca60
error 4 in ir-keytable[400000+7000]
kernel: rc0: i2c IR (Hauppauge) as /devices/virtual/rc/rc0
kernel: ir-kbd-i2c: i2c IR (Hauppauge) detected at i2c-0/0-0018/ir0 [ivtv i2c
driver #0]
That's udev invoking ir-keyboard when the ir-kbd-i2c kernel module is loaded,
and that is also ir-keyboard (userspace) segfaulting when run.
Note: I tried to capture an strace of ir-keyboard segfaulting during boot
(as above), but doing so kills the system (hangs on boot).
The command from udev was: /usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0
Does it die when you try to invoke the command by hand? Can you see where?
No, it does not seem to segfault when I unload/reload ir-kbd-i2c
and then invoke it by hand with the same parameters.
Quite possibly the environment is different when udev invokes it,
and my strace attempt with udev killed the system, so no info there.
Hmm, what about compiling with debug and getting a core then?
--
Dmitry
On Thu, Jan 27, 2011 at 08:30:00AM -0200, Mauro Carvalho Chehab wrote:
On my tests here, this is working fine, with Fedora and RHEL 6, on my
usual test devices, so I don't believe that the tool itself is broken,
nor I think that the issue is due to the fix patch.
I remember that when Kay added a persistence utility tool that opens a V4L
device in order to read some capabilities, this caused a race condition
into a number of drivers that use to register the video device too early.
The result is that udev were opening the device before the end of the
register process, causing OOPS and other problems.
Well, this is quite possible. The usev ruls in the v4l-utils reads:
ACTION=="add", SUBSYSTEM=="rc", RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $name"
So we act when we add RC device to the system. The corresponding input
device has not been registered yet (and will not be for some time
because before creating input ddevice we invoke request_module() to load
initial rc map module) so the tool runs simultaneously with kernel
registering input device and it could very well be it can't find
something it really wants.
This would explain why Mark sees the segfault only when invoked via
udev but not when ran manually.
However I still do not understand why Mark does not see the same issue
without the patch. Like I said, maybe if Mark could recompile with
debug data and us a core we'd see what is going on.
BTW, that means that we need to redo udev rules. Maybe we should split
the utility into 2 parts - one dealing with rcX device and for keymap
setting reuse udev's existing utility that adjusts maps on ann input
devices, not for RCs only.
--
Dmitry
From: Mark Lord <hidden> Date: 2011-01-27 18:12:53
On 11-01-27 11:39 AM, Dmitry Torokhov wrote:
On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote:
quoted
No, it does not seem to segfault when I unload/reload ir-kbd-i2c
and then invoke it by hand with the same parameters.
Quite possibly the environment is different when udev invokes it,
and my strace attempt with udev killed the system, so no info there.
Hmm, what about compiling with debug and getting a core then?
Sure. debug is easy, -g, but you'll have to tell me how to get it
do produce a core dump.
Cheers