rc-core - protocol in keytables

The device core infrastructure is based on the presumption that once a driver calls device_add(), it must be ready to accept userspace interaction.

This requires splitting rc_setup_rx_device() into two functions and reorganizing rc_register_device() so that as much work as possible is performed before calling device_add().

Signed-off-by David Härdeman <[email protected]>

---
 drivers/media/rc/rc-core-priv.h |    2 +
 drivers/media/rc/rc-ir-raw.c    |   34 ++++++++++++------
 drivers/media/rc/rc-main.c      |   75 +++++++++++++++++++++++++--------------
 3 files changed, 73 insertions(+), 38 deletions(-)

drivers/media/rc/rc-core-priv.h +2 -0

index 0455b273c2fc..b3e7cac2c3ee 100644
@@ -263,7 +263,9 @@ int ir_raw_gen_pl(struct ir_raw_event **ev, unsigned int max,
  * Routines from rc-raw.c to be used internally and by decoders
  */
 u64 ir_raw_get_allowed_protocols(void);
+int ir_raw_event_prepare(struct rc_dev *dev);
 int ir_raw_event_register(struct rc_dev *dev);
+void ir_raw_event_free(struct rc_dev *dev);
 void ir_raw_event_unregister(struct rc_dev *dev);
 int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler);
 void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler);

drivers/media/rc/rc-ir-raw.c +23 -11

index 90f66dc7c0d7..ae7785c4fbe7 100644
@@ -486,14 +486,18 @@ EXPORT_SYMBOL(ir_raw_encode_scancode);
 /*
  * Used to (un)register raw event clients
  */
-int ir_raw_event_register(struct rc_dev *dev)
+int ir_raw_event_prepare(struct rc_dev *dev)
 {
-	int rc;
-	struct ir_raw_handler *handler;
+	static bool raw_init; /* 'false' default value, raw decoders loaded? */
 
 	if (!dev)
 		return -EINVAL;
 
+	if (!raw_init) {
+		request_module("ir-lirc-codec");
+		raw_init = true;
+	}
+
 	dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL);
 	if (!dev->raw)
 		return -ENOMEM;

@@ -502,6 +506,13 @@ int ir_raw_event_register(struct rc_dev *dev)
 	dev->change_protocol = change_protocol;
 	INIT_KFIFO(dev->raw->kfifo);
 
