On Fri, Feb 24, 2012 at 11:39:13AM -0500, Alan Stern wrote: > On Thu, 23 Feb 2012, Sarah Sharp wrote: > > > So it seems like usb_probe_interface, usb_driver_claim_interface, and > > usb_unbind_interface would be the interesting places to do timeout > > enabling/disabling. > > Yeah. But remember that the driver may call usb_set_interface while > usb_probe_interface is running. Yes. That's why I wanted to allow the usb_set_interface call to finish before evaluating the timeouts. > > There's this note in usb_unbind_interface that's a bit concerning: > > > > /* Reset other interface state. > > * We cannot do a Set-Interface if the device is suspended or > > * if it is prepared for a system sleep (since installing a new > > * altsetting means creating new endpoint device entries). > > * When either of these happens, defer the Set-Interface. > > */ > > > > Why are we trying to avoid creating new endpoint device entries? > > Are we trying to avoid memory allocation during driver unbind when the > > system is going to sleep? If so, that's important to the xHCI driver > > design for setting/clearing timeouts. > > This has nothing to do with memory allocation. It is a restriction > imposed by the PM layer; while the system is going to sleep, you are > not allowed to register new devices. Otherwise the PM core could never > know for sure when it had called all the devices' suspend methods. Ah, ok, good. That makes my life simpler. > > Yes, I think it does make sense to update the timeouts and max exit > > latencies whenever there's a bandwidth change. I do think I should > > submit the bandwidth changes due to config/alt setting changes before > > attempting to change the timeouts. That way a new config/alt setting > > will have a better chance of succeeding, but we may not get the best > > power out of it. > > > > All the information I need about what's changing is really in the places > > where the bandwidth calculation changes happen. Putting aside the > > question of drivers disabling hub-initiated timeouts, here's the design > > I have in mind (sorry in advance for the length): > > > > 1. For each usb_device, add two new fields to track the maximum exit > > latency for U1 and U2. This would be for the entire path from the > > device to the roothub, including host controller U1/U2 exit latency. > > Also add two new fields to track the current U1 and U2 timeouts that are > > programmed into the parent hub (possibly this belongs in the parent hub > > structure instead as a per port array). The usb_device also gets two > > new bit fields that track whether device-initiated U1 or U2 is enabled. > > Might as well store those timeouts in the device structure, because > they don't mean anything if no device is plugged into the port. Ok, sure. > > 2. Reuse the lpm_capable field to indicate that a device is LPM capable. > > All certified USB 3.0 devices are *supposed* to support LPM, but they > > need to indicate their U1 and U2 max exit latencies in the SuperSpeed > > Extended Capabilities BOS descriptor, and we need to handle the odd case > > where that's not present (or a non-certified hub made it out to the > > market without LPM support). > > > > 3. When a new configuration is selected, save the old U1 and U2 > > timeouts, and then disable the timeouts and device-initiated U1/U2 (if > > the device is configured). When the USB core calls into the xHCI driver > > to add a new periodic endpoint for a bandwidth change, add the service > > interval and endpoint type to a small linked list of endpoints, sorted > > by configuration and interface numbers. If the USB core removes an > > endpoint, delete it from the list. We might even be able to reuse some > > of those awful bandwidth tables I had to create for the Panther Point > > xHCI host controller. > > > > 4. Once the bandwidth change has successfully (but before the interfaces > > are bound), call into the xHCI driver twice to calculate the new U1 and > > U2 timeouts. It will attempt to submit an Evaluate Context command to > > change the max exit latency for the device. If the xHC host agrees to > > that latency, the xHCI driver returns a timeout. Otherwise the call > > returns an error. (We have to increase the max exit latency stored by > > the xHC before we change any timeouts in parent hubs, so that the xHC > > will have added extra latency in its schedule before we enable U1/U2.) > > > > 4b. If the configuration change fails due to the SetConfig control > > transfer failing, and we fall back to a previously enabled > > configuration, > > We don't. If a Set-Config request fails, we give up and assume the > device is unconfigured. > > > then call into the xHCI reset bandwidth function. That > > will roll back any changes to the endpoints, and issue xHC commands to > > put the MEL back to what it was before the timeouts where disabled. If > > those fail, the timeouts remain disabled. If they succeed, then the USB > > core can proceed to step 5 with the old U1/U2 timeouts that it saved in > > step 3. > > You might as well just leave the timeouts disabled, or set them to some > standard value. It won't matter much because we won't communicate with > the device again until another config change is attempted. If the device is unconfigured, I can't set or clear the device-initiated U1/U2 features, so I think the best option is to leave the timeouts disabled. > > 5. The USB core sends the request to the device to enable > > device-initiated U1 or U2. If that fails, just clear the > > device-initiated U1 or U2 flag in the usb_device. Then attempt to send > > the request to the parent hub to set the U1 or U2 timeout. We have to > > set a non-zero timeout (0xFF) to tell the parent hub to accept U1 or U2 > > requests from the device if we are only doing device-initiated U1 and > > U2. If we're setting parent hub U1 and U2 timeouts for hub-initiate U1 > > or U2, we'll set it to a non-zero (and non-0xFF) value that the xHCI > > driver calculated. > > When does this take place? If you're not careful, you will end up > enabling the timeouts at a point where they're supposed to remain > disabled. I was going to evaluate the timeouts after the set configuration succeeded, but before the interface devices were created (and thus before the interface drivers were bound). > > 6a. If the request to the parent hub to set the timeout fails three > > times, disable LPM completely: send a request to the device to disable > > device-initiated U1/U2, and call into the xHCI driver to set the max > > exit latency for the device to zero. Possibly mark the hub as not being > > lpm_capable. > > > > 6b. If the request succeeds, update the device with the system exit > > latency (total max exit latency, plus hub delays for hub header > > decoding). If it rejects that, disable LPM, and mark the device as not > > lpm_capable. > > > > > > Now, with all that in place, what is the best way to accommodate drivers > > that don't want the hub U1/U2 timeouts to be set? One way I thought of > > doing it was to look at all the device drivers in step 4 and leave the > > U1/U2 timeouts disabled if any of them wanted to disable hub-initiated > > LPM. That has issues though, because we might be changing to alt > > setting 0 just before we unbind a driver, and drivers can bind to an > > interface anytime later when they are finally loaded. That solution > > might be necessary, but it's not sufficient to fix the problem. > > > > It seems like on top the described system, I need to do some work during > > the bind and unbind of drivers. In bind, if we need to change to alt > > setting 0, do that. Before the driver's probe function is called, check > > whether any of the drivers for the currently installed interfaces need > > to have the hub U1/U2 timeouts disabled. If none of them do, repeat > > steps 4-6, otherwise disable the timeouts. The probe function might > > install a different alt setting, but I think the usb_set_interface call > > will be able to see the driver that was being bound to the interface. > > > > On a call to usb_driver_claim_interface, check for any interface drivers > > that need the hub U1/U2 timeouts disabled (including the driver that's > > claiming the interface). If so, disable the timeouts. > > > > On a call to usb_unbind_interface, if we need to switch to alt setting > > 0, attempt that. Once the driver is unbound, check whether any driver > > in the new set of bound drivers needs to have hub U1/U2 timeouts > > disabled. If so, disable the timeouts. If not, repeat steps 4-6 to see > > if we can we the timeouts. > > It sounds like you need to add a new counter to the device structure: > the number of outstanding requests for U1/U2 timeouts to be disabled. > The counter gets incremented each time an interface is bound to a > driver that wants to disable LPM (and decremented during unbinding). > It also gets incremented around each call to change an altsetting or > config. Then you enable the timeouts whenever the counter decrements > to 0. Yes, that sounds like a very useful idea. Then I don't have to go traversing the driver interface structures every time. As long as drivers don't change their hub-initiated LPM disable flag, I think we're fine. Or maybe they need to call into a USB core function to change that flag. > Mutual exclusion might be a little tricky, though. One driver can > change an altsetting at the same time as another driver is being bound > to a different interface. We could use the bandwidth_mutex to do mutual exclusion. And yes, it might be tricky. I think as long as we narrow the exclusion range to exactly when we're adding or dropping endpoints, and around the function that decrements the hub-initiated LPM counter and updates the timers, I think we should be fine. Then usb_set_interface would block on the mutex if the driver that's being bound needs to disable the hub-initiated timeouts. If the set interface grabbed the mutex first, and enabled the timeouts, that's fine, because the bind will grab the mutex, disable the timeouts, and drop the mutex before the driver probe function gets called. As long as the timeouts are disabled before the requesting driver can use the new interface or configuration, it doesn't matter if they flip-flop between enabled and disabled in that intermediate period. It will cause the xHCI driver to execute some extra commands, but drivers don't change interfaces or bind/unbind very often, so I don't think there's a performance issue there. 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
