[PATCH] xserver: add udev/systemd multi-seat support

[Dan Nicholson]

On Mon, Jul 18, 2011 at 11:56 AM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Mon, 18.07.11 19:29, Lennart Poettering (lennart at poettering.net) wrote:
>
>> If this is supposed to help with the wacom driver, then this is bogus
>> too, as the associated rule in /lib/udev/rules.d/70-wacom.rules actually
>> does check for ID_INPUT.
>
> So, I had another closer look on this, and now understand what is going
> on. This rules file is actually really really borked, and really needs
> to be fixed. And not only this files is borked, the matching code in X11
> is borked too.

I'd never seen that wacom rules file, but I feel like I can comment on
the udev matching in xorg a little more generally. I agree it looks
pretty gross on further inspection.

> If I understand this properly, then the wacom serial tablets come with
> their own special ISAPNP serial port?
>
> So here's what needs to be fixed:
>
> 1) You cannot overide ENV{SUBSYSTEM}. This env var encodes to which
> kernel subsystem a device belongs to. Just by overriding this in
> userspace you cannot make it move to a different piece of code! This
> will break things if left in this way. In fact, after I told Kay about
> this he has now added code to udev to explicitly disallow lines like
> this. That means these lines will stop working with newer udev unless
> they are fixed to stop doing this.
>
> 2) If you introduce your own env vars on devices, you are not allowed
> not to prefix them. Unprefixed variables are private property of the
> kernel. Userspace may only add prefixed variables to that. A common
> prefix used is ID_, but you can choose whatever you want. This needs
> fixing too. You need to find a better variable than "NAME" to store
> your device name.

This was a sad hack that was introduced to deal with the fact that the
serial devices don't seem to have a name attribute exported from the
kernel. I don't have a serial tablet, so I don't know what the devices
look like at all. I agree that using the variable NAME is a poor
choice.

> 3) We try to avoid setting pretty names in udev rules, since they might
> need translations later on. Hence having a property like "NAME" here
> isn't a good idea at all -- not even if you rename it.

This is already done in the kernel, so I don't see why we're doing
anything wrong here.

$ udevadm info --attribute-walk
--name=/dev/input/by-id/usb-Silitek_Standard_USB_Keyboard-event-kbd

Udevadm info starts with the device specified by the devpath and then
walks up the chain of parent devices. It prints for every device
found, all possible attributes in the udev rules key format.
A rule to match, can be composed by the attributes of the device
and the attributes from one single parent device.

  looking at device
'/devices/pci0000:00/0000:00:1d.1/usb6/6-1/6-1:1.0/input/input2/event2':
    KERNEL=="event2"
    SUBSYSTEM=="input"
    DRIVER==""

  looking at parent device
'/devices/pci0000:00/0000:00:1d.1/usb6/6-1/6-1:1.0/input/input2':
    KERNELS=="input2"
    SUBSYSTEMS=="input"
    DRIVERS==""
    ATTRS{name}=="Silitek Standard USB Keyboard "
...

I'm pretty sure hal did the same thing, which is where I looked when I
added that code. What I always thought would be much more appropriate
is if one of the udev rules standardized this name under a
ID_INPUT_NAME property or something like that. The other thing that
always kind of bugged me here is that this is the name that's matched
with MatchProduct and it's not really the product name, but that's
another story.

> 4) If you just check attr{id} like you do in those rules this will cause
> *all* devices' id attribute to be read, and if if they happen to include
> "FUJ" as prefix, you'll mark them as tablet. Example: an audio driver
> also exposes an id property, you can even set it to whatevr you want via
> a module argument. If a poor soul has a sound card with an id of "FUJ"
> then your rule will mark it as input device for X.
>
> To fix this: since the devices in question show up as ISAPNP devices,
> this is what you need to check for. i.e. instead of this:
>
> ATTRS{id}=="FUJ*", ...
>
> use this:
>
> SUBSYSTEMS="pnp", ATTRS{id}=="FUJ*", ...
>
> 5) You are now adding your properties to *every* child device of the pnp
> device which is very much broken. You should only add it to the serial
> port itself. I.e. you also need to check SUBSYSTEM="tty". (Singular, not
> plural in this case).
>
> 6) Where a device shows up in the device tree is not considered ABI,
> kernel devs are free and happy to add new layers to the tree where
> necessary. That means in the X11 udev code you cannot use
> udev_device_get_parent() to check the parent device of the wacom tablet,
> assuming it was a pnp device. This is borked and will break at any
> time. Use udev_device_get_parent_with_subsystem_devtype() instead, or
> much better: assign the properties you are looking for to the serial
> device itself, via the udev rule, by inheriting them as necessary.

This is something that affects the usb devices equally. We really need
to get to the NAME and PRODUCT properties on the parent of the event
node. Do you have a suggestion how that would be better solved in udev
rules?

--
Dan