Bug 16351 - Various issues with imon remote control driver
Summary: Various issues with imon remote control driver
Status: CLOSED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Input Devices (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Jarod Wilson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-07 17:13 UTC by Anssi Hannula
Modified: 2011-01-23 19:41 UTC (History)
3 users (show)

See Also:
Kernel Version: 2.6.35-rc4
Tree: Mainline
Regression: No


Attachments
Patch to address a number of issues raised here (6.78 KB, patch)
2010-09-14 00:54 UTC, Jarod Wilson
Details | Diff

Description Anssi Hannula 2010-07-07 17:13:52 UTC
Jarod Wilson (the imon driver maintainer) asked my to open a report here so that the issues are not forgotten.

- There is no fallback timer if a release code is missed (i.e. remote pointed 
away from receiver or some other anomaly), causing a key stuck on autorepeat. 
The driver should inject a release code itself if there is no release/repeat 
code in 500ms after initial press or in 120ms after a repeat code.

- No release code is sent for 0x02XXXXXX keys if pressing any other button 
before release, examples:

example 1:
hold '5', then press 'Play' once, then release '5'
The 'Play' codes are relayed properly, but the release code for '5' (the 'all 
0x02XXXXXX keys released' (i.e. empty HID input report) which the hardware 
does send properly) is wrongly interpreted as a release code for 'Play'.
The driver should either release '5' when the empty report is received, or, as 
this is just a remote control, just inject a release code for '5' when 'Play' 
is pressed.

example 2:
hold '5', then hold '4', then release '5', then release '4'
As the 0x02XXXXXX range is not completely released until after '4' is 
released, the zeroed bitfield is not sent until after '4', and the driver 
doesn't release '5' at this point anymore. The driver should've injected a 
release code for '5' when '4' was pressed.

- imon_mce_timeout() timer function seems to access ictx->last_keycode without 
locking

- imon_parse_press_type() tests for ictx->release_code which is in an 
undefined state if ktype isn't IMON_KEY_IMON

- when the dpad is in keyboard mode and you hold it down to one direction, 
instead of autorepeat there is a constant stream of release/press events
Comment 1 Jarod Wilson 2010-07-07 17:39:39 UTC
Additional bits from ml thread:

BTW, I wonder if we should also disable kernel autorepeat and use the remote
control's own repeat signals instead?
Then extra repeat codes would not not emitted if the release signal is missed.
Comment 2 Jarod Wilson 2010-07-26 19:04:43 UTC
Some additional issues pointed out by Dmitry Torokhov:

Random notes about irmon:

imon_init_idev():
       memcpy(&ir->dev, ictx->dev, sizeof(struct device));

This is... scary.  Devices are refcounted and if you copy them around
all hell may break loose. On an unrelated note you do not need memcpy to
copy a structire, *it->dev = *ictx->dev will do.

imon_init_idev(), imon_init_touch(): - consizer returning proper error
codes via ERR_PTR() and check wit IS_ERR().
Comment 3 Jarod Wilson 2010-09-14 00:54:55 UTC
Created attachment 29852 [details]
Patch to address a number of issues raised here

I think this patch handles the bulk of the issues reported here. This is still a bit of a work-in-progress though, as I'm also looking to incorporate some changes from David Hardeman to split the mouse/panel keys from the remote keys in preparation for some further ir-core interface abstraction work he's doing.
Comment 4 Jarod Wilson 2010-09-15 19:17:27 UTC
David's patch actually converts the imon driver over to using more native ir-core functionality, which actually addresses the key release issues better, in addition to fixing the repeat issue when the dpad is in keyboard mode. I've got a follow-up patch that adds some locking around access to ictx->kc and ictx->last_keycode, and with that, I think we've got just about everything here licked... Will be posted to linux-media list shortly.
Comment 5 Florian Mickler 2011-01-23 16:28:05 UTC
fixed in .37-rc1:
commit 693508df9824ecc2bf308a35b58159aa2fecf91f
Author: Jarod Wilson <jarod@redhat.com>
Date:   Wed Sep 15 15:56:03 2010 -0300

    V4L/DVB: IR/imon: protect ictx's kc and last_keycode w/spinlock

Note You need to log in before you can comment on or make changes to this bug.