keytable: ensure udev rule fires on rc input device

Commit Message

Sean Young Aug. 16, 2017, 4:56 p.m. UTC

On Mon, Aug 07, 2017 at 09:09:26AM +0200, Matthias Reichl wrote:
> Hi Sean!
> 
> On Sun, Aug 06, 2017 at 09:56:55AM +0100, Sean Young wrote:
> > The rc device is created before the input device, so if ir-keytable runs
> > too early the input device does not exist yet.
> > 
> > Ensure that rule fires on creation of a rc device's input device.
> > 
> > Note that this also prevents udev from starting ir-keytable on an
> > transmit only device, which has no input device.
> > 
> > Signed-off-by: Sean Young <[e-mail]>
> 
> Signed-off-by: Matthias Reichl <[e-mail]>
> 
> One comment though, see below
> 
> > ---
> >  utils/keytable/70-infrared.rules | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > Matthias, can I have your Signed-off-by please? Thank you.
> > 
> > 
> > diff --git a/utils/keytable/70-infrared.rules b/utils/keytable/70-infrared.rules
> > index afffd951..b3531727 100644
> > --- a/utils/keytable/70-infrared.rules
> > +++ b/utils/keytable/70-infrared.rules
> > @@ -1,4 +1,12 @@
> >  # Automatically load the proper keymaps after the Remote Controller device
> >  # creation.  The keycode tables rules should be at /etc/rc_maps.cfg
> >  
> > -ACTION=="add", SUBSYSTEM=="rc", RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $name"
> > +ACTION!="add", SUBSYSTEMS!="rc", GOTO="rc_dev_end"
> 
> This line doesn't quite what we want it to do.
> 
> As SUBSYSTEMS!="rc" is basically a no-op and would only be
> evaluated on change/remove events anyways that line boils down to
> 
> ACTION!="add", GOTO="rc_dev_end"
> 
> and the following rules are evaluated on all add events.

Yes, you're right. The goto is only executed if all the preceeding matches,
and for ACTION=add that is never the case.

> While that'll still work it'll do unnecessary work, like importing
> rc_sydev for all input devices and could bite us (or users) later
> if we change/extend the ruleset.
> 
> Better do it like in my original comment (using positive logic and
> a GOTO="begin") or use ACTION!="add", GOTO="rc_dev_end" and add
> SUBSYSTEMS=="rc" to the IMPORT and RUN rules below.

I've found a shorter way of doing this.


Sean

----
From: Sean Young <[e-mail]>
Date: Wed, 16 Aug 2017 17:41:53 +0100
Subject: [PATCH] keytable: ensure the udev rule fires on creation of the input
 device

The rc device is created before the input device, so if ir-keytable runs
too early the input device does not exist yet.

Ensure that rule fires on creation of a rc device's input device.

Note that this also prevents udev from starting ir-keytable on an
transmit only device, which has no input device.

Note that $id in RUN will not work, since that is expanded after all the
rules are matched, at which point the the parent might have been changed
by another match in another rule. The argument to $env{key} is expanded
immediately.

Signed-off-by: Sean Young <[e-mail]>
---
 utils/keytable/70-infrared.rules | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Matthias Reichl Aug. 21, 2017, 7:28 p.m. UTC | #1

Hi Sean!

On Wed, Aug 16, 2017 at 05:56:25PM +0100, Sean Young wrote:
> I've found a shorter way of doing this.

That's a really clever trick of dealing with the RUN/$id issue,
I like it a lot!

> ----
> From: Sean Young <[e-mail]>
> Date: Wed, 16 Aug 2017 17:41:53 +0100
> Subject: [PATCH] keytable: ensure the udev rule fires on creation of the input
>  device
> 
> The rc device is created before the input device, so if ir-keytable runs
> too early the input device does not exist yet.
> 
> Ensure that rule fires on creation of a rc device's input device.
> 
> Note that this also prevents udev from starting ir-keytable on an
> transmit only device, which has no input device.
> 
> Note that $id in RUN will not work, since that is expanded after all the
> rules are matched, at which point the the parent might have been changed
> by another match in another rule. The argument to $env{key} is expanded
> immediately.
> 
> Signed-off-by: Sean Young <[e-mail]>

Tested-by: Matthias Reichl <[e-mail]>

so long & thanks for the fix,

Hias
> ---
>  utils/keytable/70-infrared.rules | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utils/keytable/70-infrared.rules b/utils/keytable/70-infrared.rules
> index afffd951..41ca2089 100644
> --- a/utils/keytable/70-infrared.rules
> +++ b/utils/keytable/70-infrared.rules
> @@ -1,4 +1,4 @@
>  # Automatically load the proper keymaps after the Remote Controller device
>  # creation.  The keycode tables rules should be at /etc/rc_maps.cfg
>  
> -ACTION=="add", SUBSYSTEM=="rc", RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $name"
> +ACTION=="add", SUBSYSTEM=="input", SUBSYSTEMS=="rc", KERNEL=="event*", ENV{.rc_sysdev}="$id", RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $env{.rc_sysdev}"
> -- 
> 2.13.5
>

Patch

diff --git a/utils/keytable/70-infrared.rules b/utils/keytable/70-infrared.rules
index afffd951..41ca2089 100644
--- a/utils/keytable/70-infrared.rules
+++ b/utils/keytable/70-infrared.rules
@@ -1,4 +1,4 @@ 
 # Automatically load the proper keymaps after the Remote Controller device
 # creation.  The keycode tables rules should be at /etc/rc_maps.cfg
 
-ACTION=="add", SUBSYSTEM=="rc", RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $name"
+ACTION=="add", SUBSYSTEM=="input", SUBSYSTEMS=="rc", KERNEL=="event*", ENV{.rc_sysdev}="$id", RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $env{.rc_sysdev}"