[PATCH v2 1/2] [media] rc-core: fix protocol_change regression in ir_raw_event_register

[PATCH v2 1/2] [media] rc-core: fix protocol_change regression in ir_raw_event_register

[Tomas Melin]

IR reciever using nuvoton-cir and lirc required additional configuration
steps after upgrade from kernel 3.16 to 3.17-rcX.
Bisected regression to commit da6e162d6a4607362f8478c715c797d84d449f8b
("[media] rc-core: simplify sysfs code").

The regression comes from adding empty function change_protocol in
ir-raw.c. It changes behaviour so that only the protocol enabled by driver's
map_name will be active after registration. This breaks user space behaviour,
lirc does not get key press signals anymore.

Changed back to original behaviour by removing empty function
change_protocol in ir-raw.c. Instead only calling change_protocol for
drivers that actually have the function set up.

Signed-off-by: Tomas Melin <tomas.melin@iki.fi>
---
 drivers/media/rc/rc-ir-raw.c |    7 -------
 drivers/media/rc/rc-main.c   |   19 ++++++++-----------
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index e8fff2a..a118539 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -240,12 +240,6 @@ ir_raw_get_allowed_protocols(void)
 	return protocols;
 }
 
-static int change_protocol(struct rc_dev *dev, u64 *rc_type)
-{
-	/* the caller will update dev->enabled_protocols */
-	return 0;
-}
-
 /*
  * Used to (un)register raw event clients
  */
@@ -263,7 +257,6 @@ int ir_raw_event_register(struct rc_dev *dev)
 
 	dev->raw->dev = dev;
 	dev->enabled_protocols = ~0;
-	dev->change_protocol = change_protocol;
 	rc = kfifo_alloc(&dev->raw->kfifo,
 			 sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE,
 			 GFP_KERNEL);
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index a7991c7..633c682 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1001,11 +1001,6 @@ static ssize_t store_protocols(struct device *device,
 		set_filter = dev->s_wakeup_filter;
 	}
 
-	if (!change_protocol) {
-		IR_dprintk(1, "Protocol switching not supported\n");
-		return -EINVAL;
-	}
-
 	mutex_lock(&dev->lock);
 
 	old_protocols = *current_protocols;
