On Sat, 27 Dec 2003, David Brownell wrote: > Yes, a bug! I fixed it in the updated patch which I've attached; > it applies cleanly to Greg's current usb-devel-2.6 BK. > > [ Split out patches will coming out too, in dribs and drabs. ]
Okay. I won't have time to look at the updated patch for a little while. In fact, I would like to see this stuff get into one of Greg's trees before I begin adding the "device-morphed" parts. I hate sending in patches that keep needing to be updated because someone else is working on parts of the same code at the same time! > > For that matter, does > > it really make sense to have a parent pointer in struct usb_device when it > > will always echo the parent pointer in the embedded struct device? > > Not that I can tell, so long as there's a different cheap way to detect > root hubs. If nothing else, we can test for address == 1. Also one of the bus data structures contains a pointer to the root hub device. > That's another part of usbcore that hasn't yet replaced the > original homebrew solution by using the driver model: if the parent is > really a usb_device, dev->parent == to_usb_device(dev->dev.parent). > > Likely dev->children[] could be done away with at some point too. > That'd seem a bit trickier to change. These seem like good things to add to the 2.7 TODO list. > > hub_port_connect_change() doesn't acquire dev->serialize. > > Not needed before usb_new_device(), surely -- nobody else can even see > the device, until it's published part way through usb_new_device(). > I suspect the simplest fix is to just grab it early in usb_new_device(), > and drop it so that usb_set_configuration() can re-acquire that lock. Um, that won't work with the upcoming usb_reset_device changes. The lock has to be acquired before calling usb_new_device. > Maybe by the time we finish, usb_set_configuration() will rely on callers > to grab the lock ... easy to do through sysfs, and usbfs needs changes > in any case. That would make Duncan's life easier :-) > At this point I'm not even sure we should allow device > drivers to call usb_set_configuration ... sysfs is sufficient, or even > usbfs. > > > Hmm, the device isn't published in hub->children[] until after it's > configured. That was a recent change Greg merged; it changed from > one "half visible" device state to another one. (Now the driver model > shows the device during new_device, but usb doesn't....) Of course if > dev->children[] went away, or became hub-internal, that's be simplest. It ought to be hub-internal if it exists at all. Non-hubs don't have children anyway. And given that the same information is also available through the driver model... > > Is the SET_CONFIG_TRIES loop in hub_port_connect_change() really needed? > > It was in the old code, and it probably was unnecessary there too, though > > I can't be sure of that. There's no comment indicating why it should be > > there, and hub_port_init() has its own retry loops. > > I don't know that it's needed, and added a comment to that effect. > > This loop acts differently though ... it includes config setting, > so it retries things the port initialization doesn't. Some devices > might need it. (Though I've never seen that loop run, and since > it was originally misnamed as a HUB_PROBE_TRIES, I wonder if maybe > it's always been some kind of misunderstanding.) Maybe so. But is there any good reason for having both hub_port_init() and usb_new_device() within the same loop? Is there any point in thinking that failure in usb_new_device() could be fixed by redoing a successful hub_port_init()? Probably nobody knows any more. > > Speaking of which, in hub_port_init() there's no reason to have > > usb_set_address() called within the GET_DESCRIPTOR_TRIES loop. If it > > needs a retry loop, it should have one of its own. > > Hmm, not exactly. I added a comment in this version ... device > hardware (!) and firmware sometimes has bugs in this area, and > I think that logic was a partial workaround for some of them. > Bugs at the level of "address 3 doesn't work" ... MS-Windows > and Linux don't act quite the same here (which hasn't been a > real issue, unlike with usb-storage and SCSI support). You mean the device replies successfully to the set-address message but then fails to respond at the new address? Makes sense, so long as it's properly documented in the code. > > Nowhere does the core call disable_endpoint for ep0 at address 0. I > > suppose the host drivers are supposed to take care of that themselves when > > they exit. > > Actually that's one of the few remaining bits of automagic in the > interface to an HCD: they're expected to morph that address's ep0 > into a descriptor for the "real" address. It's a UHCI-ism that > OHCI (and now EHCI) have had to cope with forever. > > Removing that would be an excuse to clean up some minor cruft in > OHCI and EHCI. Now might be a good time to take care of it. Alan Stern ------------------------------------------------------- This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
