On Wed, 13 Jun 2012, Lan Tianyu wrote:
> Move child's pointer to the struct usb_hub_port since the child device
> is directly associated with the port. Provide usb_get_hub_child()
> to get child's pointer and macrio macro usb_get_hub_each_child to
> iterate all child devices on the hub. The child device's refcount will
> be increased before return. The caller should invoke usb_put_dev() to
> decrease the device's refcount when the child will not be used.
>
> In the usb_get_hub_child(), still check the port1 param since the
> usb_get_hub_each_child() will pass maxchild+1 as port1 at end of last
> loop.
>
> v3
> add marco usb_get_hub_each_child() to iterate all hub devices
> usb_get_hub_child() increases child's refcount before return
> --- a/drivers/usb/core/devices.c
> +++ b/drivers/usb/core/devices.c
> @@ -589,20 +590,20 @@ static ssize_t usb_device_dump(char __user **buffer, size_t *nbytes,
> free_pages((unsigned long)pages_start, 1);
>
> /* Now look at all of this device's children. */
> - for (chix = 0; chix < usbdev->maxchild; chix++) {
> - struct usb_device *childdev = usbdev->children[chix];
> -
> + usb_get_hub_each_child(usbdev, chix, childdev)
> + pr_info("%s: %d\n", __func__, chix);
> if (childdev) {
> usb_lock_device(childdev);
> ret = usb_device_dump(buffer, nbytes, skip_bytes,
> file_offset, childdev, bus,
> - level + 1, chix, ++cnt);
> + level + 1, chix - 1, ++cnt);
> usb_unlock_device(childdev);
> + usb_put_dev(childdev);
> if (ret == -EFAULT)
> return total_written;
> total_written += ret;
> }
> - }
> +
This is wrong. Without any {}'s, only the pr_info() statement is
inside the loop.
Why did you add the pr_info() statement, anyway?
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4904,3 +4905,31 @@ void usb_queue_reset_device(struct usb_interface *iface)
> schedule_work(&iface->reset_ws);
> }
> EXPORT_SYMBOL_GPL(usb_queue_reset_device);
> +
> +/**
> + * usb_get_hub_child - Get the pointer of child device
> + * attached to the port which is specified by @port1.
> + * @hdev: USB device belonging to the usb hub
> + * @port1: port num to indicate which port the child device
> + * is attached to.
> + *
> + * USB drivers call this function to get hub's child device
> + * pointer. The child device's refcount will be increased
> + * before return. The caller should invoke usb_put_dev() to
> + * decrease the device's refcount when the child will not be
> + * used.
Why bother to do this? If there's a race, and the child could be
removed after this function returns, then in fact the child could be
removed even while this function is running. usb_get_dev() won't
protect against that.
> + *
> + * Return NULL if input param is invalid and
> + * child's usb_device pointer if non-NULL.
> + */
> +struct usb_device *usb_get_hub_child(struct usb_device *hdev,
> + int port1)
> +{
> + struct usb_hub *hub = hdev_to_hub(hdev);
> +
> + if (port1 < 1 || port1 > hdev->maxchild)
> + return NULL;
> + return usb_get_dev(hub->port_data[port1 - 1].child);
What is the reason for calling usb_get_dev() here? If the old code
didn't need it, why does the new code?
> --- a/drivers/usb/host/r8a66597-hcd.c
> +++ b/drivers/usb/host/r8a66597-hcd.c
> @@ -2033,18 +2033,18 @@ static int r8a66597_get_frame(struct usb_hcd *hcd)
> static void collect_usb_address_map(struct usb_device *udev, unsigned long *map)
> {
> int chix;
> + struct usb_device *childdev;
>
> if (udev->state == USB_STATE_CONFIGURED &&
> udev->parent && udev->parent->devnum > 1 &&
> udev->parent->descriptor.bDeviceClass == USB_CLASS_HUB)
> map[udev->devnum/32] |= (1 << (udev->devnum % 32));
>
> - for (chix = 0; chix < udev->maxchild; chix++) {
> - struct usb_device *childdev = udev->children[chix];
> -
> - if (childdev)
> + usb_get_hub_each_child(udev, chix, childdev)
> + if (childdev) {
> collect_usb_address_map(childdev, map);
> - }
> + usb_put_dev(childdev);
> + }
The braces you removed aren't strictly necessary, but the code looks
funny without them. IMO you should keep them.
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
