On 2012年06月13日 23:35, Alan Stern wrote:
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?
Oh. Sorry. This is debug code which should be removed.
It should be
usb_get_hub_each_child(usbdev, chix, childdev)
if (childdev) {
usb_lock_device(childdev);
ret = usb_device_dump(buffer, nbytes, skip_bytes,
file_offset, childdev, bus,
level + 1, chix - 1, ++cnt);
}
--- 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.
This is suggested by Greg. Refcount increase can prevent child's struct
usb_device from being free. At last this can keep child point vaild.
+ *
+ * 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?
Just for more safe, the function return the struct usb_device and the point
may be invalid caused by device being released(memory being freed). But caller
may still use it. This will cause oops. Last time, we discussed this may happen
in the r8a66597-hcd.c and cc the maintainer but have not got response.
--- 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.
Ok.
Alan Stern
--
Best Regards
Tianyu Lan
linux kernel enabling team
--
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
