On Wed, 12 May 2004, David Brownell wrote: > Alan Stern wrote: > > > (8): Reading port status messages and marking the child device > > as USB_STATE_NOTATTACHED when a connect change occurs > > And issuing disconnect() notifications ... that's three separate > things, and I think only the third one (notifications) should > require holding the device/port's lock.
The third part was already present on my list as (1). Clearly only that third part requires holding the device's lock. But the first part (reading port status messages) requires holding the port's lock -- so if the port's lock _is_ the device's lock we end up with a possibility for blockage. > Have a look at the "OHCI resume/reset stops deadlocking in PM code" > patch I sent, and see how mark_children_gone() is used as part > of forcibly quiescing the device (in a context where khubd can't > help) ... there should be a safely spinlocked version of that, used > also inside khubd, along with a variant that just changes state for > any device that's actually attached ... we've discussed that before. Yes, I noticed that. When this other hub stuff is fixed up, the OHCI driver and khubd should use the same core routine to do the marking. > > Unfortunately, the scheme relies on children[n]->serialize to provide > > mutual exclusion for (6) and (7), so (8) has to acquire that same > > semaphore. This means that (8) can't take place simultaneously with > > (1)-(5) on the child, since they will also hold the semaphore. > > Or (referencing my secret decoder ring!) basically: if khubd grabs > each port's lock before checking port status change events, it'll > be delayed by usbfs and/or sysfs operations that need it too. No, sorry about the proliferation of uninformative numeric labels. In simpler terms: If we use the child device's ->serialize also to lock the hub port, then khubd's attempts to read the port status could be delayed by things like child device config changes -- things that could just as easily proceed in parallel with reading the port status. (That's because config changes have to lock the child device whereas port status messages merely have to lock the port.) In short, maybe it's not a good idea to use the child device's lock as a port lock. What do you think? (Remember that it was you who asked earlier why I wanted to distinguish between the two halves, hub port vs. device, of a single link! "Why separate entities", you said.) The alternative doesn't have to be complicated. A simple atomic bitmask and a global wait_queue would suffice to provide per-port locking. > Actually khubd is pretty stupid ... when it gets a notification > that one port changed, should just check that port. That'll reduce > the opportunities other tasks have to cause problems for khubd. That's what it will do. One of the enhancements I have planned for khubd is to store a bitmask of ports that require attention (essentially the contents of the status interrupt message). > > What do you mean? When and why should khubd use locktree? > > To get the lock on the hub it's looking at. If that policy > is ever going to be used, it should be used everywhere; else > the inconsistency will create problems. At the moment I don't see any problems arising. In fact, the only reason I know of for using locktree is that when you suspend a hub, you want to make sure that every device below the hub gets suspended and _stays_ suspended until you're done. The "stays suspended" part could be violated by resume() if resume() didn't use locktree. The same sort of reasoning also holds the other way around (resuming every device below a hub). Apart from that, there doesn't appear to be any need for locktree. Although other operations can affect entire subtrees, they don't do so in ways that require extended periods of protection. > >>>Now maybe I've overlooked something here or oversimplified in some way. > >>>But altogether I think this makes a good case that using a single lock is > >>>the way to go. > >> > >>That is, one per device, not changing away from dev->serialize? > > > > > > Yes, that's what I mean. > > How were you going to solve the register_driver() -> probe() path? That was described in my earlier message: http://marc.theaimsgroup.com/?l=linux-usb-devel&m=108387954820032&w=2 The basic idea is to use an overall rwsem in addition to all the ->serialize locks. To lock a single device you would first get read-access to the rwsem. When registering or deregistering a driver you first get write-access to the rwsem, thereby effectively locking _all_ the usb_devices. (Maybe hubs should be exempt from that, since the hub driver can only be registered/deregistered when no devices are present, and no other driver can be bound to a hub.) > > Good. So apart from the questions above, it looks like we've found a > > solution. > > Other than removing khubd_sem, sounds like what's in my BK tree now. You don't have the new rwsem, and many of the details are different. For example, we would never need to explicitly use the USB subsystem rwsem since binding events will be protected by the device lock. Also we have to add a spinlock to protect udev->state, so that the NOTATTACHED marking can be done safely. Right now, the only question I'm still concerned about is how to handle the port locking. Use the child device's ->serialize, or have a separate per-port bitmask for each hub? Alan Stern ------------------------------------------------------- This SF.Net email is sponsored by Sleepycat Software Learn developer strategies Cisco, Motorola, Ericsson & Lucent use to deliver higher performing products faster, at low TCO. http://www.sleepycat.com/telcomwpreg.php?From=osdnemail3 _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
