Re: [PATCH V2 1/3] usb: add kerneldoc for usb_get_hub_child_device function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

On 2012年05月17日 22:31, Alan Stern wrote:
On Thu, 17 May 2012, Lan Tianyu wrote:

Also, you should check to see whether the usb_lock_device() call is
really needed.  If the caller already owns the lock for the parent hub
then you don't need to lock the child -- you don't even need the
usb_{get,put}_dev() calls.

How about this?
#define usb_get_hub_each_child(hdev, port1, child) \
	for (port1 = 1, child = usb_get_hub_child(hdev, port1); \
		port1<= hdev->maxchild; child = \
	 	usb_get_hub_child(hdev, ++port1))

usb_get_hub_child(hdev, port1)
	struct usb_hub *hub = hdev_to_hub(hdev);
   	return usb_get_dev(hub->port_data[port1 - 1].child);

For more general, usb_get_hub_child() increases child refcount
to prevent child from being removed during processing and the caller
should invoke usb_put_dev() when child will never be used.

Have you looked through the callers to see whether the child's refcount
really needs to be incremented?  I think it probably doesn't.  (Hint:
If the current code doesn't increment the refcount, that's a good sign
that the new code doesn't need to increment it either.)
In the r8a66597-hcd.c

static void collect_usb_address_map(struct usb_device *udev, unsigned long *map)
	int chix;

	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 =
			usb_get_hub_child_device(udev, chix + 1);

		if (childdev)
			collect_usb_address_map(childdev, map);

The function will be invoked in the rh_timer_func().
The code path  is usb_hcd_poll_rh_status() -> hcd->driver->hub_status_data()
-> r8a66597_hub_status_data() -> r8a66597_check_detect_child() ->

The function is to traverse all the device tree of the hcd. There maybe
a case that after child pointer being passing to collect_usb_address_map()
as param, the child was removed and pointer became invaild. This will cause
oops and maybe small probability event. Does this make sense?

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

B and H Foto and Electronics Corp.

[Linux Media]     [Video for Linux]     [Linux Input]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]     [More Archives]

Add to Google Powered by Linux