Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[David Härdeman]

On Sun, Jun 11, 2017 at 05:17:40PM +0100, Sean Young wrote:
>On Sat, Apr 29, 2017 at 10:44:58AM +0200, David Härdeman wrote:
>> >This can be implemented without breaking userspace.
>> 
>> How?
>
>The current keymaps we have do not specify the protocol variant, only
>the protocol (rc6 vs rc6-mce). So to support this, we have to be able
>to specify multiple protocols at the same time. So I think the protocol
>should be a bitmask.
>
>Also, in your example you re-used RC_TYPE_OTHER to match any protocol;
>I don't think that is a good solution since there are already keymaps
>which use other.
>
>So if we have an "struct rc_scancode" which looks like:
>
>struct rc_scancode {
>   u64 protocol;
>   u64 scancode;
>};
>
>Then if the keymap protocol is rc6, ir-keytable should set the protocol
>to RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | RC_BIT_RC6_6A_32
> | RC_BIT_RC6_MCE.
>
>If the old ioctl is used, then the protocol should be set to RC_BIT_ALL.
>
>I can't think of anything what would break with this scheme.

I've tried coding up that solution before and the problem is that it'll
still require heuristics in the presence of a mix of old and new style
ioctl():s.

For example:

old_ioctl_set(0x1234 = KEY_A);
new_ioctl_set(PROTOCOL_NEC, 0x1234 = KEY_B);
new_ioctl_set(PROTOCOL_JVC, 0x1234 = KEY_C);
old_ioctl_get(0x1234) = ?
old_ioctl_set(0x1234 = KEY_D);
new_ioctl_get(PROTOCOL_NEC, 0x1234) = ?

-- 
David Härdeman

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[Sean Young]

On Sat, Apr 29, 2017 at 10:44:58AM +0200, David Härdeman wrote:
> >This can be implemented without breaking userspace.
> 
> How?

The current keymaps we have do not specify the protocol variant, only
the protocol (rc6 vs rc6-mce). So to support this, we have to be able
to specify multiple protocols at the same time. So I think the protocol
should be a bitmask.

Also, in your example you re-used RC_TYPE_OTHER to match any protocol;
I don't think that is a good solution since there are already keymaps
which use other.

So if we have an "struct rc_scancode" which looks like:

struct rc_scancode {
u64 protocol;
u64 scancode;
};

Then if the keymap protocol is rc6, ir-keytable should set the protocol
to RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | RC_BIT_RC6_6A_32
 | RC_BIT_RC6_MCE.

If the old ioctl is used, then the protocol should be set to RC_BIT_ALL.

I can't think of anything what would break with this scheme.

Thanks
Sean

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[David Härdeman]

On Fri, Apr 28, 2017 at 08:42:13PM +0100, Sean Young wrote:
>On Fri, Apr 28, 2017 at 06:59:11PM +0200, David Härdeman wrote:
>> On Fri, Apr 28, 2017 at 08:31:33AM -0300, Mauro Carvalho Chehab wrote:
>> >Em Thu, 27 Apr 2017 22:34:23 +0200
>> >David Härdeman  escreveu:
>> ...
>> >> This patch changes how the "input_keymap_entry" struct is interpreted
>> >> by rc-core by casting it to "rc_keymap_entry":
>> >> 
>> ...
>> >
>> >Nack.
>> 
>> That's not a very constructive approach. If you have a better approach
>> in mind I'm all ears. Because you're surely not suggesting that we stay
>> with the current protocol-less approach forever?
>
>Well, what problem are we trying to solve actually?

I'm not sure what the confusion is? Last time around we discussed this
there seemed to be general agreement that protocol information is
useful?

>Looking at the keymaps we have already, there are many scancodes which
>overlap and only a few of them use a different protocol. So having this
>feature will not suddenly make it possible to load all our keymaps, it
>will just make it possible to simultaneously load a few more.

That's not really the point, overlaps in scancode && protocol cannot be
distinguished. But overlaps in scancode can be. I have remotes which
overlap in scancode even though they have different protocols.

>> 
>> That's a gross oversimplification.
>
>This can be implemented without breaking userspace.

How?

-- 
David Härdeman

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[Sean Young]

