* 2.6.36/2.6.37: broken compatibility with userspace input-utils ? @ 2011-01-23 17:03 Mark Lord 2011-01-24 17:54 ` Dmitry Torokhov 2011-02-02 14:31 ` Chase Douglas 0 siblings, 2 replies; 95+ messages in thread From: Mark Lord @ 2011-01-23 17:03 UTC (permalink / raw) To: Linux Kernel, linux-input 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-23 17:03 2.6.36/2.6.37: broken compatibility with userspace input-utils ? Mark Lord @ 2011-01-24 17:54 ` Dmitry Torokhov 2011-01-25 0:32 ` Mark Lord 2011-02-02 14:31 ` Chase Douglas 1 sibling, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-24 17:54 UTC (permalink / raw) To: Mark Lord; +Cc: Linux Kernel, linux-input 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-24 17:54 ` Dmitry Torokhov @ 2011-01-25 0:32 ` Mark Lord 2011-01-25 0:55 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-25 0:32 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Linux Kernel, linux-input 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: >> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 0:32 ` Mark Lord @ 2011-01-25 0:55 ` Dmitry Torokhov 2011-01-25 4:13 ` Mark Lord 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 0:55 UTC (permalink / raw) To: Mark Lord; +Cc: Linux Kernel, linux-input On Mon, Jan 24, 2011 at 07:32:08PM -0500, Mark Lord wrote: > 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: > >> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 0:55 ` Dmitry Torokhov @ 2011-01-25 4:13 ` Mark Lord 2011-01-25 4:20 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-25 4:13 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Linux Kernel, linux-input 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. ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 4:13 ` Mark Lord @ 2011-01-25 4:20 ` Dmitry Torokhov 2011-01-25 4:37 ` Mark Lord 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 4:20 UTC (permalink / raw) To: Mark Lord; +Cc: Linux Kernel, linux-input On Mon, Jan 24, 2011 at 11:13:05PM -0500, Mark Lord wrote: > 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. > Where are the sources? I can take a look... -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 4:20 ` Dmitry Torokhov @ 2011-01-25 4:37 ` Mark Lord 2011-01-25 4:43 ` Mark Lord 2011-01-25 4:55 ` Dmitry Torokhov 0 siblings, 2 replies; 95+ messages in thread From: Mark Lord @ 2011-01-25 4:37 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Linux Kernel, linux-input On 11-01-24 11:20 PM, Dmitry Torokhov wrote: > On Mon, Jan 24, 2011 at 11:13:05PM -0500, Mark Lord wrote: >> 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. >> > > 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? ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 4:37 ` Mark Lord @ 2011-01-25 4:43 ` Mark Lord 2011-01-25 4:55 ` Dmitry Torokhov 1 sibling, 0 replies; 95+ messages in thread From: Mark Lord @ 2011-01-25 4:43 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Linux Kernel, linux-input On 11-01-24 11:37 PM, Mark Lord wrote: > On 11-01-24 11:20 PM, Dmitry Torokhov wrote: >> On Mon, Jan 24, 2011 at 11:13:05PM -0500, Mark Lord wrote: >>> 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. >>> >> >> 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! ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 4:37 ` Mark Lord 2011-01-25 4:43 ` Mark Lord @ 2011-01-25 4:55 ` Dmitry Torokhov 2011-01-25 5:04 ` Mark Lord 1 sibling, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 4:55 UTC (permalink / raw) To: Mark Lord; +Cc: Linux Kernel, linux-input On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: > On 11-01-24 11:20 PM, Dmitry Torokhov wrote: > > On Mon, Jan 24, 2011 at 11:13:05PM -0500, Mark Lord wrote: > >> 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. > >> > > > > 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 4:55 ` Dmitry Torokhov @ 2011-01-25 5:04 ` Mark Lord 2011-01-25 5:07 ` Mark Lord 2011-01-25 5:29 ` Dmitry Torokhov 0 siblings, 2 replies; 95+ messages in thread From: Mark Lord @ 2011-01-25 5:04 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Linux Kernel, linux-input On 11-01-24 11:55 PM, Dmitry Torokhov wrote: > On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: .. >> 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. ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 5:04 ` Mark Lord @ 2011-01-25 5:07 ` Mark Lord 2011-01-25 5:15 ` Mark Lord 2011-01-25 5:31 ` Dmitry Torokhov 2011-01-25 5:29 ` Dmitry Torokhov 1 sibling, 2 replies; 95+ messages in thread From: Mark Lord @ 2011-01-25 5:07 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Linux Kernel, linux-input On 11-01-25 12:04 AM, Mark Lord wrote: > On 11-01-24 11:55 PM, Dmitry Torokhov wrote: >> On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: > .. >>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 5:07 ` Mark Lord @ 2011-01-25 5:15 ` Mark Lord 2011-01-25 5:31 ` Dmitry Torokhov 1 sibling, 0 replies; 95+ messages in thread From: Mark Lord @ 2011-01-25 5:15 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Linux Kernel, linux-input On 11-01-25 12:07 AM, Mark Lord wrote: > On 11-01-25 12:04 AM, Mark Lord wrote: .. >> 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. :) >> 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. ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 5:07 ` Mark Lord 2011-01-25 5:15 ` Mark Lord @ 2011-01-25 5:31 ` Dmitry Torokhov 2011-01-25 6:52 ` Dmitry Torokhov 2011-01-25 11:42 ` Mauro Carvalho Chehab 1 sibling, 2 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 5:31 UTC (permalink / raw) To: Mark Lord; +Cc: Linux Kernel, linux-input, Mauro Carvalho Chehab, linux-media On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote: > On 11-01-25 12:04 AM, Mark Lord wrote: > > On 11-01-24 11:55 PM, Dmitry Torokhov wrote: > >> On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: > > .. > >>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 5:31 ` Dmitry Torokhov @ 2011-01-25 6:52 ` Dmitry Torokhov 2011-01-25 14:42 ` Extending rc-core/userspace to handle bigger scancodes - Was: " Mauro Carvalho Chehab 2011-01-25 11:42 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 6:52 UTC (permalink / raw) To: Mark Lord; +Cc: Linux Kernel, linux-input, Mauro Carvalho Chehab, linux-media 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: > > On 11-01-25 12:04 AM, Mark Lord wrote: > > > On 11-01-24 11:55 PM, Dmitry Torokhov wrote: > > >> On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: > > > .. > > >>> 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(-) diff --git a/input-kbd.c b/input-kbd.c index e94529d..5d93d54 100644 --- a/input-kbd.c +++ b/input-kbd.c @@ -9,9 +9,27 @@ #include "input.h" +struct input_keymap_entry_v1 { + uint32_t scancode; + uint32_t keycode; +}; + +struct input_keymap_entry_v2 { +#define KEYMAP_BY_INDEX (1 << 0) + uint8_t flags; + uint8_t len; + uint16_t index; + uint32_t keycode; + uint8_t scancode[32]; +}; + +#ifndef EVIOCGKEYCODE2 +#define EVIOCGKEYCODE2 _IOR('E', 0x04, struct input_keymap_entry_v2) +#endif + struct kbd_entry { - int scancode; - int keycode; + unsigned int scancode; + unsigned int keycode; }; struct kbd_map { @@ -23,7 +41,7 @@ struct kbd_map { /* ------------------------------------------------------------------ */ -static struct kbd_map* kbd_map_read(int fd) +static struct kbd_map* kbd_map_read(int fd, unsigned int version) { struct kbd_entry entry; struct kbd_map *map; @@ -32,16 +50,37 @@ static struct kbd_map* kbd_map_read(int fd) 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 (version < 0x10001) { + struct input_keymap_entry_v1 ke = { + .scancode = map->size, + .keycode = KEY_RESERVED, + }; + + rc = ioctl(fd, EVIOCGKEYCODE, &ke); + if (rc < 0) + break; + } else { + struct input_keymap_entry_v2 ke = { + .index = map->size, + .flags = KEYMAP_BY_INDEX, + .len = sizeof(uint32_t), + .keycode = KEY_RESERVED, + }; + + rc = ioctl(fd, EVIOCGKEYCODE2, &ke); + if (rc < 0) + break; + + memcpy(&entry.scancode, ke.scancode, + sizeof(entry.scancode)); + entry.keycode = ke.keycode; } + 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) @@ -155,40 +194,27 @@ static void kbd_print_bits(int fd) } } -static void show_kbd(int nr) +static void show_kbd(int fd, unsigned int protocol_version) { struct kbd_map *map; - int fd; - 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); + else kbd_print_bits(fd); - } - - close(fd); } -static int set_kbd(int nr, char *mapfile) +static int set_kbd(int fd, unsigned int protocol_version, char *mapfile) { struct kbd_map *map; FILE *fp; - int fd; - - 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) { printf("device has no map\n"); - close(fd); return -1; } @@ -198,18 +224,15 @@ static int set_kbd(int nr, char *mapfile) fp = fopen(mapfile,"r"); if (NULL == fp) { printf("open %s: %s\n",mapfile,strerror(errno)); - close(fd); return -1; } } - + if (0 != kbd_map_parse(fp,map) || 0 != kbd_map_write(fd,map)) { - close(fd); return -1; } - close(fd); return 0; } @@ -223,8 +246,10 @@ static int usage(char *prog, int error) int main(int argc, char *argv[]) { - int c,devnr; + int c, devnr, fd; char *mapfile = NULL; + unsigned int protocol_version; + int rc = 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) + goto out; + + if (ioctl(fd, EVIOCGVERSION, &protocol_version) < 0) { + fprintf(stderr, + "Unable to query evdev protocol version: %s\n", + strerror(errno)); + goto out_close; } - return 0; + + if (mapfile) + set_kbd(fd, protocol_version, mapfile); + else + show_kbd(fd, protocol_version); + + rc = EXIT_SUCCESS; + +out_close: + close(fd); +out: + return rc; } /* --------------------------------------------------------------------- -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 95+ messages in thread
* Extending rc-core/userspace to handle bigger scancodes - Was: Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 6:52 ` Dmitry Torokhov @ 2011-01-25 14:42 ` Mauro Carvalho Chehab 2011-01-25 16:55 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-25 14:42 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Mark Lord, Linux Kernel, linux-input, linux-media Em 25-01-2011 04:52, Dmitry Torokhov escreveu: > 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: >>> On 11-01-25 12:04 AM, Mark Lord wrote: >>>> On 11-01-24 11:55 PM, Dmitry Torokhov wrote: >>>>> On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: >>>> .. >>>>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: Extending rc-core/userspace to handle bigger scancodes - Was: Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 14:42 ` Extending rc-core/userspace to handle bigger scancodes - Was: " Mauro Carvalho Chehab @ 2011-01-25 16:55 ` Dmitry Torokhov 2011-01-26 9:44 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 16:55 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Mark Lord, Linux Kernel, linux-input, linux-media On Tue, Jan 25, 2011 at 12:42:57PM -0200, Mauro Carvalho Chehab wrote: > Em 25-01-2011 04:52, Dmitry Torokhov escreveu: > > 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: > >>> On 11-01-25 12:04 AM, Mark Lord wrote: > >>>> On 11-01-24 11:55 PM, Dmitry Torokhov wrote: > >>>>> On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: > >>>> .. > >>>>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: Extending rc-core/userspace to handle bigger scancodes - Was: Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 16:55 ` Dmitry Torokhov @ 2011-01-26 9:44 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-26 9:44 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Mark Lord, Linux Kernel, linux-input, linux-media Em 25-01-2011 14:55, Dmitry Torokhov escreveu: > On Tue, Jan 25, 2011 at 12:42:57PM -0200, Mauro Carvalho Chehab wrote: >> Em 25-01-2011 04:52, Dmitry Torokhov escreveu: >>> 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: >>>>> On 11-01-25 12:04 AM, Mark Lord wrote: >>>>>> On 11-01-24 11:55 PM, Dmitry Torokhov wrote: >>>>>>> On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: >>>>>> .. >>>>>>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 5:31 ` Dmitry Torokhov 2011-01-25 6:52 ` Dmitry Torokhov @ 2011-01-25 11:42 ` Mauro Carvalho Chehab 2011-01-25 14:32 ` Mark Lord 2011-01-25 16:48 ` Dmitry Torokhov 1 sibling, 2 replies; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-25 11:42 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Mark Lord, Linux Kernel, linux-input, linux-media Em 25-01-2011 03:31, Dmitry Torokhov escreveu: > On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote: >> On 11-01-25 12:04 AM, Mark Lord wrote: >>> On 11-01-24 11:55 PM, Dmitry Torokhov wrote: >>>> On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: >>> .. >>>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 11:42 ` Mauro Carvalho Chehab @ 2011-01-25 14:32 ` Mark Lord 2011-01-25 16:48 ` Dmitry Torokhov 1 sibling, 0 replies; 95+ messages in thread From: Mark Lord @ 2011-01-25 14:32 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Dmitry Torokhov, Linux Kernel, linux-input, linux-media, Linus Torvalds, Andrew Morton, rjw 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 11:42 ` Mauro Carvalho Chehab 2011-01-25 14:32 ` Mark Lord @ 2011-01-25 16:48 ` Dmitry Torokhov 2011-01-25 20:09 ` Linus Torvalds 1 sibling, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 16:48 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Mark Lord, Linux Kernel, linux-input, linux-media On Tue, Jan 25, 2011 at 09:42:44AM -0200, Mauro Carvalho Chehab wrote: > Em 25-01-2011 03:31, Dmitry Torokhov escreveu: > > On Tue, Jan 25, 2011 at 12:07:29AM -0500, Mark Lord wrote: > >> On 11-01-25 12:04 AM, Mark Lord wrote: > >>> On 11-01-24 11:55 PM, Dmitry Torokhov wrote: > >>>> On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: > >>> .. > >>>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 16:48 ` Dmitry Torokhov @ 2011-01-25 20:09 ` Linus Torvalds 2011-01-25 20:54 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Linus Torvalds @ 2011-01-25 20:09 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Mark Lord, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 20:09 ` Linus Torvalds @ 2011-01-25 20:54 ` Dmitry Torokhov 2011-01-25 21:01 ` Dmitry Torokhov 2011-01-25 22:00 ` Mauro Carvalho Chehab 0 siblings, 2 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 20:54 UTC (permalink / raw) To: Linus Torvalds Cc: Mauro Carvalho Chehab, Mark Lord, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote: > On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> 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 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 20:54 ` Dmitry Torokhov @ 2011-01-25 21:01 ` Dmitry Torokhov 2011-01-25 21:20 ` Linus Torvalds 2011-01-25 22:00 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 21:01 UTC (permalink / raw) To: Linus Torvalds Cc: Mauro Carvalho Chehab, Mark Lord, Linux Kernel, linux-input, linux-media 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: > > On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov > > <dmitry.torokhov@gmail.com> 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 > > 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. > 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 21:01 ` Dmitry Torokhov @ 2011-01-25 21:20 ` Linus Torvalds 0 siblings, 0 replies; 95+ messages in thread From: Linus Torvalds @ 2011-01-25 21:20 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Mark Lord, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 7:01 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? @ 2011-01-25 21:20 ` Linus Torvalds 0 siblings, 0 replies; 95+ messages in thread From: Linus Torvalds @ 2011-01-25 21:20 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Mark Lord, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 7:01 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 21:20 ` Linus Torvalds (?) @ 2011-01-25 21:50 ` Dmitry Torokhov -1 siblings, 0 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 21:50 UTC (permalink / raw) To: Linus Torvalds Cc: Mauro Carvalho Chehab, Mark Lord, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 07:20:07AM +1000, Linus Torvalds wrote: > On Wed, Jan 26, 2011 at 7:01 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> 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. 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. > > > 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 20:54 ` Dmitry Torokhov 2011-01-25 21:01 ` Dmitry Torokhov @ 2011-01-25 22:00 ` Mauro Carvalho Chehab 2011-01-25 22:22 ` Mark Lord 2011-01-25 22:25 ` Linus Torvalds 1 sibling, 2 replies; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-25 22:00 UTC (permalink / raw) To: Dmitry Torokhov Cc: Linus Torvalds, Mark Lord, Linux Kernel, linux-input, linux-media Em 25-01-2011 18:54, Dmitry Torokhov escreveu: > On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote: >> On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov >> <dmitry.torokhov@gmail.com> 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 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. >> 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: > 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(-) > > diff --git a/input-kbd.c b/input-kbd.c > index e94529d..5d93d54 100644 > --- a/input-kbd.c > +++ b/input-kbd.c <snip> > @@ -23,7 +41,7 @@ struct kbd_map { > > /* ------------------------------------------------------------------ */ > > -static struct kbd_map* kbd_map_read(int fd) > +static struct kbd_map* kbd_map_read(int fd, unsigned int version) > { > struct kbd_entry entry; > struct kbd_map *map; > @@ -32,16 +50,37 @@ static struct kbd_map* kbd_map_read(int fd) > map = malloc(sizeof(*map)); > memset(map,0,sizeof(*map)); > for (map->size = 0; map->size < 65536; map->size++) { 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. > >> >> 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. > >> >> 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. > Regards, Mauro. ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 22:00 ` Mauro Carvalho Chehab @ 2011-01-25 22:22 ` Mark Lord 2011-01-25 23:29 ` Dmitry Torokhov 2011-01-25 22:25 ` Linus Torvalds 1 sibling, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-25 22:22 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Dmitry Torokhov, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote: > Em 25-01-2011 18:54, Dmitry Torokhov escreveu: >> On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote: >>> On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov >>> <dmitry.torokhov@gmail.com> 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 > > 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. 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. ... >>> 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. >> >>> >>> 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. ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 22:22 ` Mark Lord @ 2011-01-25 23:29 ` Dmitry Torokhov 2011-01-26 2:00 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 23:29 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Tue, Jan 25, 2011 at 05:22:09PM -0500, Mark Lord wrote: > On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote: > > Em 25-01-2011 18:54, Dmitry Torokhov escreveu: > >> On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote: > >>> On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov > >>> <dmitry.torokhov@gmail.com> 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 > > > > 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. > > 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. > > ... > > > >>> 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. > >> > >>> > >>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 23:29 ` Dmitry Torokhov @ 2011-01-26 2:00 ` Dmitry Torokhov 2011-01-26 11:26 ` Mauro Carvalho Chehab 2011-01-26 15:05 ` Mark Lord 0 siblings, 2 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 2:00 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media 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: > > On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote: > > > Em 25-01-2011 18:54, Dmitry Torokhov escreveu: > > >> On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote: > > >>> On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov > > >>> <dmitry.torokhov@gmail.com> 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 > > > > > > 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. > > > > 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. > > > > ... > > > > > > >>> 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. > > >> > > >>> > > >>> 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 <dtor@mail.ru> --- drivers/media/IR/ir-keytable.c | 28 +++++++++++++++++----------- 1 files changed, 17 insertions(+), 11 deletions(-) diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c index f60107c..c4645d7 100644 --- a/drivers/media/IR/ir-keytable.c +++ b/drivers/media/IR/ir-keytable.c @@ -374,21 +374,27 @@ static int ir_getkeycode(struct input_dev *dev, index = ir_lookup_by_scancode(rc_tab, scancode); } - if (index >= rc_tab->len) { - if (!(ke->flags & INPUT_KEYMAP_BY_INDEX)) - IR_dprintk(1, "unknown key for scancode 0x%04x\n", - scancode); + if (index < rc_tab->len) { + entry = &rc_tab->scan[index]; + + ke->index = index; + ke->keycode = entry->keycode; + ke->len = sizeof(entry->scancode); + memcpy(ke->scancode, &entry->scancode, sizeof(entry->scancode)); + + } else if (!(ke->flags & INPUT_KEYMAP_BY_INDEX)) { + /* + * We do not really know the valid range of scancodes + * so let's respond with KEY_RESERVED to anything we + * do not have mapping for [yet]. + */ + ke->index = index; + ke->keycode = KEY_RESERVED; + } else { retval = -EINVAL; goto out; } - entry = &rc_tab->scan[index]; - - ke->index = index; - ke->keycode = entry->keycode; - ke->len = sizeof(entry->scancode); - memcpy(ke->scancode, &entry->scancode, sizeof(entry->scancode)); - retval = 0; out: ^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 2:00 ` Dmitry Torokhov @ 2011-01-26 11:26 ` Mauro Carvalho Chehab 2011-01-26 13:08 ` Gerd Hoffmann 2011-01-26 14:58 ` Mark Lord 2011-01-26 15:05 ` Mark Lord 1 sibling, 2 replies; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-26 11:26 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media, Gerd Hoffmann Hi Dmitry, Em 26-01-2011 00:00, Dmitry Torokhov escreveu: > 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: >>> On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote: >>>> Em 25-01-2011 18:54, Dmitry Torokhov escreveu: >>>>> On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote: >>>>>> On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov >>>>>> <dmitry.torokhov@gmail.com> 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 >>>> >>>> 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. >>> >>> 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. >>> >>> ... >>> >>> >>>>>> 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. >>>>> >>>>>> >>>>>> 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 <dtor@mail.ru> 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 <mchehab@redhat.com> - 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 <kraxel@bytesex.org> [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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 11:26 ` Mauro Carvalho Chehab @ 2011-01-26 13:08 ` Gerd Hoffmann 2011-01-26 14:18 ` Mauro Carvalho Chehab 2011-01-26 14:58 ` Mark Lord 1 sibling, 1 reply; 95+ messages in thread From: Gerd Hoffmann @ 2011-01-26 13:08 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Dmitry Torokhov, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Hi, > 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<kraxel@bytesex.org> [SUSE Labs] > > This is an old testing tool written by Gerd Hoffmann probably used for him > to test the V4L early Remote Controller implementations. Indeed. > The last "official" version seems to be this one: > http://dl.bytesex.org/cvs-snapshots/input-20081014-101501.tar.gz Just moved the bits to git a few days ago. http://bigendian.kraxel.org/cgit/input/ Code is unchanged since 2008 though. > 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 Hmm, doesn't apply cleanly ... cheers, Gerd ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 13:08 ` Gerd Hoffmann @ 2011-01-26 14:18 ` Mauro Carvalho Chehab 2011-01-26 14:52 ` Gerd Hoffmann ` (2 more replies) 0 siblings, 3 replies; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-26 14:18 UTC (permalink / raw) To: Gerd Hoffmann Cc: Dmitry Torokhov, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Em 26-01-2011 11:08, Gerd Hoffmann escreveu: > Hi, > >> 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<kraxel@bytesex.org> [SUSE Labs] >> >> This is an old testing tool written by Gerd Hoffmann probably used for him >> to test the V4L early Remote Controller implementations. > > Indeed. > >> The last "official" version seems to be this one: >> http://dl.bytesex.org/cvs-snapshots/input-20081014-101501.tar.gz > > Just moved the bits to git a few days ago. > http://bigendian.kraxel.org/cgit/input/ > > Code is unchanged since 2008 though. > >> 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 > > Hmm, doesn't apply cleanly ... 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 <mchehab@redhat.com> diff --git a/input-kbd.c b/input-kbd.c index c432d0d..aaf23b9 100644 --- a/input-kbd.c +++ b/input-kbd.c @@ -9,9 +9,22 @@ #include "input.h" +struct input_keymap_entry_v2 { +#define KEYMAP_BY_INDEX (1 << 0) + uint8_t flags; + uint8_t len; + uint16_t index; + uint32_t keycode; + uint8_t scancode[32]; +}; + +#ifndef EVIOCGKEYCODE_V2 +#define EVIOCGKEYCODE_V2 _IOR('E', 0x04, struct input_keymap_entry_v2) +#endif + struct kbd_entry { - int scancode; - int keycode; + unsigned int scancode; + unsigned int keycode; }; struct kbd_map { @@ -23,7 +36,7 @@ struct kbd_map { /* ------------------------------------------------------------------ */ -static struct kbd_map* kbd_map_read(int fd) +static struct kbd_map* kbd_map_read(int fd, unsigned int version) { struct kbd_entry entry; struct kbd_map *map; @@ -32,17 +45,35 @@ static struct kbd_map* kbd_map_read(int fd) 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) { - map->size--; - break; + if (version < 0x10001) { + entry.scancode = map->size; + entry.keycode = KEY_RESERVED; + rc = ioctl(fd, EVIOCGKEYCODE, &entry); + if (rc < 0) { + map->size--; + break; + } + } else { + struct input_keymap_entry_v2 ke = { + .index = map->size, + .flags = KEYMAP_BY_INDEX, + .len = sizeof(uint32_t), + .keycode = KEY_RESERVED, + }; + + rc = ioctl(fd, EVIOCGKEYCODE_V2, &ke); + if (rc < 0) + break; + memcpy(&entry.scancode, ke.scancode, + sizeof(entry.scancode)); + entry.keycode = ke.keycode; } + 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) @@ -156,37 +187,25 @@ static void kbd_print_bits(int fd) } } -static void show_kbd(int nr) +static void show_kbd(int fd, unsigned int protocol_version) { struct kbd_map *map; - int fd; - 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); + else kbd_print_bits(fd); - } - - close(fd); } -static int set_kbd(int nr, char *mapfile) +static int set_kbd(int fd, unsigned int protocol_version, char *mapfile) { struct kbd_map *map; FILE *fp; - int fd; - 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); return 0; } @@ -224,8 +241,10 @@ static int usage(char *prog, int error) int main(int argc, char *argv[]) { - int c,devnr; + int c, devnr, fd; char *mapfile = NULL; + unsigned int protocol_version; + int rc = 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) + goto out; + + if (ioctl(fd, EVIOCGVERSION, &protocol_version) < 0) { + fprintf(stderr, + "Unable to query evdev protocol version: %s\n", + strerror(errno)); + goto out_close; } - return 0; + + if (mapfile) + set_kbd(fd, protocol_version, mapfile); + else + show_kbd(fd, protocol_version); + + rc = EXIT_SUCCESS; + +out_close: + close(fd); +out: + return rc; } /* --------------------------------------------------------------------- diff --git a/input.c b/input.c index d57a31e..a9bd5e8 100644 --- a/input.c +++ b/input.c @@ -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); close(fd); return -1; ^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 14:18 ` Mauro Carvalho Chehab @ 2011-01-26 14:52 ` Gerd Hoffmann 2011-01-26 16:46 ` Dmitry Torokhov 2011-01-26 16:51 ` Dmitry Torokhov 2 siblings, 0 replies; 95+ messages in thread From: Gerd Hoffmann @ 2011-01-26 14:52 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Dmitry Torokhov, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Hi, >> Hmm, doesn't apply cleanly ... > > 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. Applied, thanks. cheers, Gerd ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 14:18 ` Mauro Carvalho Chehab 2011-01-26 14:52 ` Gerd Hoffmann @ 2011-01-26 16:46 ` Dmitry Torokhov 2011-01-26 16:51 ` Dmitry Torokhov 2 siblings, 0 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 16:46 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Gerd Hoffmann, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 12:18:29PM -0200, Mauro Carvalho Chehab wrote: > Em 26-01-2011 11:08, Gerd Hoffmann escreveu: > > Hi, > > > >> 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<kraxel@bytesex.org> [SUSE Labs] > >> > >> This is an old testing tool written by Gerd Hoffmann probably used for him > >> to test the V4L early Remote Controller implementations. > > > > Indeed. > > > >> The last "official" version seems to be this one: > >> http://dl.bytesex.org/cvs-snapshots/input-20081014-101501.tar.gz > > > > Just moved the bits to git a few days ago. > > http://bigendian.kraxel.org/cgit/input/ > > > > Code is unchanged since 2008 though. > > > >> 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 > > > > Hmm, doesn't apply cleanly ... > > 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. Actually it was from Ubuntu, so it is based on 2008 checkout, but they also have more patches, take a peek here: https://launchpad.net/ubuntu/+archive/primary/+files/input-utils_0.0.20081014-1.debian.tar.gz Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 14:18 ` Mauro Carvalho Chehab 2011-01-26 14:52 ` Gerd Hoffmann 2011-01-26 16:46 ` Dmitry Torokhov @ 2011-01-26 16:51 ` Dmitry Torokhov 2011-01-26 17:29 ` Mauro Carvalho Chehab 2 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 16:51 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Gerd Hoffmann, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 12:18:29PM -0200, Mauro Carvalho Chehab wrote: > diff --git a/input.c b/input.c > index d57a31e..a9bd5e8 100644 > --- a/input.c > +++ b/input.c > @@ -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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 16:51 ` Dmitry Torokhov @ 2011-01-26 17:29 ` Mauro Carvalho Chehab 2011-01-26 18:24 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-26 17:29 UTC (permalink / raw) To: Dmitry Torokhov Cc: Gerd Hoffmann, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Em 26-01-2011 14:51, Dmitry Torokhov escreveu: > On Wed, Jan 26, 2011 at 12:18:29PM -0200, Mauro Carvalho Chehab wrote: >> diff --git a/input.c b/input.c >> index d57a31e..a9bd5e8 100644 >> --- a/input.c >> +++ b/input.c >> @@ -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 diff --git a/input.c b/input.c index d57a31e..a9bd5e8 100644 --- a/input.c +++ b/input.c @@ -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) + if (EV_VERSION > version) { + fprintf(stderr, "protocol version mismatch (expected >= %d, got EV_VERSION, version); close(fd); return -1; And, with your KEY_RESERVED patch applied to 2.6.37, the tool works fine, with 16-bits keytables: $ 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: 28 keys, size: 65536/65536 0x0021 = 363 # KEY_CHANNEL 0x0031 = 52 # KEY_DOT 0x0035 = 359 # KEY_TIME 0x0037 = 167 # KEY_RECORD 0x0038 = 212 # KEY_CAMERA 0x0039 = 154 # KEY_CYCLEWINDOWS 0x003a = 181 # KEY_NEW 0x0060 = 403 # KEY_CHANNELDOWN 0x0061 = 405 # KEY_LAST 0x0062 = 11 # KEY_0 0x0063 = 28 # KEY_ENTER 0x0064 = 113 # KEY_MIN_INTERESTING 0x0066 = 358 # KEY_INFO 0x0070 = 356 # KEY_POWER2 0x0072 = 393 # KEY_VIDEO 0x0073 = 372 # KEY_ZOOM 0x0074 = 115 # KEY_VOLUMEUP 0x0075 = 2 # KEY_1 0x0076 = 3 # KEY_2 0x0077 = 4 # KEY_3 0x0078 = 114 # KEY_VOLUMEDOWN 0x0079 = 5 # KEY_4 0x007a = 6 # KEY_5 0x007b = 7 # KEY_6 0x007c = 402 # KEY_CHANNELUP 0x007d = 8 # KEY_7 0x007e = 9 # KEY_8 0x007f = 10 # KEY_9 (this is the original RC-5 table for the IR that comes with this device) It will fail, however, it the keytable has 24 bits keycode: $ sudo ir-keytable -cw /etc/rc_keymaps/pixelview_mk12 (loads a 24 bits NEC-extended keycode, found on Pixelview MK12 remote control) $ 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 bits: KEY_1 bits: KEY_2 bits: KEY_3 bits: KEY_4 bits: KEY_5 bits: KEY_6 bits: KEY_7 bits: KEY_8 bits: KEY_9 bits: KEY_0 bits: KEY_MIN_INTERESTING bits: KEY_VOLUMEDOWN bits: KEY_VOLUMEUP bits: KEY_PAUSE bits: KEY_STOP bits: KEY_AGAIN bits: KEY_FORWARD bits: KEY_RECORD bits: KEY_REWIND bits: KEY_PLAY bits: KEY_CAMERA bits: KEY_SEARCH bits: KEY_POWER2 bits: KEY_ZOOM bits: KEY_TV bits: KEY_RADIO bits: KEY_TUNER bits: KEY_VIDEO bits: KEY_CHANNELUP bits: KEY_CHANNELDOWN bits: KEY_DIGITS Instead of showing the scancode/keycode table, it shows the mapped keys as if they were some event bits. The current kraxel tree at http://bigendian.kraxel.org/cgit/input/ with your userspace input patch, plus my backports for upstream work fine also with the 24-bits keycode table: $ git reset --hard && make [mchehab@nehalem input]$ git reset --hard && make HEAD is now at 52f533a input-kbd - switch to using EVIOCGKEYCODE2 when available CC input-kbd.o LD input-kbd $ 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 0x866b04 = 168 # KEY_REWIND 0x866b05 = 5 # KEY_4 0x866b06 = 8 # KEY_7 0x866b07 = 385 # KEY_RADIO 0x866b08 = 207 # KEY_PLAY 0x866b09 = 6 # KEY_5 0x866b0a = 9 # KEY_8 0x866b0b = 3 # KEY_2 0x866b0c = 159 # KEY_FORWARD 0x866b0d = 377 # KEY_TV 0x866b0e = 167 # KEY_RECORD 0x866b0f = 119 # KEY_PAUSE 0x866b10 = 413 # KEY_DIGITS 0x866b12 = 10 # KEY_9 0x866b13 = 129 # KEY_AGAIN 0x866b14 = 403 # KEY_CHANNELDOWN 0x866b15 = 7 # KEY_6 0x866b16 = 402 # KEY_CHANNELUP 0x866b17 = 114 # KEY_VOLUMEDOWN 0x866b18 = 113 # KEY_MIN_INTERESTING 0x866b19 = 212 # KEY_CAMERA 0x866b1a = 217 # KEY_SEARCH 0x866b1b = 4 # KEY_3 0x866b1c = 372 # KEY_ZOOM 0x866b1d = 128 # KEY_STOP 0x866b1e = 356 # KEY_POWER2 0x866b1f = 115 # KEY_VOLUMEUP Cheers, Mauro ^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 17:29 ` Mauro Carvalho Chehab @ 2011-01-26 18:24 ` Dmitry Torokhov 2011-01-26 19:16 ` Gerd Hoffmann 2011-01-26 19:27 ` Mark Lord 0 siblings, 2 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 18:24 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Gerd Hoffmann, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 03:29:09PM -0200, Mauro Carvalho Chehab wrote: > Em 26-01-2011 14:51, Dmitry Torokhov escreveu: > > On Wed, Jan 26, 2011 at 12:18:29PM -0200, Mauro Carvalho Chehab wrote: > >> diff --git a/input.c b/input.c > >> index d57a31e..a9bd5e8 100644 > >> --- a/input.c > >> +++ b/input.c > >> @@ -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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 18:24 ` Dmitry Torokhov @ 2011-01-26 19:16 ` Gerd Hoffmann 2011-01-26 19:28 ` Mark Lord ` (2 more replies) 2011-01-26 19:27 ` Mark Lord 1 sibling, 3 replies; 95+ messages in thread From: Gerd Hoffmann @ 2011-01-26 19:16 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Hi, >>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 19:16 ` Gerd Hoffmann @ 2011-01-26 19:28 ` Mark Lord 2011-01-26 20:09 ` Gerd Hoffmann 2011-01-26 19:28 ` Mauro Carvalho Chehab 2011-01-26 19:32 ` Dmitry Torokhov 2 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-26 19:28 UTC (permalink / raw) To: Gerd Hoffmann Cc: Dmitry Torokhov, Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-26 02:16 PM, Gerd Hoffmann wrote: > Hi, > >>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 19:28 ` Mark Lord @ 2011-01-26 20:09 ` Gerd Hoffmann 0 siblings, 0 replies; 95+ messages in thread From: Gerd Hoffmann @ 2011-01-26 20:09 UTC (permalink / raw) To: Mark Lord Cc: Dmitry Torokhov, Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media Hi, >> 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! Check removed. thanks, Gerd ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 19:16 ` Gerd Hoffmann 2011-01-26 19:28 ` Mark Lord @ 2011-01-26 19:28 ` Mauro Carvalho Chehab 2011-01-26 19:32 ` Dmitry Torokhov 2 siblings, 0 replies; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-26 19:28 UTC (permalink / raw) To: Gerd Hoffmann Cc: Dmitry Torokhov, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Em 26-01-2011 17:16, Gerd Hoffmann escreveu: > Hi, > >>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 19:16 ` Gerd Hoffmann 2011-01-26 19:28 ` Mark Lord 2011-01-26 19:28 ` Mauro Carvalho Chehab @ 2011-01-26 19:32 ` Dmitry Torokhov 2011-01-26 20:07 ` Gerd Hoffmann 2 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 19:32 UTC (permalink / raw) To: Gerd Hoffmann Cc: Mauro Carvalho Chehab, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 08:16:09PM +0100, Gerd Hoffmann wrote: > Hi, > > >>>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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 19:32 ` Dmitry Torokhov @ 2011-01-26 20:07 ` Gerd Hoffmann 0 siblings, 0 replies; 95+ messages in thread From: Gerd Hoffmann @ 2011-01-26 20:07 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Hi, > 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. Guess that is the best way indeed. cheers, Gerd. ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 18:24 ` Dmitry Torokhov 2011-01-26 19:16 ` Gerd Hoffmann @ 2011-01-26 19:27 ` Mark Lord 1 sibling, 0 replies; 95+ messages in thread From: Mark Lord @ 2011-01-26 19:27 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Gerd Hoffmann, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-26 01:24 PM, Dmitry Torokhov wrote: > On Wed, Jan 26, 2011 at 03:29:09PM -0200, Mauro Carvalho Chehab wrote: >> Em 26-01-2011 14:51, Dmitry Torokhov escreveu: >>> On Wed, Jan 26, 2011 at 12:18:29PM -0200, Mauro Carvalho Chehab wrote: >>>> diff --git a/input.c b/input.c >>>> index d57a31e..a9bd5e8 100644 >>>> --- a/input.c >>>> +++ b/input.c >>>> @@ -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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 11:26 ` Mauro Carvalho Chehab 2011-01-26 13:08 ` Gerd Hoffmann @ 2011-01-26 14:58 ` Mark Lord 2011-01-26 17:41 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-26 14:58 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Dmitry Torokhov, Linux Kernel, linux-input, linux-media, Gerd Hoffmann 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. ??? ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 14:58 ` Mark Lord @ 2011-01-26 17:41 ` Mauro Carvalho Chehab 2011-01-26 17:59 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-26 17:41 UTC (permalink / raw) To: Mark Lord Cc: Dmitry Torokhov, Linux Kernel, linux-input, linux-media, Gerd Hoffmann Em 26-01-2011 12:58, Mark Lord escreveu: > 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. 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 17:41 ` Mauro Carvalho Chehab @ 2011-01-26 17:59 ` Dmitry Torokhov 2011-01-26 19:30 ` Mark Lord 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 17:59 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Mark Lord, Linux Kernel, linux-input, linux-media, Gerd Hoffmann On Wed, Jan 26, 2011 at 03:41:01PM -0200, Mauro Carvalho Chehab wrote: > Em 26-01-2011 12:58, Mark Lord escreveu: > > 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. > > 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 17:59 ` Dmitry Torokhov @ 2011-01-26 19:30 ` Mark Lord 0 siblings, 0 replies; 95+ messages in thread From: Mark Lord @ 2011-01-26 19:30 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linux Kernel, linux-input, linux-media, Gerd Hoffmann On 11-01-26 12:59 PM, Dmitry Torokhov wrote: > On Wed, Jan 26, 2011 at 03:41:01PM -0200, Mauro Carvalho Chehab wrote: >> Em 26-01-2011 12:58, Mark Lord escreveu: >>> 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. >> >> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 2:00 ` Dmitry Torokhov 2011-01-26 11:26 ` Mauro Carvalho Chehab @ 2011-01-26 15:05 ` Mark Lord 2011-01-26 16:44 ` Dmitry Torokhov ` (2 more replies) 1 sibling, 3 replies; 95+ messages in thread From: Mark Lord @ 2011-01-26 15:05 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-25 09:00 PM, Dmitry Torokhov wrote: > 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: >>> On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote: >>>> Em 25-01-2011 18:54, Dmitry Torokhov escreveu: .. >>>>> 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) >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 <dtor@mail.ru> >--- > > drivers/media/IR/ir-keytable.c | 28 +++++++++++++++++----------- > 1 files changed, 17 insertions(+), 11 deletions(-) > > >diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c >index f60107c..c4645d7 100644 >--- a/drivers/media/IR/ir-keytable.c >+++ b/drivers/media/IR/ir-keytable.c >@@ -374,21 +374,27 @@ static int ir_getkeycode(struct input_dev *dev, > index = ir_lookup_by_scancode(rc_tab, scancode); > } > >- if (index >= rc_tab->len) { >- if (!(ke->flags & INPUT_KEYMAP_BY_INDEX)) >- IR_dprintk(1, "unknown key for scancode 0x%04x\n", >- scancode); >+ if (index < rc_tab->len) { >+ entry = &rc_tab->scan[index]; >+ >+ ke->index = index; >+ ke->keycode = entry->keycode; >+ ke->len = sizeof(entry->scancode); >+ memcpy(ke->scancode, &entry->scancode, sizeof(entry->scancode)); >+ >+ } else if (!(ke->flags & INPUT_KEYMAP_BY_INDEX)) { >+ /* >+ * We do not really know the valid range of scancodes >+ * so let's respond with KEY_RESERVED to anything we >+ * do not have mapping for [yet]. >+ */ >+ ke->index = index; >+ ke->keycode = KEY_RESERVED; >+ } else { > retval = -EINVAL; > goto out; > } > >- entry = &rc_tab->scan[index]; >- >- ke->index = index; >- ke->keycode = entry->keycode; >- ke->len = sizeof(entry->scancode); >- memcpy(ke->scancode, &entry->scancode, sizeof(entry->scancode)); >- > retval = 0; > > out: ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 15:05 ` Mark Lord @ 2011-01-26 16:44 ` Dmitry Torokhov 2011-01-26 19:31 ` Mark Lord 2011-01-26 17:32 ` Mauro Carvalho Chehab 2011-01-27 1:01 ` Mark Lord 2 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 16:44 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 10:05:57AM -0500, Mark Lord wrote: > On 11-01-25 09:00 PM, Dmitry Torokhov wrote: > > 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: > >>> On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote: > >>>> Em 25-01-2011 18:54, Dmitry Torokhov escreveu: > .. > >>>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 16:44 ` Dmitry Torokhov @ 2011-01-26 19:31 ` Mark Lord 2011-01-26 19:38 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-26 19:31 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-26 11:44 AM, Dmitry Torokhov wrote: > On Wed, Jan 26, 2011 at 10:05:57AM -0500, Mark Lord wrote: .. >> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 19:31 ` Mark Lord @ 2011-01-26 19:38 ` Dmitry Torokhov 0 siblings, 0 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 19:38 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 02:31:44PM -0500, Mark Lord wrote: > On 11-01-26 11:44 AM, Dmitry Torokhov wrote: > > On Wed, Jan 26, 2011 at 10:05:57AM -0500, Mark Lord wrote: > .. > >> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 15:05 ` Mark Lord 2011-01-26 16:44 ` Dmitry Torokhov @ 2011-01-26 17:32 ` Mauro Carvalho Chehab 2011-01-26 19:33 ` Mark Lord 2011-01-27 1:01 ` Mark Lord 2 siblings, 1 reply; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-26 17:32 UTC (permalink / raw) To: Mark Lord Cc: Dmitry Torokhov, Linus Torvalds, Linux Kernel, linux-input, linux-media Em 26-01-2011 13:05, Mark Lord escreveu: > On 11-01-25 09:00 PM, Dmitry Torokhov wrote: >> 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: >>>> On 11-01-25 05:00 PM, Mauro Carvalho Chehab wrote: >>>>> Em 25-01-2011 18:54, Dmitry Torokhov escreveu: > .. >>>>>> 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. ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 17:32 ` Mauro Carvalho Chehab @ 2011-01-26 19:33 ` Mark Lord 2011-01-26 19:41 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-26 19:33 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Dmitry Torokhov, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-26 12:32 PM, Mauro Carvalho Chehab wrote: > Em 26-01-2011 13:05, Mark Lord escreveu: .. >> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 19:33 ` Mark Lord @ 2011-01-26 19:41 ` Dmitry Torokhov 2011-01-26 19:47 ` Mark Lord 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 19:41 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 02:33:17PM -0500, Mark Lord wrote: > On 11-01-26 12:32 PM, Mauro Carvalho Chehab wrote: > > Em 26-01-2011 13:05, Mark Lord escreveu: > .. > >> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 19:41 ` Dmitry Torokhov @ 2011-01-26 19:47 ` Mark Lord 2011-01-26 19:50 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-26 19:47 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 19:47 ` Mark Lord @ 2011-01-26 19:50 ` Dmitry Torokhov 2011-01-26 21:41 ` Mark Lord 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 19:50 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 02:47:18PM -0500, Mark Lord wrote: > 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. > You are more than welcome spend more time on reviews. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 19:50 ` Dmitry Torokhov @ 2011-01-26 21:41 ` Mark Lord 2011-01-26 21:49 ` Mark Lord 2011-01-26 22:04 ` Dmitry Torokhov 0 siblings, 2 replies; 95+ messages in thread From: Mark Lord @ 2011-01-26 21:41 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-26 02:50 PM, Dmitry Torokhov wrote: > On Wed, Jan 26, 2011 at 02:47:18PM -0500, Mark Lord wrote: >> 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. >> > > 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 21:41 ` Mark Lord @ 2011-01-26 21:49 ` Mark Lord 2011-01-26 22:07 ` Dmitry Torokhov 2011-01-26 22:04 ` Dmitry Torokhov 1 sibling, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-26 21:49 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 21:49 ` Mark Lord @ 2011-01-26 22:07 ` Dmitry Torokhov 0 siblings, 0 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 22:07 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 21:41 ` Mark Lord 2011-01-26 21:49 ` Mark Lord @ 2011-01-26 22:04 ` Dmitry Torokhov 1 sibling, 0 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-26 22:04 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 04:41:07PM -0500, Mark Lord wrote: > On 11-01-26 02:50 PM, Dmitry Torokhov wrote: > > On Wed, Jan 26, 2011 at 02:47:18PM -0500, Mark Lord wrote: > >> 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. > >> > > > > 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-26 15:05 ` Mark Lord 2011-01-26 16:44 ` Dmitry Torokhov 2011-01-26 17:32 ` Mauro Carvalho Chehab @ 2011-01-27 1:01 ` Mark Lord 2011-01-27 1:07 ` Mark Lord 2 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-27 1:01 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-26 10:05 AM, Mark Lord wrote: > On 11-01-25 09:00 PM, Dmitry Torokhov wrote: .. >> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 1:01 ` Mark Lord @ 2011-01-27 1:07 ` Mark Lord 2011-01-27 2:12 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-27 1:07 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-26 08:01 PM, Mark Lord wrote: > On 11-01-26 10:05 AM, Mark Lord wrote: >> On 11-01-25 09:00 PM, Dmitry Torokhov wrote: > .. >>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 1:07 ` Mark Lord @ 2011-01-27 2:12 ` Dmitry Torokhov 2011-01-27 3:18 ` Mark Lord 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-27 2:12 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote: > On 11-01-26 08:01 PM, Mark Lord wrote: > > On 11-01-26 10:05 AM, Mark Lord wrote: > >> On 11-01-25 09:00 PM, Dmitry Torokhov wrote: > > .. > >>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 2:12 ` Dmitry Torokhov @ 2011-01-27 3:18 ` Mark Lord 2011-01-27 6:38 ` Dmitry Torokhov 2011-01-27 16:39 ` Dmitry Torokhov 0 siblings, 2 replies; 95+ messages in thread From: Mark Lord @ 2011-01-27 3:18 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-26 09:12 PM, Dmitry Torokhov wrote: > On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote: >> On 11-01-26 08:01 PM, Mark Lord wrote: >>> On 11-01-26 10:05 AM, Mark Lord wrote: >>>> On 11-01-25 09:00 PM, Dmitry Torokhov wrote: >>> .. >>>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 3:18 ` Mark Lord @ 2011-01-27 6:38 ` Dmitry Torokhov 2011-01-27 10:30 ` Mauro Carvalho Chehab 2011-01-27 14:54 ` Mark Lord 2011-01-27 16:39 ` Dmitry Torokhov 1 sibling, 2 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-27 6:38 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote: > On 11-01-26 09:12 PM, Dmitry Torokhov wrote: > > On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote: > >> On 11-01-26 08:01 PM, Mark Lord wrote: > >>> On 11-01-26 10:05 AM, Mark Lord wrote: > >>>> On 11-01-25 09:00 PM, Dmitry Torokhov wrote: > >>> .. > >>>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 6:38 ` Dmitry Torokhov @ 2011-01-27 10:30 ` Mauro Carvalho Chehab 2011-01-27 15:00 ` Mark Lord 2011-01-27 17:21 ` Dmitry Torokhov 2011-01-27 14:54 ` Mark Lord 1 sibling, 2 replies; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-27 10:30 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Em 27-01-2011 04:38, Dmitry Torokhov escreveu: > On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote: >> On 11-01-26 09:12 PM, Dmitry Torokhov wrote: >>> On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote: >>>> On 11-01-26 08:01 PM, Mark Lord wrote: >>>>> On 11-01-26 10:05 AM, Mark Lord wrote: >>>>>> On 11-01-25 09:00 PM, Dmitry Torokhov wrote: >>>>> .. >>>>>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 10:30 ` Mauro Carvalho Chehab @ 2011-01-27 15:00 ` Mark Lord 2011-01-27 17:21 ` Dmitry Torokhov 1 sibling, 0 replies; 95+ messages in thread From: Mark Lord @ 2011-01-27 15:00 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Dmitry Torokhov, Linus Torvalds, Linux Kernel, linux-input, linux-media 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 10:30 ` Mauro Carvalho Chehab 2011-01-27 15:00 ` Mark Lord @ 2011-01-27 17:21 ` Dmitry Torokhov 2011-01-27 18:58 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-27 17:21 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 17:21 ` Dmitry Torokhov @ 2011-01-27 18:58 ` Mauro Carvalho Chehab 2011-01-28 9:39 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-27 18:58 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Em 27-01-2011 15:21, Dmitry Torokhov escreveu: > 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. Race conditions are hard to track... probably the new code added some delay, and this allowed the request_module() to finish his job. > BTW, that means that we need to redo udev rules. If there's a race condition, then the proper fix is to lock the driver until it is ready to receive a fops. Maybe we'll need a mutex to preventing opening the device until it is completely initialized. It is hard to tell, as Mark didn't provide us yet the dmesg info (at least on the emails I was c/c), so I don't even know what device he has, and what drivers are used. > 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. It could be done, but then we'll need to pollute the existing input tools with RC-specific stuff. For IR, there are some additional steps, like the need to select the IR protocol, otherwise the keytable is useless. Also, the keytable and persistent info is provided via /sys/class/rc/rc?/uevent. So, the tool need to first read the RC class, check what keytable should be associated with that device (based on a custom file), and load the proper table. Also, I'm currently working on a way to map media keys for remote controllers into X11 (basically, mapping them into the keyspace between 8-255, passing through Xorg evdev.c, and then mapping back into some X11 symbols). This way, we don't need to violate the X11 protocol. (Yeah, I know this is hacky, but while X11 cannot pass the full evdev keycode, at least the Remote Controllers will work). This probably means that we may need to add some DBus logic inside ir-keytable, when called via udev, to allow it to announce to X11. Regards, Mauro ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 18:58 ` Mauro Carvalho Chehab @ 2011-01-28 9:39 ` Dmitry Torokhov 2011-01-28 11:55 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-28 9:39 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media On Thu, Jan 27, 2011 at 04:58:57PM -0200, Mauro Carvalho Chehab wrote: > Em 27-01-2011 15:21, Dmitry Torokhov escreveu: > > 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. > > Race conditions are hard to track... probably the new code added some delay, > and this allowed the request_module() to finish his job. > > > BTW, that means that we need to redo udev rules. > > If there's a race condition, then the proper fix is to lock the driver > until it is ready to receive a fops. Maybe we'll need a mutex to preventing > opening the device until it is completely initialized. No, not at all. The devices are ready to handle everything when they are created, it's just some devices are not there yet. What you do with current udev rule is similar to trying to mount filesystem as soon as you discover a PCI SCSI card. The controller is there but disks have not been discovered, block devices have not been created, and so on. > > It is hard to tell, as Mark didn't provide us yet the dmesg info (at least > on the emails I was c/c), so I don't even know what device he has, and what > drivers are used. I belie you have been copied on the mail that had the following snippet: > 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] > > > 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. > > It could be done, but then we'll need to pollute the existing input tools > with RC-specific stuff. For IR, there are some additional steps, like > the need to select the IR protocol, otherwise the keytable is useless. That should be done by the separate utility that fires up when udev gets event for /sys/class/rc/rcX device. > Also, the keytable and persistent info is provided via /sys/class/rc/rc?/uevent. > So, the tool need to first read the RC class, check what keytable should be > associated with that device (based on a custom file), and load the proper > table. And this could be easily added to the udev's keymap utility that is fired up when we discover evdevX devices. > > Also, I'm currently working on a way to map media keys for remote controllers > into X11 (basically, mapping them into the keyspace between 8-255, passing > through Xorg evdev.c, and then mapping back into some X11 symbols). This way, > we don't need to violate the X11 protocol. (Yeah, I know this is hacky, but > while X11 cannot pass the full evdev keycode, at least the Remote Controllers > will work). This probably means that we may need to add some DBus logic > inside ir-keytable, when called via udev, to allow it to announce to X11. The same issue is present with other types of input devices (multimedia keyboards emitting codes that X can't consume) and so it again would make sense to enhance udev's utility instead of confining it all to ir-keytable. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-28 9:39 ` Dmitry Torokhov @ 2011-01-28 11:55 ` Mauro Carvalho Chehab 2011-01-28 16:40 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-28 11:55 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Em 28-01-2011 07:39, Dmitry Torokhov escreveu: > On Thu, Jan 27, 2011 at 04:58:57PM -0200, Mauro Carvalho Chehab wrote: >> Em 27-01-2011 15:21, Dmitry Torokhov escreveu: >>> 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. >> >> Race conditions are hard to track... probably the new code added some delay, >> and this allowed the request_module() to finish his job. >> >>> BTW, that means that we need to redo udev rules. >> >> If there's a race condition, then the proper fix is to lock the driver >> until it is ready to receive a fops. Maybe we'll need a mutex to preventing >> opening the device until it is completely initialized. > > No, not at all. The devices are ready to handle everything when they are > created, it's just some devices are not there yet. What you do with > current udev rule is similar to trying to mount filesystem as soon as > you discover a PCI SCSI card. The controller is there but disks have not > been discovered, block devices have not been created, and so on. The rc-core register (and the corresponding input register) is done when the device detected a remote controller, so, it should be safe to register on that point. If not, IMHO, there's a bug somewhere. Yet, I agree that udev tries to set devices too fast. It would be better if it would wait for a few milisseconds, to reduce the risk of race conditions. >> It is hard to tell, as Mark didn't provide us yet the dmesg info (at least >> on the emails I was c/c), so I don't even know what device he has, and what >> drivers are used. > > I belie you have been copied on the mail that had the following snippet: > >> 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] Ok, the last line says it is a ivtv board, using IR. However, it doesn't show the I2C detection of other devices that might be racing to gain access to the I2C bus, nor if some OOPS were hit by kernel. I don't have any ivtv boards handy, but there are some developers at linux-media ML that may help with this. >>> 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. >> >> It could be done, but then we'll need to pollute the existing input tools >> with RC-specific stuff. For IR, there are some additional steps, like >> the need to select the IR protocol, otherwise the keytable is useless. > > That should be done by the separate utility that fires up when udev gets > event for /sys/class/rc/rcX device. > >> Also, the keytable and persistent info is provided via /sys/class/rc/rc?/uevent. >> So, the tool need to first read the RC class, check what keytable should be >> associated with that device (based on a custom file), and load the proper >> table. > > And this could be easily added to the udev's keymap utility that is > fired up when we discover evdevX devices. Yes, it can, if you add the IR protocol selection on that tool. A remote controller keycode table has both the protocol and the keycodes. This basically means to merge 99% of the logic inside ir-keytable into the evdev generic tool. I'm not against it, although I prefer a specialized tool for RC. >> Also, I'm currently working on a way to map media keys for remote controllers >> into X11 (basically, mapping them into the keyspace between 8-255, passing >> through Xorg evdev.c, and then mapping back into some X11 symbols). This way, >> we don't need to violate the X11 protocol. (Yeah, I know this is hacky, but >> while X11 cannot pass the full evdev keycode, at least the Remote Controllers >> will work). This probably means that we may need to add some DBus logic >> inside ir-keytable, when called via udev, to allow it to announce to X11. > > The same issue is present with other types of input devices (multimedia > keyboards emitting codes that X can't consume) and so it again would > make sense to enhance udev's utility instead of confining it all to > ir-keytable. I agree with you, but I'm not sure if we can find a solution that will work for both RC and media keyboards, as X11 evdev just maps keyboards on the 8-255 range. I was thinking to add a detection there for RC, and use a separate map for them, as RC don't need most of the normal keyboard keys. Cheers, Mauro ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-28 11:55 ` Mauro Carvalho Chehab @ 2011-01-28 16:40 ` Dmitry Torokhov 2011-01-28 17:01 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-28 16:40 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media On Fri, Jan 28, 2011 at 09:55:58AM -0200, Mauro Carvalho Chehab wrote: > Em 28-01-2011 07:39, Dmitry Torokhov escreveu: > > On Thu, Jan 27, 2011 at 04:58:57PM -0200, Mauro Carvalho Chehab wrote: > >> Em 27-01-2011 15:21, Dmitry Torokhov escreveu: > >>> 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. > >> > >> Race conditions are hard to track... probably the new code added some delay, > >> and this allowed the request_module() to finish his job. > >> > >>> BTW, that means that we need to redo udev rules. > >> > >> If there's a race condition, then the proper fix is to lock the driver > >> until it is ready to receive a fops. Maybe we'll need a mutex to preventing > >> opening the device until it is completely initialized. > > > > No, not at all. The devices are ready to handle everything when they are > > created, it's just some devices are not there yet. What you do with > > current udev rule is similar to trying to mount filesystem as soon as > > you discover a PCI SCSI card. The controller is there but disks have not > > been discovered, block devices have not been created, and so on. > > The rc-core register (and the corresponding input register) is done when > the device detected a remote controller, so, it should be safe to register > on that point. If not, IMHO, there's a bug somewhere. It is not a matter of safe or unsafe registration. Registration is fine. The problem is that with the current set up is that utility is fired when trunk of [sub]tree is created, but the utility wants to operate on leaves which may not be there yet. > > Yet, I agree that udev tries to set devices too fast. It tries to set devices exacty when you tell it to do so. It's not like it goes trolling for random devices is sysfs. > It would be better if > it would wait for a few milisseconds, to reduce the risk of race conditions. Gah, I really prefer using properly engineered solutions instead of adding crutches. > > >> It is hard to tell, as Mark didn't provide us yet the dmesg info (at least > >> on the emails I was c/c), so I don't even know what device he has, and what > >> drivers are used. > > > > I belie you have been copied on the mail that had the following snippet: > > > >> 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] > > Ok, the last line says it is a ivtv board, using IR. However, it doesn't show > the I2C detection of other devices that might be racing to gain access to the > I2C bus, nor if some OOPS were hit by kernel. > > I don't have any ivtv boards handy, but there are some developers at > linux-media ML that may help with this. > > >>> 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. > >> > >> It could be done, but then we'll need to pollute the existing input tools > >> with RC-specific stuff. For IR, there are some additional steps, like > >> the need to select the IR protocol, otherwise the keytable is useless. > > > > That should be done by the separate utility that fires up when udev gets > > event for /sys/class/rc/rcX device. > > > >> Also, the keytable and persistent info is provided via /sys/class/rc/rc?/uevent. > >> So, the tool need to first read the RC class, check what keytable should be > >> associated with that device (based on a custom file), and load the proper > >> table. > > > > And this could be easily added to the udev's keymap utility that is > > fired up when we discover evdevX devices. > > Yes, it can, if you add the IR protocol selection on that tool. A remote > controller keycode table has both the protocol and the keycodes. > This basically means to merge 99% of the logic inside ir-keytable into the > evdev generic tool. Or just have an utility producing keymap name and feed it as input to the generic tools. The way most of utilities work... > > I'm not against it, although I prefer a specialized tool for RC. > > >> Also, I'm currently working on a way to map media keys for remote controllers > >> into X11 (basically, mapping them into the keyspace between 8-255, passing > >> through Xorg evdev.c, and then mapping back into some X11 symbols). This way, > >> we don't need to violate the X11 protocol. (Yeah, I know this is hacky, but > >> while X11 cannot pass the full evdev keycode, at least the Remote Controllers > >> will work). This probably means that we may need to add some DBus logic > >> inside ir-keytable, when called via udev, to allow it to announce to X11. > > > > The same issue is present with other types of input devices (multimedia > > keyboards emitting codes that X can't consume) and so it again would > > make sense to enhance udev's utility instead of confining it all to > > ir-keytable. > > I agree with you, but I'm not sure if we can find a solution that will > work for both RC and media keyboards, as X11 evdev just maps keyboards > on the 8-255 range. I was thinking to add a detection there for RC, and > use a separate map for them, as RC don't need most of the normal keyboard > keys. Well, there will always be clashes - there is reason why evdev goes beyond 255 keycodes... Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-28 16:40 ` Dmitry Torokhov @ 2011-01-28 17:01 ` Mauro Carvalho Chehab 2011-01-28 17:33 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-28 17:01 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Em 28-01-2011 14:40, Dmitry Torokhov escreveu: > On Fri, Jan 28, 2011 at 09:55:58AM -0200, Mauro Carvalho Chehab wrote: >> The rc-core register (and the corresponding input register) is done when >> the device detected a remote controller, so, it should be safe to register >> on that point. If not, IMHO, there's a bug somewhere. > > It is not a matter of safe or unsafe registration. Registration is fine. > The problem is that with the current set up is that utility is fired > when trunk of [sub]tree is created, but the utility wants to operate on > leaves which may not be there yet. I'm not an udev expert. Is there a udev event that hits only after having the driver completely loaded? Starting an udev rule while modprobe is still running is asking for race conditions. I'm not entirely convinced that this is the bug that Mark is hitting, as rc-core does all needed setups before registering the evdev device. We need the core and the dmesg to be sure about what's happening there. >> Yet, I agree that udev tries to set devices too fast. > > It tries to set devices exacty when you tell it to do so. It's not like > it goes trolling for random devices is sysfs. > >> It would be better if >> it would wait for a few milisseconds, to reduce the risk of race conditions. > > Gah, I really prefer using properly engineered solutions instead of > adding crutches. I agree. >>> And this could be easily added to the udev's keymap utility that is >>> fired up when we discover evdevX devices. >> >> Yes, it can, if you add the IR protocol selection on that tool. A remote >> controller keycode table has both the protocol and the keycodes. >> This basically means to merge 99% of the logic inside ir-keytable into the >> evdev generic tool. > > Or just have an utility producing keymap name and feed it as input to > the generic tools. The way most of utilities work... I don't like the idea of running a some logic at udev that would generate such keymap in runtime just before calling the generic tool. The other alternative (e. g.) to maintain the RC-protocol dependent keytables separate from the RC protocol used by each table will be a maintenance nightmare. >>>> Also, I'm currently working on a way to map media keys for remote controllers >>>> into X11 (basically, mapping them into the keyspace between 8-255, passing >>>> through Xorg evdev.c, and then mapping back into some X11 symbols). This way, >>>> we don't need to violate the X11 protocol. (Yeah, I know this is hacky, but >>>> while X11 cannot pass the full evdev keycode, at least the Remote Controllers >>>> will work). This probably means that we may need to add some DBus logic >>>> inside ir-keytable, when called via udev, to allow it to announce to X11. >>> >>> The same issue is present with other types of input devices (multimedia >>> keyboards emitting codes that X can't consume) and so it again would >>> make sense to enhance udev's utility instead of confining it all to >>> ir-keytable. >> >> I agree with you, but I'm not sure if we can find a solution that will >> work for both RC and media keyboards, as X11 evdev just maps keyboards >> on the 8-255 range. I was thinking to add a detection there for RC, and >> use a separate map for them, as RC don't need most of the normal keyboard >> keys. > > Well, there will always be clashes - there is reason why evdev goes > beyond 255 keycodes... Yeah. The most appropriate fix would be for X to just use the full evdev keycode range. However, I'm not seeing any indication that such change will happen soon. Not sure if there are some news about it at LCA, as there were one speech about this subject there. Cheers, Mauro ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-28 17:01 ` Mauro Carvalho Chehab @ 2011-01-28 17:33 ` Dmitry Torokhov 2011-01-28 18:15 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-28 17:33 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media On Fri, Jan 28, 2011 at 03:01:58PM -0200, Mauro Carvalho Chehab wrote: > Em 28-01-2011 14:40, Dmitry Torokhov escreveu: > > On Fri, Jan 28, 2011 at 09:55:58AM -0200, Mauro Carvalho Chehab wrote: > > >> The rc-core register (and the corresponding input register) is done when > >> the device detected a remote controller, so, it should be safe to register > >> on that point. If not, IMHO, there's a bug somewhere. > > > > It is not a matter of safe or unsafe registration. Registration is fine. > > The problem is that with the current set up is that utility is fired > > when trunk of [sub]tree is created, but the utility wants to operate on > > leaves which may not be there yet. > > I'm not an udev expert. Is there a udev event that hits only after having > the driver completely loaded? Define completely loaded? For a PCI SCSI controller does fully loaded mean all attached devices are discovered and registered with block layer? For a wireless NIC does it mean that it assocuated with an AP? What if you have more than one device that driver serves? So teh answer is no and there should not be. > > Starting an udev rule while modprobe is > still running is asking for race conditions. Not if we write stuff properly. > > I'm not entirely convinced that this is the bug that Mark is hitting, as I do not know yet. > rc-core does all needed setups before registering the evdev device. We > need the core and the dmesg to be sure about what's happening there. I will say it again. Your udev rule triggers when you create rcX device. eventX device may apeear 2 hours after that (I could have evdev as a module and blacklisted and load it later manually). You need to split it into 2 separate steps: 1. Triggers when rcX appears, accesses only rcX and it's parents and does rcX related stuff. 2. Triggers when eventX appears and loads keymap and what not. Because it is a child of rcX (in specific case of remotes) it may examine rcX attributes as well. > > >> Yet, I agree that udev tries to set devices too fast. > > > > It tries to set devices exacty when you tell it to do so. It's not like > > it goes trolling for random devices is sysfs. > > > >> It would be better if > >> it would wait for a few milisseconds, to reduce the risk of race conditions. > > > > Gah, I really prefer using properly engineered solutions instead of > > adding crutches. > > I agree. > > >>> And this could be easily added to the udev's keymap utility that is > >>> fired up when we discover evdevX devices. > >> > >> Yes, it can, if you add the IR protocol selection on that tool. A remote > >> controller keycode table has both the protocol and the keycodes. > >> This basically means to merge 99% of the logic inside ir-keytable into the > >> evdev generic tool. > > > > Or just have an utility producing keymap name and feed it as input to > > the generic tools. The way most of utilities work... > > I don't like the idea of running a some logic at udev that would generate > such keymap in runtime just before calling the generic tool. The other Why? You'd just call something like: keymap $name `rc-keymap-name -d $name` where 'keymap' is udev's utility and 'rc-keymap-name' is new utility that incorporates map selection logic currently found in rc-keytable. It looks like format of the keymaps is compatible between 'keymap' and 'ir-keytable' and metadata that is present in your keymaps will not confuse 'keymap' utility. > alternative (e. g.) to maintain the RC-protocol dependent keytables separate > from the RC protocol used by each table will be a maintenance nightmare. I do not propose splitting keytables, I propose splittign utilities. ir-keytable is a kitchen sink now. It implements 'keymap', 'evtest' and bucnch of other stuff and would be much cleaner if split apart. > > >>>> Also, I'm currently working on a way to map media keys for remote controllers > >>>> into X11 (basically, mapping them into the keyspace between 8-255, passing > >>>> through Xorg evdev.c, and then mapping back into some X11 symbols). This way, > >>>> we don't need to violate the X11 protocol. (Yeah, I know this is hacky, but > >>>> while X11 cannot pass the full evdev keycode, at least the Remote Controllers > >>>> will work). This probably means that we may need to add some DBus logic > >>>> inside ir-keytable, when called via udev, to allow it to announce to X11. > >>> > >>> The same issue is present with other types of input devices (multimedia > >>> keyboards emitting codes that X can't consume) and so it again would > >>> make sense to enhance udev's utility instead of confining it all to > >>> ir-keytable. > >> > >> I agree with you, but I'm not sure if we can find a solution that will > >> work for both RC and media keyboards, as X11 evdev just maps keyboards > >> on the 8-255 range. I was thinking to add a detection there for RC, and > >> use a separate map for them, as RC don't need most of the normal keyboard > >> keys. > > > > Well, there will always be clashes - there is reason why evdev goes > > beyond 255 keycodes... > > Yeah. The most appropriate fix would be for X to just use the full evdev > keycode range. However, I'm not seeing any indication that such change > will happen soon. Not sure if there are some news about it at LCA, as > there were one speech about this subject there. > Would be nice... Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-28 17:33 ` Dmitry Torokhov @ 2011-01-28 18:15 ` Mauro Carvalho Chehab 2011-01-28 18:34 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-28 18:15 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media Em 28-01-2011 15:33, Dmitry Torokhov escreveu: > On Fri, Jan 28, 2011 at 03:01:58PM -0200, Mauro Carvalho Chehab wrote: >> Em 28-01-2011 14:40, Dmitry Torokhov escreveu: >>> On Fri, Jan 28, 2011 at 09:55:58AM -0200, Mauro Carvalho Chehab wrote: >> >>>> The rc-core register (and the corresponding input register) is done when >>>> the device detected a remote controller, so, it should be safe to register >>>> on that point. If not, IMHO, there's a bug somewhere. >>> >>> It is not a matter of safe or unsafe registration. Registration is fine. >>> The problem is that with the current set up is that utility is fired >>> when trunk of [sub]tree is created, but the utility wants to operate on >>> leaves which may not be there yet. >> >> I'm not an udev expert. Is there a udev event that hits only after having >> the driver completely loaded? > > Define completely loaded? For a PCI SCSI controller does fully loaded > mean all attached devices are discovered and registered with block layer? > For a wireless NIC does it mean that it assocuated with an AP? What if > you have more than one device that driver serves? > > So teh answer is no and there should not be. > >> >> Starting an udev rule while modprobe is >> still running is asking for race conditions. > > Not if we write stuff properly. > >> >> I'm not entirely convinced that this is the bug that Mark is hitting, as > > I do not know yet. > >> rc-core does all needed setups before registering the evdev device. We >> need the core and the dmesg to be sure about what's happening there. > > I will say it again. Your udev rule triggers when you create rcX device. > eventX device may apeear 2 hours after that (I could have evdev as a > module and blacklisted and load it later manually). Blacklisting it won't (or shouldn't work). >From rc-main, the registering sequence is: dev_set_name(&dev->dev, "rc%ld", dev->devno); dev_set_drvdata(&dev->dev, dev); rc = device_add(&dev->dev); if (rc) return rc; rc = ir_setkeytable(dev, rc_map); if (rc) goto out_dev; dev->input_dev->dev.parent = &dev->dev; memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id)); dev->input_dev->phys = dev->input_phys; dev->input_dev->name = dev->input_name; rc = input_register_device(dev->input_dev); if (rc) goto out_table; rc-main will wait for input_register_device() to finish, so even if you blacklist it, rc-core will load it, in order to solve the symbol dependency. Btw, there's really a race issue there: device_add is happening before input_register_device(), so the udev rule will cause troubles. > You need to split it into 2 separate steps: > > 1. Triggers when rcX appears, accesses only rcX and it's parents and > does rcX related stuff. > > 2. Triggers when eventX appears and loads keymap and what not. Because > it is a child of rcX (in specific case of remotes) it may examine > rcX attributes as well. The fix is probably simpler: we need to change the udev rules to work at evdev registration and only if the device is a remote controller. This should solve the current issue. >>>> Yet, I agree that udev tries to set devices too fast. >>> >>> It tries to set devices exacty when you tell it to do so. It's not like >>> it goes trolling for random devices is sysfs. >>> >>>> It would be better if >>>> it would wait for a few milisseconds, to reduce the risk of race conditions. >>> >>> Gah, I really prefer using properly engineered solutions instead of >>> adding crutches. >> >> I agree. >> >>>>> And this could be easily added to the udev's keymap utility that is >>>>> fired up when we discover evdevX devices. >>>> >>>> Yes, it can, if you add the IR protocol selection on that tool. A remote >>>> controller keycode table has both the protocol and the keycodes. >>>> This basically means to merge 99% of the logic inside ir-keytable into the >>>> evdev generic tool. >>> >>> Or just have an utility producing keymap name and feed it as input to >>> the generic tools. The way most of utilities work... >> >> I don't like the idea of running a some logic at udev that would generate >> such keymap in runtime just before calling the generic tool. The other > > Why? You'd just call something like: > > keymap $name `rc-keymap-name -d $name` > > where 'keymap' is udev's utility and 'rc-keymap-name' is new utility > that incorporates map selection logic currently found in rc-keytable. > > It looks like format of the keymaps is compatible between 'keymap' and > 'ir-keytable' and metadata that is present in your keymaps will not > confuse 'keymap' utility. The format is, currently compatible. However, we'll likely need to change it (or to allow the tool to handle also a different format), due to some reasons: 1) Protocol and the device name where it is found by default is currently a comment; 2) We'll need to add a field there specifying the number of the bits to be used by the keymap table, in order to use the proper length with _V2 ioctls; 3) There are hundreds of keymaps already created for lircd. It would be nice to support lircd format, in order to make life easier for those that use lirc. If you want to add these to the generic tool, that's fine for me, but, IMO, this doesn't sound a good idea. There are already specialized tools for other kinds of input devices (mouse: gpm; joystick: jstest; etc). It seems a bad idea to merge them into the generic tools. I think it is better to have a tool that just handle one kind of device, but does it well, than trying to extend a generic tool to cover all possible devices, and adding lots of caveats there for it to handle all the specifics. >> alternative (e. g.) to maintain the RC-protocol dependent keytables separate >> from the RC protocol used by each table will be a maintenance nightmare. > > I do not propose splitting keytables, I propose splittign utilities. > ir-keytable is a kitchen sink now. It implements 'keymap', 'evtest' and > bucnch of other stuff and would be much cleaner if split apart. It may be broken into a few utilities, by creating a library with the common code. I'll think about that when I have some spare time for it. Cheers, Mauro. ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-28 18:15 ` Mauro Carvalho Chehab @ 2011-01-28 18:34 ` Dmitry Torokhov 2011-01-28 20:53 ` Mark Lord 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-28 18:34 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Mark Lord, Linus Torvalds, Linux Kernel, linux-input, linux-media On Fri, Jan 28, 2011 at 04:15:51PM -0200, Mauro Carvalho Chehab wrote: > Em 28-01-2011 15:33, Dmitry Torokhov escreveu: > > On Fri, Jan 28, 2011 at 03:01:58PM -0200, Mauro Carvalho Chehab wrote: > >> Em 28-01-2011 14:40, Dmitry Torokhov escreveu: > >>> On Fri, Jan 28, 2011 at 09:55:58AM -0200, Mauro Carvalho Chehab wrote: > >> > >>>> The rc-core register (and the corresponding input register) is done when > >>>> the device detected a remote controller, so, it should be safe to register > >>>> on that point. If not, IMHO, there's a bug somewhere. > >>> > >>> It is not a matter of safe or unsafe registration. Registration is fine. > >>> The problem is that with the current set up is that utility is fired > >>> when trunk of [sub]tree is created, but the utility wants to operate on > >>> leaves which may not be there yet. > >> > >> I'm not an udev expert. Is there a udev event that hits only after having > >> the driver completely loaded? > > > > Define completely loaded? For a PCI SCSI controller does fully loaded > > mean all attached devices are discovered and registered with block layer? > > For a wireless NIC does it mean that it assocuated with an AP? What if > > you have more than one device that driver serves? > > > > So teh answer is no and there should not be. > > > >> > >> Starting an udev rule while modprobe is > >> still running is asking for race conditions. > > > > Not if we write stuff properly. > > > >> > >> I'm not entirely convinced that this is the bug that Mark is hitting, as > > > > I do not know yet. > > > >> rc-core does all needed setups before registering the evdev device. We > >> need the core and the dmesg to be sure about what's happening there. > > > > I will say it again. Your udev rule triggers when you create rcX device. > > eventX device may apeear 2 hours after that (I could have evdev as a > > module and blacklisted and load it later manually). > > Blacklisting it won't (or shouldn't work). > > From rc-main, the registering sequence is: > > dev_set_name(&dev->dev, "rc%ld", dev->devno); > dev_set_drvdata(&dev->dev, dev); > rc = device_add(&dev->dev); > if (rc) > return rc; > > rc = ir_setkeytable(dev, rc_map); > if (rc) > goto out_dev; > > dev->input_dev->dev.parent = &dev->dev; > memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id)); > dev->input_dev->phys = dev->input_phys; > dev->input_dev->name = dev->input_name; > rc = input_register_device(dev->input_dev); > if (rc) > goto out_table; > > rc-main will wait for input_register_device() to finish, so even if you > blacklist it, rc-core will load it, in order to solve the symbol dependency. No, input_register_device() and input core itself does not have symbol dependency on evdev (which provides /dev/input/eventX), which is just an input handler. A very important, but still an optional, handler. > Btw, there's really a race issue there: device_add is happening before > input_register_device(), so the udev rule will cause troubles. That's what I have been saying in the last 3+ emails, yes. > > > You need to split it into 2 separate steps: > > > > 1. Triggers when rcX appears, accesses only rcX and it's parents and > > does rcX related stuff. > > > > 2. Triggers when eventX appears and loads keymap and what not. Because > > it is a child of rcX (in specific case of remotes) it may examine > > rcX attributes as well. > > The fix is probably simpler: we need to change the udev rules to work at > evdev registration and only if the device is a remote controller. This > should solve the current issue. The problem I have with it is that it violates layering. You affect different subsystems/layers, why do you insist on jamming them togetehr? Don't do the kitchen sink style, pretty please. > > >>>> Yet, I agree that udev tries to set devices too fast. > >>> > >>> It tries to set devices exacty when you tell it to do so. It's not like > >>> it goes trolling for random devices is sysfs. > >>> > >>>> It would be better if > >>>> it would wait for a few milisseconds, to reduce the risk of race conditions. > >>> > >>> Gah, I really prefer using properly engineered solutions instead of > >>> adding crutches. > >> > >> I agree. > >> > >>>>> And this could be easily added to the udev's keymap utility that is > >>>>> fired up when we discover evdevX devices. > >>>> > >>>> Yes, it can, if you add the IR protocol selection on that tool. A remote > >>>> controller keycode table has both the protocol and the keycodes. > >>>> This basically means to merge 99% of the logic inside ir-keytable into the > >>>> evdev generic tool. > >>> > >>> Or just have an utility producing keymap name and feed it as input to > >>> the generic tools. The way most of utilities work... > >> > >> I don't like the idea of running a some logic at udev that would generate > >> such keymap in runtime just before calling the generic tool. The other > > > > Why? You'd just call something like: > > > > keymap $name `rc-keymap-name -d $name` > > > > where 'keymap' is udev's utility and 'rc-keymap-name' is new utility > > that incorporates map selection logic currently found in rc-keytable. > > > > It looks like format of the keymaps is compatible between 'keymap' and > > 'ir-keytable' and metadata that is present in your keymaps will not > > confuse 'keymap' utility. > > The format is, currently compatible. However, we'll likely need to change it > (or to allow the tool to handle also a different format), due to some reasons: > 1) Protocol and the device name where it is found by default is > currently a comment; So? Keep it there, it does not hurt anything. > 2) We'll need to add a field there specifying the number of the bits > to be used by the keymap table, in order to use the proper length > with _V2 ioctls; You can't calculate this automatically given the length of the "scancode"? I mean if you have keymap file with 0x12345678936 KEY_COFFEE that means that scancode is at least 36 bit. > 3) There are hundreds of keymaps already created for lircd. It > would be nice to support lircd format, in order to make life > easier for those that use lirc. or write a script to convert and be done with it? > > If you want to add these to the generic tool, that's fine for me, but, IMO, this > doesn't sound a good idea. There are already specialized tools for other kinds > of input devices (mouse: gpm; joystick: jstest; etc). It seems a bad idea > to merge them into the generic tools. gpm is different and more akin to X itself and is not limited to mice; jstest uses differnt interface (jsX, not eventX) so you are right, we should not mix them into utilities working with event interface. keymap/ir-keytable both work with event interface. > > I think it is better to have a tool that just handle one kind of device, but > does it well, than trying to extend a generic tool to cover all possible > devices, and adding lots of caveats there for it to handle all the specifics. > > >> alternative (e. g.) to maintain the RC-protocol dependent keytables separate > >> from the RC protocol used by each table will be a maintenance nightmare. > > > > I do not propose splitting keytables, I propose splittign utilities. > > ir-keytable is a kitchen sink now. It implements 'keymap', 'evtest' and > > bucnch of other stuff and would be much cleaner if split apart. > > It may be broken into a few utilities, by creating a library with the > common code. I'll think about that when I have some spare time for it. > Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-28 18:34 ` Dmitry Torokhov @ 2011-01-28 20:53 ` Mark Lord 0 siblings, 0 replies; 95+ messages in thread From: Mark Lord @ 2011-01-28 20:53 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media Dmitry / Mauro, I'm encouraged by all of the good dialog happening here, and regret that I am unable to poke any further at the issue with ir-keytable for now. The system in question is now getting rebuilt with new/modern userspace, and I expect the original issue to "vanish" as a result. If I do see ir-keytable acting up again afterward, I'll let you know. But it will be whatever version Ubuntu 10.10 installs, not the newer one I used earlier in this thread. Cheers! -Mark ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 6:38 ` Dmitry Torokhov 2011-01-27 10:30 ` Mauro Carvalho Chehab @ 2011-01-27 14:54 ` Mark Lord 1 sibling, 0 replies; 95+ messages in thread From: Mark Lord @ 2011-01-27 14:54 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-27 01:38 AM, Dmitry Torokhov wrote: .. > 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... .. I downloaded/built/installed it from the link you gave earlier in this thread. Cheers ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 3:18 ` Mark Lord 2011-01-27 6:38 ` Dmitry Torokhov @ 2011-01-27 16:39 ` Dmitry Torokhov 2011-01-27 18:12 ` Mark Lord 1 sibling, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-27 16:39 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote: > On 11-01-26 09:12 PM, Dmitry Torokhov wrote: > > On Wed, Jan 26, 2011 at 08:07:29PM -0500, Mark Lord wrote: > >> On 11-01-26 08:01 PM, Mark Lord wrote: > >>> On 11-01-26 10:05 AM, Mark Lord wrote: > >>>> On 11-01-25 09:00 PM, Dmitry Torokhov wrote: > >>> .. > >>>>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 16:39 ` Dmitry Torokhov @ 2011-01-27 18:12 ` Mark Lord 2011-01-27 19:53 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-27 18:12 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-27 11:39 AM, Dmitry Torokhov wrote: > On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote: >> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 18:12 ` Mark Lord @ 2011-01-27 19:53 ` Dmitry Torokhov 2011-01-28 16:42 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-27 19:53 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Thu, Jan 27, 2011 at 01:12:48PM -0500, Mark Lord wrote: > On 11-01-27 11:39 AM, Dmitry Torokhov wrote: > > On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote: > >> 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. > See if adjusting /etc/security/limits.conf will enable it to dump core. Otherwise you'll have to stick 'ulimit -c unlimited' somewhere... -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-27 19:53 ` Dmitry Torokhov @ 2011-01-28 16:42 ` Dmitry Torokhov 2011-01-28 20:55 ` Mark Lord 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-28 16:42 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Thu, Jan 27, 2011 at 11:53:25AM -0800, Dmitry Torokhov wrote: > On Thu, Jan 27, 2011 at 01:12:48PM -0500, Mark Lord wrote: > > On 11-01-27 11:39 AM, Dmitry Torokhov wrote: > > > On Wed, Jan 26, 2011 at 10:18:53PM -0500, Mark Lord wrote: > > >> 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. > > > > See if adjusting /etc/security/limits.conf will enable it to dump core. > Otherwise you'll have to stick 'ulimit -c unlimited' somewhere... > Mark, Any luck with getting the core? I'd really like to resolve this issue. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-28 16:42 ` Dmitry Torokhov @ 2011-01-28 20:55 ` Mark Lord 2011-01-28 21:03 ` Mark Lord 0 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-28 20:55 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-28 11:42 AM, Dmitry Torokhov wrote: > On Thu, Jan 27, 2011 at 11:53:25AM -0800, Dmitry Torokhov wrote: >> On Thu, Jan 27, 2011 at 01:12:48PM -0500, Mark Lord wrote: >>> On 11-01-27 11:39 AM, Dmitry Torokhov wrote: .. >>>> 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. >>> >> >> See if adjusting /etc/security/limits.conf will enable it to dump core. >> Otherwise you'll have to stick 'ulimit -c unlimited' somewhere... .. > Any luck with getting the core? I'd really like to resolve this issue. .. I'm upgrading the box to new userspace now. But I still have the old installation drive, so perhaps I'll go there now and try this. My plan is to replace /usr/bin/ir-keytable with a script that issues the 'ulimit -c unlimited' command and then invokes the original /usr/bin/ir-keytable binary. Should take half an hour or so before I get back here again. -ml ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-28 20:55 ` Mark Lord @ 2011-01-28 21:03 ` Mark Lord 2011-01-28 21:09 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-01-28 21:03 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On 11-01-28 03:55 PM, Mark Lord wrote: > On 11-01-28 11:42 AM, Dmitry Torokhov wrote: >> On Thu, Jan 27, 2011 at 11:53:25AM -0800, Dmitry Torokhov wrote: >>> On Thu, Jan 27, 2011 at 01:12:48PM -0500, Mark Lord wrote: >>>> On 11-01-27 11:39 AM, Dmitry Torokhov wrote: > .. >>>>> 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. >>>> >>> >>> See if adjusting /etc/security/limits.conf will enable it to dump core. >>> Otherwise you'll have to stick 'ulimit -c unlimited' somewhere... > .. >> Any luck with getting the core? I'd really like to resolve this issue. > .. > > I'm upgrading the box to new userspace now. > But I still have the old installation drive, > so perhaps I'll go there now and try this. > > My plan is to replace /usr/bin/ir-keytable with a script > that issues the 'ulimit -c unlimited' command and then > invokes the original /usr/bin/ir-keytable binary. > > Should take half an hour or so before I get back here again. No-go. According to the syslog, the segfault has not happened since I reconfigured the kernel and startup sequence two days ago to resolve an XFS mount issue. Something in there changed the init timing just enough to make it go away, I believe. Now I'm blowing it all away in favour of fresh userspace, with a whole new set of issues to resolve. :) Off-Topic: Ubuntu (Mythbuntu) really has a ton of timing issues with this upstart thing at startup and shutdown.. running from an SSD really exposes the flaws. Cheers ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-28 21:03 ` Mark Lord @ 2011-01-28 21:09 ` Dmitry Torokhov 0 siblings, 0 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-28 21:09 UTC (permalink / raw) To: Mark Lord Cc: Mauro Carvalho Chehab, Linus Torvalds, Linux Kernel, linux-input, linux-media On Fri, Jan 28, 2011 at 04:03:07PM -0500, Mark Lord wrote: > On 11-01-28 03:55 PM, Mark Lord wrote: > > On 11-01-28 11:42 AM, Dmitry Torokhov wrote: > >> On Thu, Jan 27, 2011 at 11:53:25AM -0800, Dmitry Torokhov wrote: > >>> On Thu, Jan 27, 2011 at 01:12:48PM -0500, Mark Lord wrote: > >>>> On 11-01-27 11:39 AM, Dmitry Torokhov wrote: > > .. > >>>>> 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. > >>>> > >>> > >>> See if adjusting /etc/security/limits.conf will enable it to dump core. > >>> Otherwise you'll have to stick 'ulimit -c unlimited' somewhere... > > .. > >> Any luck with getting the core? I'd really like to resolve this issue. > > .. > > > > I'm upgrading the box to new userspace now. > > But I still have the old installation drive, > > so perhaps I'll go there now and try this. > > > > My plan is to replace /usr/bin/ir-keytable with a script > > that issues the 'ulimit -c unlimited' command and then > > invokes the original /usr/bin/ir-keytable binary. > > > > Should take half an hour or so before I get back here again. > > No-go. According to the syslog, the segfault has not happened > since I reconfigured the kernel and startup sequence two days > ago to resolve an XFS mount issue. > > Something in there changed the init timing just enough to make > it go away, I believe. OK, this reinforces my suspicion that the cause of segfault is the race we were discussing with Mauro, not the keymap retrieval fix. I shall be sending the patch to Linus/stable in the next pull then. Thank you for your help. -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 22:00 ` Mauro Carvalho Chehab 2011-01-25 22:22 ` Mark Lord @ 2011-01-25 22:25 ` Linus Torvalds 1 sibling, 0 replies; 95+ messages in thread From: Linus Torvalds @ 2011-01-25 22:25 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Dmitry Torokhov, Mark Lord, Linux Kernel, linux-input, linux-media On Wed, Jan 26, 2011 at 8:00 AM, Mauro Carvalho Chehab <mchehab@redhat.com> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 5:04 ` Mark Lord 2011-01-25 5:07 ` Mark Lord @ 2011-01-25 5:29 ` Dmitry Torokhov 2011-01-25 14:28 ` Mark Lord 1 sibling, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-01-25 5:29 UTC (permalink / raw) To: Mark Lord; +Cc: Linux Kernel, linux-input On Tue, Jan 25, 2011 at 12:04:10AM -0500, Mark Lord wrote: > On 11-01-24 11:55 PM, Dmitry Torokhov wrote: > > On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: > .. > >> 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. :) > -- Dmitry ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-25 5:29 ` Dmitry Torokhov @ 2011-01-25 14:28 ` Mark Lord 0 siblings, 0 replies; 95+ messages in thread From: Mark Lord @ 2011-01-25 14:28 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Linux Kernel, linux-input, mchehab On 11-01-25 12:29 AM, Dmitry Torokhov wrote: > On Tue, Jan 25, 2011 at 12:04:10AM -0500, Mark Lord wrote: >> On 11-01-24 11:55 PM, Dmitry Torokhov wrote: >>> On Mon, Jan 24, 2011 at 11:37:06PM -0500, Mark Lord wrote: .. >>> 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 ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-01-23 17:03 2.6.36/2.6.37: broken compatibility with userspace input-utils ? Mark Lord 2011-01-24 17:54 ` Dmitry Torokhov @ 2011-02-02 14:31 ` Chase Douglas 2011-02-02 16:58 ` Dmitry Torokhov 1 sibling, 1 reply; 95+ messages in thread From: Chase Douglas @ 2011-02-02 14:31 UTC (permalink / raw) To: Mark Lord; +Cc: Linux Kernel, linux-input, Dmitry Torokhov, kraxel On 01/23/2011 12:03 PM, 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. I planned on taking another look at this before we release Ubuntu 11.04 at the end of April. That doesn't prevent someone else from helping out :), but I'll make sure the utility is working again if not. At this point it sounds like we should switch over to Gerd's new repo of sources. Gerd, will you be making any kind of official release? Thanks, -- Chase ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-02-02 14:31 ` Chase Douglas @ 2011-02-02 16:58 ` Dmitry Torokhov 2011-02-02 20:00 ` Chase Douglas 0 siblings, 1 reply; 95+ messages in thread From: Dmitry Torokhov @ 2011-02-02 16:58 UTC (permalink / raw) To: Chase Douglas; +Cc: Mark Lord, Linux Kernel, linux-input, kraxel On Wed, Feb 02, 2011 at 09:31:08AM -0500, Chase Douglas wrote: > On 01/23/2011 12:03 PM, 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. > > I planned on taking another look at this before we release Ubuntu 11.04 > at the end of April. That doesn't prevent someone else from helping out > :), but I'll make sure the utility is working again if not. > Here you go: >From dbe48c8044987da6e64036fa3f36d7b2826994d5 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov <dmitry.torokhov@gmail.com> Date: Mon, 24 Jan 2011 22:06:10 -0800 Subject: [PATCH 1/2] Do not require exact version of EVDEV protocol, we can work with other versions too. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- input.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/input.c b/input.c index 73d5df1..08f4046 100644 --- a/input.c +++ b/input.c @@ -101,9 +101,11 @@ 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", - EV_VERSION, version); + +#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); close(fd); return -1; } -- Dmitry ^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-02-02 16:58 ` Dmitry Torokhov @ 2011-02-02 20:00 ` Chase Douglas 2011-02-02 21:54 ` Mark Lord 0 siblings, 1 reply; 95+ messages in thread From: Chase Douglas @ 2011-02-02 20:00 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Mark Lord, Linux Kernel, linux-input, kraxel On 02/02/2011 11:58 AM, Dmitry Torokhov wrote: > On Wed, Feb 02, 2011 at 09:31:08AM -0500, Chase Douglas wrote: >> On 01/23/2011 12:03 PM, 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. >> >> I planned on taking another look at this before we release Ubuntu 11.04 >> at the end of April. That doesn't prevent someone else from helping out >> :), but I'll make sure the utility is working again if not. >> > > Here you go: > > From dbe48c8044987da6e64036fa3f36d7b2826994d5 Mon Sep 17 00:00:00 2001 > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Date: Mon, 24 Jan 2011 22:06:10 -0800 > Subject: [PATCH 1/2] Do not require exact version of EVDEV protocol, we can work with > other versions too. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Yeah that's the easy part :). I need to find time to go through the packaging and bug handling churn... Thanks, -- Chase ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-02-02 20:00 ` Chase Douglas @ 2011-02-02 21:54 ` Mark Lord 2011-02-02 22:04 ` Dmitry Torokhov 0 siblings, 1 reply; 95+ messages in thread From: Mark Lord @ 2011-02-02 21:54 UTC (permalink / raw) To: Chase Douglas; +Cc: Dmitry Torokhov, Linux Kernel, linux-input, kraxel On 11-02-02 03:00 PM, Chase Douglas wrote: > On 02/02/2011 11:58 AM, Dmitry Torokhov wrote: >> On Wed, Feb 02, 2011 at 09:31:08AM -0500, Chase Douglas wrote: >>> On 01/23/2011 12:03 PM, 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. >>> >>> I planned on taking another look at this before we release Ubuntu 11.04 >>> at the end of April. That doesn't prevent someone else from helping out >>> :), but I'll make sure the utility is working again if not. >>> >> >> Here you go: That patch was not enough for me to get input-kbd working here on Ubuntu-10.10. It would still only let me map 10 keys of my 56(?) button remote. I also had to hack it to ignore the read-in map->size value when writing-out the new keymap. Cheers ^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ? 2011-02-02 21:54 ` Mark Lord @ 2011-02-02 22:04 ` Dmitry Torokhov 0 siblings, 0 replies; 95+ messages in thread From: Dmitry Torokhov @ 2011-02-02 22:04 UTC (permalink / raw) To: Mark Lord; +Cc: Chase Douglas, Linux Kernel, linux-input, kraxel On Wed, Feb 02, 2011 at 04:54:50PM -0500, Mark Lord wrote: > On 11-02-02 03:00 PM, Chase Douglas wrote: > > On 02/02/2011 11:58 AM, Dmitry Torokhov wrote: > >> On Wed, Feb 02, 2011 at 09:31:08AM -0500, Chase Douglas wrote: > >>> On 01/23/2011 12:03 PM, 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. > >>> > >>> I planned on taking another look at this before we release Ubuntu 11.04 > >>> at the end of April. That doesn't prevent someone else from helping out > >>> :), but I'll make sure the utility is working again if not. > >>> > >> > >> Here you go: > > That patch was not enough for me to get input-kbd working here on Ubuntu-10.10. > It would still only let me map 10 keys of my 56(?) button remote. > > I also had to hack it to ignore the read-in map->size value when writing-out > the new keymap. I assumed that the fix to make RCs compatible with the pre-v2 getkeycode will trickle into Ubuntu through staging (I will be sending a patch to the stable team today now that it is in mainline). But if Chase wants to also add V2 keycode hdling then here's another patch. Thanks. -- Dmitry >From cf5954a4db2d0e411bd6f717fb50bcdf37e80c9b 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 2/2] 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(-) diff --git a/input-kbd.c b/input-kbd.c index e94529d..5d93d54 100644 --- a/input-kbd.c +++ b/input-kbd.c @@ -9,9 +9,27 @@ #include "input.h" +struct input_keymap_entry_v1 { + uint32_t scancode; + uint32_t keycode; +}; + +struct input_keymap_entry_v2 { +#define KEYMAP_BY_INDEX (1 << 0) + uint8_t flags; + uint8_t len; + uint16_t index; + uint32_t keycode; + uint8_t scancode[32]; +}; + +#ifndef EVIOCGKEYCODE2 +#define EVIOCGKEYCODE2 _IOR('E', 0x04, struct input_keymap_entry_v2) +#endif + struct kbd_entry { - int scancode; - int keycode; + unsigned int scancode; + unsigned int keycode; }; struct kbd_map { @@ -23,7 +41,7 @@ struct kbd_map { /* ------------------------------------------------------------------ */ -static struct kbd_map* kbd_map_read(int fd) +static struct kbd_map* kbd_map_read(int fd, unsigned int version) { struct kbd_entry entry; struct kbd_map *map; @@ -32,16 +50,37 @@ static struct kbd_map* kbd_map_read(int fd) 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 (version < 0x10001) { + struct input_keymap_entry_v1 ke = { + .scancode = map->size, + .keycode = KEY_RESERVED, + }; + + rc = ioctl(fd, EVIOCGKEYCODE, &ke); + if (rc < 0) + break; + } else { + struct input_keymap_entry_v2 ke = { + .index = map->size, + .flags = KEYMAP_BY_INDEX, + .len = sizeof(uint32_t), + .keycode = KEY_RESERVED, + }; + + rc = ioctl(fd, EVIOCGKEYCODE2, &ke); + if (rc < 0) + break; + + memcpy(&entry.scancode, ke.scancode, + sizeof(entry.scancode)); + entry.keycode = ke.keycode; } + 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) @@ -155,40 +194,27 @@ static void kbd_print_bits(int fd) } } -static void show_kbd(int nr) +static void show_kbd(int fd, unsigned int protocol_version) { struct kbd_map *map; - int fd; - 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); + else kbd_print_bits(fd); - } - - close(fd); } -static int set_kbd(int nr, char *mapfile) +static int set_kbd(int fd, unsigned int protocol_version, char *mapfile) { struct kbd_map *map; FILE *fp; - int fd; - - 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) { printf("device has no map\n"); - close(fd); return -1; } @@ -198,18 +224,15 @@ static int set_kbd(int nr, char *mapfile) fp = fopen(mapfile,"r"); if (NULL == fp) { printf("open %s: %s\n",mapfile,strerror(errno)); - close(fd); return -1; } } - + if (0 != kbd_map_parse(fp,map) || 0 != kbd_map_write(fd,map)) { - close(fd); return -1; } - close(fd); return 0; } @@ -223,8 +246,10 @@ static int usage(char *prog, int error) int main(int argc, char *argv[]) { - int c,devnr; + int c, devnr, fd; char *mapfile = NULL; + unsigned int protocol_version; + int rc = 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) + goto out; + + if (ioctl(fd, EVIOCGVERSION, &protocol_version) < 0) { + fprintf(stderr, + "Unable to query evdev protocol version: %s\n", + strerror(errno)); + goto out_close; } - return 0; + + if (mapfile) + set_kbd(fd, protocol_version, mapfile); + else + show_kbd(fd, protocol_version); + + rc = EXIT_SUCCESS; + +out_close: + close(fd); +out: + return rc; } /* --------------------------------------------------------------------- -- 1.7.3.5 ^ permalink raw reply related [flat|nested] 95+ messages in thread
end of thread, other threads:[~2011-02-02 22:04 UTC | newest] Thread overview: 95+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-01-23 17:03 2.6.36/2.6.37: broken compatibility with userspace input-utils ? Mark Lord 2011-01-24 17:54 ` Dmitry Torokhov 2011-01-25 0:32 ` Mark Lord 2011-01-25 0:55 ` Dmitry Torokhov 2011-01-25 4:13 ` Mark Lord 2011-01-25 4:20 ` Dmitry Torokhov 2011-01-25 4:37 ` Mark Lord 2011-01-25 4:43 ` Mark Lord 2011-01-25 4:55 ` Dmitry Torokhov 2011-01-25 5:04 ` Mark Lord 2011-01-25 5:07 ` Mark Lord 2011-01-25 5:15 ` Mark Lord 2011-01-25 5:31 ` Dmitry Torokhov 2011-01-25 6:52 ` Dmitry Torokhov 2011-01-25 14:42 ` Extending rc-core/userspace to handle bigger scancodes - Was: " Mauro Carvalho Chehab 2011-01-25 16:55 ` Dmitry Torokhov 2011-01-26 9:44 ` Mauro Carvalho Chehab 2011-01-25 11:42 ` Mauro Carvalho Chehab 2011-01-25 14:32 ` Mark Lord 2011-01-25 16:48 ` Dmitry Torokhov 2011-01-25 20:09 ` Linus Torvalds 2011-01-25 20:54 ` Dmitry Torokhov 2011-01-25 21:01 ` Dmitry Torokhov 2011-01-25 21:20 ` Linus Torvalds 2011-01-25 21:20 ` Linus Torvalds 2011-01-25 21:50 ` Dmitry Torokhov 2011-01-25 22:00 ` Mauro Carvalho Chehab 2011-01-25 22:22 ` Mark Lord 2011-01-25 23:29 ` Dmitry Torokhov 2011-01-26 2:00 ` Dmitry Torokhov 2011-01-26 11:26 ` Mauro Carvalho Chehab 2011-01-26 13:08 ` Gerd Hoffmann 2011-01-26 14:18 ` Mauro Carvalho Chehab 2011-01-26 14:52 ` Gerd Hoffmann 2011-01-26 16:46 ` Dmitry Torokhov 2011-01-26 16:51 ` Dmitry Torokhov 2011-01-26 17:29 ` Mauro Carvalho Chehab 2011-01-26 18:24 ` Dmitry Torokhov 2011-01-26 19:16 ` Gerd Hoffmann 2011-01-26 19:28 ` Mark Lord 2011-01-26 20:09 ` Gerd Hoffmann 2011-01-26 19:28 ` Mauro Carvalho Chehab 2011-01-26 19:32 ` Dmitry Torokhov 2011-01-26 20:07 ` Gerd Hoffmann 2011-01-26 19:27 ` Mark Lord 2011-01-26 14:58 ` Mark Lord 2011-01-26 17:41 ` Mauro Carvalho Chehab 2011-01-26 17:59 ` Dmitry Torokhov 2011-01-26 19:30 ` Mark Lord 2011-01-26 15:05 ` Mark Lord 2011-01-26 16:44 ` Dmitry Torokhov 2011-01-26 19:31 ` Mark Lord 2011-01-26 19:38 ` Dmitry Torokhov 2011-01-26 17:32 ` Mauro Carvalho Chehab 2011-01-26 19:33 ` Mark Lord 2011-01-26 19:41 ` Dmitry Torokhov 2011-01-26 19:47 ` Mark Lord 2011-01-26 19:50 ` Dmitry Torokhov 2011-01-26 21:41 ` Mark Lord 2011-01-26 21:49 ` Mark Lord 2011-01-26 22:07 ` Dmitry Torokhov 2011-01-26 22:04 ` Dmitry Torokhov 2011-01-27 1:01 ` Mark Lord 2011-01-27 1:07 ` Mark Lord 2011-01-27 2:12 ` Dmitry Torokhov 2011-01-27 3:18 ` Mark Lord 2011-01-27 6:38 ` Dmitry Torokhov 2011-01-27 10:30 ` Mauro Carvalho Chehab 2011-01-27 15:00 ` Mark Lord 2011-01-27 17:21 ` Dmitry Torokhov 2011-01-27 18:58 ` Mauro Carvalho Chehab 2011-01-28 9:39 ` Dmitry Torokhov 2011-01-28 11:55 ` Mauro Carvalho Chehab 2011-01-28 16:40 ` Dmitry Torokhov 2011-01-28 17:01 ` Mauro Carvalho Chehab 2011-01-28 17:33 ` Dmitry Torokhov 2011-01-28 18:15 ` Mauro Carvalho Chehab 2011-01-28 18:34 ` Dmitry Torokhov 2011-01-28 20:53 ` Mark Lord 2011-01-27 14:54 ` Mark Lord 2011-01-27 16:39 ` Dmitry Torokhov 2011-01-27 18:12 ` Mark Lord 2011-01-27 19:53 ` Dmitry Torokhov 2011-01-28 16:42 ` Dmitry Torokhov 2011-01-28 20:55 ` Mark Lord 2011-01-28 21:03 ` Mark Lord 2011-01-28 21:09 ` Dmitry Torokhov 2011-01-25 22:25 ` Linus Torvalds 2011-01-25 5:29 ` Dmitry Torokhov 2011-01-25 14:28 ` Mark Lord 2011-02-02 14:31 ` Chase Douglas 2011-02-02 16:58 ` Dmitry Torokhov 2011-02-02 20:00 ` Chase Douglas 2011-02-02 21:54 ` Mark Lord 2011-02-02 22:04 ` Dmitry Torokhov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.