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

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

[Tomas Melin]

IR reciever using nuvoton-cir and lirc was not working anymore 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 2/2] [media] rc-core: change enabled_protocol default setting

[Tomas Melin]

Change default setting for enabled protocols.
Instead of enabling all protocols, disable all except lirc during registration.
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 |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index a118539..63d23d0 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -256,7 +256,8 @@ int ir_raw_event_register(struct rc_dev *dev)
 		return -ENOMEM;
 
 	dev->raw->dev = dev;
-	dev->enabled_protocols = ~0;
+	/* by default, disable all but lirc*/
+	dev->enabled_protocols = RC_BIT_LIRC;
 	rc = kfifo_alloc(&dev->raw->kfifo,
 			 sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE,
 			 GFP_KERNEL);
-- 
1.7.10.4

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

[David Härdeman]

On 2014-10-19 12:21, Tomas Melin wrote:
> IR reciever using nuvoton-cir and lirc was not working anymore after
> upgrade from kernel 3.16 to 3.17-rcX.
> Bisected regression to commit da6e162d6a4607362f8478c715c797d84d449f8b
> ("[media] rc-core: simplify sysfs code").

FWIW, I think "not working" is slightly misleading. "Required additional 
configuration steps" is a better description, isn't it?

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

[David Härdeman]

On 2014-10-19 12:21, Tomas Melin wrote:
> Change default setting for enabled protocols.
> Instead of enabling all protocols, disable all except lirc during 
> registration.
> Reduces overhead since all protocols not handled by default.
> Protocol to use will be enabled when keycode table is written by 
> userspace.

I can see the appeal in this, but now you've disabled automatic decoding 
for the protocol specified by the keymap for raw drivers? So this would 
also be a change, right?

I agree with Mauro that the "proper" long-term fix would be to teach the 
LIRC userspace daemon to enable the lirc protocol as/when necessary, but 
something similar to the patch below (but lirc + keymap protocol...if 
that's possible to implement in a non-intrusive manner, I haven't 
checked TBH) might be a good idea as an interim measure?


> 
> Signed-off-by: Tomas Melin <tomas.melin@iki.fi>
> ---
>  drivers/media/rc/rc-ir-raw.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/rc-ir-raw.c 
> b/drivers/media/rc/rc-ir-raw.c
> index a118539..63d23d0 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -256,7 +256,8 @@ int ir_raw_event_register(struct rc_dev *dev)
>  		return -ENOMEM;
> 
>  	dev->raw->dev = dev;
> -	dev->enabled_protocols = ~0;
> +	/* by default, disable all but lirc*/
> +	dev->enabled_protocols = RC_BIT_LIRC;
>  	rc = kfifo_alloc(&dev->raw->kfifo,
>  			 sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE,
>  			 GFP_KERNEL);

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

[Tomas Melin]

On Mon, Oct 20, 2014 at 5:12 PM, David Härdeman <david@hardeman.nu> wrote:
> On 2014-10-19 12:21, Tomas Melin wrote:
>>
>> Change default setting for enabled protocols.
>> Instead of enabling all protocols, disable all except lirc during
>> registration.
>> Reduces overhead since all protocols not handled by default.
>> Protocol to use will be enabled when keycode table is written by
>> userspace.
>
>
> I can see the appeal in this, but now you've disabled automatic decoding for
> the protocol specified by the keymap for raw drivers? So this would also be
> a change, right?

Yes, you are right. Atleast it potentially still could change expected
behaviour.

>
> I agree with Mauro that the "proper" long-term fix would be to teach the
> LIRC userspace daemon to enable the lirc protocol as/when necessary, but
> something similar to the patch below (but lirc + keymap protocol...if that's
> possible to implement in a non-intrusive manner, I haven't checked TBH)
> might be a good idea as an interim measure?
>
I think it looks feasible to implement that way. I'll look in to it
and send a new patch series.

Tomas

>
>
>>
>> Signed-off-by: Tomas Melin <tomas.melin@iki.fi>
>> ---
>>  drivers/media/rc/rc-ir-raw.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
>> index a118539..63d23d0 100644
>> --- a/drivers/media/rc/rc-ir-raw.c
>> +++ b/drivers/media/rc/rc-ir-raw.c
>> @@ -256,7 +256,8 @@ int ir_raw_event_register(struct rc_dev *dev)
>>                 return -ENOMEM;
>>
>>         dev->raw->dev = dev;
>> -       dev->enabled_protocols = ~0;
>> +       /* by default, disable all but lirc*/
>> +       dev->enabled_protocols = RC_BIT_LIRC;
>>         rc = kfifo_alloc(&dev->raw->kfifo,
>>                          sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE,
>>                          GFP_KERNEL);