Re: [PATCH] udev: add tags also on bind action

[Aleksander Morgado]

On Wed, May 30, 2018 at 10:24 PM, Dan Williams  wrote:
> On Sun, 2018-05-27 at 17:03 +0200, Aleksander Morgado wrote:
>> When a new USB device is hotplugged, e.g. a USB<->RS232 converter
>> that
>> exposes a single ttyUSB0, these udev events happen:
>>
>>   add  /devices/pci:00/:00:14.0/usb2/2-1 (usb/usb-device)
>>   add  /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0 (usb/usb-
>> interface)
>>   add  /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/ttyUSB0
>> (usb-serial)
>>   add  /devices/pci:00/:00:14.0/usb2/2-1/2-
>> 1:1.0/ttyUSB0/tty/ttyUSB0 (tty)
>>   bind /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/ttyUSB0
>> (usb-serial)
>>   bind /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0 (usb/usb-
>> interface)
>>   bind /devices/pci:00/:00:14.0/usb2/2-1 (usb/usb-device)
>>
>> Our udev rules in MM only added tags in the 'add' events, and it
>> looks
>> like the only ones 'persistent' after this sequence are those of the
>> last event happening on the specific path.
>>
>> This meant that all TTY subsystem rules (e.g. ID_MM_CANDIDATE) would
>> be stored for later check (e.g. if ModemManager is started after
>> these
>> rules have been applied), which was ok. "udevadm info -p ..." would
>> show these tags correctly always.
>>
>> But this also meant that the 'bind' udev event happening for the USB
>> device didn't get any of our device-specific tags, and so we would be
>> missing them (e.g. ID_MM_DEVICE_MANUAL_SCAN_ONLY) if MM is started
>> after the last event has happened. "udevadm info -p ..." would
>> not show these tags.
>>
>> Modify all our rules to also run at the 'bind' events.
>
> Unfortunate, but LGTM.
>

Pushed to git master.

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] udev: add tags also on bind action

[Dan Williams]

On Sun, 2018-05-27 at 17:03 +0200, Aleksander Morgado wrote:
> When a new USB device is hotplugged, e.g. a USB<->RS232 converter
> that
> exposes a single ttyUSB0, these udev events happen:
> 
>   add  /devices/pci:00/:00:14.0/usb2/2-1 (usb/usb-device)
>   add  /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0 (usb/usb-
> interface)
>   add  /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/ttyUSB0
> (usb-serial)
>   add  /devices/pci:00/:00:14.0/usb2/2-1/2-
> 1:1.0/ttyUSB0/tty/ttyUSB0 (tty)
>   bind /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/ttyUSB0
> (usb-serial)
>   bind /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0 (usb/usb-
> interface)
>   bind /devices/pci:00/:00:14.0/usb2/2-1 (usb/usb-device)
> 
> Our udev rules in MM only added tags in the 'add' events, and it
> looks
> like the only ones 'persistent' after this sequence are those of the
> last event happening on the specific path.
> 
> This meant that all TTY subsystem rules (e.g. ID_MM_CANDIDATE) would
> be stored for later check (e.g. if ModemManager is started after
> these
> rules have been applied), which was ok. "udevadm info -p ..." would
> show these tags correctly always.
> 
> But this also meant that the 'bind' udev event happening for the USB
> device didn't get any of our device-specific tags, and so we would be
> missing them (e.g. ID_MM_DEVICE_MANUAL_SCAN_ONLY) if MM is started
> after the last event has happened. "udevadm info -p ..." would
> not show these tags.
> 
> Modify all our rules to also run at the 'bind' events.

Unfortunate, but LGTM.

Dan

