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() ->
collect_usb_address_map().

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  http://vger.kernel.org/majordomo-info.html


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