On Wed, Jan 14, 2004 at 05:10:05PM -0500, Alan Stern wrote: > Upcoming changes > > It looks like certain changes are looming for the USB system, and I wanted > to present a brief summary to make sure everyone knows about them and > agrees they ought to be made. Simple ones first...
Thanks for writing this. I know I've been pretty much staying out of some of these discussions, mainly because I don't think we need most of these changes. In other words, these seem like pretty big changes that do not need to be done during a stable kernel series. Remember, bug fixes now. But if some of the bug fixes require semi-big changes, that's ok. We have some very good stability right now in the USB core, let's keep it that way :) On to specific comments... > The halted[] member of struct usb_device is going to change > to refuse[]. I posted another message about this a couple of > weeks ago and nobody has responded, so I guess that's okay. > Not surprising, since this feature is pretty much unused. sorry I missed that message. Why change this? > > The children[] and maxchild members will go away. They are > meaningful only for hubs, and most of the information is > already present in the driver model's children lists. I think this will be a lot of effort for basically no benefit. Why do you want to do this? Save the memory of that tiny pointer array? If you do this, a whole lot of other parts of the usb code will get quite complex. In short, I do not recommend doing this. > The information missing in the driver model is the mapping from > port number to child device. That mapping will be turned around; > each struct usb_device will get a new member called portnum. Again, if the above change is not made, this is not needed, right? > The state member of struct usb_device will be explicitly > _not_ under the protection of the serialize semaphore. Instead > there will be a single global spinlock to protect all the > states. Events like disconnection will be cause state to > change as soon as they are detected, without having to wait for > a driver to release usbdev->serialize. So all device state changes will be under the same spinlock? That's not a good idea. I want to get rid of locks, not add a bigger one. What happens when I change the driver core to run multiple device probes in parallel (yeah, I know that will break a lot of existing stuff, but let's not add things that make it worse. And yes, that's 2.7 work...) > Now a more interesting item, not yet entirely certain: > > struct usb_device->serialize will become a read-write semaphore. Why? I don't see the benefit of this. We want to reduce the contention on this lock, not make it bigger. And read-write locks are generally a bad thing, and should be avoided as much as possible... > The serialize semaphore protects against major changes, things that would > involve binding/unbinding drivers or altering the device's physical state. > In particular, the following routines will require the caller to hold the > semaphore (or, in the new version, to have a write-lock): > > usb_driver_claim_interface, > usb_driver_release_interface, > usb_disconnect, > usb_new_device, > usb_disable_device, > usb_reset_configuration, > usb_set_configuration, > usb_reset_device > > Also, a driver is guaranteed that the semaphore will be held when its > probe() and disconnect() routines are called. That's already the case, right? > In general, a driver can acquire the serialize semaphore whenever it wants > to block one of the functions above temporarily. Ick. > A good example would be when calling usb_ifnum_to_if(); that function > scans the interfaces in the current configuration and the results > would be meaningless if the configuration unexpectedly changed. A very rare occurrence. > There are some good reasons for making serialize into a read-write > semaphore. Duncan Sands reports that the usbfs implementation would be > greatly simplified. I'm all for fixing bugs in the existing usbfs. But I think that's basically beating a dead horse into a prettier lump of goo. We really need to start over and make usbfs2. It should look something like the gadgetfs interface. > Also, if a device has multiple interfaces each with > its own driver, then the separate drivers might individually want to > prevent configuration changes without interfering with each other > activities; getting a read-lock is the natural way to do this. Again, a very rare occurrence, and not one I've ever heard of happening. Because of all of this, please don't do this now. It's a big change, one that should be held off till 2.7 to try to do (if you really want to then.) > Some longer-range changes: > > Add a new usb_device_state: USB_STATE_RESET. Although it > doesn't correspond exactly to any state given in the USB spec > it could be very useful. A device is in this state while it > or a hub upstream from it is undergoing a reset. > > The somewhat obscure reason for having this is that it allows us to > recover the devices attached to a hub when that hub is reset. Resetting a > hub disables all its ports; after the hub revives, each of its children > will have undergone the equivalent of a reset. Knowing that this has > happened, we will be able to retain the child device structures and their > driver bindings. They will resume activity rather than going through the > process of logical disconnect followed by logical connect -- which would > have the effect of replacing each device with a new incarnation, breaking > things like filesystem mounts. But as the device did just loose power, and then get it back again, it's the only sane thing to do. We have to drop the whole device and initialize it again. Think of downloading firmware to devices. And also, when is a hub ever reset? :) > This may turn out to be excessive design. I don't know that hubs need to > be reset at all often, and it might not be worth the trouble to do all > this. Furthermore, it would greatly complicate the set of possible state > transitions in the hub driver. Exactly. > Add new callbacks to struct usb_driver to notify drivers that > their device is about to be reset or has just finished a reset. > I don't like the name "reset" for a callback as it implies that > the driver should reset the device. Maybe better names would > be "notify_reset" and "reinit" (or "notify_reinit"). > > As things stand now, we don't handle resets well. A device can have > multiple drivers bound to multiple interfaces; when any one of them > requests a reset it will affect all the others but they have no way to > know what's going on. Likewise, if a hub is reset it automatically has > the effect of resetting all the downstream devices, and the drivers need > to know about it. Again, what's wrong with the current way of dropping everything and starting over? I think it's the only sane way, and our existing interfaces handle it quite well. > Design patterns for USB drivers > > Many USB device drivers follow a design pattern that is typified by > usb-skeleton. The driver has a per-device private data structure which > includes an in-use semaphore and a refcount (or open_count). The > semaphore serves to prevent the disconnect() routine from running while > the driver is actively using the device. The refcount serves the usual > purpose of preventing deallocation of the private structure until nothing > is using it any more. I might say that the usb-skeleton driver has grown into a monster. I really don't like a lot of stuff that this driver does (the semaphore, etc.) Using a kobject to keep around a device's structure is much more sane (actually you can just grab the device in the interface and use that, more on that below.) > With the new changes, another pattern might work better. In particular, > there's no need for the private data structure to include an in-use > semaphore since usbdev->serialize (read-locked) would do just as well. > But in fact, the whole idea of using a semaphore to block disconnect() may > be unnecessary. > > There has been a requirement that after a driver's disconnect() routine > returns, the driver will not try to access the device at all -- all URBs > must be unlinked and the struct usb_device may be deallocated at any time. No, that is not a requirement at all. All of the usb-serial devices violate this "requirement" today. It is perfectly legal to try to access a usb device structure after disconnect() is called. That's what the reference count is there in the structure for. So just grab a reference when you are first probed() and then only give it back when you are completely done with the device (after both disconnect() and your release() function is done.) Then the device will be automatically cleaned up and everything is fine. The usb core already can handle us trying to submit urbs against a usb device that has long gone away, let's not break that functionality. > The core is now (or soon will be) sufficiently well protected that this > requirement is no longer needed. When disconnect() is called, > usbdev->state will already have been set to USB_STATE_NOTATTACHED so no > new URBs will be accepted, and all URBs in flight will have been unlinked. We already do this today. > All the driver really needs to do is make sure that the struct usb_device > isn't deallocated until it has finished cleaning up, and that can be > handled with usb_get_dev()/usb_put_dev(). Again, you can do this today. So I think you already have this design pattern today without any extra work needed :) thanks, greg k-h ------------------------------------------------------- The SF.Net email is sponsored by EclipseCon 2004 Premiere Conference on Open Tools Development and Integration See the breadth of Eclipse activity. February 3-5 in Anaheim, CA. http://www.eclipsecon.org/osdn _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
