On 13.11.2014 17:45, Alan Stern wrote: > On Thu, 13 Nov 2014, Mathias Nyman wrote: > >> Currently we queue a reset endpoint command from the .endpoint_reset callback in host, this is far too late and should be moved >> to when we get a STALL event. >> >> xhci needs to reset control endpoints on stall as well [1] >> >> I got a testpatch for this, but the more I look into how we handle reset endpoint for clearing halts, stop endpoint for urb dequeue, and reset device, >> the more I notice that there are several other cases that needs fixing. testpatch for the halted ep is here: >> >> https://git.kernel.org/cgit/linux/kernel/git/mnyman/xhci.git/commit/?h=ep_reset_halt_test&id=fe43d559e0816f65e5373e863a7da8062d311cd7 >> >> It's hard to see from patch diff itself what it does, but basically we call xhci_cleanup_halted_endpoint() in finish_td() if the transfer event status >> is STALL, or a TX error that requires resetting the endpoint. > > That sounds right. You also need to ring the doorbell if the endpoint > queue is non-empty. > Right, we'll ring the doorbell when the set dq pointer command completes. Previously the doorbell was rung also on completed reset endpoint commans, which caused issues Felipe ran into. That is fixed in an earlier patch. >> There are still issues with setting the dequeue pointer correctly after stop or reset endpoint, I think this >> is because we try to find the next TD based on a saved "stopped TD" value that might not valid anymore (i.e. a reset device in between reset endpoint and set dq pointer) >> this issue is seen with DVB tuners when changing channels: > > I'm not aware of the details. Don't you always want to move to the > start of the first TRB following the TD that got the error? Yes, thats how I also understood it. > > The algorithm described in the DVB tuner bug is clearly wrong, since it > doesn't move the dequeue pointer until usb_clear_halt() is called, > which is far too late. The right approach is to fix up the dequeue > pointer before giving back the URB (so there's no need to save a > "stopped TD" value). Otherwise there will be TDs in the endpoint ring > containing stale DMA pointers to buffers that have already been > unmapped. Thats right, I cleaned up the patch and removed resetting the endpoint from the .endpoint_reset() callback which was called as a result of usb_clear_halt(). Now we queue a reset endpoint and set dequeue pointer before giving back the URB. I still set a "stopped td" value, but could as well just pass it as function parameter. Actually I'll do that in later cleanup patch. Latest version of the patch is now in my tree in a reset-rework-v2 branch, with fixes comments and removed The brach includes the other dorbell ringing patch as well. both are on top of 3.18-rc4. I still need to test it before sending it further, the tree is here: git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git reset-rework-v2 latest reset stall patch: https://git.kernel.org/cgit/linux/kernel/git/mnyman/xhci.git/commit/?h=reset-rework-v2&id=263ae54010ffadec17741f7215de64ad40a4bf5e fix doorbell ring patch (already tested by Felipe): https://git.kernel.org/cgit/linux/kernel/git/mnyman/xhci.git/commit/?h=reset-rework-v2&id=c96885c658294fef593f2109d194fa07d140c384 -Mathias -- 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
