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

[Lan Tianyu <tianyu.lan@xxxxxxxxx>]

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