On 2012年05月15日 00:23, Greg KH wrote:
On Mon, May 14, 2012 at 09:18:38AM -0700, Greg KH wrote:
On Mon, May 14, 2012 at 11:25:29PM +0800, Lan Tianyu wrote:
Signed-off-by: Lan Tianyu<tianyu.lan@xxxxxxxxx>
---
drivers/usb/core/hub.c | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 6bf7124..6f909db 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4236,6 +4236,20 @@ void usb_queue_reset_device(struct usb_interface *iface)
}
EXPORT_SYMBOL_GPL(usb_queue_reset_device);
+/**
+ * usb_get_hub_child_device - 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.
+ *
+ * Return NULL if input param is invalid and
+ * child's usb_device pointer if non-NULL.
+ */
struct usb_device *usb_get_hub_child_device(struct usb_device *hdev,
The more I look at this function, the worse I like it, here's why:
- you don't increment the reference count of the device, what is to
keep it from going away from when you find it and the caller uses
it?
- why all of the checking of the arguments, how, and who, would ever
mess up calling this function incorrectly?
Do you mean the check is redundant?
+ if (!hub || port1 > hdev->maxchild || port1 < 1)
+ return NULL;
- Whenever you are using this function, it's in a loop, which
indicates that what you really want is a function to iterate over
the child devices of a hub, right? So that is the function you need
to write, which will handle the proper locking and incrementing of
device reference counts.
ok. How about this?
+
+struct usb_device *usb_get_hub_child_device(struct usb_device *hdev,
+ int port1)
+{
+ struct usb_hub *hub = hdev_to_hub(hdev);
+
+ if (!hub || port1 > hdev->maxchild || port1 < 1)
+ return NULL;
+ get_device(hub->port_data[port1 - 1].child->dev]);
+ return hub->port_data[port1 - 1].child;
+}
+EXPORT_SYMBOL_GPL(usb_get_hub_child_device);
+
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -2040,10 +2040,15 @@ static void collect_usb_address_map(struct usb_device
*udev, unsigned long *map)
map[udev->devnum/32] |= (1 << (udev->devnum % 32));
for (chix = 0; chix < udev->maxchild; chix++) {
- struct usb_device *childdev = udev->children[chix];
+ struct usb_device *childdev =
+ usb_get_hub_child_device(udev, chix + 1);
- if (childdev)
+ if (childdev) {
+ usb_lock_device(childdev);
collect_usb_address_map(childdev, map);
+ usb_unlock_device(childdev);
+ put_device(child_device->dev);
+ }
}
}
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -590,7 +590,8 @@ static ssize_t usb_device_dump(char __user **buffer, size_t
*nbytes,
/* Now look at all of this device's children. */
for (chix = 0; chix < usbdev->maxchild; chix++) {
- struct usb_device *childdev = usbdev->children[chix];
+ struct usb_device *childdev =
+ usb_get_hub_child_device(usbdev, chix + 1);
if (childdev) {
usb_lock_device(childdev);
@@ -598,6 +599,7 @@ static ssize_t usb_device_dump(char __user **buffer, size_t
*nbytes,
file_offset, childdev, bus,
level + 1, chix, ++cnt);
usb_unlock_device(childdev);
+ put_device(childdev->dev);
if (ret == -EFAULT)
return total_written;
total_written += ret;
So, care to just delete the function and do this correctly instead? As
it is, it seems very fragile.
Because of this, I've reverted the patch that added this function, and
the one prior to this, as this series really isn't that thought out just
yet, and I don't want it into 3.5.
Please take the time to rework these patches and do it correctly.
Yeah. I'd like to do this.
thanks,
greg k-h
--
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
