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.
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?
Patrick
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);