> ---
> 
> Hey Bjørn, Dan, Ben and all,
> 
> I have no idea if this is something that has recently changed in
> systemd/udev (I'm using systemd 236.81-1 in Arch Linux), or if it's
> some other issue triggered by my setup, so I would like to ask you
> all a favor and review if this makes sense or if you can reproduce
> it...
> 
> The thing is, with our current set of rules, I could see how the
> port-specific udev tags (those applied in the ttyUSBx device for
> example) would be persistent and browsable with "udevadm info -p
> /sys...". But the device-specific tags (those applied to the "usb-
> device") wouldn't be persistent.
> 
> The outcome of this is that if ModemManager gets the events while
> it's running, everything works perfectly, because for the 'udev-
> device' we would be notified of both 'add' and 'bind' events. But if
> MM starts after the events have happened, ModemManager only gets the
> last event ('bind' in this case) and so we would lose the device-
> specific tags because we were not adding them in the bind event. Does
> this make sense?
> 
> There's no need to reboot laptop to test this ;) you can just: stop
> ModemManager, plug in e.g. a USB<->serial adapter (which shouldn't be
> probed automatically), and then start ModemManager. If the port is
> probed automatically, the tag wasn't applied. Running "udevadm
> monitor -e" during the hotplug event also helps to see the contents
> of each of the events.
> 
> What do you all think?
> 
> ---
>  plugins/cinterion/77-mm-cinterion-port-types.rules | 2 +-
>  plugins/dell/77-mm-dell-port-types.rules   | 2 +-
>  plugins/haier/77-mm-haier-port-types.rules | 2 +-
>  plugins/huawei/77-mm-huawei-net-port-types.rules   | 2 +-
>  plugins/longcheer/77-mm-longcheer-port-types.rules | 2 +-
>  plugins/mbm/77-mm-ericsson-mbm.rules   | 2 +-
>  plugins/mtk/77-mm-mtk-port-types.rules | 2 +-
>  plugins/nokia/77-mm-nokia-port-types.rules | 2 +-
>  plugins/sierra/77-mm-sierra.rules  | 2 +-
>  plugins/simtech/77-mm-simtech-port-types.rules | 2 +-
>  plugins/telit/77-mm-telit-port-types.rules | 2 +-
>  plugins/ublox/77-mm-ublox-port-types.rules | 2 +-
>  plugins/x22x/77-mm-x22x-port-types.rules   | 2 +-
>  plugins/zte/77-mm-zte-port-types.rules | 2 +-
>  src/77-mm-pcmcia-device-blacklist.rules| 2 +-
>  src/77-mm-usb-device-blacklist.rules   | 2 +-
>  src/77-mm-usb-serial-adapters-greylist.rules   | 2 +-
>  src/80-mm-candidate.rules  | 2 +-
>  18 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/plugins/cinterion/77-mm-cinterion-port-types.rules
> b/plugins/cinterion/77-mm-cinterion-port-types.rules
> index 7c63a1aa..7ef80719 100644
> --- a/plugins/cinterion/77-mm-cinterion-port-types.rules
> +++ b/plugins/cinterion/77-mm-cinterion-port-types.rules
> @@ -1,6 +1,6 @@
>  # do not edit this file, it will be overwritten on update
> 
> -ACTION!="add|change|move", GOTO="mm_cinterion_port_types_end"
> +ACTION!="add|change|move|bind", GOTO="mm_cinterion_port_types_end"
>  SUBSYSTEMS=="usb", ATTRS{idVendor}=="1e2d",
> GOTO="mm_cinterion_port_types"
>  GOTO="mm_cinterion_port_types_end"
> 
> diff --git a/plugins/dell/77-mm-dell-port-types.rules
> b/plugins/dell/77-mm-dell-port-types.rules
> index 206fb6e1..eb271fc7 100644
> --- 

Re: [PATCH] udev: add tags also on bind action

[Aleksander Morgado]

Hey!

On Sun, May 27, 2018 at 6:16 PM, Bjørn Mork  wrote:
> Never heard of "bind" events before.  Googling them led me straight to
> https://github.com/systemd/systemd/issues/8221
> which tells me that that I haven't been paying much attention for the
> last 5 kernel releases...
>
> Except for that, this looks like a systemd bug.  The Debian bug is
> reported agains v237, so your v236 will be affected.
>

Ohh, this explains everything I'm seeing indeed.

> I will test your patch, but adding "bind" to every existing rule for
> every device and every application cannot be right?  This should be
> handled by systemd-udev.
>

Yes, should be handled by systemd-udev, definitely. This is extremely
unfortunate...

But in the meantime, all the logic we have in place to handle udev
tags at device level are affected. The udev tags we set for e.g.
tagging port types aren't affected (as there's no bind event for the
tty device), but all the udev tags for the blacklist and greylist are
affected currently. So I believe there's something we need to do...
maybe adding the tag on the bind event is overkill?

There's another possibility that I see we could use, and that would be
not to tag the device, but tag all the ports of the device instead.
This is already supported in git master because the "udev-less" logic
used in OpenWRT applies device-level tags in the different ports, so
we could do the same to avoid this... The problem is that this would
force us to keep this in place forever I guess... and that brings me
back to thinking just adding the rules also in bind wouldn't be that
bad.

Note anyway, that this udev bug affects MM running in DEFAULT filter
mode. If using the STRICT filter, as we're suggesting all
distributions to use now, they would not be affected (as all
port-level udev tags wouldn't be affected, and there's no
blacklist/greylist filtering in STRICT mode).

Well, a third option would be to hack our own port type rules,
blacklist, greylist... without using udev at all. But I really
wouldn't want to have to do that right now...

Whenever we find a solution for this issue in our side, even if
temporary, I'll suggest we release 1.8.

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] udev: add tags also on bind action

[Bjørn Mork]

Never heard of "bind" events before.  Googling them led me straight to
https://github.com/systemd/systemd/issues/8221
which tells me that that I haven't been paying much attention for the
last 5 kernel releases...

