On Fri, 18 May 2012, Lan Tianyu wrote:
> > 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?
No, not really. Why does a host controller driver need to traverse the
entire tree device of devices below the controller? It shouldn't care
about any of that stuff. Or if it does, it should monitor as
individual devices are added and removed rather than scanning the whole
tree at interrupt time.
If for some reasy it does need to traverse the tree, then it has to
make sure that it does so in a safe manner. From your description, it
sounds like the current code is not safe -- and what you want to add
won't help sufficiently. For example, how can you prevent the hub's
port data array from changing while this code runs?
You should ask the maintainer of this driver why this code is needed
and how it should work.
Alan Stern
--
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
