Re: [Qemu-devel] [PATCH 1/2] usb-linux: fix device path aka physical port handling

[Markus Armbruster]

Good stuff, just a few questions.

Gerd Hoffmann <address@hidden> writes:

> The device path isn't just a number.  It specifies the physical port
> the device is connected to and in case the device is connected via
> usb hub you'll have two numbers there, like this: "5.1".  The first
> specifies the root port where the hub is plugged into, the second
> specifies the port number of the hub where the device is plugged in.
> With multiple hubs chained the string can become longer.

"5.1"?  Do you mean "5-1"?

I think you're talking about the file names in /sys/bus/usb/devices.
Kernel names them in drivers/usb/core/usb.c's usb_alloc_dev().

Roots:
                dev->devpath[0] = '0';
                dev_set_name(&dev->dev, "usb%d", bus->busnum);

Hubs:
                if (parent->devpath[0] == '0') {
                        snprintf(dev->devpath, sizeof dev->devpath,
                                "%d", port1);
                } else {
                        snprintf(dev->devpath, sizeof dev->devpath,
                                "%s.%d", parent->devpath, port1);
                }
                dev_set_name(&dev->dev, "%d-%s", bus->busnum, dev->devpath);

On my system:

$ ls /sys/bus/usb/devices
1-0:1.0  2-0:1.0  3-0:1.0  4-0:1.0  5-0:1.0  usb1  usb2  usb3  usb4  usb5

> This patch renames devpath to port and makes it a string.   It also
> adapts the sysfs parsing code accordingly.  The "info usbhost" monitor
> command now prints bus number, (os-assigned) device address and physical
> port for each device.
>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  usb-linux.c |   38 ++++++++++++++++++++------------------
>  1 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/usb-linux.c b/usb-linux.c
> index 72fe6f5..9543a69 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -62,7 +62,7 @@ struct usb_ctrlrequest {
>      uint16_t wLength;
>  };
>  
> -typedef int USBScanFunc(void *opaque, int bus_num, int addr, int devpath,
> +typedef int USBScanFunc(void *opaque, int bus_num, int addr, char *port,
>                          int class_id, int vendor_id, int product_id,
>                          const char *product_name, int speed);
>  
> @@ -79,6 +79,7 @@ typedef int USBScanFunc(void *opaque, int bus_num, int 
> addr, int devpath,
>  #define USBPROCBUS_PATH "/proc/bus/usb"
>  #define PRODUCT_NAME_SZ 32
>  #define MAX_ENDPOINTS 15
> +#define MAX_PORTLEN 8

Where does 8 come from?  For what it's worth, kernel's struct usb_device
has char devpath[16].

>  #define USBDEVBUS_PATH "/dev/bus/usb"
>  #define USBSYSBUS_PATH "/sys/bus/usb"
>  
> @@ -155,7 +156,7 @@ typedef struct USBHostDevice {
>      /* Host side address */
>      int bus_num;
>      int addr;
> -    int devpath;
> +    char port[MAX_PORTLEN];
>      struct USBAutoFilter match;
>  
>      QTAILQ_ENTRY(USBHostDevice) next;
> @@ -1092,7 +1093,7 @@ static int usb_linux_get_configuration(USBHostDevice *s)
>          char device_name[32], line[1024];
>          int configuration;
>  
> -        sprintf(device_name, "%d-%d", s->bus_num, s->devpath);
> +        sprintf(device_name, "%d-%s", s->bus_num, s->port);
>  
>          if (!usb_host_read_file(line, sizeof(line), "bConfigurationValue",
>                                  device_name)) {
> @@ -1138,7 +1139,7 @@ static uint8_t usb_linux_get_alt_setting(USBHostDevice 
> *s,
>          char device_name[64], line[1024];
>          int alt_setting;
>  
> -        sprintf(device_name, "%d-%d:%d.%d", s->bus_num, s->devpath,
> +        sprintf(device_name, "%d-%s:%d.%d", s->bus_num, s->port,
>                  (int)configuration, (int)interface);
>  
>          if (!usb_host_read_file(line, sizeof(line), "bAlternateSetting",
> @@ -1257,7 +1258,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
>  }
>  
>  static int usb_host_open(USBHostDevice *dev, int bus_num,
> -                         int addr, int devpath, const char *prod_name)
> +                         int addr, char *port, const char *prod_name)
>  {
>      int fd = -1, ret;
>      struct usbdevfs_connectinfo ci;
> @@ -1283,7 +1284,7 @@ static int usb_host_open(USBHostDevice *dev, int 
> bus_num,
>  
>      dev->bus_num = bus_num;
>      dev->addr = addr;
> -    dev->devpath = devpath;
> +    strcpy(dev->port, port);
>      dev->fd = fd;
>  
>      /* read the device description */
> @@ -1655,8 +1656,9 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc 
> *func)
>  {
>      DIR *dir = NULL;
>      char line[1024];
> -    int bus_num, addr, devpath, speed, class_id, product_id, vendor_id;
> +    int bus_num, addr, speed, class_id, product_id, vendor_id;
>      int ret = 0;
> +    char port[MAX_PORTLEN];
>      char product_name[512];
>      struct dirent *de;
>  
> @@ -1672,8 +1674,8 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc 
> *func)
>              if (!strncmp(de->d_name, "usb", 3)) {
>                  tmpstr += 3;
>              }

Sloppy parsing, but that's not your fault.

> -            if (sscanf(tmpstr, "%d-%d", &bus_num, &devpath) < 1) {
> -                goto the_end;
> +            if (sscanf(tmpstr, "%d-%7[0-9.]", &bus_num, port) < 2) {
> +                continue;
>              }
>  
>              if (!usb_host_read_file(line, sizeof(line), "devnum", 
> de->d_name)) {

The old sscanf() succeeds if at least one item is assigned, i.e. tmpstr
starts with an integer.  I suspect this is broken for roots.  Consider
d_name "usb1": tmpstr is "1", sscan() returns 1, and devpath remains
uninitialized.  It's passed to the func() callback.  Bug?  If yes, the
commit message should mention it.

The new sscan() succeeds only if both items are assigned, i.e. tmpstr
starts with an integer, '-', and up to 7 of [0-9.].  Unlike the old one,
it fails for roots.  Intentional?

[...]