Except for that, this looks like a systemd bug.  The Debian bug is
reported agains v237, so your v236 will be affected.

I will test your patch, but adding "bind" to every existing rule for
every device and every application cannot be right?  This should be
handled by systemd-udev.



Bjørn


Aleksander Morgado  writes:

> When a new USB device is hotplugged, e.g. a USB<->RS232 converter that
> exposes a single ttyUSB0, these udev events happen:
>
>   add  /devices/pci:00/:00:14.0/usb2/2-1 (usb/usb-device)
>   add  /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0 (usb/usb-interface)
>   add  /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/ttyUSB0 (usb-serial)
>   add  /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/ttyUSB0/tty/ttyUSB0 
> (tty)
>   bind /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/ttyUSB0 (usb-serial)
>   bind /devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0 (usb/usb-interface)
>   bind /devices/pci:00/:00:14.0/usb2/2-1 (usb/usb-device)
>
> Our udev rules in MM only added tags in the 'add' events, and it looks
> like the only ones 'persistent' after this sequence are those of the
> last event happening on the specific path.
>
> This meant that all TTY subsystem rules (e.g. ID_MM_CANDIDATE) would
> be stored for later check (e.g. if ModemManager is started after these
> rules have been applied), which was ok. "udevadm info -p ..." would
> show these tags correctly always.
>
> But this also meant that the 'bind' udev event happening for the USB
> device didn't get any of our device-specific tags, and so we would be
> missing them (e.g. ID_MM_DEVICE_MANUAL_SCAN_ONLY) if MM is started
> after the last event has happened. "udevadm info -p ..." would
> not show these tags.
>
> Modify all our rules to also run at the 'bind' events.
> ---
>
> Hey Bjørn, Dan, Ben and all,
>
> I have no idea if this is something that has recently changed in systemd/udev 
> (I'm using systemd 236.81-1 in Arch Linux), or if it's some other issue 
> triggered by my setup, so I would like to ask you all a favor and review if 
> this makes sense or if you can reproduce it...
>
> The thing is, with our current set of rules, I could see how the 
> port-specific udev tags (those applied in the ttyUSBx device for example) 
> would be persistent and browsable with "udevadm info -p /sys...". But the 
> device-specific tags (those applied to the "usb-device") wouldn't be 
> persistent.
>
> The outcome of this is that if ModemManager gets the events while it's 
> running, everything works perfectly, because for the 'udev-device' we would 
> be notified of both 'add' and 'bind' events. But if MM starts after the 
> events have happened, ModemManager only gets the last event ('bind' in this 
> case) and so we would lose the device-specific tags because we were not 
> adding them in the bind event. Does this make sense?
>
> There's no need to reboot laptop to test this ;) you can just: stop 
> ModemManager, plug in e.g. a USB<->serial adapter (which shouldn't be probed 
> automatically), and then start ModemManager. If the port is probed 
> automatically, the tag wasn't applied. Running "udevadm monitor -e" during 
> the hotplug event also helps to see the contents of each of the events.
>
> What do you all think?
>
> ---
>  plugins/cinterion/77-mm-cinterion-port-types.rules | 2 +-
>  plugins/dell/77-mm-dell-port-types.rules   | 2 +-
>  plugins/haier/77-mm-haier-port-types.rules | 2 +-
>  plugins/huawei/77-mm-huawei-net-port-types.rules   | 2 +-
>  plugins/longcheer/77-mm-longcheer-port-types.rules | 2 +-
>  plugins/mbm/77-mm-ericsson-mbm.rules   | 2 +-
>  plugins/mtk/77-mm-mtk-port-types.rules | 2 +-
>  plugins/nokia/77-mm-nokia-port-types.rules | 2 +-
>  plugins/sierra/77-mm-sierra.rules  | 2 +-
>  plugins/simtech/77-mm-simtech-port-types.rules | 2 +-
>  plugins/telit/77-mm-telit-port-types.rules | 2 +-
>  plugins/ublox/77-mm-ublox-port-types.rules | 2 +-
>  plugins/x22x/77-mm-x22x-port-types.rules   | 2 +-
>  plugins/zte/77-mm-zte-port-types.rules | 2 +-
>  src/77-mm-pcmcia-device-blacklist.rules| 2 +-
>  src/77-mm-usb-device-blacklist.rules   | 2 +-
>  src/77-mm-usb-serial-adapters-greylist.rules   | 2 +-
>  src/80-mm-candidate.rules  | 2 +-
>  18 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/plugins/cinterion/77-mm-cinterion-port-types.rules 
> b/plugins/cinterion/77-mm-cinterion-port-types.rules
> index 7c63a1aa..7ef80719 100644
> --- a/plugins/cinterion/77-mm-cinterion-port-types.rules
> +++ b/plugins/cinterion/77-mm-cinterion-port-types.rules
> @@ -1,6 +1,6 @@
>  # do not edit this file,