Discussion:
[PATCH 1/2] [media] rc-core: fix protocol_change regression in ir_raw_event_register
(too old to reply)
Tomas Melin
2014-10-19 10:21:52 UTC
Permalink
Raw Message
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 <***@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
Tomas Melin
2014-10-19 10:21:53 UTC
Permalink
Raw Message
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 <***@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
David Härdeman
2014-10-20 14:12:54 UTC
Permalink
Raw Message
Post by 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.
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?
Post by Tomas Melin
---
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);
Tomas Melin
2014-10-21 17:44:28 UTC
Permalink
Raw Message
Post by 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.
I can see the appeal in this, but now you've disabled automatic decod=
ing for
the protocol specified by the keymap for raw drivers? So this would a=
lso 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 TB=
H)
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
Post by Tomas Melin
---
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-r=
aw.c
Post by Tomas Melin
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 =3D dev;
- dev->enabled_protocols =3D ~0;
+ /* by default, disable all but lirc*/
+ dev->enabled_protocols =3D RC_BIT_LIRC;
rc =3D kfifo_alloc(&dev->raw->kfifo,
sizeof(struct ir_raw_event) * MAX_IR_EVENT_=
SIZE,
Post by Tomas Melin
GFP_KERNEL);
David Härdeman
2014-10-20 14:09:50 UTC
Permalink
Raw Message
Post by 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").
FWIW, I think "not working" is slightly misleading. "Required additional
configuration steps" is a better description, isn't it?
Loading...