+	return 0;
+}
+
+int ir_raw_event_register(struct rc_dev *dev)
+{
+	struct ir_raw_handler *handler;
+
 	/*
 	 * raw transmitters do not need any event registration
 	 * because the event is coming from userspace

@@ -510,10 +521,8 @@ int ir_raw_event_register(struct rc_dev *dev)
 		dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
 					       "rc%u", dev->minor);
 
-		if (IS_ERR(dev->raw->thread)) {
-			rc = PTR_ERR(dev->raw->thread);
-			goto out;
-		}
+		if (IS_ERR(dev->raw->thread))
+			return PTR_ERR(dev->raw->thread);
 	}
 
 	mutex_lock(&ir_raw_handler_lock);

@@ -524,11 +533,15 @@ int ir_raw_event_register(struct rc_dev *dev)
 	mutex_unlock(&ir_raw_handler_lock);
 
 	return 0;
+}
+
+void ir_raw_event_free(struct rc_dev *dev)
+{
+	if (!dev)
+		return;
 
-out:
 	kfree(dev->raw);
 	dev->raw = NULL;
-	return rc;
 }
 
 void ir_raw_event_unregister(struct rc_dev *dev)

@@ -547,8 +560,7 @@ void ir_raw_event_unregister(struct rc_dev *dev)
 			handler->raw_unregister(dev);
 	mutex_unlock(&ir_raw_handler_lock);
 
-	kfree(dev->raw);
-	dev->raw = NULL;
+	ir_raw_event_free(dev);
 }
 
 /*

drivers/media/rc/rc-main.c +48 -27

index 802e559cc30e..44189366f232 100644
@@ -1663,7 +1663,7 @@ struct rc_dev *devm_rc_allocate_device(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_rc_allocate_device);
 
-static int rc_setup_rx_device(struct rc_dev *dev)
+static int rc_prepare_rx_device(struct rc_dev *dev)
 {
 	int rc;
 	struct rc_map *rc_map;

@@ -1708,10 +1708,22 @@ static int rc_setup_rx_device(struct rc_dev *dev)
 	dev->input_dev->phys = dev->input_phys;
 	dev->input_dev->name = dev->input_name;
 
+	return 0;
+
+out_table:
+	ir_free_table(&dev->rc_map);
+
+	return rc;
+}
+
+static int rc_setup_rx_device(struct rc_dev *dev)
+{
+	int rc;
+
 	/* rc_open will be called here */
 	rc = input_register_device(dev->input_dev);
 	if (rc)
-		goto out_table;
+		return rc;
 
 	/*
 	 * Default delay of 250ms is too short for some protocols, especially

@@ -1729,27 +1741,23 @@ static int rc_setup_rx_device(struct rc_dev *dev)
 	dev->input_dev->rep[REP_PERIOD] = 125;
 
 	return 0;
-
-out_table:
-	ir_free_table(&dev->rc_map);
-
-	return rc;
 }
 
 static void rc_free_rx_device(struct rc_dev *dev)
 {
-	if (!dev || dev->driver_type == RC_DRIVER_IR_RAW_TX)
+	if (!dev)
 		return;
 
-	ir_free_table(&dev->rc_map);
+	if (dev->input_dev) {
+		input_unregister_device(dev->input_dev);
+		dev->input_dev = NULL;
+	}
 
-	input_unregister_device(dev->input_dev);
-	dev->input_dev = NULL;
+	ir_free_table(&dev->rc_map);
 }
 
 int rc_register_device(struct rc_dev *dev)
 {
-	static bool raw_init; /* 'false' default value, raw decoders loaded? */
 	const char *path;
 	int attr = 0;
 	int minor;

@@ -1776,30 +1784,39 @@ int rc_register_device(struct rc_dev *dev)
 		dev->sysfs_groups[attr++] = &rc_dev_wakeup_filter_attr_grp;
 	dev->sysfs_groups[attr++] = NULL;
 
+	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
+		rc = rc_prepare_rx_device(dev);
+		if (rc)
+			goto out_minor;
+	}
+
+	if (dev->driver_type == RC_DRIVER_IR_RAW ||
+	    dev->driver_type == RC_DRIVER_IR_RAW_TX) {
+		rc = ir_raw_event_prepare(dev);
+		if (rc < 0)
+			goto out_rx_free;
+	}
+
 	rc = device_add(&dev->dev);
 	if (rc)
-		goto out_unlock;
+		goto out_raw;
 
 	path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
 	dev_info(&dev->dev, "%s as %s\n",
 		dev->input_name ?: "Unspecified device", path ?: "N/A");
 	kfree(path);
 
+	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
+		rc = rc_setup_rx_device(dev);
+		if (rc)
+			goto out_dev;
+	}
+
 	if (dev->driver_type == RC_DRIVER_IR_RAW ||
 	    dev->driver_type == RC_DRIVER_IR_RAW_TX) {
-		if (!raw_init) {
-			request_module_nowait("ir-lirc-codec");
-			raw_init = true;
-		}
 		rc = ir_raw_event_register(dev);
 		if (rc < 0)
-			goto out_dev;
-	}
-
-	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
-		rc = rc_setup_rx_device(dev);
-		if (rc)
-			goto out_raw;
+			goto out_rx;
 	}
 
 	/* Allow the RC sysfs nodes to be accessible */

@@ -1811,11 +1828,15 @@ int rc_register_device(struct rc_dev *dev)
 
 	return 0;
 
-out_raw:
-	ir_raw_event_unregister(dev);
+out_rx:
+	rc_free_rx_device(dev);
 out_dev:
 	device_del(&dev->dev);
-out_unlock:
+out_raw:
+	ir_raw_event_free(dev);
+out_rx_free:
+	ir_free_table(&dev->rc_map);
+out_minor:
 	ida_simple_remove(&rc_ida, minor);
 	return rc;
 }

Quoted 8 lines · David Härdeman

The device core infrastructure is based on the presumption that once a driver calls device_add(), it must be ready to accept userspace interaction.

This requires splitting rc_setup_rx_device() into two functions and reorganizing rc_register_device() so that as much work as possible is performed before calling device_add().

With this patch applied, I'm no longer getting any scancodes from my rc devices.

David, please can you test your patches before submitting. I have to go over them meticulously because I cannot assume you've tested them.

Quoted 10 lines · Sean Young

Quoted 8 lines · David Härdeman

The device core infrastructure is based on the presumption that once a driver calls device_add(), it must be ready to accept userspace interaction.

This requires splitting rc_setup_rx_device() into two functions and reorganizing rc_register_device() so that as much work as possible is performed before calling device_add().

