On Wed, 13 Jul 2011, Andiry Xu wrote:
> This commit gets BOS(Binary Device Object Store) descriptor set for Super
> Speed devices and High Speed devices which support BOS descriptor.
>
> BOS descriptor is used to report additional USB device-level capabilities
> that are not reported via the Device descriptor. By getting BOS descriptor
> set, driver can check device's device-level capability such as LPM
> capability.
I don't like this patch. It is wasteful. See the comments below.
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 5701e85..cc54e57 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -916,6 +916,149 @@ int usb_get_device_descriptor(struct usb_device *dev, unsigned int size)
> return ret;
> }
>
> +void usb_release_bos_descriptor(struct usb_device *dev)
> +{
> + if (dev->bos) {
> + kfree(dev->bos->ext_cap_desc);
> + kfree(dev->bos->ss_cap_desc);
> + kfree(dev->bos->ss_id_desc);
> + kfree(dev->bos);
> + dev->bos = NULL;
> + }
> +}
> +
> +/* Get BOS descriptor set */
> +int usb_get_bos_descriptor(struct usb_device *dev)
> +{
These routines involve parsing complex descriptors; therefore they
belong in config.c rather than message.c.
> + struct device *ddev = &dev->dev;
> + struct usb_bos_descriptor *bos;
> + struct usb_dev_cap_header *cap;
> + unsigned char *buffer, *header;
> + int length, total_len, num, i;
> + int ret;
> +
> + bos = kzalloc(sizeof(struct usb_bos_descriptor), GFP_KERNEL);
> + if (!bos)
> + return -ENOMEM;
> +
> + /* Get BOS descriptor */
> + ret = usb_get_descriptor(dev, USB_DT_BOS, 0, bos, USB_DT_BOS_SIZE);
> + if (ret < USB_DT_BOS_SIZE) {
> + dev_err(ddev, "unable to get BOS descriptor\n");
> + goto err1;
> + }
> +
> + dev->bos = kzalloc(sizeof(struct usb_host_bos), GFP_KERNEL);
> + if (!dev->bos) {
> + ret = -ENOMEM;
> + goto err1;
> + }
> + memcpy(&dev->bos->desc, bos, USB_DT_BOS_SIZE);
This memcpy wouldn't be necessary if you reorganized the data
structure. See below.
> + length = (int)le16_to_cpu(bos->bLength);
> + total_len = (int)le16_to_cpu(bos->wTotalLength);
> + num = (int)le16_to_cpu(bos->bNumDeviceCaps);
At this point you are finished with bos. It should be deallocated now,
not later.
> + /* Now let's get the whole BOS descriptor set */
> + buffer = kzalloc(total_len, GFP_KERNEL);
> + if (!buffer) {
> + ret = -ENOMEM;
> + goto err2;
> + }
> +
> + ret = usb_get_descriptor(dev, USB_DT_BOS, 0, buffer, total_len);
> + if (ret < total_len) {
> + dev_err(ddev, "unable to get BOS descriptor set\n");
> + goto err3;
> + }
> +
> + header = buffer;
If dev->bos->desc were a pointer to a descriptor, you could simply set
dev->bos->desc = buffer.
> + for (i = 0; i < num; i++) {
> + if (total_len < length)
> + break;
> + total_len -= length;
> +
> + header += length;
> + cap = (struct usb_dev_cap_header *)header;
> + length = (int)le16_to_cpu(cap->bLength);
> +
> + if (cap->bDescriptorType != USB_DT_DEVICE_CAPABILITY) {
> + dev_warn(ddev, "descriptor type invalid, skip\n");
> + continue;
> + }
> +
> + switch (cap->bDevCapabilityType) {
> + case USB_CAP_TYPE_WIRELESS_USB:
> + /* Wireless USB cap descriptor is handled by wusb */
> + break;
> + case USB_CAP_TYPE_EXT:
> + if (dev->bos->ext_cap_desc)
> + break;
> + dev->bos->ext_cap_desc =
> + kzalloc(sizeof(struct usb_ext_cap_descriptor),
> + GFP_KERNEL);
> + if (!dev->bos->ext_cap_desc) {
> + ret = -ENOMEM;
> + goto err4;
> + }
> + memcpy(dev->bos->ext_cap_desc,
> + (struct usb_ext_cap_descriptor *)header,
> + USB_DT_USB_EXT_CAP_SIZE);
Why do you need to allocate a new area of memory just to hold a copy
this data? Simply set dev->bos->ext_cap_desc equal to header.
> + break;
> + case USB_SS_CAP_TYPE:
> + if (dev->bos->ss_cap_desc)
> + break;
> + dev->bos->ss_cap_desc =
> + kzalloc(sizeof(struct usb_ss_cap_descriptor),
> + GFP_KERNEL);
> + if (!dev->bos->ss_cap_desc) {
> + ret = -ENOMEM;
> + goto err4;
> + }
> + memcpy(dev->bos->ss_cap_desc,
> + (struct usb_ss_cap_descriptor *)header,
> + USB_DT_USB_SS_CAP_SIZE);
Same here.
> + break;
> + case CONTAINER_ID_TYPE:
> + if (dev->bos->ss_id_desc)
> + break;
> + dev->bos->ss_id_desc =
> + kzalloc(sizeof(struct usb_ss_container_id_descriptor),
> + GFP_KERNEL);
> + if (!dev->bos->ss_id_desc) {
> + ret = -ENOMEM;
> + goto err4;
> + }
> + memcpy(dev->bos->ss_id_desc,
> + (struct usb_ss_container_id_descriptor *)header,
> + USB_DT_USB_SS_CONTN_ID_SIZE);
And here.
> + break;
> + default:
> + break;
> + }
> + }
> +
> + kfree(bos);
> + kfree(buffer);
With the changes mentioned above, you would not deallocate buffer here.
Instead, the whole buffer would be deallocated in
usb_release_bos_descriptor() above.
> +
> + return 0;
> +
> +err4:
> + kfree(dev->bos->ext_cap_desc);
> + kfree(dev->bos->ss_cap_desc);
> + kfree(dev->bos->ss_id_desc);
> +err3:
> + kfree(buffer);
> +err2:
> + kfree(dev->bos);
> + dev->bos = NULL;
> +err1:
> + kfree(bos);
> +
> + return ret;
> +}
> +
> /**
> * usb_get_status - issues a GET_STATUS call
> * @dev: the device whose status is being checked
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
