On Mon, Jun 06, 2011 at 04:30:55PM -0400, Alan Stern wrote: > On Mon, 6 Jun 2011, Sarah Sharp wrote: > > > I have always assumed that when a driver requests a device reset, a > > change of configuration or alternate interface setting, that it has > > already either canceled active URBs or completed all the URBs for the > > endpoints that are going away. Otherwise the device could see a packet > > bound for an endpoint that isn't part of the current interface. If > > those race conditions do exist, then we need to revisit the > > implementation, but it seems like only a broken driver would leave URBs > > pending. > > Surprisingly, usbcore does not enforce the proper order of operations > (disable and flush endpoints, then issue Set-Config or Set-Interface) > requests in usb_set_interface(). This really should be fixed. I guess > the original motivation was to make error recovery simpler -- if the > device doesn't accept the request then none of the data structures need > to be changed. I see. Yeah, that probably should be fixed. > > I think the issue was that usb_disable_endpoint() and > > usb_enable_endpoint() are only called after the control transfer to the > > device to change the configuration or alternate interface setting > > succeeds. The bandwidth checking needs to be done before the USB core > > issues that control transfer. > > Is this requirement in the xHCI spec? I don't recall seeing it (but > then I haven't read the whole thing). Yes, it is a requirement of the xHCI spec. It's really that they want to allow virtualization hosts the opportunity to deny guests the chance to switch to certain configurations or alternate interface settings. I suppose if two guests could share different interfaces of the same device, then you wouldn't want one guest to suddenly switch configurations. > > I also I think I didn't want to use those functions because I wanted to > > batch all the changes to the endpoints associated with an alternate > > interface setting change or a configuration change into one command to > > issue to the xHCI host. > > Yes, that's understandable. > > > The code to drop old endpoints and add new ones really needs to be done > > atomically in the functions that attempt to set up the new configuration > > or alternate interface settings. The code in those functions might look > > ad-hock, but it's because I'm trying to satisfy the atomicity > > requirements and the requirement to change the xHCI bandwidth structures > > before the control transfer is issued. > > That's why we have hcd->bandwidth_mutex. You don't have to remove the > old endpoints and set up the new ones in the same command, do you? > That is, you could have two commands: one to drop the old endpoints and > one to add the new endpoints. Why wouldn't I want to add and drop the endpoints in the same command? It's really more efficient from the xHCI driver standpoint, since we only have to wait for one command. But yes, it could be done as long as the two commands are protected by the bandwidth mutex. > > > In the end, it would be really nice to make all this stuff totally > > > rational. Right now it's kind of an ad-hoc mess. We should go through > > > a unified procedure whenever updating an altsetting or config: > > > > > > Disable and flush the current endpoints; > > > > > > Remove the endpoints' hardware resources (including bandwidth); > > > > > > Allocate hardware resources for the new endpoints; > > > > > > Enable the new endpoints. > > > > Mmm, but the removing of old endpoint bandwidth resources has to happen > > atomically with adding new endpoint bandwidth resources. > > Your meaning isn't clear. Do you mean the overall bandwidth change has > to be handled as a single transaction, including both removals and > additions? Yes, sorry, there is no requirement that we remove and add the endpoints in one command. I just meant we need to be careful to protect bandwidth changes with the bandwidth mutex, because the device can still choose to stall the control transfer to change the configuration or alternate interface setting. > Or do you mean that bandwidth changes for two devices must > not overlap? The changing of the endpoints (removals and adds) and issuing the control transfer all need to happen under the bandwidth mutex. The device could reject the control transfer, and we need to be able to revert the xHCI host back to the old setup. We can't let another bandwidth change happen in between the control transfer submission and the revert, because that device could steal too much of the bus bandwidth to allow the revert back to a more bandwidth intensive alt setting. Because of the requirement that we do all that under the bandwidth mutex, there can only be one bandwidth change command outstanding for the xHCI host controller. > Regardless, this is how we should do it. At the same time, we could > include a feature allowing drivers to decrease the de facto maxpacket > values below the values given in the endpoint descriptors, for improved > scheduling performance. Well, ok, but the xHCI host controller will only accept powers of 2 max packet sizes, so will there be any drivers that will benefit from that? Also, what about USB 3.0 devices, where multiple max packets can be burst at one time? The devices that set bMaxBurst are required to set the max packet size to a fixed value. I think USB 3.0 isochronous devices can do bursts, but I would have to check. I'm more interested in making sure that the drivers that use a different polling interval than advertised in the endpoint descriptors are able to change the xHCI interval. > > > We should also try to come up with a good strategy for handling errors. > > > It's too bad that xHCI doesn't provide any way to check a new bandwidth > > > setting without disabling the current endpoints. > > > > Yeah, the driver could just attempt to add the new endpoints, but if any > > of the endpoint addresses overlap, the host controller is going to > > reject them, as Tanya has found out. So we really need to drop the old > > endpoints before adding new ones. > > That's not what I meant. Suppose an attempt to change a config or > altsetting fails -- this could be because of limitations on the host > side or on the device side. What state should we end up in? > > In the Set-Interface case, the kernel should be able to return to its > original state (but goodness knows what state the device will be in). > In the Set-Config case, the kernel won't be able to do much of anything > -- it will have to behave as though the device is unconfigured, even > though it probably isn't. If the host rejects a new configuration or alt setting, we should stay with the current setup, without attempting to change the device. If the device rejects the change by stalling the control transfer to set the new configuration or alt setting, we need to revert the host state back to the old setup. My intent was to make sure this worked properly when drivers called usb_set_configuration() and usb_set_interface(). It looks like usb_set_configuration() works for the first configuration setup, but if the device is already configured when usb_set_configuration() is called, the guarantee isn't met. I forgot about the bandwidth mutex in my patch last night, so the patch probably isn't correct. I think I need to move the lock of the bandwidth mutex above the call to usb_disable_device(), and add a lock and unlock around the call in hub.c:usb_disconnect. Updated patch shortly. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
