Bug 16351

Summary:	Various issues with imon remote control driver
Product:	Drivers	Reporter:	Anssi Hannula (anssi.hannula)
Component:	Input Devices	Assignee:	Jarod Wilson (jarod)
Status:	CLOSED CODE_FIX
Severity:	normal	CC:	drivers_input-devices, florian, jarod
Priority:	P1
Hardware:	All
OS:	Linux
Kernel Version:	2.6.35-rc4	Subsystem:
Regression:	No	Bisected commit-id:
Attachments:	Patch to address a number of issues raised here

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