On Fri, Apr 28, 2017 at 06:59:11PM +0200, David Härdeman wrote:
> On Fri, Apr 28, 2017 at 08:31:33AM -0300, Mauro Carvalho Chehab wrote:
> >Em Thu, 27 Apr 2017 22:34:23 +0200
> >David Härdeman  escreveu:
> ...
> >> This patch changes how the "input_keymap_entry" struct is interpreted
> >> by rc-core by casting it to "rc_keymap_entry":
> >> 
> >> struct rc_scancode {
> >>__u16 protocol;
> >>__u16 reserved[3];
> >>__u64 scancode;
> >> }
> >> 
> >> struct rc_keymap_entry {
> >>__u8  flags;
> >>__u8  len;
> >>__u16 index;
> >>__u32 keycode;
> >>union {
> >>struct rc_scancode rc;
> >>__u8 raw[32];
> >>};
> >> };
> >> 
> ...
> >
> >Nack.
> 
> That's not a very constructive approach. If you have a better approach
> in mind I'm all ears. Because you're surely not suggesting that we stay
> with the current protocol-less approach forever?

Well, what problem are we trying to solve actually?

Looking at the keymaps we have already, there are many scancodes which
overlap and only a few of them use a different protocol. So having this
feature will not suddenly make it possible to load all our keymaps, it
will just make it possible to simultaneously load a few more.

> >No userspace breakages are allowed.
> 
> That's a gross oversimplification.

This can be implemented without breaking userspace.


Sean

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[David Härdeman]

On Fri, Apr 28, 2017 at 08:31:33AM -0300, Mauro Carvalho Chehab wrote:
>Em Thu, 27 Apr 2017 22:34:23 +0200
>David Härdeman  escreveu:
...
>> This patch changes how the "input_keymap_entry" struct is interpreted
>> by rc-core by casting it to "rc_keymap_entry":
>> 
>> struct rc_scancode {
>>  __u16 protocol;
>>  __u16 reserved[3];
>>  __u64 scancode;
>> }
>> 
>> struct rc_keymap_entry {
>>  __u8  flags;
>>  __u8  len;
>>  __u16 index;
>>  __u32 keycode;
>>  union {
>>  struct rc_scancode rc;
>>  __u8 raw[32];
>>  };
>> };
>> 
...
>
>Nack.

That's not a very constructive approach. If you have a better approach
in mind I'm all ears. Because you're surely not suggesting that we stay
with the current protocol-less approach forever?

>No userspace breakages are allowed.

That's a gross oversimplification. A cursory glance at the linux-api
mailing list shows plenty of examples of changes that might not be 100%
backwards-compatible. Here's an example:
http://marc.info/?l=linux-fsdevel=149089166918069

That's the kind of discussion we need to have - i.e. the best way to go
about this and to minimize the damage to userspace. In that vein, I'll
post an alternative approach shortly as the basis for further
discussion.

>There's no way to warrant that
>ir-keytable version is compatible with a certain Kernel version.

I know. But we know when an ioctl() is made whether it is a
protocol-aware one or not.

-- 
David Härdeman

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[David Härdeman]

On Fri, Apr 28, 2017 at 12:40:53PM +0100, Sean Young wrote:
>On Thu, Apr 27, 2017 at 10:34:23PM +0200, David Härdeman wrote:
...
>> This patch changes how the "input_keymap_entry" struct is interpreted
>> by rc-core by casting it to "rc_keymap_entry":
>> 
>> struct rc_scancode {
>>  __u16 protocol;
>>  __u16 reserved[3];
>>  __u64 scancode;
>> }
>> 
>> struct rc_keymap_entry {
>>  __u8  flags;
>>  __u8  len;
>>  __u16 index;
>>  __u32 keycode;
>>  union {
>>  struct rc_scancode rc;
>>  __u8 raw[32];
>>  };
>> };
>> 
>> The u64 scancode member is large enough for all current protocols and it
>> would be possible to extend it in the future should it be necessary for
>> some exotic protocol.
>> 
>> The main advantage with this change is that the protocol is made explicit,
>> which means that we're not throwing away data (the protocol type).
>> 
>> This also means that struct rc_map no longer hardcodes the protocol, meaning
>> that keytables with mixed entries are possible.
>> 
>> Heuristics are also added to hopefully do the right thing with older
>> ioctls in order to preserve backwards compatibility.
>
>The current ioctls do not provide any protocol information, so they should
>continue to match any protocol. Those heuristics aren't good enough.
>
>Another way of doing is to have a bitmask of protocols, and default to
>RC_BIT_ALL for current ioctls.

