On Wed, Jul 20, 2011 at 08:23:15PM +0200, Lennart Poettering wrote: > On Wed, 20.07.11 06:08, Dan Nicholson ([email protected]) wrote: > > > > 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. > > Well, the current Wacom userspace rule adds in a pretty name in > userspace, which however is something we generally try to avoid, simply > because sooner or later people will ask that "Wacom Serial Tablet" shows > up as "Wacom Serielles Eingabetablett" on de_DE machines. Having pretty > names in such a low level database is problematic.
how is this different to assigning the name to ID_MODEL or whatever else in userspace? "Serial Wacom Tablet" beats the empty string. > > 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. > > Well, HAL is pretty good as an excercise how not do things ;-) > > Most devices use ID_MODEL and ID_MODEL_FROM_DATABASE as generic model > description strings. ok, I sat down yesterday and wrote the two small patches attached to parse ID_MODEL before the parent's NAME. and right now, it's not usable. NAME on the parent gives me NAME="Apple Wireless Trackpad" ID_MODEL on the device gives me ID_MODEL=BCM2045B I know which one I'd prefer in the logs... so unless I did something majorly wrong in the first patch (attached), I don't think we can switch to ID_MODEL without fixing udev first and requiring the new version. > > > 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? > > So, generally there are two approaches to this: > > a) write your C code that it is able to go up the tree if it looks for a > certain property > > or > > b) write your udev rules so that it inherits properties down the tree. > > I think b) is much smarter, and this is what most subsystems do. We do > that for sound cards (PulseAudio as a consumer) or block devices. > > Now, as it turns out /lib/udev/rules.d/60-persistent-input.rules already > includes the right lines to inherit these properties down the tree, so > you can actually just use ID_MODEL_ID and ID_PRODUCT_ID on the device > itself and it will just work. you're mixing two different things here: ID_MODEL vs NAME: decent device name. this currently doesn't work, see above. ID_MODEL_ID/ID_PRODUCT_ID vs scanf(): this works, see patch 2 attached. Either way, b) must first be fixed in udev. > Kay has now fixed the default udev rules not to inherit > USB metadata properties onto the devices connected to a bt dongle. I assume you're talking about 2ba5c37829c657a1bce3f3775fbaaf2131c1070b in the udev tree? That's the only commit that may be related. If so, can I assume this will be part of udev 173? Cheers, Peter
>From 63b6c7a6822d360860a4e5f01bb4b3ec65ec78c5 Mon Sep 17 00:00:00 2001 From: Peter Hutterer <[email protected]> Date: Wed, 20 Jul 2011 16:08:00 +1000 Subject: [PATCH 1/2] config: Prefer ID_MODEL over the parent's NAME NAME is the kernel-space value, ID_MODEL the userspace value. Prefer that if available, otherwise fall back to NAME. Signed-off-by: Peter Hutterer <[email protected]> --- config/udev.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-) diff --git a/config/udev.c b/config/udev.c index 9ac34ee..5cc071c 100644 --- a/config/udev.c +++ b/config/udev.c @@ -90,6 +90,9 @@ device_added(struct udev_device *udev_device) if (!options->key || !options->value) goto unwind; + name = udev_device_get_property_value(parent, "ID_MODEL"); + LOG_SYSATTR(path, "ID_MODEL", name); + parent = udev_device_get_parent(udev_device); if (parent) { const char *ppath = udev_device_get_devnode(parent); @@ -97,11 +100,13 @@ device_added(struct udev_device *udev_device) const char *pnp_id = udev_device_get_sysattr_value(parent, "id"); unsigned int usb_vendor, usb_model; - name = udev_device_get_sysattr_value(parent, "name"); - LOG_SYSATTR(ppath, "name", name); if (!name) { - name = udev_device_get_property_value(parent, "NAME"); - LOG_PROPERTY(ppath, "NAME", name); + name = udev_device_get_sysattr_value(parent, "name"); + LOG_SYSATTR(ppath, "name", name); + if (!name) { + name = udev_device_get_property_value(parent, "NAME"); + LOG_PROPERTY(ppath, "NAME", name); + } } if (pnp_id) -- 1.7.6
>From 95de9beff4e3940c3bd10ff4ec10614171060a82 Mon Sep 17 00:00:00 2001 From: Peter Hutterer <[email protected]> Date: Wed, 20 Jul 2011 16:19:13 +1000 Subject: [PATCH 2/2] config: construct the USB ID from ID_MODEL_ID and ID_VENDOR_ID_ID The scanf relies on the kernel never changing the format. Use the actual variables instead (if they exist). Signed-off-by: Peter Hutterer <[email protected]> --- config/udev.c | 28 ++++++++++++++++++++-------- 1 files changed, 20 insertions(+), 8 deletions(-) diff --git a/config/udev.c b/config/udev.c index 5cc071c..e21dc47 100644 --- a/config/udev.c +++ b/config/udev.c @@ -65,6 +65,7 @@ device_added(struct udev_device *udev_device) struct udev_list_entry *set, *entry; struct udev_device *parent; int rc; + const char *vendor, *model; path = udev_device_get_devnode(udev_device); @@ -93,12 +94,19 @@ device_added(struct udev_device *udev_device) name = udev_device_get_property_value(parent, "ID_MODEL"); LOG_SYSATTR(path, "ID_MODEL", name); + vendor = udev_device_get_property_value(parent, "ID_VENDOR_ID"); + LOG_PROPERTY(path, "ID_VENDOR_ID", vendor); + model = udev_device_get_property_value(parent, "ID_MODEL_ID"); + LOG_PROPERTY(path, "ID_MODEL_ID", model); + if (vendor && model) { + if (asprintf("%s:%s", vendor, model) == -1) + attrs.usb_id = NULL; + } + parent = udev_device_get_parent(udev_device); if (parent) { const char *ppath = udev_device_get_devnode(parent); - const char *product = udev_device_get_property_value(parent, "PRODUCT"); const char *pnp_id = udev_device_get_sysattr_value(parent, "id"); - unsigned int usb_vendor, usb_model; if (!name) { name = udev_device_get_sysattr_value(parent, "name"); @@ -114,12 +122,16 @@ device_added(struct udev_device *udev_device) LOG_SYSATTR(ppath, "id", pnp_id); /* construct USB ID in lowercase hex - "0000:ffff" */ - if (product && sscanf(product, "%*x/%4x/%4x/%*x", &usb_vendor, &usb_model) == 2) { - if (asprintf(&attrs.usb_id, "%04x:%04x", usb_vendor, usb_model) - == -1) - attrs.usb_id = NULL; - else - LOG_PROPERTY(path, "PRODUCT", product); + if (!attrs.usb_id) { + unsigned int usb_vendor, usb_model; + const char *product = udev_device_get_property_value(parent, "PRODUCT"); + if (product && sscanf(product, "%*x/%4x/%4x/%*x", &usb_vendor, &usb_model) == 2) { + if (asprintf(&attrs.usb_id, "%04x:%04x", usb_vendor, usb_model) + == -1) + attrs.usb_id = NULL; + else + LOG_PROPERTY(path, "PRODUCT", product); + } } } if (!name) -- 1.7.6
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