Quoted 5 lines

With this patch applied, I'm no longer getting any scancodes from my rc devices.

David, please can you test your patches before submitting. I have to go over them meticulously because I cannot assume you've tested them.

I did test this patch and I just redid the tests, both with rc-loopback and with a mceusb receiver. I'm seeing scancodes on the input device as well as pulse-space readings on the lirc device in both tests.

I did the tests with only this patch applied and the lirc-use-after-free (v3). What hardware did you test with?

Meanwhile, I'll try rebasing my patches to the latest version of the media-master git tree and test again.

Quoted 24 lines · David Härdeman

Quoted 10 lines · Sean Young

Quoted 8 lines · David Härdeman

The device core infrastructure is based on the presumption that once a driver calls device_add(), it must be ready to accept userspace interaction.

This requires splitting rc_setup_rx_device() into two functions and reorganizing rc_register_device() so that as much work as possible is performed before calling device_add().

Quoted 5 lines

With this patch applied, I'm no longer getting any scancodes from my rc devices.

David, please can you test your patches before submitting. I have to go over them meticulously because I cannot assume you've tested them.

I did test this patch and I just redid the tests, both with rc-loopback and with a mceusb receiver. I'm seeing scancodes on the input device as well as pulse-space readings on the lirc device in both tests.

I did the tests with only this patch applied and the lirc-use-after-free (v3). What hardware did you test with?

Quoted 2 lines

Meanwhile, I'll try rebasing my patches to the latest version of the media-master git tree and test again.

I rebased the patches onto media-master (commit 3622d3e77ecef090b5111e3c5423313f11711dfa) and tested again. I still can't reproduce the problems you're having :/

Quoted 28 lines · David Härdeman

Quoted 26 lines · David Härdeman

Quoted 10 lines · Sean Young

Quoted 8 lines · David Härdeman

The device core infrastructure is based on the presumption that once a driver calls device_add(), it must be ready to accept userspace interaction.

This requires splitting rc_setup_rx_device() into two functions and reorganizing rc_register_device() so that as much work as possible is performed before calling device_add().

Quoted 5 lines

With this patch applied, I'm no longer getting any scancodes from my rc devices.

David, please can you test your patches before submitting. I have to go over them meticulously because I cannot assume you've tested them.

I did test this patch and I just redid the tests, both with rc-loopback and with a mceusb receiver. I'm seeing scancodes on the input device as well as pulse-space readings on the lirc device in both tests.

I did the tests with only this patch applied and the lirc-use-after-free (v3). What hardware did you test with?

Meanwhile, I'll try rebasing my patches to the latest version of the media-master git tree and test again.

Quoted 3 lines

I rebased the patches onto media-master (commit 3622d3e77ecef090b5111e3c5423313f11711dfa) and tested again. I still can't reproduce the problems you're having :/

The protocol is not set properly. In rc_prepare_rx_device(), dev->change_protocol() is call if not null. At this point it still is null, since it will only be set up in ir_raw_event_prepare(), which is called afterwards.

Presumably you have udev set up to execute ir-keytable, which sets the protocol up (again).

There is another problem with your patches, you've introduced a race condition. When device_add() is called, the protocol is not set up yet. So anyone reading the protocols sysfs attribute early enough will get false information. Is it not possible to make sure that it is all setup correctly at the point of device_add()?

Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core and the nec decoder without any loss of functionality. At the same time it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and removes lots of duplication (as you can see from the patch, the same NEC disambiguation logic is contained in several different drivers).

Using NEC32 also removes ambiguity. For example, consider these two NEC messages:
NEC16 message to address 0x05, command 0x03
NEC24 message to address 0x0005, command 0x03

They'll both have scancode 0x00000503, and there's no way to tell which message was received.

In order to maintain backwards compatibility, some heuristics are added in rc-main.c to convert scancodes to NEC32 as necessary when userspace adds entries to the keytable using the regular input ioctls. These heuristics are essentially the same as the ones that are currently in drivers/media/rc/img-ir/img-ir-nec.c (which are rendered unecessary with this patch).

Quoted 11 lines · David Härdeman

Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core and the nec decoder without any loss of functionality. At the same time it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and removes lots of duplication (as you can see from the patch, the same NEC disambiguation logic is contained in several different drivers).

Using NEC32 also removes ambiguity. For example, consider these two NEC messages:
NEC16 message to address 0x05, command 0x03
NEC24 message to address 0x0005, command 0x03

Quoted 2 lines