I've been mulling that approach as well, but slightly different. My
alternative approach is based on repurposing RC_TYPE_UNKNOWN as a kind
of catch-all which will match any scancode. I'll post a patch showing
the alternative approach straight away.

>> Note that the heuristics are not 100% guaranteed to get things right.
>> That is unavoidable since the protocol information simply isn't there
>> when userspace calls the previous ioctl() types.
>> 
>> However, that is somewhat mitigated by the fact that the "only"
>> userspace binary which might need to change is ir-keytable. Userspace
>> programs which simply consume input events (i.e. the vast majority)
>> won't have to change.
>
>For this to be accepted we would need ir-keytable changes too so it can
>be tested.

I know. But I'll postpone those patches until we have more of a
consensus on the right approach to take.

-- 
David Härdeman

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[Sean Young]

On Thu, Apr 27, 2017 at 10:34:23PM +0200, David Härdeman wrote:
> It is currently impossible to distinguish between scancodes which have
> been generated using different protocols (and scancodes can, and will,
> overlap).
> 
> For example:
> RC5 message to address 0x00, command 0x03 has scancode 0x0503
> JVC message to address 0x00, command 0x03 has scancode 0x0503
> 
> It is only possible to distinguish (and parse) scancodes by known the
> scancode *and* the protocol.
> 
> Setting and getting keycodes in the input subsystem used to be done via
> the EVIOC[GS]KEYCODE ioctl and "unsigned int[2]" (one int for scancode
> and one for the keycode).
> 
> The interface has now been extended to use the EVIOC[GS]KEYCODE_V2 ioctl
> which uses the following struct:
> 
> struct input_keymap_entry {
>   __u8  flags;
>   __u8  len;
>   __u16 index;
>   __u32 keycode;
>   __u8  scancode[32];
> };
> 
> (scancode can of course be even bigger, thanks to the len member).
> 
> This patch changes how the "input_keymap_entry" struct is interpreted
> by rc-core by casting it to "rc_keymap_entry":
> 
> struct rc_scancode {
>   __u16 protocol;
>   __u16 reserved[3];
>   __u64 scancode;
> }
> 
> struct rc_keymap_entry {
>   __u8  flags;
>   __u8  len;
>   __u16 index;
>   __u32 keycode;
>   union {
>   struct rc_scancode rc;
>   __u8 raw[32];
>   };
> };
> 
> The u64 scancode member is large enough for all current protocols and it
> would be possible to extend it in the future should it be necessary for
> some exotic protocol.
> 
> The main advantage with this change is that the protocol is made explicit,
> which means that we're not throwing away data (the protocol type).
> 
> This also means that struct rc_map no longer hardcodes the protocol, meaning
> that keytables with mixed entries are possible.
> 
> Heuristics are also added to hopefully do the right thing with older
> ioctls in order to preserve backwards compatibility.

The current ioctls do not provide any protocol information, so they should
continue to match any protocol. Those heuristics aren't good enough.

Another way of doing is to have a bitmask of protocols, and default to
RC_BIT_ALL for current ioctls.
 
> Note that the heuristics are not 100% guaranteed to get things right.
> That is unavoidable since the protocol information simply isn't there
> when userspace calls the previous ioctl() types.
> 
> However, that is somewhat mitigated by the fact that the "only"
> userspace binary which might need to change is ir-keytable. Userspace
> programs which simply consume input events (i.e. the vast majority)
> won't have to change.

For this to be accepted we would need ir-keytable changes too so it can
be tested.