@@ -1013,12 +1008,14 @@ static ssize_t store_protocols(struct device *device,
 	rc = parse_protocol_change(&new_protocols, buf);
 	if (rc < 0)
 		goto out;
-
-	rc = change_protocol(dev, &new_protocols);
-	if (rc < 0) {
-		IR_dprintk(1, "Error setting protocols to 0x%llx\n",
-			   (long long)new_protocols);
-		goto out;
+	/* Only if protocol change set up in driver */
+	if (change_protocol) {
+		rc = change_protocol(dev, &new_protocols);
+		if (rc < 0) {
+			IR_dprintk(1, "Error setting protocols to 0x%llx\n",
+				   (long long)new_protocols);
+			goto out;
+		}
 	}
 
 	if (new_protocols == old_protocols) {
-- 
1.7.10.4

[PATCH v2 2/2] [media] rc-core: change enabled_protocol default setting

[Tomas Melin]

Change default setting for enabled protocols.
Instead of enabling all protocols during registration,
disable all except default keymap and lirc.
Reduces overhead since all protocols not handled by default.
Protocol to use will be enabled when keycode table is written by userspace.

Signed-off-by: Tomas Melin <tomas.melin@iki.fi>
---
 drivers/media/rc/rc-ir-raw.c |    1 -
 drivers/media/rc/rc-main.c   |    6 ++++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index a118539..d3b1e76 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -256,7 +256,6 @@ int ir_raw_event_register(struct rc_dev *dev)
 		return -ENOMEM;
 
 	dev->raw->dev = dev;
-	dev->enabled_protocols = ~0;
 	rc = kfifo_alloc(&dev->raw->kfifo,
 			 sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE,
 			 GFP_KERNEL);
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 633c682..dcdf4cd 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1320,6 +1320,8 @@ int rc_register_device(struct rc_dev *dev)
 		rc_map = rc_map_get(RC_MAP_EMPTY);
 	if (!rc_map || !rc_map->scan || rc_map->size == 0)
 		return -EINVAL;
+	/* get default keymap type */
+	u64 rc_type = (1 << rc_map->rc_type);
 
 	set_bit(EV_KEY, dev->input_dev->evbit);
 	set_bit(EV_REP, dev->input_dev->evbit);
@@ -1412,16 +1414,16 @@ int rc_register_device(struct rc_dev *dev)
 			raw_init = true;
 		}
 		rc = ir_raw_event_register(dev);
+		dev->enabled_protocols = (rc_type | RC_BIT_LIRC);
 		if (rc < 0)
 			goto out_input;
 	}
 
 	if (dev->change_protocol) {
-		u64 rc_type = (1 << rc_map->rc_type);
 		rc = dev->change_protocol(dev, &rc_type);
 		if (rc < 0)
 			goto out_raw;
-		dev->enabled_protocols = rc_type;
+		dev->enabled_protocols = (rc_type | RC_BIT_LIRC);
 	}
 
 	mutex_unlock(&dev->lock);
-- 
1.7.10.4

Re: [PATCH v2 1/2] [media] rc-core: fix protocol_change regression in ir_raw_event_register

[David Härdeman]

On Tue, Oct 21, 2014 at 09:30:17PM +0300, Tomas Melin wrote:
>IR reciever using nuvoton-cir and lirc required additional configuration
>steps after upgrade from kernel 3.16 to 3.17-rcX.
>Bisected regression to commit da6e162d6a4607362f8478c715c797d84d449f8b
>("[media] rc-core: simplify sysfs code").
>
>The regression comes from adding empty function change_protocol in
>ir-raw.c. It changes behaviour so that only the protocol enabled by driver's
>map_name will be active after registration. This breaks user space behaviour,
>lirc does not get key press signals anymore.
>
>Changed back to original behaviour by removing empty function
>change_protocol in ir-raw.c. Instead only calling change_protocol for

Wouldn't something like this be a simpler way of achieving the same
result? (untested):


diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index a7991c7..d521f20 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1421,6 +1421,9 @@ int rc_register_device(struct rc_dev *dev)
 
 	if (dev->change_protocol) {
 		u64 rc_type = (1 << rc_map->rc_type);
+		if (dev->driver_type == RC_DRIVER_IR_RAW)
+			rc_type |= RC_BIT_LIRC;
+
 		rc = dev->change_protocol(dev, &rc_type);
 		if (rc < 0)
 			goto out_raw;

Re: [PATCH v2 1/2] [media] rc-core: fix protocol_change regression in ir_raw_event_register

[Tomas Melin]

On Sat, Oct 25, 2014 at 12:03 PM, David Härdeman <david@hardeman.nu> wrote:
> Wouldn't something like this be a simpler way of achieving the same
> result? (untested):

The idea was to remove the empty change_protocol function that had
been added in the breaking commit.
IMHO, it would be better to not have functions that don't do anything.
Actually, another problem with that empty function is that if the
driver first sets up a "real" change_protocol function and related
data, and then calls rc_register_device, the driver defined
change_protocol function would be overwritten.


> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index a7991c7..d521f20 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1421,6 +1421,9 @@ int rc_register_device(struct rc_dev *dev)
>
>         if (dev->change_protocol) {
>                 u64 rc_type = (1 << rc_map->rc_type);
> +               if (dev->driver_type == RC_DRIVER_IR_RAW)
> +                       rc_type |= RC_BIT_LIRC;
> +
>                 rc = dev->change_protocol(dev, &rc_type);
>                 if (rc < 0)
>                         goto out_raw;

But otherwise yes, your suggestion could work, with the addition that
we still need to update enabled_protocols (and not init
enabled_protocols anymore in ir_raw_event_register() ).
+               dev->enabled_protocols = (rc_type | RC_BIT_LIRC);

Please let me know your preferences on which you prefer, and, if
needed, I'll make a new patch version.
Tomas

Re: [PATCH v2 1/2] [media] rc-core: fix protocol_change regression in ir_raw_event_register

[David Härdeman]

On 2014-10-26 20:33, Tomas Melin wrote:
> On Sat, Oct 25, 2014 at 12:03 PM, David Härdeman <david@hardeman.nu> 
> wrote:
>> Wouldn't something like this be a simpler way of achieving the same
>> result? (untested):
> 
> The idea was to remove the empty change_protocol function that had
> been added in the breaking commit.

The empty function was added for a reason? The presence of a 
change_protocol function implies that the receiver supports protocol 
changing (whether it's via the raw IR decoding or in hardware).

> IMHO, it would be better to not have functions that don't do anything.
> Actually, another problem with that empty function is that if the
> driver first sets up a "real" change_protocol function and related
> data, and then calls rc_register_device, the driver defined
> change_protocol function would be overwritten.

change_protocol is only set if it's a driver that does in-kernel 
decoding...meaning that it generates pulse/space timings...meaning that 
hardware protocol changes aren't necessary?

>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index a7991c7..d521f20 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -1421,6 +1421,9 @@ int rc_register_device(struct rc_dev *dev)
>> 
>>         if (dev->change_protocol) {
>>                 u64 rc_type = (1 << rc_map->rc_type);
>> +               if (dev->driver_type == RC_DRIVER_IR_RAW)
>> +                       rc_type |= RC_BIT_LIRC;
>> +
>>                 rc = dev->change_protocol(dev, &rc_type);
>>                 if (rc < 0)
>>                         goto out_raw;
> 
> But otherwise yes, your suggestion could work, with the addition that
> we still need to update enabled_protocols (and not init
> enabled_protocols anymore in ir_raw_event_register() ).

First, enabled_protocols is already taken care of in the above patch 
(the line after "goto out_raw" is "dev->enabled_protocols = rc_type;")?

Second, initializing enabled_protocols to some default in 
ir_raw_event_register() might not be strictly necessary but it also 
doesn't hurt since that happens before dev->change_protocol() is called 
in rc_register_device()?

> +               dev->enabled_protocols = (rc_type | RC_BIT_LIRC);
> 
> Please let me know your preferences on which you prefer, and, if
> needed, I'll make a new patch version.

I'd prefer the above, minimal, approach. But it's Mauro who decides in 
the end.

Regards,
David

Re: [PATCH v2 1/2] [media] rc-core: fix protocol_change regression in ir_raw_event_register

[Tomas Melin]

On Tue, Oct 28, 2014 at 10:42 AM, David Härdeman <david@hardeman.nu> wrote:
> On 2014-10-26 20:33, Tomas Melin wrote:
>>
>> Please let me know your preferences on which you prefer, and, if
>> needed, I'll make a new patch version.
>
>
> I'd prefer the above, minimal, approach. But it's Mauro who decides in the
> end.
Then let's just go with that approach and see if it's ok with Mauro.

Tomas