On Wed, 12 May 2004, David Brownell wrote: > Reading port status doesn't require a lock ... responding to changes, > or initiating them, should.
That's an excellent point, something I had not ever considered. It almost solves the entire problem! There's just one or two loose ends to consider, like what happens if a device is unplugged during a reset. We don't want hub_port_init() in the reset thread deleting the usb_device structure; that should be left up to khubd. > It still doesn't seem to me that any > lock other than hub->serialize should be needed for that task. Well obviously, if the response involves doing things to the child device (like calling usb_disconnect) then the child has to be locked too. But basically you're right. > But if we use hub->serialize to lock ports, they WILL go in parallel, > and without adding more locks. > Using a lock on the hub -- vs each of its ports -- would seem > equally correct, at the cost of some potential concurrency. > And it's a LOT simpler! We _can't_ use hubdev->serialize to lock ports. (Fortunately we don't have to, and using the child's lock is just as simple.) The reason is clear: Sometimes a thread will need to lock a port while already holding the child device's lock. If that meant locking the hub, it would involve acquiring locks in the wrong order. > >>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. > > I was overlooking that part of the message ... but still, what I > was wondering is where the locks would be gotten. How would the > hub_port_init() paths (enumeration, reset) get that readlock? hub_port_init() is called in two different contexts: initializing a new device and resetting an existing one. Resetting is easy; the caller will already hold the appropriate lock. In the initialization path the hub driver will acquire the readlock in the usual way, by calling usb_lock_device() before calling usb_new_device(). That's the correct thing to do, since there might be a new driver registering at that same time -- we wouldn't want it to probe the new device until the initialization procedure is finished. > As you said, usb_register() gets the writelock, but shouldn't > usb_probe_interface() then be grabbing dev->serialize if it's > going to be held on all probe() paths? That'd change the early > enumeration locking a bunch, and maybe fault cleanup (since > dev->serialize no longer suffices). No, usb_probe_interface() doesn't need to lock anything. The locks will already have been acquired, either by usb_register() or by the caller of usb_set_configuration(). (There's another obscure pathway to consider, USBDEVFS_CONNECT -- it should be treated the same as usb_register().) Maybe you're thinking that once you hold the writelock you still need to hold dev->serialize? That's not necessary. Holding the writelock will prevent everyone else from locking _any_ device. > > 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. > > The spinlock has been a separate issue for a while. I think the > first patch would be to make that rwsem work. Yes, that would be the first patch. That, and removing unnecessary locking of the subsystem rwsem. The state spinlock can come next. When you think about all the details involved, there's quite a lot that needs to be changed. Alan Stern ------------------------------------------------------- This SF.Net email is sponsored by: SourceForge.net Broadband Sign-up now for SourceForge Broadband and get the fastest 6.0/768 connection for only $19.95/mo for the first 3 months! http://ads.osdn.com/?ad_id=2562&alloc_id=6184&op=click _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