They'll both have scancode 0x00000503, and there's no way to tell which message was received.

It's not ambiguous, the protocol is different (RC_TYPE_NEC vs RC_TYPE_NECX).

Quoted 6 lines

In order to maintain backwards compatibility, some heuristics are added in rc-main.c to convert scancodes to NEC32 as necessary when userspace adds entries to the keytable using the regular input ioctls. These heuristics are essentially the same as the ones that are currently in drivers/media/rc/img-ir/img-ir-nec.c (which are rendered unecessary with this patch).

There are issues with the patch which breaks userspace, as discussed in the previous patch. None of those issues have been addressed.

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 0x00000503 JVC message to address 0x00, command 0x03 has scancode 0x00000503

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.

Em Thu, 27 Apr 2017 22:34:23 +0200
David Härdeman <[email protected]> escreveu:

Quoted 20 lines

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.

No userspace breakages are allowed.

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

Quoted 2 lines · Mauro Carvalho Chehab

Em Thu, 27 Apr 2017 22:34:23 +0200
David Härdeman <[email protected]> escreveu:

...

Quoted 9 lines

Quoted 9 lines

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;
}

Quoted 11 lines

Quoted 11 lines

struct rc_keymap_entry {
	__u8  flags;
	__u8  len;
	__u16 index;
	__u32 keycode;
	union {
		struct rc_scancode rc;
		__u8 raw[32];
	};
};

...

Quoted 2 lines

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?

Quoted 1 line

No userspace breakages are allowed.

That's a gross oversimplification.

Quoted 9 lines · David Härdeman

Quoted 2 lines · Mauro Carvalho Chehab

Em Thu, 27 Apr 2017 22:34:23 +0200
David Härdeman <[email protected]> escreveu:

...

Quoted 3 lines

Quoted 3 lines

This patch changes how the "input_keymap_entry" struct is interpreted by rc-core by casting it to "rc_keymap_entry":

...

Quoted 1 line

Quoted 5 lines

Quoted 1 line

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.

Quoted 2 lines

That's a gross oversimplification.

This can be implemented without breaking userspace.

Quoted 1 line

This can be implemented without breaking userspace.

How?

Quoted 3 lines · David Härdeman

Quoted 1 line

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.

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 0x00000503 JVC message to address 0x00, command 0x03 has scancode 0x00000503

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.

Signed-off-by David Härdeman <[email protected]>

---
 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(-)

drivers/media/rc/ati_remote.c +1 -0

index 9cf3e69de16a..cc81b938471f 100644
@@ -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) {

drivers/media/rc/imon.c +4 -3

index 3489010601b5..c724a1a5e9cd 100644
@@ -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 | MCE_TOGGLE_BIT;
 
 	ictx->rc_scancode = scancode;
-	keycode = rc_g_keycode_from_table(ictx->rdev, scancode);
+	keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type, scancode);
 
 	/* not used in mce mode, but make sure we know its false */
 	ictx->release_code = false;

drivers/media/rc/rc-main.c +135 -53

