[v4l-utils] 70-infrared.rules starts ir-keytable too early

[v4l-utils] 70-infrared.rules starts ir-keytable too early

[Matthias Reichl]

While testing serial_ir on kernel 4.11.8 with ir-keytable 1.12.3
I noticed that my /etc/rc_maps.cfg configuration wasn't applied.
Manually running "ir-keytable -a /etc/rc_maps.cfg -s rc0" always
worked fine, though.

Digging further into this I tracked it down to the udev rule
being racy. The udev rule triggers on the rc subsystem, but this
is before the input and event devices are created.

The kernel creates 3 events relevant to this context, on rcX
device creation, on inputY creation and on inputY/eventZ creation
- the latter 2 usually in quick succession, but some time can
elapse between the first 2.

In my case ir-keytable -a was executing during this small time
window and failed with an error because it couldn't find the
input/event devices.

Excerpt from log with udev debugging enabled:

Jul 16 11:02:11 LibreELEC systemd-udevd[272]: seq 2099 queued, 'add' 'rc'
...
Jul 16 11:02:11 LibreELEC systemd-udevd[614]: '/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0'(err) 'Couldn't find any node at /sys/class/rc/rc0/input*.'
Jul 16 11:02:11 LibreELEC systemd-udevd[614]: Process '/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s rc0' failed with exit code 255.
...
Jul 16 11:02:11 LibreELEC systemd-udevd[272]: seq 2106 queued, 'add' 'input'
Jul 16 11:02:11 LibreELEC systemd-udevd[272]: seq 2106 forked new worker [622]
Jul 16 11:02:11 LibreELEC systemd-udevd[272]: seq 2107 queued, 'add' 'input'

The full log is available here:
http://www.horus.com/~hias/tmp/journalctl-ir-keytable-failed

One solution is to trigger ir-keytable -a execution from the event device
creation instead. I'm currently testing with the following udev rule:

ACTION=="add", SUBSYSTEMS=="rc", GOTO="begin"
GOTO="end"

LABEL="begin"

SUBSYSTEM=="rc", ENV{rc_sysdev}="$name"

SUBSYSTEM=="input", IMPORT{parent}="rc_sysdev"

KERNEL=="event[0-9]*", ENV{rc_sysdev}=="?*", \
   RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $env{rc_sysdev}"

LABEL="end"

That udev rule is a bit messy, ir-keytable -a needs the rcX sysdev,
which doesn't seem to be easily available from the event node in
the input subsystem, so I'm propagating that info through an
environment variable.

So far testing is working fine, but hints for better/nicer solutions
are welcome!

so long,

Hias

Re: [v4l-utils] 70-infrared.rules starts ir-keytable too early

[Sean Young]

On Mon, Jul 17, 2017 at 11:20:38AM +0200, Matthias Reichl wrote:
> While testing serial_ir on kernel 4.11.8 with ir-keytable 1.12.3
> I noticed that my /etc/rc_maps.cfg configuration wasn't applied.
> Manually running "ir-keytable -a /etc/rc_maps.cfg -s rc0" always
> worked fine, though.
> 
> Digging further into this I tracked it down to the udev rule
> being racy. The udev rule triggers on the rc subsystem, but this
> is before the input and event devices are created.

That's an interesting race condition, I haven't seen this before.

> One solution is to trigger ir-keytable -a execution from the event device
> creation instead. I'm currently testing with the following udev rule:
> 
> ACTION=="add", SUBSYSTEMS=="rc", GOTO="begin"
> GOTO="end"
> 
> LABEL="begin"
> 
> SUBSYSTEM=="rc", ENV{rc_sysdev}="$name"
> 
> SUBSYSTEM=="input", IMPORT{parent}="rc_sysdev"
> 
> KERNEL=="event[0-9]*", ENV{rc_sysdev}=="?*", \
>    RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $env{rc_sysdev}"
> 
> LABEL="end"
> 
> That udev rule is a bit messy, ir-keytable -a needs the rcX sysdev,
> which doesn't seem to be easily available from the event node in
> the input subsystem, so I'm propagating that info through an
> environment variable.

This is a good idea; this also solves the problem of udev firing off
ir-keytable for transmit-only devices, which have no input device.

> So far testing is working fine, but hints for better/nicer solutions
> are welcome!

So far I've only come up with a minor change:

ACTION!="add", SUBSYSTEMS!="rc", GOTO="rc_dev_end"

SUBSYSTEM=="rc", ENV{rc_sysdev}="$name"

SUBSYSTEM=="input", IMPORT{parent}="rc_sysdev"

KERNEL=="event[0-9]*", ENV{rc_sysdev}=="?*", RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $env{rc_sysdev}"

LABEL="rc_dev_end"

I think we should get this merged into v4l-utils, it solves a real issue.


Sean

[PATCH] keytable: ensure udev rule fires on rc input device

[Sean Young]

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 <sean@mess.org>
---
 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"
+
+SUBSYSTEM=="rc", ENV{rc_sysdev}="$name"
+
+SUBSYSTEM=="input", IMPORT{parent}="rc_sysdev"
+
+KERNEL=="event[0-9]*", ENV{rc_sysdev}=="?*", RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $env{rc_sysdev}"
+
+LABEL="rc_dev_end"
-- 
2.11.0

Re: [PATCH] keytable: ensure udev rule fires on rc input device

[Matthias Reichl]

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 <sean@mess.org>

Signed-off-by: Matthias Reichl <hias@horus.com>

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.

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.

so long,

Hias

> +
> +SUBSYSTEM=="rc", ENV{rc_sysdev}="$name"
> +
> +SUBSYSTEM=="input", IMPORT{parent}="rc_sysdev"
> +
> +KERNEL=="event[0-9]*", ENV{rc_sysdev}=="?*", RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $env{rc_sysdev}"
> +
> +LABEL="rc_dev_end"
> -- 
> 2.11.0
> 

Re: [PATCH] keytable: ensure udev rule fires on rc input device

[Sean Young]

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 <sean@mess.org>
> 
> Signed-off-by: Matthias Reichl <hias@horus.com>
> 
> 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 <sean@mess.org>
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 <sean@mess.org>
---
 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

Re: [PATCH] keytable: ensure udev rule fires on rc input device

[Matthias Reichl]

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 <sean@mess.org>
> 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 <sean@mess.org>

Tested-by: Matthias Reichl <hias@horus.com>

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
>