[PATCH] Fix udev population of input device USB product IDs

[Peter Hutterer]

On Thu, Aug 26, 2010 at 06:36:01PM -0400, Chase Douglas wrote:
> From: Chase Douglas <chase.douglas at ubuntu.com>
> 
> The udev device_added function takes the vendor and model IDs of added
> devices and converts them into an attribute that can be matched for by
> an InputClass configuration. Currently, the udev mechanism is broken
> because it looks for the product ID on evdev event nodes instead of the
> input node.

at least on my system, ID_VENDOR_ID is provided on the event node and
ID_MODEL* is the _name_ of the device. The bug I noticed here was that
instead of ID_VENDOR_MODEL this should probably be just ID_MODEL_ID.

> This patch reads the product ID from the PRODUCT property of the parent
> of the added device. It has been tested on a bluetooth trackpad and a
> builtin touchscreen connected over USB.

right, but AFAICT, this patch always provides the hex IDs of the product.
xorg.conf documentation for MatchProduct claims

       MatchProduct  "matchproduct"
              This entry can be used to check if the substring  "matchproduct"
              occurs in the device's product name.

so it seems there are two overlapping issues at hand: one that the usb_id
should either be constructed like you propose or from
ID_VENDOR_ID:ID_MODEL_ID, and the other one that ID_VENDOR_MODEL should just
be ID_VENDOR_MODEL. do I read this right? Dan, any comments?


> Signed-off-by: Chase Douglas <chase.douglas at canonical.com>
> ---
> I don't know if this is correct for USB/bluetooth non-evdev devices. Are
> there any? Do we care?
> 
>  config/udev.c |   45 ++++++++++++++++++++++++++++-----------------
>  1 files changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/config/udev.c b/config/udev.c
> index 9934490..5e60052 100644
> --- a/config/udev.c
> +++ b/config/udev.c
> @@ -58,7 +58,7 @@ device_added(struct udev_device *udev_device)
>      char *config_info = NULL;
>      const char *syspath;
>      const char *tags_prop;
> -    const char *usb_vendor = NULL, *usb_model = NULL;
> +    int usb_vendor, usb_model;

could move this one down into the if (parent) block, it's not used outside
anymore.

>      const char *key, *value, *tmp;
>      InputOption *options = NULL, *tmpo;
>      InputAttributes attrs = {};
> @@ -94,6 +94,9 @@ device_added(struct udev_device *udev_device)
>      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 *cur;
> +        char *next;
>  
>          name = udev_device_get_sysattr_value(parent, "name");
>          LOG_SYSATTR(ppath, "name", name);
> @@ -104,7 +107,31 @@ device_added(struct udev_device *udev_device)
>  
>          attrs.pnp_id = udev_device_get_sysattr_value(parent, "id");
>          LOG_SYSATTR(ppath, "id", attrs.pnp_id);
> +
> +        /* construct USB ID in lowercase hex - "0000:ffff" */
> +        if (!product)
> +            goto noproduct;
> +
> +        cur = strchr(product, '/');
> +        if (!cur)
> +            goto noproduct;
> +
> +        usb_vendor = strtoul(cur + 1, &next, 16);
> +        if (!next || next == cur + 1 || usb_vendor > 0xffff)
> +            goto noproduct;
> +
> +        cur = next;
> +        usb_model = strtoul(cur + 1, &next, 16);
> +        if (!next || next == cur + 1 || usb_model > 0xffff)
> +            goto noproduct;

sscanf() may be a simpler approach.

> +
> +        attrs.usb_id = Xprintf("%04x:%04x", usb_vendor, usb_model);
> +        if (attrs.usb_id)
> +            LOG_PROPERTY(path, "PRODUCT", product);
>      }
> +
> +noproduct:
> +
>      if (!name)
>          name = "(unnamed)";
>      else
> @@ -152,12 +179,6 @@ device_added(struct udev_device *udev_device)
>          } else if (!strcmp(key, "ID_VENDOR")) {
>              LOG_PROPERTY(path, key, value);
>              attrs.vendor = value;
> -        } else if (!strcmp(key, "ID_VENDOR_ID")) {
> -            LOG_PROPERTY(path, key, value);
> -            usb_vendor = value;
> -        } else if (!strcmp(key, "ID_VENDOR_MODEL")) {
> -            LOG_PROPERTY(path, key, value);
> -            usb_model = value;
>          } else if (!strcmp(key, "ID_INPUT_KEY")) {
>              LOG_PROPERTY(path, key, value);
>              attrs.flags |= ATTR_KEYBOARD;
> @@ -179,16 +200,6 @@ device_added(struct udev_device *udev_device)
>          }
>      }
>  
> -    /* construct USB ID in lowercase hex - "0000:ffff" */
> -    if (usb_vendor && usb_model) {
> -        attrs.usb_id = Xprintf("%s:%s", usb_vendor, usb_model);
> -        if (attrs.usb_id) {
> -            char *cur;
> -            for (cur = attrs.usb_id; *cur; cur++)
> -                *cur = tolower(*cur);
> -        }
> -    }
> -
>      LogMessage(X_INFO, "config/udev: Adding input device %s (%s)\n",
>                 name, path);
>      rc = NewInputDeviceRequest(options, &attrs, &dev);
> -- 
> 1.7.1