Wed, 18 May 2016 19:08:29 +0200 Martin Pieuchot <[email protected]>
> 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.
Should not, but in reality.. especially some non-uniform sneaky USB
devices, are programmable and tinker accessible, often with intent.
Even if rare and/or non std usage, USB ID strings are not immutable.
> > 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);
> >
>
>
