Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On Fri, Nov 14, 2014 at 06:37:33PM +0200, Mathias Nyman wrote: > On 14.11.2014 17:49, Felipe Balbi wrote: > > Hi, > > > > On Fri, Nov 14, 2014 at 09:04:16AM -0600, Felipe Balbi wrote: > >> On Fri, Nov 14, 2014 at 10:39:15AM +0200, Mathias Nyman wrote: > >>> On 13.11.2014 20:46, Felipe Balbi wrote: > On Thu, Nov 13, 2014 at 12:31:28PM -0600, Felipe Balbi wrote: > > Hi, > > > > (your mailing is un-wrapping emails, I always end up with pretty long > > lines and have to rewrap them) > > > > On Thu, Nov 13, 2014 at 07:58:28PM +0200, Mathias Nyman wrote: > >>> 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 > > > > I'll test this one. > > for both commits on that branch, you can add my: > > Tested-by: Felipe Balbi > > But please also add proper fixes and Cc: stable, so older kernels can > use those. > > >>> > >>> Thanks a lot for testing again. > >>> > >>> I'll CC stable and add your tested-by, but I still don't know exactly > >>> which commit it Fixes. This might have been there since the early days > >>> of xhci. > >> > >> git blame usually helps. Just look for code which was differentiating > >> COMP_STALL and treating it differently. I know that at least v3.14 has > >> the problem. > > > > Here you go: > > > > Fixes: bcef3fd (USB: xhci: Handle errors that cause endpoint halts.) > > Cc: # v2.6.33+ > > > > Please make sure to send these patches ASAP otherwise they will not hit > > v3.18-final, it's getting pretty late. > > > > Amazing, was still struggling trying to figure out which one it was. > It goes back a long way, I'll send the patches right away. cool, thanks :-) > I btw now got a device I should be able to load gzero on, once I figure > out how to flash it. let me know if you need help with getting dwc3 running (I'm assuming you have one of the baytrail skus with peripheral mode support). -- balbi signature.asc Description: Digital signature
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On 14.11.2014 17:49, Felipe Balbi wrote: > Hi, > > On Fri, Nov 14, 2014 at 09:04:16AM -0600, Felipe Balbi wrote: >> On Fri, Nov 14, 2014 at 10:39:15AM +0200, Mathias Nyman wrote: >>> On 13.11.2014 20:46, Felipe Balbi wrote: On Thu, Nov 13, 2014 at 12:31:28PM -0600, Felipe Balbi wrote: > Hi, > > (your mailing is un-wrapping emails, I always end up with pretty long > lines and have to rewrap them) > > On Thu, Nov 13, 2014 at 07:58:28PM +0200, Mathias Nyman wrote: >>> 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 > > I'll test this one. for both commits on that branch, you can add my: Tested-by: Felipe Balbi But please also add proper fixes and Cc: stable, so older kernels can use those. >>> >>> Thanks a lot for testing again. >>> >>> I'll CC stable and add your tested-by, but I still don't know exactly >>> which commit it Fixes. This might have been there since the early days >>> of xhci. >> >> git blame usually helps. Just look for code which was differentiating >> COMP_STALL and treating it differently. I know that at least v3.14 has >> the problem. > > Here you go: > > Fixes: bcef3fd (USB: xhci: Handle errors that cause endpoint halts.) > Cc: # v2.6.33+ > > Please make sure to send these patches ASAP otherwise they will not hit > v3.18-final, it's getting pretty late. > Amazing, was still struggling trying to figure out which one it was. It goes back a long way, I'll send the patches right away. I btw now got a device I should be able to load gzero on, once I figure out how to flash it. Thanks again -Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
Hi, On Fri, Nov 14, 2014 at 09:04:16AM -0600, Felipe Balbi wrote: > On Fri, Nov 14, 2014 at 10:39:15AM +0200, Mathias Nyman wrote: > > On 13.11.2014 20:46, Felipe Balbi wrote: > > > On Thu, Nov 13, 2014 at 12:31:28PM -0600, Felipe Balbi wrote: > > >> Hi, > > >> > > >> (your mailing is un-wrapping emails, I always end up with pretty long > > >> lines and have to rewrap them) > > >> > > >> On Thu, Nov 13, 2014 at 07:58:28PM +0200, Mathias Nyman wrote: > > 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 > > >> > > >> I'll test this one. > > > > > > for both commits on that branch, you can add my: > > > > > > Tested-by: Felipe Balbi > > > > > > But please also add proper fixes and Cc: stable, so older kernels can > > > use those. > > > > > > > Thanks a lot for testing again. > > > > I'll CC stable and add your tested-by, but I still don't know exactly > > which commit it Fixes. This might have been there since the early days > > of xhci. > > git blame usually helps. Just look for code which was differentiating > COMP_STALL and treating it differently. I know that at least v3.14 has > the problem. Here you go: Fixes: bcef3fd (USB: xhci: Handle errors that cause endpoint halts.) Cc: # v2.6.33+ Please make sure to send these patches ASAP otherwise they will not hit v3.18-final, it's getting pretty late. cheers -- balbi signature.asc Description: Digital signature
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On Fri, Nov 14, 2014 at 10:39:15AM +0200, Mathias Nyman wrote: > On 13.11.2014 20:46, Felipe Balbi wrote: > > On Thu, Nov 13, 2014 at 12:31:28PM -0600, Felipe Balbi wrote: > >> Hi, > >> > >> (your mailing is un-wrapping emails, I always end up with pretty long > >> lines and have to rewrap them) > >> > >> On Thu, Nov 13, 2014 at 07:58:28PM +0200, Mathias Nyman wrote: > 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 > >> > >> I'll test this one. > > > > for both commits on that branch, you can add my: > > > > Tested-by: Felipe Balbi > > > > But please also add proper fixes and Cc: stable, so older kernels can > > use those. > > > > Thanks a lot for testing again. > > I'll CC stable and add your tested-by, but I still don't know exactly > which commit it Fixes. This might have been there since the early days > of xhci. git blame usually helps. Just look for code which was differentiating COMP_STALL and treating it differently. I know that at least v3.14 has the problem. -- balbi signature.asc Description: Digital signature
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On 13.11.2014 20:46, Felipe Balbi wrote: > On Thu, Nov 13, 2014 at 12:31:28PM -0600, Felipe Balbi wrote: >> Hi, >> >> (your mailing is un-wrapping emails, I always end up with pretty long >> lines and have to rewrap them) >> >> On Thu, Nov 13, 2014 at 07:58:28PM +0200, Mathias Nyman wrote: 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 >> >> I'll test this one. > > for both commits on that branch, you can add my: > > Tested-by: Felipe Balbi > > But please also add proper fixes and Cc: stable, so older kernels can > use those. > Thanks a lot for testing again. I'll CC stable and add your tested-by, but I still don't know exactly which commit it Fixes. This might have been there since the early days of xhci. -Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On Thu, Nov 13, 2014 at 12:31:28PM -0600, Felipe Balbi wrote: > Hi, > > (your mailing is un-wrapping emails, I always end up with pretty long > lines and have to rewrap them) > > On Thu, Nov 13, 2014 at 07:58:28PM +0200, Mathias Nyman wrote: > > > 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 > > I'll test this one. for both commits on that branch, you can add my: Tested-by: Felipe Balbi But please also add proper fixes and Cc: stable, so older kernels can use those. -- balbi signature.asc Description: Digital signature
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
Hi, (your mailing is un-wrapping emails, I always end up with pretty long lines and have to rewrap them) On Thu, Nov 13, 2014 at 07:58:28PM +0200, Mathias Nyman wrote: > > 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 I'll test this one. > 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 Looks like both patches need to have a Fixes and Cc: stable added to them. Now let me build and run this branch. -- balbi signature.asc Description: Digital signature
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
> > 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. > ..with fixed function comments. The branch incudes the other doorbell patch as well. -Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On Thu, 13 Nov 2014, Mathias Nyman wrote: > The comment for xhci_endpoint_reset says its called in_interrupt context? > Does the usb core really call the .endpoint_reset in interrupt > context? I tried to follow the codepaths in the usb core but couln't figure > it out It doesn't really say anywhere. I'm pretty sure the intention was that the endpoint_reset callback is always called in process context. It wouldn't hurt to add a comment to usb_hcd_reset_endpoint() documenting this. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On Thu, Nov 13, 2014 at 10:31:55AM -0500, Alan Stern wrote: > On Wed, 12 Nov 2014, Felipe Balbi wrote: > > > > By the way, does the same sort of thing happen after a transfer > > > error (such as a CRC mismatch)? Does the xHCI controller change the > > > state to EP_STATE_HALTED? Or does it instead go directly to > > > > There are a few conditions in which XHCI will change EP state to > > EP_STATE_HALTED, one of them is a STALL token from the peripheral and > > the others would be really error conditions: Babble, CRC error, etc. > > > > The spec even has a note about it, which I quote: > > > > " > > A Halt condition or USB Transaction error detected on a USB pipe > > shall cause a Running Endpoint to transition to the Halted > > state. A Reset Endpoint Command shall be used to clear the Halt > > condition on the endpoint and transition the endpoint to the > > Stopped state. A Stop Endpoint Command received while an > > endpoint is in the Halted state shall have no effect and shall > > generate a Command Completion Event with the Completion Code set > > to Context State Error. > > " > > > > > EP_STATE_STOPPED? You probably want to treat that case and the STALL > > > case as similarly as possible. > > > > There's already code to deal with that, take a look at > > xhci_requires_manual_halt_cleanup() and its callers: > > Then shouldn't the recovery from a STALL be exactly the same as the > recovery from any other sort of transfer error? and it is :-) if (requres_manual_halt_cleanup()) xhci_endpoint_cleanup_halt(); that's basically what happens. > > > Right. In theory you could do it any time up until the completion > > > routine returns. Doing it when you process the failed TD seems like a > > > good choice -- advance the dequeue pointer and issue the command at the > > > same time. > > > > My concern here is that this will happen when the first usb_submit_urb() > > completes. I wonder if moving this to when the following > > usb_submit_urb() is about to start would be better ? > > No, because there may be some other URBs already on the endpoint queue. > If no further URBs are submitted then the queue won't get cleaned up. oh, ok... makes sense. > > I think this has a higher probability of being correct. Class driver > > might not queue any URB to a particular after the first Stall, so why > > would be move the endpoint away from EP_STATE_HALTED prematurely ? > > In order to handle the queue. Of course, if the queue is empty then > there's no harm in leaving the ep in EP_STATE_HALTED until another URB > is submitted. > > By the same reasoning, when the state is changed to EP_STATE_STOPPED, > the doorbell should be rung. right. -- balbi signature.asc Description: Digital signature
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
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. > 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? 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. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On Wed, 12 Nov 2014, Felipe Balbi wrote: > > By the way, does the same sort of thing happen after a transfer > > error (such as a CRC mismatch)? Does the xHCI controller change the > > state to EP_STATE_HALTED? Or does it instead go directly to > > There are a few conditions in which XHCI will change EP state to > EP_STATE_HALTED, one of them is a STALL token from the peripheral and > the others would be really error conditions: Babble, CRC error, etc. > > The spec even has a note about it, which I quote: > > " > A Halt condition or USB Transaction error detected on a USB pipe > shall cause a Running Endpoint to transition to the Halted > state. A Reset Endpoint Command shall be used to clear the Halt > condition on the endpoint and transition the endpoint to the > Stopped state. A Stop Endpoint Command received while an > endpoint is in the Halted state shall have no effect and shall > generate a Command Completion Event with the Completion Code set > to Context State Error. > " > > > EP_STATE_STOPPED? You probably want to treat that case and the STALL > > case as similarly as possible. > > There's already code to deal with that, take a look at > xhci_requires_manual_halt_cleanup() and its callers: Then shouldn't the recovery from a STALL be exactly the same as the recovery from any other sort of transfer error? > > Right. In theory you could do it any time up until the completion > > routine returns. Doing it when you process the failed TD seems like a > > good choice -- advance the dequeue pointer and issue the command at the > > same time. > > My concern here is that this will happen when the first usb_submit_urb() > completes. I wonder if moving this to when the following > usb_submit_urb() is about to start would be better ? No, because there may be some other URBs already on the endpoint queue. If no further URBs are submitted then the queue won't get cleaned up. > I think this has a higher probability of being correct. Class driver > might not queue any URB to a particular after the first Stall, so why > would be move the endpoint away from EP_STATE_HALTED prematurely ? In order to handle the queue. Of course, if the queue is empty then there's no harm in leaving the ep in EP_STATE_HALTED until another URB is submitted. By the same reasoning, when the state is changed to EP_STATE_STOPPED, the doorbell should be rung. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
Hi On 13.11.2014 17:11, Felipe Balbi wrote: > Hi, > > On Thu, Nov 13, 2014 at 01:16:34PM +0200, Mathias Nyman wrote: >> On 13.11.2014 00:28, Felipe Balbi wrote: >>> Hi, >>> >>> On Wed, Nov 12, 2014 at 04:54:06PM -0500, Alan Stern wrote: >>> >>> [...] >>> > and the doorbell will never rung. But even if I drop EP_HALTED from the > check below and let EP doorbell be rung, nothing will happen because, > according to XHCI spec 1.0, we *must* first issue a Reset Endpoint > command to get Endpoint to move to EP_STATE_STOPPED, in which case, > ringing that EP's doorbell will actually cause a transfer to happen. > > Right now, what happens is that second usb_submit_urb() does nothing and > the 10 second timer expires, causing the URB to be canceled and test > failing with -ETIMEDOUT. Okay, I see. What usbcore and usbtest expect to happen is this: (1)An URB fails because the endpoint sent a STALL. The completion routine is called with status -EPIPE. (2)When the completion routine returns, the next URB in the endpoint's queue should get handled by the hardware. If the endpoint is still halted, this URB should fail with -EPIPE just like the first. (3)Etc. Eventually the endpoint queue empties out or the class driver calls usb_clear_halt(). >>> >>> perfect :-) >>> So (1) works as desired, but (2) doesn't work because the doorbell never gets rung. >>> >>> right. >>> And the easiest way to make (2) work is to issue a Reset Endpoint command. >>> >>> There's one extra bit of information here, see below. >>> (There are other, more complicated ways to get the same result. For instance, you could loop through the remaining queued URBs, giving them back one by one with -EPIPE. And each time an URB is submitted, you could give it back right away. But Reset Endpoint is simpler.) >>> >>> IMO issuing a Reset Endpoint is the only correct way of implementing >>> this. See, there are two sides to usbtest + g_zero love relationship: >>> >>> (a) it will help you test your UDC driver. >>> (b) it will help you test your HCD driver. >>> >>> Currently, we have a bug with (b), but if iterate over the list of >>> submitted URBs we can create two problems: >>> >>> (i) There's no way to synchronize class driver's usb_submit_urb() with >>> the exact time when we iterate over the list of pending URBs in >>> xhci-hcd. Which means that we could very well iterate over an empty list >>> and think everything was fine. Heck, this would even happen with >>> usbtest, note that simple_io() always waits 10 seconds for a transfer >>> completion before returning control to the caller. >>> >>> (ii) We would fall into the possibility of not catching bugs with UDCs >>> running g_zero because HCD would just return early -EPIPE for us without >>> asking UDC to handle another transfer :-) >>> >>> Because of these two cases, I think the *only* way to solve this is by >>> issuing a Reset Endpoint cmd so another token can be shifted onto the >>> data lines. >>> In the patch, you talked about clearing the endpoint halt. But that's not what you want to do; you want to issue a Reset Endpoint command, which affects only the host controller. The endpoint's status in the >>> >>> this is exactly what xhci_cleanup_halted_endpoint() does. Sure, it's a >>> misnamer and should probably be renamed to either >>> xhci_reset_endpoint_cmd() or xhci_clear_endpoint_state_halted() to make >>> it clearer as to what's happening. But that can happen later, we don't >>> need to clean that up in order to fix the bug :-) >>> peripheral device will remain unchanged -- no halt will be cleared. That contributed to my confusion on reading the patch. >>> >>> yeah, that got me for a while too. I had to keep reminding myself what >>> xhci_cleanup_halted_endpoint() was doing ;-) >>> By the way, does the same sort of thing happen after a transfer error (such as a CRC mismatch)? Does the xHCI controller change the state to EP_STATE_HALTED? Or does it instead go directly to >>> >>> There are a few conditions in which XHCI will change EP state to >>> EP_STATE_HALTED, one of them is a STALL token from the peripheral and >>> the others would be really error conditions: Babble, CRC error, etc. >>> >>> The spec even has a note about it, which I quote: >>> >>> " >>> A Halt condition or USB Transaction error detected on a USB pipe >>> shall cause a Running Endpoint to transition to the Halted >>> state. A Reset Endpoint Command shall be used to clear the Halt >>> condition on the endpoint and transition the endpoint to the >>> Stopped state. A Stop Endpoint Command received while an >>> endpoint is in the Halted state shall have no effect and shall >>> generate a Command Completion Event with the Completion Code set >>> to Context Sta
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
Hi, On Thu, Nov 13, 2014 at 01:16:34PM +0200, Mathias Nyman wrote: > On 13.11.2014 00:28, Felipe Balbi wrote: > > Hi, > > > > On Wed, Nov 12, 2014 at 04:54:06PM -0500, Alan Stern wrote: > > > > [...] > > > >>> and the doorbell will never rung. But even if I drop EP_HALTED from the > >>> check below and let EP doorbell be rung, nothing will happen because, > >>> according to XHCI spec 1.0, we *must* first issue a Reset Endpoint > >>> command to get Endpoint to move to EP_STATE_STOPPED, in which case, > >>> ringing that EP's doorbell will actually cause a transfer to happen. > >>> > >>> Right now, what happens is that second usb_submit_urb() does nothing and > >>> the 10 second timer expires, causing the URB to be canceled and test > >>> failing with -ETIMEDOUT. > >> > >> Okay, I see. What usbcore and usbtest expect to happen is this: > >> > >> (1)An URB fails because the endpoint sent a STALL. The completion > >>routine is called with status -EPIPE. > >> > >> (2)When the completion routine returns, the next URB in the > >>endpoint's queue should get handled by the hardware. If the > >>endpoint is still halted, this URB should fail with -EPIPE > >>just like the first. > >> > >> (3)Etc. Eventually the endpoint queue empties out or the class > >>driver calls usb_clear_halt(). > > > > perfect :-) > > > >> So (1) works as desired, but (2) doesn't work because the doorbell > >> never gets rung. > > > > right. > > > >> And the easiest way to make (2) work is to issue a Reset Endpoint > >> command. > > > > There's one extra bit of information here, see below. > > > >> (There are other, more complicated ways to get the same result. For > >> instance, you could loop through the remaining queued URBs, giving them > >> back one by one with -EPIPE. And each time an URB is submitted, you > >> could give it back right away. But Reset Endpoint is simpler.) > > > > IMO issuing a Reset Endpoint is the only correct way of implementing > > this. See, there are two sides to usbtest + g_zero love relationship: > > > > (a) it will help you test your UDC driver. > > (b) it will help you test your HCD driver. > > > > Currently, we have a bug with (b), but if iterate over the list of > > submitted URBs we can create two problems: > > > > (i) There's no way to synchronize class driver's usb_submit_urb() with > > the exact time when we iterate over the list of pending URBs in > > xhci-hcd. Which means that we could very well iterate over an empty list > > and think everything was fine. Heck, this would even happen with > > usbtest, note that simple_io() always waits 10 seconds for a transfer > > completion before returning control to the caller. > > > > (ii) We would fall into the possibility of not catching bugs with UDCs > > running g_zero because HCD would just return early -EPIPE for us without > > asking UDC to handle another transfer :-) > > > > Because of these two cases, I think the *only* way to solve this is by > > issuing a Reset Endpoint cmd so another token can be shifted onto the > > data lines. > > > >> In the patch, you talked about clearing the endpoint halt. But that's > >> not what you want to do; you want to issue a Reset Endpoint command, > >> which affects only the host controller. The endpoint's status in the > > > > this is exactly what xhci_cleanup_halted_endpoint() does. Sure, it's a > > misnamer and should probably be renamed to either > > xhci_reset_endpoint_cmd() or xhci_clear_endpoint_state_halted() to make > > it clearer as to what's happening. But that can happen later, we don't > > need to clean that up in order to fix the bug :-) > > > >> peripheral device will remain unchanged -- no halt will be cleared. > >> That contributed to my confusion on reading the patch. > > > > yeah, that got me for a while too. I had to keep reminding myself what > > xhci_cleanup_halted_endpoint() was doing ;-) > > > >> By the way, does the same sort of thing happen after a transfer > >> error (such as a CRC mismatch)? Does the xHCI controller change the > >> state to EP_STATE_HALTED? Or does it instead go directly to > > > > There are a few conditions in which XHCI will change EP state to > > EP_STATE_HALTED, one of them is a STALL token from the peripheral and > > the others would be really error conditions: Babble, CRC error, etc. > > > > The spec even has a note about it, which I quote: > > > > " > > A Halt condition or USB Transaction error detected on a USB pipe > > shall cause a Running Endpoint to transition to the Halted > > state. A Reset Endpoint Command shall be used to clear the Halt > > condition on the endpoint and transition the endpoint to the > > Stopped state. A Stop Endpoint Command received while an > > endpoint is in the Halted state shall have no effect and shall > > generate a Command Completion Event with the Completion Code set > > to Context State Error. > > " > > > >>
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On 13.11.2014 00:28, Felipe Balbi wrote: > Hi, > > On Wed, Nov 12, 2014 at 04:54:06PM -0500, Alan Stern wrote: > > [...] > >>> and the doorbell will never rung. But even if I drop EP_HALTED from the >>> check below and let EP doorbell be rung, nothing will happen because, >>> according to XHCI spec 1.0, we *must* first issue a Reset Endpoint >>> command to get Endpoint to move to EP_STATE_STOPPED, in which case, >>> ringing that EP's doorbell will actually cause a transfer to happen. >>> >>> Right now, what happens is that second usb_submit_urb() does nothing and >>> the 10 second timer expires, causing the URB to be canceled and test >>> failing with -ETIMEDOUT. >> >> Okay, I see. What usbcore and usbtest expect to happen is this: >> >> (1) An URB fails because the endpoint sent a STALL. The completion >> routine is called with status -EPIPE. >> >> (2) When the completion routine returns, the next URB in the >> endpoint's queue should get handled by the hardware. If the >> endpoint is still halted, this URB should fail with -EPIPE >> just like the first. >> >> (3) Etc. Eventually the endpoint queue empties out or the class >> driver calls usb_clear_halt(). > > perfect :-) > >> So (1) works as desired, but (2) doesn't work because the doorbell >> never gets rung. > > right. > >> And the easiest way to make (2) work is to issue a Reset Endpoint >> command. > > There's one extra bit of information here, see below. > >> (There are other, more complicated ways to get the same result. For >> instance, you could loop through the remaining queued URBs, giving them >> back one by one with -EPIPE. And each time an URB is submitted, you >> could give it back right away. But Reset Endpoint is simpler.) > > IMO issuing a Reset Endpoint is the only correct way of implementing > this. See, there are two sides to usbtest + g_zero love relationship: > > (a) it will help you test your UDC driver. > (b) it will help you test your HCD driver. > > Currently, we have a bug with (b), but if iterate over the list of > submitted URBs we can create two problems: > > (i) There's no way to synchronize class driver's usb_submit_urb() with > the exact time when we iterate over the list of pending URBs in > xhci-hcd. Which means that we could very well iterate over an empty list > and think everything was fine. Heck, this would even happen with > usbtest, note that simple_io() always waits 10 seconds for a transfer > completion before returning control to the caller. > > (ii) We would fall into the possibility of not catching bugs with UDCs > running g_zero because HCD would just return early -EPIPE for us without > asking UDC to handle another transfer :-) > > Because of these two cases, I think the *only* way to solve this is by > issuing a Reset Endpoint cmd so another token can be shifted onto the > data lines. > >> In the patch, you talked about clearing the endpoint halt. But that's >> not what you want to do; you want to issue a Reset Endpoint command, >> which affects only the host controller. The endpoint's status in the > > this is exactly what xhci_cleanup_halted_endpoint() does. Sure, it's a > misnamer and should probably be renamed to either > xhci_reset_endpoint_cmd() or xhci_clear_endpoint_state_halted() to make > it clearer as to what's happening. But that can happen later, we don't > need to clean that up in order to fix the bug :-) > >> peripheral device will remain unchanged -- no halt will be cleared. >> That contributed to my confusion on reading the patch. > > yeah, that got me for a while too. I had to keep reminding myself what > xhci_cleanup_halted_endpoint() was doing ;-) > >> By the way, does the same sort of thing happen after a transfer >> error (such as a CRC mismatch)? Does the xHCI controller change the >> state to EP_STATE_HALTED? Or does it instead go directly to > > There are a few conditions in which XHCI will change EP state to > EP_STATE_HALTED, one of them is a STALL token from the peripheral and > the others would be really error conditions: Babble, CRC error, etc. > > The spec even has a note about it, which I quote: > > " > A Halt condition or USB Transaction error detected on a USB pipe > shall cause a Running Endpoint to transition to the Halted > state. A Reset Endpoint Command shall be used to clear the Halt > condition on the endpoint and transition the endpoint to the > Stopped state. A Stop Endpoint Command received while an > endpoint is in the Halted state shall have no effect and shall > generate a Command Completion Event with the Completion Code set > to Context State Error. > " > >> EP_STATE_STOPPED? You probably want to treat that case and the STALL >> case as similarly as possible. > > There's already code to deal with that, take a look at > xhci_requires_manual_halt_cleanup() and its callers: > > 1754 static int xhci_req
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
Hi,
On Wed, Nov 12, 2014 at 04:54:06PM -0500, Alan Stern wrote:
[...]
> > and the doorbell will never rung. But even if I drop EP_HALTED from the
> > check below and let EP doorbell be rung, nothing will happen because,
> > according to XHCI spec 1.0, we *must* first issue a Reset Endpoint
> > command to get Endpoint to move to EP_STATE_STOPPED, in which case,
> > ringing that EP's doorbell will actually cause a transfer to happen.
> >
> > Right now, what happens is that second usb_submit_urb() does nothing and
> > the 10 second timer expires, causing the URB to be canceled and test
> > failing with -ETIMEDOUT.
>
> Okay, I see. What usbcore and usbtest expect to happen is this:
>
> (1) An URB fails because the endpoint sent a STALL. The completion
> routine is called with status -EPIPE.
>
> (2) When the completion routine returns, the next URB in the
> endpoint's queue should get handled by the hardware. If the
> endpoint is still halted, this URB should fail with -EPIPE
> just like the first.
>
> (3) Etc. Eventually the endpoint queue empties out or the class
> driver calls usb_clear_halt().
perfect :-)
> So (1) works as desired, but (2) doesn't work because the doorbell
> never gets rung.
right.
> And the easiest way to make (2) work is to issue a Reset Endpoint
> command.
There's one extra bit of information here, see below.
> (There are other, more complicated ways to get the same result. For
> instance, you could loop through the remaining queued URBs, giving them
> back one by one with -EPIPE. And each time an URB is submitted, you
> could give it back right away. But Reset Endpoint is simpler.)
IMO issuing a Reset Endpoint is the only correct way of implementing
this. See, there are two sides to usbtest + g_zero love relationship:
(a) it will help you test your UDC driver.
(b) it will help you test your HCD driver.
Currently, we have a bug with (b), but if iterate over the list of
submitted URBs we can create two problems:
(i) There's no way to synchronize class driver's usb_submit_urb() with
the exact time when we iterate over the list of pending URBs in
xhci-hcd. Which means that we could very well iterate over an empty list
and think everything was fine. Heck, this would even happen with
usbtest, note that simple_io() always waits 10 seconds for a transfer
completion before returning control to the caller.
(ii) We would fall into the possibility of not catching bugs with UDCs
running g_zero because HCD would just return early -EPIPE for us without
asking UDC to handle another transfer :-)
Because of these two cases, I think the *only* way to solve this is by
issuing a Reset Endpoint cmd so another token can be shifted onto the
data lines.
> In the patch, you talked about clearing the endpoint halt. But that's
> not what you want to do; you want to issue a Reset Endpoint command,
> which affects only the host controller. The endpoint's status in the
this is exactly what xhci_cleanup_halted_endpoint() does. Sure, it's a
misnamer and should probably be renamed to either
xhci_reset_endpoint_cmd() or xhci_clear_endpoint_state_halted() to make
it clearer as to what's happening. But that can happen later, we don't
need to clean that up in order to fix the bug :-)
> peripheral device will remain unchanged -- no halt will be cleared.
> That contributed to my confusion on reading the patch.
yeah, that got me for a while too. I had to keep reminding myself what
xhci_cleanup_halted_endpoint() was doing ;-)
> By the way, does the same sort of thing happen after a transfer
> error (such as a CRC mismatch)? Does the xHCI controller change the
> state to EP_STATE_HALTED? Or does it instead go directly to
There are a few conditions in which XHCI will change EP state to
EP_STATE_HALTED, one of them is a STALL token from the peripheral and
the others would be really error conditions: Babble, CRC error, etc.
The spec even has a note about it, which I quote:
"
A Halt condition or USB Transaction error detected on a USB pipe
shall cause a Running Endpoint to transition to the Halted
state. A Reset Endpoint Command shall be used to clear the Halt
condition on the endpoint and transition the endpoint to the
Stopped state. A Stop Endpoint Command received while an
endpoint is in the Halted state shall have no effect and shall
generate a Command Completion Event with the Completion Code set
to Context State Error.
"
> EP_STATE_STOPPED? You probably want to treat that case and the STALL
> case as similarly as possible.
There's already code to deal with that, take a look at
xhci_requires_manual_halt_cleanup() and its callers:
1754 static int xhci_requires_manual_halt_cleanup(struct xhci_hcd *xhci,
1755 struct xhci_ep_ctx *ep_ctx,
1756 unsigned int trb_comp_code)
1757 {
1758 /* TRB completion co
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On Wed, 12 Nov 2014, Felipe Balbi wrote: > Hi, > > On Wed, Nov 12, 2014 at 03:03:10PM -0500, Alan Stern wrote: > > On Wed, 12 Nov 2014, Felipe Balbi wrote: > > > > > If class driver wants to SetFeature(ENDPOINT_HALT) and > > > later tries to talk to the stalled endpoint, xhci will > > > move endpoint to EP_STATE_STALL and subsequent usb_submit_urb() > > > will not cause a USB token to be shifted into the data lines. > > > > > > Because of that, peripheral will never have any means of > > > STALLing a follow up token. > > > > > > This is a known error at least with g_zero + testusb -t 13 > > > and can be easily reproduced. > > > > Can you elaborate this description a bit more? I don't understand what > > the problem is. > > Look at drivers/usb/misc/usbtest.c::test_halt(). Here's a dump of the > code: ... > Note, specially, what verify_halted() does. It will issue a GetStatus() > followed by two usb_submit_urb(). The first usb_submit_urb() will be > STALLed by the function (g_zero) and that will cause the endpoint to > move from EP_STATE_RUNNING to EP_STATE_HALTED. The following > usb_submit_urb() will trigger: > > 2815 case EP_STATE_HALTED: > 2816 xhci_dbg(xhci, "WARN halted endpoint, queueing URB > anyway.\n"); > > But, because EP_HALTED flag is set, xhci_ring_ep_doorbell() will return > early: ... > and the doorbell will never rung. But even if I drop EP_HALTED from the > check below and let EP doorbell be rung, nothing will happen because, > according to XHCI spec 1.0, we *must* first issue a Reset Endpoint > command to get Endpoint to move to EP_STATE_STOPPED, in which case, > ringing that EP's doorbell will actually cause a transfer to happen. > > Right now, what happens is that second usb_submit_urb() does nothing and > the 10 second timer expires, causing the URB to be canceled and test > failing with -ETIMEDOUT. Okay, I see. What usbcore and usbtest expect to happen is this: (1) An URB fails because the endpoint sent a STALL. The completion routine is called with status -EPIPE. (2) When the completion routine returns, the next URB in the endpoint's queue should get handled by the hardware. If the endpoint is still halted, this URB should fail with -EPIPE just like the first. (3) Etc. Eventually the endpoint queue empties out or the class driver calls usb_clear_halt(). So (1) works as desired, but (2) doesn't work because the doorbell never gets rung. And the easiest way to make (2) work is to issue a Reset Endpoint command. (There are other, more complicated ways to get the same result. For instance, you could loop through the remaining queued URBs, giving them back one by one with -EPIPE. And each time an URB is submitted, you could give it back right away. But Reset Endpoint is simpler.) In the patch, you talked about clearing the endpoint halt. But that's not what you want to do; you want to issue a Reset Endpoint command, which affects only the host controller. The endpoint's status in the peripheral device will remain unchanged -- no halt will be cleared. That contributed to my confusion on reading the patch. By the way, does the same sort of thing happen after a transfer error (such as a CRC mismatch)? Does the xHCI controller change the state to EP_STATE_HALTED? Or does it instead go directly to EP_STATE_STOPPED? You probably want to treat that case and the STALL case as similarly as possible. > > For instance, if an endpoint is halted then there's no reason for the > > controller to shift any USB tokens for it onto the data lines. Doing > > so would just be a waste of bandwidth, since the response is bound to > > be another STALL. And it doesn't matter that the peripheral has no > > means to STALL any follow-up iens, since the host controller already > > knows the endpoint is halted. > > Now you're claiming that this is a bug on usbtest which has been in tree > for many, many years and is known to work with EHCI, MUSB and UHCI (at > least, probably dummy too), which is a different statement from previous > thread [1]. No, I simply failed to understood what you wanted to do. > > The comment in the patch talks about moving the dequeue pointer past > > the STALLed TD and then clearing the halt condition. Moving the > > dequeue pointer is fine -- there's no other way to take control of the > > TD back from the hardware -- but why would you want to clear the halt? > > The HCD isn't supposed to do that; the class driver is. > > See what usbtest does. It wants to make sure that, even if we issue > several URBs for that endpoint, the function will always STALL. Sure, > it's a waste of bandwidth, but what's the probability that any class > driver will actually do this outside of a test environment ? I think > it's not up to the HCD to device and it should, rather, let the function > respond with the expected STALL again which will, once more, move the > endpoint back
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
Hi,
On Wed, Nov 12, 2014 at 03:03:10PM -0500, Alan Stern wrote:
> On Wed, 12 Nov 2014, Felipe Balbi wrote:
>
> > If class driver wants to SetFeature(ENDPOINT_HALT) and
> > later tries to talk to the stalled endpoint, xhci will
> > move endpoint to EP_STATE_STALL and subsequent usb_submit_urb()
> > will not cause a USB token to be shifted into the data lines.
> >
> > Because of that, peripheral will never have any means of
> > STALLing a follow up token.
> >
> > This is a known error at least with g_zero + testusb -t 13
> > and can be easily reproduced.
>
> Can you elaborate this description a bit more? I don't understand what
> the problem is.
Look at drivers/usb/misc/usbtest.c::test_halt(). Here's a dump of the
code:
static int verify_not_halted(struct usbtest_dev *tdev, int ep, struct urb *urb)
{
int retval;
u16 status;
/* shouldn't look or act halted */
retval = usb_get_status(urb->dev, USB_RECIP_ENDPOINT, ep, &status);
if (retval < 0) {
ERROR(tdev, "ep %02x couldn't get no-halt status, %d\n",
ep, retval);
return retval;
}
if (status != 0) {
ERROR(tdev, "ep %02x bogus status: %04x != 0\n", ep, status);
return -EINVAL;
}
retval = simple_io(tdev, urb, 1, 0, 0, __func__);
if (retval != 0)
return -EINVAL;
return 0;
}
static int verify_halted(struct usbtest_dev *tdev, int ep, struct urb *urb)
{
int retval;
u16 status;
/* should look and act halted */
retval = usb_get_status(urb->dev, USB_RECIP_ENDPOINT, ep, &status);
if (retval < 0) {
ERROR(tdev, "ep %02x couldn't get halt status, %d\n",
ep, retval);
return retval;
}
if (status != 1) {
ERROR(tdev, "ep %02x bogus status: %04x != 1\n", ep, status);
return -EINVAL;
}
retval = simple_io(tdev, urb, 1, 0, -EPIPE, __func__);
if (retval != -EPIPE)
return -EINVAL;
retval = simple_io(tdev, urb, 1, 0, -EPIPE, "verify_still_halted");
if (retval != -EPIPE)
return -EINVAL;
return 0;
}
static int test_halt(struct usbtest_dev *tdev, int ep, struct urb *urb)
{
int retval;
/* shouldn't look or act halted now */
retval = verify_not_halted(tdev, ep, urb);
if (retval < 0)
return retval;
/* set halt (protocol test only), verify it worked */
retval = usb_control_msg(urb->dev, usb_sndctrlpipe(urb->dev, 0),
USB_REQ_SET_FEATURE, USB_RECIP_ENDPOINT,
USB_ENDPOINT_HALT, ep,
NULL, 0, USB_CTRL_SET_TIMEOUT);
if (retval < 0) {
ERROR(tdev, "ep %02x couldn't set halt, %d\n", ep, retval);
return retval;
}
retval = verify_halted(tdev, ep, urb);
if (retval < 0) {
int ret;
/* clear halt anyways, else further tests will fail */
ret = usb_clear_halt(urb->dev, urb->pipe);
if (ret)
ERROR(tdev, "ep %02x couldn't clear halt, %d\n",
ep, ret);
return retval;
}
/* clear halt (tests API + protocol), verify it worked */
retval = usb_clear_halt(urb->dev, urb->pipe);
if (retval < 0) {
ERROR(tdev, "ep %02x couldn't clear halt, %d\n", ep, retval);
return retval;
}
retval = verify_not_halted(tdev, ep, urb);
if (retval < 0)
return retval;
/* NOTE: could also verify SET_INTERFACE clear halts ... */
return 0;
}
Note, specially, what verify_halted() does. It will issue a GetStatus()
followed by two usb_submit_urb(). The first usb_submit_urb() will be
STALLed by the function (g_zero) and that will cause the endpoint to
move from EP_STATE_RUNNING to EP_STATE_HALTED. The following
usb_submit_urb() will trigger:
2815 case EP_STATE_HALTED:
2816 xhci_dbg(xhci, "WARN halted endpoint, queueing URB
anyway.\n");
But, because EP_HALTED flag is set, xhci_ring_ep_doorbell() will return
early:
316 void xhci_ring_ep_doorbell(struct xhci_hcd *xhci,
317 unsigned int slot_id,
318 unsigned int ep_index,
319 unsigned int stream_id)
320 {
321 __le32 __iomem *db_addr = &xhci->dba->doorbell[slot_id];
322 struct xhci_virt_ep *ep = &xhci->devs[slot_id]->eps[ep_index];
323 unsigned int ep_state = ep->ep_state;
324
325 /* Don't ring the doorbell for this endpoint if there are pending
326 * cancellations because we don't want to interrupt processing.
327 * We don't want to restart any stream rings if there's
Re: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
On Wed, 12 Nov 2014, Felipe Balbi wrote: > If class driver wants to SetFeature(ENDPOINT_HALT) and > later tries to talk to the stalled endpoint, xhci will > move endpoint to EP_STATE_STALL and subsequent usb_submit_urb() > will not cause a USB token to be shifted into the data lines. > > Because of that, peripheral will never have any means of > STALLing a follow up token. > > This is a known error at least with g_zero + testusb -t 13 > and can be easily reproduced. Can you elaborate this description a bit more? I don't understand what the problem is. For instance, if an endpoint is halted then there's no reason for the controller to shift any USB tokens for it onto the data lines. Doing so would just be a waste of bandwidth, since the response is bound to be another STALL. And it doesn't matter that the peripheral has no means to STALL any follow-up tokens, since the host controller already knows the endpoint is halted. Of course, control endpoints behave differently from other ones. But that's not what you're talking about here, since test 13 is for bulk or interrupt endpoints. The comment in the patch talks about moving the dequeue pointer past the STALLed TD and then clearing the halt condition. Moving the dequeue pointer is fine -- there's no other way to take control of the TD back from the hardware -- but why would you want to clear the halt? The HCD isn't supposed to do that; the class driver is. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()
If class driver wants to SetFeature(ENDPOINT_HALT) and
later tries to talk to the stalled endpoint, xhci will
move endpoint to EP_STATE_STALL and subsequent usb_submit_urb()
will not cause a USB token to be shifted into the data lines.
Because of that, peripheral will never have any means of
STALLing a follow up token.
This is a known error at least with g_zero + testusb -t 13
and can be easily reproduced.
Cc: # v3.14+
Signed-off-by: Felipe Balbi
---
this is a second version of patch sent originally in January this
year [1].
I suppose this is still not the best fix (as noted on code comment below),
but it'll serve to restart the thread on what's the best way to fix this.
I thought about clearing endpoint halt if !control from prepare_ring(),
but I'm not entirely of the implications there either.
I'm adding stable to Cc for v3.14+ (which is the earlier kernel I could easily
run to reproduce this) but it's likely a much older bug. Still, if nobody other
than me complained until now, perhaps we don't care as much ?
[1] http://marc.info/?l=linux-usb&m=139025060301432&w=2
drivers/usb/host/xhci-ring.c | 40
1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index bc6fcbc..4e8c3cf 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1826,13 +1826,45 @@ static int finish_td(struct xhci_hcd *xhci, struct
xhci_td *td,
if (trb_comp_code == COMP_STALL) {
/* The transfer is completed from the driver's
* perspective, but we need to issue a set dequeue
-* command for this stalled endpoint to move the dequeue
-* pointer past the TD. We can't do that here because
-* the halt condition must be cleared first. Let the
-* USB class driver clear the stall later.
+* command for this stalled endpoint to move the
+* dequeue pointer past the TD.
+*
+* Before we can move the dequeue pointer past the TD,
+* we must first clear the halt condition, however, we
+* can't clear such condition for control endpoints
+* since that can only be cleared through a USB Reset.
+*
+* Note that this isn't 100% safe because as soon as a
+* stalled TD is completed, we will clear the halt
+* condition on the endpoint from the host side, which
+* might not always be what we want.
+*
+* Perhaps a better approach would be to differentiate
+* SetFeature(ENDPOINT_HALT) from a STALL due to error
+* or Babble and only clear halt in that case.
+*
+* Another difficulty with this case is that if class
+* driver wants to talk to a halted endpoint to verify
+* SetFeature(ENDPOINT_HALT) was implemented correctly
+* by firmware (one such case is g_zero + testusb -t
+* 13), without clearing HALT on the host side, EP
+* doorbell will be rung but endpoint will not exit
+* EP_STATE_HALTED, because we *must* first issue a
+* Reset Endpoint command to move the EP to the
+* EP_STATE_STOPPED before EP doorbell works again.
+*
+* So, we will issue 'Reset Endpoint' here to make sure
a
+* subsequent usb_submit_urb() will actually cause EP
+* doorbell to ring so a USB token is shifted into the
+* wire and peripheral has a chance of replying with
+* STALL again.
*/
ep->stopped_td = td;
ep->stopped_stream = ep_ring->stream_id;
+ if (!usb_endpoint_xfer_control(&td->urb->ep->desc))
+ xhci_cleanup_halted_endpoint(xhci,
+ slot_id, ep_index,
ep_ring->stream_id,
+ td, event_trb);
} else if (xhci_requires_manual_halt_cleanup(xhci,
ep_ctx, trb_comp_code)) {
/* Other types of errors halt the endpoint, but the
--
2.1.0.GIT
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
