Re: [PATCH 1/2] rc-core: use the full 32 bits for NEC scancodes

["David Härdeman" <david@hardeman.nu>]

On 2015-05-20 22:24, David Härdeman wrote:
> On Thu, May 14, 2015 at 05:57:39PM -0300, Mauro Carvalho Chehab wrote:
>> Em Mon, 06 Apr 2015 13:23:08 +0200
>> David Härdeman <david@hardeman.nu> escreveu:
....
>>> +static inline enum rc_type guess_protocol(struct rc_dev *rdev)
>>> +{
>>> +	struct rc_map *rc_map = &rdev->rc_map;
>>> +
>>> +	if (hweight64(rdev->enabled_protocols) == 1)
>>> +		return rc_bitmap_to_type(rdev->enabled_protocols);
>>> +	else if (hweight64(rdev->allowed_protocols) == 1)
>>> +		return rc_bitmap_to_type(rdev->allowed_protocols);
>>> +	else
>>> +		return rc_map->rc_type;
>>> +}
> 
> ^^^^
> This function is the most important one to understand in order to
> understand how the heuristics work...
> 
>>> +
>>> +/**
>>> + * to_nec32() - helper function to try to convert misc NEC scancodes 
>>> to NEC32
>>> + * @orig:	original scancode
>>> + * @return:	NEC32 scancode
>>> + *
>>> + * This helper routine is used to provide backwards compatibility.
>>> + */
>>> +static u64 to_nec32(u64 orig)
>>> +{
>>> +	u8 b3 = (u8)(orig >> 16);
>>> +	u8 b2 = (u8)(orig >>  8);
>>> +	u8 b1 = (u8)(orig >>  0);
>>> +
>>> +	if (orig <= 0xffff)
>>> +		/* Plain old NEC */
>>> +		return b2 << 24 | ((u8)~b2) << 16 |  b1 << 8 | ((u8)~b1);
>>> +	else if (orig <= 0xffffff)
>>> +		/* NEC extended */
>>> +		return b3 << 24 | b2 << 16 |  b1 << 8 | ((u8)~b1);
>>> +	else
>>> +		/* NEC32 */
>>> +		return orig;
>>> +}
>>> +
>>> +/**
>>>   * ir_setkeycode() - set a keycode in the scancode->keycode table
>>>   * @idev:	the struct input_dev device descriptor
>>>   * @scancode:	the desired scancode
>>> @@ -349,6 +392,9 @@ static int ir_setkeycode(struct input_dev *idev,
>>>  		if (retval)
>>>  			goto out;
>>> 
>>> +		if (guess_protocol(rdev) == 0
>>> +			scancode = to_nec32(scancode);
>> 
>> This function can be called from userspace. I can't see how this would 
>> do
>> the right thing if more than one protocol is enabled.
>> 
>>> +
>>>  		index = ir_establish_scancode(rdev, rc_map, scancode, true);
>>>  		if (index >= rc_map->len) {
>>>  			retval = -ENOMEM;
>>> @@ -389,7 +435,10 @@ static int ir_setkeytable(struct rc_dev *dev,
>>> 
>>>  	for (i = 0; i < from->size; i++) {
>>>  		index = ir_establish_scancode(dev, rc_map,
>>> -					      from->scan[i].scancode, false);
>>> +					      from->rc_type == RC_TYPE_NEC ?
>>> +					      to_nec32(from->scan[i].scancode) :
>>> +					      from->scan[i].scancode,
>>> +					      false);
>>>  		if (index >= rc_map->len) {
>>>  			rc = -ENOMEM;
>>>  			break;
>>> @@ -463,6 +512,8 @@ static int ir_getkeycode(struct input_dev *idev,
>>>  		if (retval)
>>>  			goto out;
>>> 
>>> +		if (guess_protocol(rdev) == RC_TYPE_NEC)
>>> +			scancode = to_nec32(scancode);
>> 
>> This also can be called from userspace. It should not return different
>> scancodes for the same mapping if just NEC is enabled or if more 
>> protocols
>> are enabled.
> 
> There is no way to do this in a 100% backwards compatible way, that's
> why the patch description uses the word "heuristics".
> 
> I've tried different approaches (such as introducing and using a
> kernel-internal RC_TYPE_ANY protocol for legacy ioctl() calls) but none
> of them solve the problem 100%.
> 
> The current API is also broken, but in a different way. If you set a
> scancode <-> keycode mapping right now using the current ioctl()s, you
> can get different results with the exact same mapping but with 
> different
> RX hardware (which defeats the whole idea of having a kernel API for
> remote controls...) even though there is enough information to do the
> right thing (one example is already given in the patch comments)...that
> is BAD.
> 
> The heuristics can also get it wrong...in slightly different situations
> (e.g. if you load a hardware driver that supports nec and rc-5, but has
> a rc-5 default keymap, then enable both nec and rc-5 from userspace, 
> and
> finally set keymap entries using nec scancodes...in which case they'll
> be interpreted as rc-5 keymap scancodes).
> 
> So, we trade one kind of breakage for another...but the alternative 
> kind
> that I'm proposing here at least paves the way for the updated ioctls
> which solve the ambiguity.
> 
> And distributions can make sure to ship updated userspace tools 
> together
> with an updated kernel (configuration tool updates together with kernel
> updates is one of those "special" cases that e.g. LWN covers every now
> and then).
> 
> I'm not saying the situation is ideal. But at least I'm trying to fix 
> it
> once and for all.
> 
> Do you have a better solution in mind other than to simply keep 
> throwing
> away all protocol information and ignoring scancode overlaps and
> inconsistencies?

It wasn't a rhetorical question...this is an issue that needs to be 
fixed one way or another...do you have a better solution in mind?