> 
> Signed-off-by: David Härdeman 
> ---
>  drivers/media/rc/ati_remote.c |1 
>  drivers/media/rc/imon.c   |7 +-
>  drivers/media/rc/rc-main.c|  188 
> +
>  include/media/rc-core.h   |   26 +-
>  include/media/rc-map.h|5 +
>  5 files changed, 165 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
> index 9cf3e69de16a..cc81b938471f 100644
> --- a/drivers/media/rc/ati_remote.c
> +++ b/drivers/media/rc/ati_remote.c
> @@ -546,6 +546,7 @@ static void ati_remote_input_report(struct urb *urb)
>* set, assume this is a scrollwheel up/down event.
>*/
>   wheel_keycode = rc_g_keycode_from_table(ati_remote->rdev,
> + RC_TYPE_OTHER,
>   scancode & 0x78);
>  
>   if (wheel_keycode == KEY_RESERVED) {
> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> index 3489010601b5..c724a1a5e9cd 100644
> --- a/drivers/media/rc/imon.c
> +++ b/drivers/media/rc/imon.c
> @@ -1274,14 +1274,15 @@ static u32 imon_remote_key_lookup(struct imon_context 
> *ictx, u32 scancode)
>   bool is_release_code = false;
>  
>   /* Look for the initial press of a button */
> - keycode = rc_g_keycode_from_table(ictx->rdev, scancode);
> + keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type, scancode);
>   ictx->rc_toggle = 0x0;
>   ictx->rc_scancode = scancode;
>  
>   /* Look for the release of a button */
>   if (keycode == KEY_RESERVED) {
>   release = scancode & ~0x4000;
> - keycode = rc_g_keycode_from_table(ictx->rdev, release);
> + keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type,
> +   release);
>   if (keycode != KEY_RESERVED)
>   is_release_code = true;
>   }
> @@ -1310,7 +1311,7 @@ static u32 imon_mce_key_lookup(struct imon_context 
> *ictx, u32 scancode)
>   scancode = scancode | MCE_KEY_MASK | 

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[Mauro Carvalho Chehab]

Em Thu, 27 Apr 2017 22:34:23 +0200
David Härdeman  escreveu:

> It is currently impossible to distinguish between scancodes which have
> been generated using different protocols (and scancodes can, and will,
> overlap).
> 
> For example:
> RC5 message to address 0x00, command 0x03 has scancode 0x0503
> JVC message to address 0x00, command 0x03 has scancode 0x0503
> 
> It is only possible to distinguish (and parse) scancodes by known the
> scancode *and* the protocol.
> 
> Setting and getting keycodes in the input subsystem used to be done via
> the EVIOC[GS]KEYCODE ioctl and "unsigned int[2]" (one int for scancode
> and one for the keycode).
> 
> The interface has now been extended to use the EVIOC[GS]KEYCODE_V2 ioctl
> which uses the following struct:
> 
> struct input_keymap_entry {
>   __u8  flags;
>   __u8  len;
>   __u16 index;
>   __u32 keycode;
>   __u8  scancode[32];
> };
> 
> (scancode can of course be even bigger, thanks to the len member).
> 
> This patch changes how the "input_keymap_entry" struct is interpreted
> by rc-core by casting it to "rc_keymap_entry":
> 
> struct rc_scancode {
>   __u16 protocol;
>   __u16 reserved[3];
>   __u64 scancode;
> }
> 
> struct rc_keymap_entry {
>   __u8  flags;
>   __u8  len;
>   __u16 index;
>   __u32 keycode;
>   union {
>   struct rc_scancode rc;
>   __u8 raw[32];
>   };
> };
> 
> The u64 scancode member is large enough for all current protocols and it
> would be possible to extend it in the future should it be necessary for
> some exotic protocol.
> 
> The main advantage with this change is that the protocol is made explicit,
> which means that we're not throwing away data (the protocol type).
> 
> This also means that struct rc_map no longer hardcodes the protocol, meaning
> that keytables with mixed entries are possible.
> 
> Heuristics are also added to hopefully do the right thing with older
> ioctls in order to preserve backwards compatibility.
> 
> Note that the heuristics are not 100% guaranteed to get things right.
> That is unavoidable since the protocol information simply isn't there
> when userspace calls the previous ioctl() types.
> 
> However, that is somewhat mitigated by the fact that the "only"
> userspace binary which might need to change is ir-keytable. Userspace
> programs which simply consume input events (i.e. the vast majority)
> won't have to change.

Nack.

No userspace breakages are allowed. There's no way to warrant that
ir-keytable version is compatible with a certain Kernel version.

Thanks,
Mauro