On Fri, Sep 13, 2013 at 2:14 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Fri, 13 Sep 2013, Bjorn Helgaas wrote: > >> On Fri, Sep 13, 2013 at 12:25 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: >> > On Fri, 13 Sep 2013, Bjorn Helgaas wrote: >> > >> >> usb_bus_list_lock protects the usb_bus_list, and we don't touch that list >> >> in usb_disconnect(), so there's no reason to hold the lock here. >> > >> > The code says: >> > >> > /* used when updating list of hcds */ >> > DEFINE_MUTEX(usb_bus_list_lock); /* exported only for usbfs */ >> > EXPORT_SYMBOL_GPL (usb_bus_list_lock); >> > >> > Unforunately, the first comment is incomplete. usb_bus_list_lock >> > protects both the list of buses and also the set of root hubs. That's >> > why it isn't needed in hub_quiesce() and hub_port_connect_change(); >> > those routines remove only non-root devices. >> > >> > Removing the usages in usb_add_hcd() and usb_remove_hcd() isn't safe. >> > You can see where this usage is important in >> > devices.c:usb_device_read(). >> >> I don't know how the set of root hubs is managed, so I don't see the >> conflict between, e.g., usb_remove_hcd() and usb_device_read(). I'm >> not looking to become a USB expert, so I'll just take your word for it >> -- I don't want to waste more of your time on it. Thanks. Maybe this is more relevant than I thought. I'd sure like to copy your strategy rather than reinvent something. > Very briefly, usb_device_read() dereferences bus->root_hub: > > usb_lock_device(bus->root_hub); > > On the other hand, usb_remove_hcd() calls both > > usb_disconnect(&rhdev); > > and > > usb_put_dev(hcd->self.root_hub); > > (note that rhdev == hcd->self.root_hub == bus->root_hub), which > unregisters and deallocates the root_hub structure. Something has to > prevent usb_device_read() from accessing the structure after it has > been unregistered and freed; that's where usb_bus_list_lock comes in. I hadn't paid attention to hcd->rh_registered before, but that seems important. When iterating over usb_bus_list, usb_device_read() ignores buses with hcd->rh_registered == 0. usb_remove_hcd() clears hcd->rh_registered before deallocating the root_hub with usb_put_dev(). I think the mutex_lock/unlock between clearing rh_registered and deallocating root_hub will ensure that usb_device_read() cannot see rh_registered == 1 and a deallocated root_hub structure. But that seems more like a memory barrier issue than a locking issue, and I don't yet see how usb_disconnect() is related. I guess I'm hung up because I don't know how managing the set of root hubs is different from managing usb_bus_list. Maybe it has something to do with the device_add() call in register_root_hub() and the device_del() call in usb_disconnect(). We hold usb_bus_list_lock in both cases, but neither does anything with usb_bus_list. Or maybe it has something to do with the visibility of stores to hcd->rh_registered? Bjorn -- 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
