On 18/05/16(Wed) 18:28, Patrick Wildt wrote:
> Hi,
>
> I had the pleasure of debugging a USB mass storage device that showed
> interesting behaviour when used with our stack and in combination with
> the blink(1) usb device.
>
> The blink(1)-tool depends on libusb. Libusb calls ioctl(USB_DEVICEINFO)
> plenty of times to get a map of the USB tree. Per ioctl, three USB
> requests are created (to read: vendor, product, serial). This kind of
> floods all connected USB devices.
>
> I have a hotplugd script that waits for usb mass storage devices, reads
> the disklabel, fscks and tries to mount a partition. Due to the ioctl,
> one of those calls, which mostly only read from the storage device,
> freeze until they later time out. The flood of read requests caused
> by the ioctl seem to make the mass storage halt its operation.
>
> The same issue can be reproduced by calling usbdevs(8) in a loop. It
> uses the same ioctl and "breaks" this usb mass storage.
>
> An idea is to cache those three strings, so that the ioctl does not
> create any requests. NetBSD seems to have implemented a caching
> mechanism to fix a similar issue. Though they have not enabled the
> caching for that specific ioctl.
They did this caching because their stack call the function multiple
times during the attachment of a device.
> The idea of this diff, adapted from NetBSD, is that we read out the
> strings early on when the device is found, store a copy of those per
> device. Then whenever it's needed it can be used.
>
> ok? Comments?
ok mpi@
I'd also like to see if we can always use the cache. Because it is
unlikely that a vendor or product name will change.
> diff --git sys/dev/usb/usb.c sys/dev/usb/usb.c
> index 732ecfd..c198bf0 100644
> --- sys/dev/usb/usb.c
> +++ sys/dev/usb/usb.c
> @@ -520,7 +520,7 @@ usb_fill_di_task(void *arg)
> if (dev == NULL)
> return;
>
> - usbd_fill_deviceinfo(dev, di, 1);
> + usbd_fill_deviceinfo(dev, di, 0);
> }
>
> void
> diff --git sys/dev/usb/usb_subr.c sys/dev/usb/usb_subr.c
> index a255793..fef6d52 100644
> --- sys/dev/usb/usb_subr.c
> +++ sys/dev/usb/usb_subr.c
> @@ -63,6 +63,7 @@ usbd_status usbd_set_config(struct usbd_device *, int);
> void usbd_devinfo(struct usbd_device *, int, char *, size_t);
> void usbd_devinfo_vp(struct usbd_device *, char *, size_t,
> char *, size_t, int);
> +void usbd_get_device_string(struct usbd_device *, uByte, char **);
> char *usbd_get_string(struct usbd_device *, int, char *, size_t);
> int usbd_getnewaddr(struct usbd_bus *);
> int usbd_print(void *, const char *);
> @@ -212,6 +213,28 @@ usbd_trim_spaces(char *p)
> }
>
> void
> +usbd_get_device_string(struct usbd_device *dev, uByte index, char **buf)
> +{
> + char *b = malloc(USB_MAX_STRING_LEN, M_USB, M_NOWAIT);
> + if (b != NULL) {
> + usbd_get_string(dev, index, b, USB_MAX_STRING_LEN);
> + usbd_trim_spaces(b);
> + }
> + *buf = b;
> +}
> +
> +void
> +usbd_get_device_strings(struct usbd_device *dev)
> +{
> + usbd_get_device_string(dev, dev->ddesc.iManufacturer,
> + &dev->vendor);
> + usbd_get_device_string(dev, dev->ddesc.iProduct,
> + &dev->product);
> + usbd_get_device_string(dev, dev->ddesc.iSerialNumber,
> + &dev->serial);
> +}
> +
> +void
> usbd_devinfo_vp(struct usbd_device *dev, char *v, size_t vl,
> char *p, size_t pl, int usedev)
> {
> @@ -232,6 +255,11 @@ usbd_devinfo_vp(struct usbd_device *dev, char *v, size_t
> vl,
> usbd_trim_spaces(vendor);
> product = usbd_get_string(dev, udd->iProduct, p, pl);
> usbd_trim_spaces(product);
> + } else {
> + if (dev->vendor != NULL)
> + vendor = dev->vendor;
> + if (dev->product != NULL)
> + product = dev->product;
> }
> #ifdef USBVERBOSE
> if (vendor == NULL || product == NULL) {
> @@ -294,7 +322,7 @@ usbd_devinfo(struct usbd_device *dev, int showclass, char
> *base, size_t len)
> char *cp = base;
> int bcdDevice, bcdUSB;
>
> - usbd_devinfo_vp(dev, vendor, sizeof vendor, product, sizeof product, 1);
> + usbd_devinfo_vp(dev, vendor, sizeof vendor, product, sizeof product, 0);
> snprintf(cp, len, "\"%s %s\"", vendor, product);
> cp += strlen(cp);
> if (showclass) {
> @@ -1188,6 +1216,8 @@ usbd_new_device(struct device *parent, struct usbd_bus
> *bus, int depth,
> DPRINTF(("usbd_new_device: new dev (addr %d), dev=%p, parent=%p\n",
> addr, dev, parent));
>
> + usbd_get_device_strings(dev);
> +
> err = usbd_probe_and_attach(parent, dev, port, addr);
> if (err) {
> usb_free_device(dev);
> @@ -1304,8 +1334,13 @@ usbd_fill_deviceinfo(struct usbd_device *dev, struct
> usb_device_info *di,
> di->udi_nports = 0;
>
> bzero(di->udi_serial, sizeof(di->udi_serial));
> - usbd_get_string(dev, dev->ddesc.iSerialNumber, di->udi_serial,
> - sizeof(di->udi_serial));
> + if (!usedev && dev->serial != NULL) {
> + strlcpy(di->udi_serial, dev->serial,
> + sizeof(di->udi_serial));
> + } else {
> + usbd_get_string(dev, dev->ddesc.iSerialNumber,
> + di->udi_serial, sizeof(di->udi_serial));
> + }
> }
>
> /* Retrieve a complete descriptor for a certain device and index. */
> @@ -1368,6 +1403,13 @@ usb_free_device(struct usbd_device *dev)
> free(dev->subdevs, M_USB, 0);
> dev->bus->devices[dev->address] = NULL;
>
> + if (dev->vendor != NULL)
> + free(dev->vendor, M_USB, USB_MAX_STRING_LEN);
> + if (dev->product != NULL)
> + free(dev->product, M_USB, USB_MAX_STRING_LEN);
> + if (dev->serial != NULL)
> + free(dev->serial, M_USB, USB_MAX_STRING_LEN);
> +
> free(dev, M_USB, 0);
> }
>
> diff --git sys/dev/usb/usbdivar.h sys/dev/usb/usbdivar.h
> index 3a7bac3..912234f 100644
> --- sys/dev/usb/usbdivar.h
> +++ sys/dev/usb/usbdivar.h
> @@ -150,6 +150,10 @@ struct usbd_device {
> struct usbd_hub *hub; /* only if this is a hub */
> struct device **subdevs; /* sub-devices, 0 terminated */
> int ndevs; /* # of subdevs */
> +
> + char *serial; /* serial number, can be NULL */
> + char *vendor; /* vendor string, can be NULL */
> + char *product; /* product string, can be NULL */
> };
>
> struct usbd_interface {
> @@ -229,6 +233,7 @@ void usbd_dump_pipe(struct usbd_pipe *);
>
> /* Routines from usb_subr.c */
> int usbctlprint(void *, const char *);
> +void usbd_get_device_strings(struct usbd_device *);
> void usb_delay_ms(struct usbd_bus *, u_int);
> usbd_status usbd_port_disown_to_1_1(struct usbd_device *, int);
> int usbd_reset_port(struct usbd_device *, int);
>
