ChangeSet 1.1608.84.6, 2004/03/08 15:07:27-08:00, stern@rowland.harvard.edu [PATCH] USB: Remove interface/altsettings assumption from audio driver This patch updates the USB audio class driver to use the usb_ifnum_to_if() and usb_altnum_to_altsetting() routines, thereby removing assumptions about which interface or altsetting is stored in which array entry. It also simplifies the driver's probe() routine by using the raw configuration descriptor already loaded into memory instead of reading the descriptor from the device. Now, either the current driver has a bug and never deallocates the buffer used to hold the descriptor, or else I've introduced a double-free error. There's no obvious place where the buffer gets freed, but it's hard to be certain. It would be good if someone could try out this patch. I can't test it, not having any USB audio devices handy. If the double-free error is present, it will show up when the device is disconnected and the configuration data is released. drivers/usb/class/audio.c | 135 ++++++++++++++++++++-------------------------- 1 files changed, 61 insertions(+), 74 deletions(-) diff -Nru a/drivers/usb/class/audio.c b/drivers/usb/class/audio.c --- a/drivers/usb/class/audio.c Tue Mar 16 15:03:38 2004 +++ b/drivers/usb/class/audio.c Tue Mar 16 15:03:38 2004 @@ -102,6 +102,9 @@ * 2003-04-08: Oliver Neukum (oliver@neukum.name): * Setting a configuration is done by usbcore and must not be overridden * 2004-02-27: Workaround for broken synch descriptors + * 2004-03-07: Alan Stern + * Add usb_ifnum_to_if() and usb_altnum_to_altsetting() support. + * Use the in-memory descriptors instead of reading them from the device. * */ @@ -143,8 +146,8 @@ * * How does the parsing work? First, all interfaces are searched * for an AudioControl class interface. If found, the config descriptor - * that belongs to the current configuration is fetched from the device. - * Then the HEADER descriptor is fetched. It contains a list of + * that belongs to the current configuration is searched and + * the HEADER descriptor is found. It contains a list of * all AudioStreaming and MIDIStreaming devices. This list is then walked, * and all AudioStreaming interfaces are classified into input and output * interfaces (according to the endpoint0 direction in altsetting1) (MIDIStreaming @@ -1514,7 +1517,6 @@ static int set_format_in(struct usb_audiodev *as) { struct usb_device *dev = as->state->usbdev; - struct usb_host_config *config = dev->actconfig; struct usb_host_interface *alts; struct usb_interface *iface; struct usbin *u = &as->usbin; @@ -1524,9 +1526,9 @@ unsigned char data[3]; int fmtnr, ret; - if (u->interface < 0 || u->interface >= config->desc.bNumInterfaces) + iface = usb_ifnum_to_if(dev, u->interface); + if (!iface) return 0; - iface = config->interface[u->interface]; fmtnr = find_format(as->fmtin, as->numfmtin, d->format, d->srate); if (fmtnr < 0) { @@ -1535,7 +1537,7 @@ } fmt = as->fmtin + fmtnr; - alts = &iface->altsetting[fmt->altsetting]; + alts = usb_altnum_to_altsetting(iface, fmt->altsetting); u->format = fmt->format; u->datapipe = usb_rcvisocpipe(dev, alts->endpoint[0].desc.bEndpointAddress & 0xf); u->syncpipe = u->syncinterval = 0; @@ -1556,7 +1558,8 @@ d->srate = fmt->sratelo; if (d->srate > fmt->sratehi) d->srate = fmt->sratehi; - dprintk((KERN_DEBUG "usbaudio: set_format_in: usb_set_interface %u %u\n", alts->desc.bInterfaceNumber, fmt->altsetting)); + dprintk((KERN_DEBUG "usbaudio: set_format_in: usb_set_interface %u %u\n", + u->interface, fmt->altsetting)); if (usb_set_interface(dev, alts->desc.bInterfaceNumber, fmt->altsetting) < 0) { printk(KERN_WARNING "usbaudio: usb_set_interface failed, device %d interface %d altsetting %d\n", dev->devnum, u->interface, fmt->altsetting); @@ -1603,7 +1606,6 @@ static int set_format_out(struct usb_audiodev *as) { struct usb_device *dev = as->state->usbdev; - struct usb_host_config *config = dev->actconfig; struct usb_host_interface *alts; struct usb_interface *iface; struct usbout *u = &as->usbout; @@ -1613,9 +1615,9 @@ unsigned char data[3]; int fmtnr, ret; - if (u->interface < 0 || u->interface >= config->desc.bNumInterfaces) + iface = usb_ifnum_to_if(dev, u->interface); + if (!iface) return 0; - iface = config->interface[u->interface]; fmtnr = find_format(as->fmtout, as->numfmtout, d->format, d->srate); if (fmtnr < 0) { @@ -1625,7 +1627,7 @@ fmt = as->fmtout + fmtnr; u->format = fmt->format; - alts = &iface->altsetting[fmt->altsetting]; + alts = usb_altnum_to_altsetting(iface, fmt->altsetting); u->datapipe = usb_sndisocpipe(dev, alts->endpoint[0].desc.bEndpointAddress & 0xf); u->syncpipe = u->syncinterval = 0; if ((alts->endpoint[0].desc.bmAttributes & 0x0c) == 0x04) { @@ -1652,7 +1654,8 @@ d->srate = fmt->sratelo; if (d->srate > fmt->sratehi) d->srate = fmt->sratehi; - dprintk((KERN_DEBUG "usbaudio: set_format_out: usb_set_interface %u %u\n", alts->desc.bInterfaceNumber, fmt->altsetting)); + dprintk((KERN_DEBUG "usbaudio: set_format_out: usb_set_interface %u %u\n", + u->interface, fmt->altsetting)); if (usb_set_interface(dev, u->interface, fmt->altsetting) < 0) { printk(KERN_WARNING "usbaudio: usb_set_interface failed, device %d interface %d altsetting %d\n", dev->devnum, u->interface, fmt->altsetting); @@ -2701,7 +2704,6 @@ struct usb_audiodev *as = (struct usb_audiodev *)file->private_data; struct usb_audio_state *s; struct usb_device *dev; - struct usb_interface *iface; lock_kernel(); s = as->state; @@ -2711,19 +2713,15 @@ down(&open_sem); if (file->f_mode & FMODE_WRITE) { usbout_stop(as); - if (dev && as->usbout.interface >= 0) { - iface = dev->actconfig->interface[as->usbout.interface]; - usb_set_interface(dev, iface->altsetting->desc.bInterfaceNumber, 0); - } + if (dev && as->usbout.interface >= 0) + usb_set_interface(dev, as->usbout.interface, 0); dmabuf_release(&as->usbout.dma); usbout_release(as); } if (file->f_mode & FMODE_READ) { usbin_stop(as); - if (dev && as->usbin.interface >= 0) { - iface = dev->actconfig->interface[as->usbin.interface]; - usb_set_interface(dev, iface->altsetting->desc.bInterfaceNumber, 0); - } + if (dev && as->usbin.interface >= 0) + usb_set_interface(dev, as->usbin.interface, 0); dmabuf_release(&as->usbin.dma); usbin_release(as); } @@ -2828,12 +2826,11 @@ { struct usb_device *dev = s->usbdev; struct usb_audiodev *as; - struct usb_host_config *config = dev->actconfig; struct usb_host_interface *alts; struct usb_interface *iface; struct audioformat *fp; unsigned char *fmt, *csep; - unsigned int i, j, k, format; + unsigned int i, j, k, format, idx; if (!(as = kmalloc(sizeof(struct usb_audiodev), GFP_KERNEL))) return; @@ -2874,9 +2871,10 @@ /* search for input formats */ if (asifin >= 0) { as->usbin.flags = FLG_CONNECTED; - iface = config->interface[asifin]; - for (i = 0; i < iface->num_altsetting; i++) { - alts = &iface->altsetting[i]; + iface = usb_ifnum_to_if(dev, asifin); + for (idx = 0; idx < iface->num_altsetting; idx++) { + alts = &iface->altsetting[idx]; + i = alts->desc.bAlternateSetting; if (alts->desc.bInterfaceClass != USB_CLASS_AUDIO || alts->desc.bInterfaceSubClass != 2) continue; if (alts->desc.bNumEndpoints < 1) { @@ -2955,14 +2953,15 @@ /* search for output formats */ if (asifout >= 0) { as->usbout.flags = FLG_CONNECTED; - iface = config->interface[asifout]; - for (i = 0; i < iface->num_altsetting; i++) { - alts = &iface->altsetting[i]; + iface = usb_ifnum_to_if(dev, asifout); + for (idx = 0; idx < iface->num_altsetting; idx++) { + alts = &iface->altsetting[idx]; + i = alts->desc.bAlternateSetting; if (alts->desc.bInterfaceClass != USB_CLASS_AUDIO || alts->desc.bInterfaceSubClass != 2) continue; if (alts->desc.bNumEndpoints < 1) { /* altsetting 0 should never have iso EPs */ - if (alts->desc.bAlternateSetting != 0) + if (i != 0) printk(KERN_ERR "usbaudio: device %u interface %u altsetting %u does not have an endpoint\n", dev->devnum, asifout, i); continue; @@ -3659,8 +3658,8 @@ static struct usb_audio_state *usb_audio_parsecontrol(struct usb_device *dev, unsigned char *buffer, unsigned int buflen, unsigned int ctrlif) { struct usb_audio_state *s; - struct usb_host_config *config = dev->actconfig; struct usb_interface *iface; + struct usb_host_interface *alt; unsigned char ifin[USB_MAXINTERFACES], ifout[USB_MAXINTERFACES]; unsigned char *p1; unsigned int i, j, k, numifin = 0, numifout = 0; @@ -3689,54 +3688,63 @@ dev->devnum, ctrlif); for (i = 0; i < p1[7]; i++) { j = p1[8+i]; - if (j >= config->desc.bNumInterfaces) { + iface = usb_ifnum_to_if(dev, j); + if (!iface) { printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u interface %u does not exist\n", dev->devnum, ctrlif, j); continue; } - iface = config->interface[j]; - if (iface->altsetting[0].desc.bInterfaceClass != USB_CLASS_AUDIO) { - printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u interface %u is not an AudioClass interface\n", - dev->devnum, ctrlif, j); + if (iface->num_altsetting == 1) { + printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u has only 1 altsetting.\n", dev->devnum, ctrlif); continue; } - if (iface->altsetting[0].desc.bInterfaceSubClass == 3) { - printk(KERN_INFO "usbaudio: device %d audiocontrol interface %u interface %u MIDIStreaming not supported\n", + alt = usb_altnum_to_altsetting(iface, 0); + if (!alt) { + printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u interface %u has no altsetting 0\n", dev->devnum, ctrlif, j); continue; } - if (iface->altsetting[0].desc.bInterfaceSubClass != 2) { - printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u interface %u invalid AudioClass subtype\n", + if (alt->desc.bInterfaceClass != USB_CLASS_AUDIO) { + printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u interface %u is not an AudioClass interface\n", dev->devnum, ctrlif, j); continue; } - if (iface->num_altsetting == 0) { - printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u has no working interface.\n", dev->devnum, ctrlif); + if (alt->desc.bInterfaceSubClass == 3) { + printk(KERN_INFO "usbaudio: device %d audiocontrol interface %u interface %u MIDIStreaming not supported\n", + dev->devnum, ctrlif, j); continue; } - if (iface->num_altsetting == 1) { - printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u has only 1 altsetting.\n", dev->devnum, ctrlif); + if (alt->desc.bInterfaceSubClass != 2) { + printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u interface %u invalid AudioClass subtype\n", + dev->devnum, ctrlif, j); continue; } - if (iface->altsetting[0].desc.bNumEndpoints > 0) { + if (alt->desc.bNumEndpoints > 0) { /* Check all endpoints; should they all have a bandwidth of 0 ? */ - for (k = 0; k < iface->altsetting[0].desc.bNumEndpoints; k++) { - if (iface->altsetting[0].endpoint[k].desc.wMaxPacketSize > 0) { + for (k = 0; k < alt->desc.bNumEndpoints; k++) { + if (alt->endpoint[k].desc.wMaxPacketSize > 0) { printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u endpoint %d does not have 0 bandwidth at alt[0]\n", dev->devnum, ctrlif, k); break; } } - if (k < iface->altsetting[0].desc.bNumEndpoints) + if (k < alt->desc.bNumEndpoints) continue; } - if (iface->altsetting[1].desc.bNumEndpoints < 1) { + + alt = usb_altnum_to_altsetting(iface, 1); + if (!alt) { + printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u interface %u has no altsetting 1\n", + dev->devnum, ctrlif, j); + continue; + } + if (alt->desc.bNumEndpoints < 1) { printk(KERN_ERR "usbaudio: device %d audiocontrol interface %u interface %u has no endpoint\n", dev->devnum, ctrlif, j); continue; } /* note: this requires the data endpoint to be ep0 and the optional sync ep to be ep1, which seems to be the case */ - if (iface->altsetting[1].endpoint[0].desc.bEndpointAddress & USB_DIR_IN) { + if (alt->endpoint[0].desc.bEndpointAddress & USB_DIR_IN) { if (numifin < USB_MAXINTERFACES) { ifin[numifin++] = j; usb_driver_claim_interface(&usb_audio_driver, iface, (void *)-1); @@ -3783,12 +3791,9 @@ const struct usb_device_id *id) { struct usb_device *dev = interface_to_usbdev (intf); - struct usb_host_config *config = dev->actconfig; struct usb_audio_state *s; unsigned char *buffer; - unsigned char buf[8]; - unsigned int i, buflen; - int ret; + unsigned int buflen; #if 0 printk(KERN_DEBUG "usbaudio: Probing if %i: IC %x, ISC %x\n", ifnum, @@ -3800,26 +3805,8 @@ * audiocontrol interface found * find which configuration number is active */ - i = dev->actconfig - config; - - ret = usb_get_descriptor(dev, USB_DT_CONFIG, i, buf, 8); - if (ret < 0) { - printk(KERN_ERR "usbaudio: cannot get first 8 bytes of config descriptor %d of device %d (error %d)\n", i, dev->devnum, ret); - return -EIO; - } - if (buf[1] != USB_DT_CONFIG || buf[0] < 9) { - printk(KERN_ERR "usbaudio: invalid config descriptor %d of device %d\n", i, dev->devnum); - return -EIO; - } - buflen = buf[2] | (buf[3] << 8); - if (!(buffer = kmalloc(buflen, GFP_KERNEL))) - return -ENOMEM; - ret = usb_get_descriptor(dev, USB_DT_CONFIG, i, buffer, buflen); - if (ret < 0) { - kfree(buffer); - printk(KERN_ERR "usbaudio: cannot get config descriptor %d of device %d (error %d)\n", i, dev->devnum, ret); - return -EIO; - } + buffer = dev->rawdescriptors[dev->actconfig - dev->config]; + buflen = dev->actconfig->desc.wTotalLength; s = usb_audio_parsecontrol(dev, buffer, buflen, intf->altsetting->desc.bInterfaceNumber); if (s) { usb_set_intfdata (intf, s);