On Fri, Jan 16, 2004 at 03:25:27PM -0500, Alan Stern wrote: > > > 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. > > It's not a question of saving memory, it's the principle of not > duplicating information and thereby avoiding the risk of having > inconsistent records. I don't think it will complicate other parts of the > USB code very much. Yes, it will be a little harder to go from a port > number to the corresponding usb_device (it will require a search through > the list of children). But that lookup only occurs while processing a > connect or disconnect event, not very often. And also it will simplify > the device_reset code, going from usb_device to parent port number -- > the search that's in there now will become unnecessary.
The scsi people tried to do this, and ran away screaming in terror (I heard them accross the building, it wasn't pretty...) We would have to special case a lot of code in order to get this to work properly (hint, how to detect a hub? everywhere that we walk the lists we would have to export that logic, no fun... And other devices do have children in the sysfs tree, scsi, tty, etc.) > However, this is not a big deal. If you still feel we should keep the > children[] array I won't mind. But then it should be moved to the > private hub data structure instead of sitting in every usb_device. Again, every bit of code that walks the usb device lists would have to have access to those data structures in order to have a chance of walking the lists in a semi-coherient manner. Look around the USB code for everywhere we do this. Then look in the security/ directory for another user :) > > > 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...) > > This lock should not see much contention. It will be held only very > briefly in any path where a state gets changed. Something like this: > > spin_lock_irq(&global_state_lock); > if (usbdev->state != USB_STATE_NOTATTACHED) > usbdev->state = USB_STATE_CONFIGURED; > else > notattached = 1; > spin_unlock_irq(&global_state_lock); > if (notattached) ... > > Even with multiple probes running in parallel this won't cause any > significant delays. > > The reason for adding the lock was David's firm belief that we should > report a device disconnection (by setting the state to > USB_STATE_NOTATTACHED) as soon as it is detected, rather than having to > wait for a driver's disconnect() routine to run or to wait for > usbdev->serialize. If the state is not protected by serialize then it > needs some other sort of protection, and a global spinlock is the simplest > way to do it. Why does the lock have to be taken to set the state in this case? There is not more than one thread that is setting the state, right? Just multiple users. The main problem about a NOTATTACHED type state, is that we can never test properly that the state is not NOTATTACHED, but only that it is NOTATTACHED. If that's ok with your logic, then we are fine, and don't really need a lock, right? > Another possibility suggested by David would be to add an extra > "unplugged" flag to struct usb_device. That flag could be set as soon as > we know the device is unplugged, with no protection at all since it would > never get reset. That's fine with me too. But watch out, I did that with the driver model during the 2.5 development cycle, and spent a lot of time trying to clean up the mess it caused... > This would be simpler than using a spinlock, even though using the > lock would be the conventional approach. Do you prefer this scheme? I don't think the lock is necessary. I also think that if you have a "unplugged" flag you need to really think though what you are going to use it for... > > 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. > > What about backward compatibility? libusb would handle this. > > > 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. > > As David said earlier, it's _not_ legal to access a device after returning > from disconnect() because another driver might already be bound to the > interface. Again, that's the rmmod case -- for disconnect obviously it > doesn't matter. But after rmmod finished, the driver isn't present to be touching the device. Well, it had better not, otherwise we have worse problems :) > > > 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. > > Ah -- we do it when the device is unplugged. So far as I can see, we > don't do it when usb_deregister() is called. That should be changed then. 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