index 881af208a19a..ad5545301588 100644
@@ -105,7 +105,7 @@ EXPORT_SYMBOL_GPL(rc_map_unregister);
 
 
 static struct rc_map_table empty[] = {
-	{ 0x2a, KEY_COFFEE },
+	{ RC_TYPE_OTHER, 0x2a, KEY_COFFEE },
 };
 
 static struct rc_map_list empty_map = {

@@ -121,7 +121,6 @@ static struct rc_map_list empty_map = {
  * ir_create_table() - initializes a scancode table
  * @rc_map:	the rc_map to initialize
  * @name:	name to assign to the table
- * @rc_type:	ir type to assign to the new table
  * @size:	initial size of the table
  * @return:	zero on success or a negative error code
  *

@@ -129,12 +128,11 @@ static struct rc_map_list empty_map = {
  * memory to hold at least the specified number of elements.
  */
 static int ir_create_table(struct rc_map *rc_map,
-			   const char *name, u64 rc_type, size_t size)
+			   const char *name, size_t size)
 {
 	rc_map->name = kstrdup(name, GFP_KERNEL);
 	if (!rc_map->name)
 		return -ENOMEM;
-	rc_map->rc_type = rc_type;
 	rc_map->alloc = roundup_pow_of_two(size * sizeof(struct rc_map_table));
 	rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
 	rc_map->scan = kmalloc(rc_map->alloc, GFP_KERNEL);

@@ -234,16 +232,20 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
 
 	/* Did the user wish to remove the mapping? */
 	if (new_keycode == KEY_RESERVED || new_keycode == KEY_UNKNOWN) {
-		IR_dprintk(1, "#%d: Deleting scan 0x%04x\n",
-			   index, rc_map->scan[index].scancode);
+		IR_dprintk(1, "#%d: Deleting proto 0x%04x, scan 0x%08llx\n",
+			   index, rc_map->scan[index].protocol,
+			   (unsigned long long)rc_map->scan[index].scancode);
 		rc_map->len--;
 		memmove(&rc_map->scan[index], &rc_map->scan[index+ 1],
 			(rc_map->len - index) * sizeof(struct rc_map_table));
 	} else {
-		IR_dprintk(1, "#%d: %s scan 0x%04x with key 0x%04x\n",
+		IR_dprintk(1, "#%d: %s proto 0x%04x, scan 0x%08llx "
+			   "with key 0x%04x\n",
 			   index,
 			   old_keycode == KEY_RESERVED ? "New" : "Replacing",
-			   rc_map->scan[index].scancode, new_keycode);
+			   rc_map->scan[index].protocol,
+			   (unsigned long long)rc_map->scan[index].scancode,
+			   new_keycode);
 		rc_map->scan[index].keycode = new_keycode;
 		__set_bit(new_keycode, dev->input_dev->keybit);
 	}

@@ -270,9 +272,9 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
  * ir_establish_scancode() - set a keycode in the scancode->keycode table
  * @dev:	the struct rc_dev device descriptor
  * @rc_map:	scancode table to be searched
- * @scancode:	the desired scancode
- * @resize:	controls whether we allowed to resize the table to
- *		accommodate not yet present scancodes
+ * @entry:	the entry to be added to the table
+ * @resize:	controls whether we are allowed to resize the table to
+ *		accomodate not yet present scancodes
  * @return:	index of the mapping containing scancode in question
  *		or -1U in case of failure.
  *

@@ -282,7 +284,7 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
  */
 static unsigned int ir_establish_scancode(struct rc_dev *dev,
 					  struct rc_map *rc_map,
-					  unsigned int scancode,
+					  struct rc_map_table *entry,
 					  bool resize)
 {
 	unsigned int i;

@@ -296,16 +298,27 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
 	 * indicate the valid bits of the scancodes.
 	 */
 	if (dev->scancode_mask)
-		scancode &= dev->scancode_mask;
+		entry->scancode &= dev->scancode_mask;
 
-	/* First check if we already have a mapping for this ir command */
+	/*
+	 * First check if we already have a mapping for this command.
+	 * Note that the keytable is sorted first on protocol and second
+	 * on scancode (lowest to highest).
+	 */
 	for (i = 0; i < rc_map->len; i++) {
-		if (rc_map->scan[i].scancode == scancode)
-			return i;
+		if (rc_map->scan[i].protocol < entry->protocol)
+			continue;
+
+		if (rc_map->scan[i].protocol > entry->protocol)
+			break;
 
-		/* Keytable is sorted from lowest to highest scancode */
-		if (rc_map->scan[i].scancode >= scancode)
+		if (rc_map->scan[i].scancode < entry->scancode)
+			continue;
+
+		if (rc_map->scan[i].scancode > entry->scancode)
 			break;
+
+		return i;
 	}
 
 	/* No previous mapping found, we might need to grow the table */

@@ -318,7 +331,8 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
 	if (i < rc_map->len)
 		memmove(&rc_map->scan[i + 1], &rc_map->scan[i],
 			(rc_map->len - i) * sizeof(struct rc_map_table));
-	rc_map->scan[i].scancode = scancode;
+	rc_map->scan[i].scancode = entry->scancode;
+	rc_map->scan[i].protocol = entry->protocol;
 	rc_map->scan[i].keycode = KEY_RESERVED;
 	rc_map->len++;
 

@@ -340,8 +354,10 @@ static inline enum rc_type guess_protocol(struct rc_dev *rdev)
 		return rc_bitmap_to_type(rdev->enabled_protocols);
 	else if (hweight64(rdev->allowed_protocols) == 1)
 		return rc_bitmap_to_type(rdev->allowed_protocols);
+	else if (rc_map->len > 0)
+		return rc_map->scan[0].protocol;
 	else
-		return rc_map->rc_type;
+		return RC_TYPE_OTHER;
 }
 
 /**

@@ -384,10 +400,12 @@ static int ir_setkeycode(struct input_dev *idev,
 	struct rc_dev *rdev = input_get_drvdata(idev);
 	struct rc_map *rc_map = &rdev->rc_map;
 	unsigned int index;
-	unsigned int scancode;
+	struct rc_map_table entry;
 	int retval = 0;
 	unsigned long flags;
 
+	entry.keycode = ke->keycode;
+
 	spin_lock_irqsave(&rc_map->lock, flags);
 
 	if (ke->flags & INPUT_KEYMAP_BY_INDEX) {

@@ -396,19 +414,42 @@ static int ir_setkeycode(struct input_dev *idev,
 			retval = -EINVAL;
 			goto out;
 		}
-	} else {
+	} else if (ke->len == sizeof(int)) {
+		/* Old EVIOCSKEYCODE[_V2] ioctl */
+		u32 scancode;
 		retval = input_scancode_to_scalar(ke, &scancode);
 		if (retval)
 			goto out;
 
-		if (guess_protocol(rdev) == RC_TYPE_NEC)
-			scancode = to_nec32(scancode);
+		entry.scancode = scancode;
+		entry.protocol = guess_protocol(rdev);
+		if (entry.protocol == RC_TYPE_NEC)
+			entry.scancode = to_nec32(scancode);
 
-		index = ir_establish_scancode(rdev, rc_map, scancode, true);
+		index = ir_establish_scancode(rdev, rc_map, &entry, true);
 		if (index >= rc_map->len) {
 			retval = -ENOMEM;
 			goto out;
 		}
+	} else if (ke->len == sizeof(struct rc_scancode)) {
+		/* New EVIOCSKEYCODE_V2 ioctl */
+		const struct rc_keymap_entry *rke = (struct rc_keymap_entry *)ke;
+		entry.protocol = rke->rc.protocol;
+		entry.scancode = rke->rc.scancode;
+
+		if (rke->rc.reserved[0] || rke->rc.reserved[1] || rke->rc.reserved[2]) {
+			retval = -EINVAL;
+			goto out;
+		}
+
+		index = ir_establish_scancode(rdev, rc_map, &entry, true);
+		if (index >= rc_map->len) {
+			retval = -ENOMEM;
+			goto out;
+		}
+	} else {
+		retval = -EINVAL;
+		goto out;
 	}
 
 	*old_keycode = ir_update_mapping(rdev, rc_map, index, ke->keycode);

@@ -431,11 +472,11 @@ static int ir_setkeytable(struct rc_dev *dev,
 			  const struct rc_map *from)
 {
 	struct rc_map *rc_map = &dev->rc_map;
+	struct rc_map_table entry;
 	unsigned int i, index;
 	int rc;
 
-	rc = ir_create_table(rc_map, from->name,
-			     from->rc_type, from->size);
+	rc = ir_create_table(rc_map, from->name, from->size);
 	if (rc)
 		return rc;
 

@@ -443,18 +484,19 @@ static int ir_setkeytable(struct rc_dev *dev,
 		   rc_map->size, rc_map->alloc);
 
 	for (i = 0; i < from->size; i++) {
-		index = ir_establish_scancode(dev, rc_map,
-					      from->rc_type == RC_TYPE_NEC ?
-					      to_nec32(from->scan[i].scancode) :
-					      from->scan[i].scancode,
-					      false);
+		if (from->rc_type == RC_TYPE_NEC)
+			entry.scancode = to_nec32(from->scan[i].scancode);
+		else
+			entry.scancode = from->scan[i].scancode;
+
+		entry.protocol = from->rc_type;
+		index = ir_establish_scancode(dev, rc_map, &entry, false);
 		if (index >= rc_map->len) {
 			rc = -ENOMEM;
 			break;
 		}
 
-		ir_update_mapping(dev, rc_map, index,
-				  from->scan[i].keycode);
+		ir_update_mapping(dev, rc_map, index, from->scan[i].keycode);
 	}
 
 	if (rc)

@@ -466,6 +508,7 @@ static int ir_setkeytable(struct rc_dev *dev,
 /**
  * ir_lookup_by_scancode() - locate mapping by scancode
  * @rc_map:	the struct rc_map to search
+ * @protocol:	protocol to look for in the table
  * @scancode:	scancode to look for in the table
  * @return:	index in the table, -1U if not found
  *

@@ -473,17 +516,24 @@ static int ir_setkeytable(struct rc_dev *dev,
  * given scancode.
  */
 static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map,
-					  unsigned int scancode)
+					  u16 protocol, u64 scancode)
 {
 	int start = 0;
 	int end = rc_map->len - 1;
 	int mid;
+	struct rc_map_table *m;
 
 	while (start <= end) {
 		mid = (start + end) / 2;
-		if (rc_map->scan[mid].scancode < scancode)
+		m = &rc_map->scan[mid];
+
+		if (m->protocol < protocol)
+			start = mid + 1;
+		else if (m->protocol > protocol)
+			end = mid - 1;
+		else if (m->scancode < scancode)
 			start = mid + 1;
-		else if (rc_map->scan[mid].scancode > scancode)
+		else if (m->scancode > scancode)
 			end = mid - 1;
 		else
 			return mid;

@@ -504,35 +554,60 @@ static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map,
 static int ir_getkeycode(struct input_dev *idev,
 			 struct input_keymap_entry *ke)
 {
+	struct rc_keymap_entry *rke = (struct rc_keymap_entry *)ke;
 	struct rc_dev *rdev = input_get_drvdata(idev);
 	struct rc_map *rc_map = &rdev->rc_map;
 	struct rc_map_table *entry;
 	unsigned long flags;
 	unsigned int index;
-	unsigned int scancode;
 	int retval;
 
 	spin_lock_irqsave(&rc_map->lock, flags);
 
 	if (ke->flags & INPUT_KEYMAP_BY_INDEX) {
 		index = ke->index;
-	} else {
+	} else if (ke->len == sizeof(int)) {
+		/* Legacy EVIOCGKEYCODE ioctl */
+		u32 scancode;
+		u16 protocol;
+
 		retval = input_scancode_to_scalar(ke, &scancode);
 		if (retval)
 			goto out;
 
-		if (guess_protocol(rdev) == RC_TYPE_NEC)
+		protocol = guess_protocol(rdev);
+		if (protocol == RC_TYPE_NEC)
 			scancode = to_nec32(scancode);
-		index = ir_lookup_by_scancode(rc_map, scancode);
+
+		index = ir_lookup_by_scancode(rc_map, protocol, scancode);
+
+	} else if (ke->len == sizeof(struct rc_scancode)) {
+		/* New EVIOCGKEYCODE_V2 ioctl */
+		if (rke->rc.reserved[0] || rke->rc.reserved[1] || rke->rc.reserved[2]) {
+			retval = -EINVAL;
+			goto out;
+		}
+
+		index = ir_lookup_by_scancode(rc_map,
+					      rke->rc.protocol, rke->rc.scancode);
+
+	} else {
+		retval = -EINVAL;
+		goto out;
 	}
 
 	if (index < rc_map->len) {
 		entry = &rc_map->scan[index];
-
 		ke->index = index;
 		ke->keycode = entry->keycode;
-		ke->len = sizeof(entry->scancode);
-		memcpy(ke->scancode, &entry->scancode, sizeof(entry->scancode));
+		if (ke->len == sizeof(int)) {
+			u32 scancode = entry->scancode;
+			memcpy(ke->scancode, &scancode, sizeof(scancode));
+		} else {
+			ke->len = sizeof(struct rc_scancode);
+			rke->rc.protocol = entry->protocol;
+			rke->rc.scancode = entry->scancode;
+		}
 
 	} else if (!(ke->flags & INPUT_KEYMAP_BY_INDEX)) {
 		/*

@@ -557,6 +632,7 @@ static int ir_getkeycode(struct input_dev *idev,
 /**
  * rc_g_keycode_from_table() - gets the keycode that corresponds to a scancode
  * @dev:	the struct rc_dev descriptor of the device
+ * @protocol:	the protocol to look for
  * @scancode:	the scancode to look for
  * @return:	the corresponding keycode, or KEY_RESERVED
  *

@@ -564,7 +640,8 @@ static int ir_getkeycode(struct input_dev *idev,
  * keycode. Normally it should not be used since drivers should have no
  * interest in keycodes.
  */
-u32 rc_g_keycode_from_table(struct rc_dev *dev, u32 scancode)
+u32 rc_g_keycode_from_table(struct rc_dev *dev,
+			    enum rc_type protocol, u64 scancode)
 {
 	struct rc_map *rc_map = &dev->rc_map;
 	unsigned int keycode;

@@ -573,15 +650,16 @@ u32 rc_g_keycode_from_table(struct rc_dev *dev, u32 scancode)
 
 	spin_lock_irqsave(&rc_map->lock, flags);
 
-	index = ir_lookup_by_scancode(rc_map, scancode);
+	index = ir_lookup_by_scancode(rc_map, protocol, scancode);
 	keycode = index < rc_map->len ?
 			rc_map->scan[index].keycode : KEY_RESERVED;
 
 	spin_unlock_irqrestore(&rc_map->lock, flags);
 
 	if (keycode != KEY_RESERVED)
-		IR_dprintk(1, "%s: scancode 0x%04x keycode 0x%02x\n",
-			   dev->input_name, scancode, keycode);
+		IR_dprintk(1, "%s: protocol 0x%04x scancode 0x%08llx keycode 0x%02x\n",
+			   dev->input_name, protocol,
+			   (unsigned long long)scancode, keycode);
 
 	return keycode;
 }

@@ -733,10 +811,11 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_type protocol,
  * This routine is used to signal that a key has been pressed on the
  * remote control.
  */
-void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 toggle)
+void rc_keydown(struct rc_dev *dev, enum rc_type protocol,
+		u64 scancode, u8 toggle)
 {
 	unsigned long flags;
-	u32 keycode = rc_g_keycode_from_table(dev, scancode);
+	u32 keycode = rc_g_keycode_from_table(dev, protocol, scancode);
 
 	spin_lock_irqsave(&dev->keylock, flags);
 	ir_do_keydown(dev, protocol, scancode, keycode, toggle);

@@ -762,10 +841,10 @@ EXPORT_SYMBOL_GPL(rc_keydown);
  * remote control. The driver must manually call rc_keyup() at a later stage.
  */
 void rc_keydown_notimeout(struct rc_dev *dev, enum rc_type protocol,
-			  u32 scancode, u8 toggle)
+			  u64 scancode, u8 toggle)
 {
 	unsigned long flags;
-	u32 keycode = rc_g_keycode_from_table(dev, scancode);
+	u32 keycode = rc_g_keycode_from_table(dev, protocol, scancode);
 
 	spin_lock_irqsave(&dev->keylock, flags);
 	ir_do_keydown(dev, protocol, scancode, keycode, toggle);

@@ -1675,7 +1754,10 @@ static int rc_prepare_rx_device(struct rc_dev *dev)
 	if (rc)
 		return rc;
 
-	rc_type = BIT_ULL(rc_map->rc_type);
+	if (rc_map->len > 0)
+		rc_type = BIT_ULL(rc_map->scan[0].protocol);
+	else
+		rc_type = RC_TYPE_UNKNOWN;
 
 	if (dev->change_protocol) {
 		rc = dev->change_protocol(dev, &rc_type);

include/media/rc-core.h +22 -4

index 78dea39a9b39..ad0ed23a9194 100644
@@ -44,6 +44,24 @@ enum rc_driver_type {
 	RC_DRIVER_IR_RAW_TX,
 };
 
+/* This is used for the input EVIOC[SG]KEYCODE_V2 ioctls */
+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];
+	};
+};
+
 /**
  * struct rc_scancode_filter - Filter scan codes.
  * @data:	Scancode data to match.

@@ -166,7 +184,7 @@ struct rc_dev {
 	struct timer_list		timer_keyup;
 	u32				last_keycode;
 	enum rc_type			last_protocol;
-	u32				last_scancode;
+	u64				last_scancode;
 	u8				last_toggle;
 	u32				timeout;
 	u32				min_timeout;

@@ -262,10 +280,10 @@ int rc_open(struct rc_dev *rdev);
 void rc_close(struct rc_dev *rdev);
 
 void rc_repeat(struct rc_dev *dev);
-void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 toggle);
-void rc_keydown_notimeout(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 toggle);
+void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u64 scancode, u8 toggle);
+void rc_keydown_notimeout(struct rc_dev *dev, enum rc_type protocol, u64 scancode, u8 toggle);
 void rc_keyup(struct rc_dev *dev);
-u32 rc_g_keycode_from_table(struct rc_dev *dev, u32 scancode);
+u32 rc_g_keycode_from_table(struct rc_dev *dev, enum rc_type protocol, u64 scancode);
 
 /*
  * From rc-raw.c

include/media/rc-map.h +3 -2

index e5d0559dc523..02f53ed25493 100644
@@ -139,8 +139,9 @@ enum rc_type {
  * @keycode: Linux input keycode
  */
 struct rc_map_table {
-	u32	scancode;
-	u32	keycode;
+	u64		scancode;
+	u32		keycode;
+	enum rc_type	protocol;
 };
 
 /**

On Thu, Apr 27, 2017 at 10:34:23PM +0200, David Härdeman wrote:

Quoted 30 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

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.