hi alan:
Great thanks for your review.
On 2012年06月28日 22:42, Alan Stern wrote:
On Thu, 28 Jun 2012, Lan Tianyu wrote:
Move child's pointer to the struct usb_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.
Signed-off-by: Lan Tianyu<tianyu.lan@xxxxxxxxx>
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -589,14 +590,12 @@ 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) {
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);
You forget to add usb_put_dev(childdev).
Oh. sorry. I forget to remove usb_get_dev() in the usb_get_hub_child(). since
last our discussion,
the increase refcount maybe not helpful. I will remove it in next version.
http://marc.info/?l=linux-usb&m=133968372704178&w=2
if (ret == -EFAULT)
return total_written;
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1784,11 +1783,12 @@ bool usb_device_is_owned(struct usb_device *udev)
static void recursively_mark_NOTATTACHED(struct usb_device *udev)
{
+ struct usb_hub *hub = hdev_to_hub(udev);
This is dangerous, because at this point you don't know whether udev is
a hub -- it might not be -- and hdev_to_hub can crash if its argument
isn't a hub. Instead you should do:
struct usb_hub *hub;
if (udev->maxchild> 0)
hub = hdev_to_hub(udev);
Or if you prefer, add the "hdev->maxchild> 0" test into hdev_to_hub
itself.
@@ -1952,6 +1952,7 @@ static void hub_free_dev(struct usb_device *udev)
void usb_disconnect(struct usb_device **pdev)
{
struct usb_device *udev = *pdev;
+ struct usb_hub *hub = hdev_to_hub(udev);
Same here.
@@ -4965,3 +4966,27 @@ 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.
+ *
+ * Return NULL if input param is invalid and
+ * child's usb_device pointer if non-NULL.
The kerneldoc should mention that this routine increments the child's
reference count and the caller must call usb_put_dev() when it
is finished using the child.
Also, I think it would be better if the function were named
"usb_hub_get_child".
Ok. I will change it.
+ */
+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->ports[port1 - 1]->child);
+}
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -2033,17 +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) {
+ usb_put_dev(childdev);
collect_usb_address_map(childdev, map);
The usb_put_dev should come _after_ the collect_usb_address_map call.
Otherwise you're passing a stale pointer.
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -567,6 +565,20 @@ static inline struct usb_device *interface_to_usbdev(struct usb_interface *intf)
extern struct usb_device *usb_get_dev(struct usb_device *dev);
extern void usb_put_dev(struct usb_device *dev);
+extern struct usb_device *usb_get_hub_child(struct usb_device *hdev,
+ int port1);
+
+/**
+ * usb_get_hub_each_child - iterate over all child devices on the hub
+ * @hdev: USB device belonging to the usb hub
+ * @port1: portnum associated with child device
+ * @child: child device pointer
+ *
Mention that the caller must call usb_put_dev() when it is finished
using the child.
And change the name to "usb_hub_get_each_child".
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Tianyu Lan
linux kernel enabling team
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
