On Sat, 31 May 2003, Oliver Neukum wrote: > Am Samstag, 31. Mai 2003 21:32 schrieb Alan Stern: > > I don't follow your reasoning here. As far as I can see, the only errors > > to be handled are problems reading the descriptors. Are you suggesting > > the one version should skip reading the descriptors and just do the > > rebinding? The only reason for this would be if we had read the > > descriptors previously. When would that have happened? > > Yes, just rebind. > This happens if there are no errors. After a reset, the other interfaces > have to be bound. > However, there might be errors in a reset process the core won't notice, > like a failure to accept firmware after a reset. In this case the device is > screwed and should be subject to a full reset and reprobing under the > core's control (and probably disablement, but we should at least try).
Are we talking about the same thing here? In my original message I was describing the usb_device_altered() function, which we expect a driver to call after it has called usb_device_reset(). The reset() call rebinds all the drivers to the interfaces they had before the reset, explicitly assuming that the descriptors have _not_ changed. (That will be the common case for things like error recovery.) Altered() reads the new descriptors, unbinds the drivers, and binds the new interfaces. Okay, so what about errors like a failure to accept firmware? As you say, the device is screwed. I don't know what you mean by a "full reset" -- usb_device_reset() really does reset the device and usb_device_altered() will re-read the descriptors and do reprobing. If reading the descriptors fails, altered() will disable the device, just as in the case of a newly-connected device. What else remains to be done? > > The problem isn't the driver's locks; it's the core's lock. Here's the > > idea. > > > > First, a driver is very unlikely to call usb_device_altered() anywhere but > > in its probe() routine. After all, this call will unbind the driver from > > the device, so why bother to bind in the first place? Or to put it > > The other obvious place is error handling. Hmm... maybe. I'm not so sure. At any rate, probe() is clearly a high-probability place for altered() to be called from. > > another way, if a firmware download is needed for the device to work > > properly, why wouldn't that download be done first thing, during probe()? > > Right, but there it's not the core's lock, but the driver model's lock. > And neither is the problem specific to USB. Wouldn't it be more elegant > to just return -ERESTARTSYS and let the driver model reprobe? No. We need a usbcore-specific lock. It's not enough to rely on the driver model's locking. Doing that would leave open the possibility, for example, of a driver newly bound to one interface doing a set_configuration() at the same time that a driver for another interface is being probed. To avoid this, set_config and probing must use the _same_ lock. The real problem is that the driver model is only aware of separate individual interfaces, whereas usbcore realizes that several interfaces are part of the same device. We need to lock the whole device, not just a single interface. > Sorry, I was unclear. Shouldn't robust error handling look like: > disable port -> reset hub -- retry disable -> powerdown port -> > reset hub -- retry power down port -> power down hub -> recursion Would that really work? That is, if you reset the hub, won't that disable _all_ its ports? We don't want to do that if powering down a single port is possible. > > I agree partially. Probing while the device is not in sync wouldn't be a > > disaster, but it would be nice to avoid it. Perhaps the > > usb_device_altered() routine could set some flag which would tell the core > > to abort further probing until after the new descriptors have been read. > > This would not require exporting the locking to the drivers. > > Too late. Submitting URBs to nonexistant interfaces is a very bad idea. Let's be more precise. URBs aren't submitted to interfaces, they're submitted to endpoints. Submitting an URB to a nonexistent endpoint isn't very bad, it merely causes an error code to be sent to the URB's completion routine. Besides, who's going to be sending these URBs? Not the driver that just altered the device's state. And if further probing is aborted, no later drivers will get the chance. Maybe a driver that was bound earlier. Anyway, how will allowing the drivers to lock out further probing prevent these bad URBs from being sent? One way around this might be to have a combined reset_altered() function. That would make sense; it's hard to think of a case where you would want to call device_altered() without calling device_reset() immediately beforehand. Doing things this way would avoid having any period in which the core is out-of-sync with the device. > > 2. I don't see how we can have a version without locking. Setting a new > > configuration necessarily involves unbinding the existing interfaces and > > binding the new ones, and that requires holding the device lock. > > Which is held during probe, where these cases arise. I don't like that idea. It might lead to unbounded stack usage. You're saying that a driver's probe() function could tell the core to set a configuration and restart probing from the beginning, without waiting for the original probe() to return. It goes against my grain. > > helper to do the job. But the existing code would probably still need to > > be rewritten. The correct way for a driver to handle this in its probe() > > routine is: See if the proper configuration is already loaded. If yes > > then proceed normally; if no then call usb_set_configuration() and return > > a failure code (i.e., don't bind). > > Then you can fail regardless. Could you explain? I don't see the logic? I don't understand what you mean by "you can fail regardless"... but I'll explain the earlier statement. First, it's clear that if the correct configuration is already loaded then the driver doesn't need to ask for it to be loaded again. The driver can simply go on about its job. Second, if the wrong configuration is loaded, the driver _does_ need to call set_configuration(). Doing so will ultimately cause a new set of interfaces to be bound, and it will also cause the driver to be unbound from the interface it is currently probing should the probe() return success. So returning success is pointless; the driver might as well just return a failure code. Even more strongly: if the configuration is wrong then the driver has no business binding to this interface in the first place, so it should return a failure code regardless, even if set_configuration() fails. But assuming everything goes well and set_configuration() succeeds, the core will at some point probe the interfaces in the new configuration. One of them presumably will match the driver, so probe() will be called again, this time with the correct configuration in place. Then the driver will be happy and things will work as expected. > > > I would prefer B but I see no clean way to to do this. This laptop has > > > 3 UHCI and 1 EHCI for two ports. What's a bus here? The idea of > > > controllers associated with two busses is horrible. > > > > A bus is a root hub together with an address space. One controller with > > two busses isn't a problem; it just means that the controller has two root > > hubs that control two different address spaces. The fact that the busses > > share a controller doesn't really affect anything else going on here. > > OK. But again, low priority. I agree. Khubd alone would be enough as far as I'm concerned. But David has said that he would like more. > > > 1. Reset currently puts the burden of disconnecting the other interfaces > > > on the driver. This is wrong. Usbcore should do it with proper locking. > > > > This contradicts 2 below, and I go with 2: resets shouldn't unbind any > > interfaces. But it's a tough problem. For now, the safest choice is to > > do what usb-storage does: don't reset unless there's only one interface. > > That's a hack I won't mention in polite company ;-) > A company here is selling mice with integrated CF readers. Yeah, it's just a stopgap measure until we can get proper reset handling in place. > But however we inform the other interfaces' drivers about an event, > it's the core's responsibility. Very true. > > Do we currently kick off all the drivers? I thought we didn't. Or more > > precisely, the core doesn't -- it just recommends that when a driver does > > a reset, that driver should kick off all the other drivers. That's > > definitely wrong. > > Usbfs OTOH does. A clear case of wrong layering. > Or at least tries to do it. I got a bug report about lockups in this cases. Evidently these changes we've been discussing will touch many different parts of the core. > > > 3. Usbfs is a security problem and needs filtering of control requests. > > > You can use it to set a device to an occupied address thereby crashing > > > any device. > > > > Certainly some of the control requests should require the appropriate > > capability. Just something else that needs to be taken care of. > > Yes, but not low priority. Okay. Alan Stern ------------------------------------------------------- This SF.net email is sponsored by: eBay Get office equipment for less on eBay! http://adfarm.mediaplex.com/ad/ck/711-11697-6916-5 _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
