On 2012年05月19日 22:38, Alan Stern wrote:
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.
OK. CC "Yoshihiro Shimoda" who is the driver's author.
hi Yoshihiro:
Can you help us to make clear why the driver need to traverse the
entire tree deivces?
Other side, I referenced the bus_find_device() which has similar function.
struct device *bus_find_device(struct bus_type *bus,
struct device *start, void *data,
int (*match)(struct device *dev, void *data))
{
struct klist_iter i;
struct device *dev;
if (!bus)
return NULL;
klist_iter_init_node(&bus->p->klist_devices, &i,
(start ? &start->p->knode_bus : NULL));
while ((dev = next_device(&i)))
if (match(dev, data) && get_device(dev))
break;
klist_iter_exit(&i);
return dev;
}
The function increased the refcount of device before return the device pointer.
I think this maybe more safe and general for future user. Does a get_devcie() have
negative effect? :)
hi greg:
Do you have some comments on this?
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
