On 2012年06月14日 22:18, Alan Stern wrote:
On Thu, 14 Jun 2012, Lan Tianyu wrote:
--- 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);
}
Except that it really should be:
usb_get_hub_each_child(usbdev, chix, childdev) {
if (childdev) {
...
}
}
with an outer set of {}s. Otherwise somebody will make the same
mistake you did when they insert a debugging statement in the middle.
Ok. I get it. :)
+/**
+ * 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.
I'm sorry, but Greg was wrong. See below.
+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.
What happens if the child device is released after your code evaluates
hub->port_data[port1 - 1].child
but before usb_get_dev() is called? There will still be an oops.
No, the caller of usb_get_hub_child() has to be entirely responsible
for making certain that children don't get released unexpectedly.
Calling usb_get_dev() like this won't help.
Ok. One more question, what the caller should do to avoid oops?
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
