diff mbox

[RFC/PATCH,v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()

Message ID 20141112222853.GO641@saruman
State New
Headers show

Commit Message

Felipe Balbi Nov. 12, 2014, 10:28 p.m. UTC 
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 codes that may require a manual halt cleanup */
1759         if (trb_comp_code == COMP_TX_ERR ||
1760                         trb_comp_code == COMP_BABBLE ||
1761                         trb_comp_code == COMP_SPLIT_ERR)
1762                 /* The 0.96 spec says a babbling control endpoint
1763                  * is not halted. The 0.96 spec says it is.  Some HW
1764                  * claims to be 0.95 compliant, but it halts the control
1765                  * endpoint anyway.  Check if a babble halted the
1766                  * endpoint.
1767                  */
1768                 if ((ep_ctx->ep_info & cpu_to_le32(EP_STATE_MASK)) ==
1769                     cpu_to_le32(EP_STATE_HALTED))
1770                         return 1;
1771 
1772         return 0;
1773 }

In fact, there is a typo on that comment. It needs this patch:



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 ?

What do you think ?

Comments

Mathias Nyman Nov. 13, 2014, 11:16 a.m. UTC | #1 
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_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 codes that may require a manual halt cleanup */
> 1759         if (trb_comp_code == COMP_TX_ERR ||
> 1760                         trb_comp_code == COMP_BABBLE ||
> 1761                         trb_comp_code == COMP_SPLIT_ERR)
> 1762                 /* The 0.96 spec says a babbling control endpoint
> 1763                  * is not halted. The 0.96 spec says it is.  Some HW
> 1764                  * claims to be 0.95 compliant, but it halts the control
> 1765                  * endpoint anyway.  Check if a babble halted the
> 1766                  * endpoint.
> 1767                  */
> 1768                 if ((ep_ctx->ep_info & cpu_to_le32(EP_STATE_MASK)) ==
> 1769                     cpu_to_le32(EP_STATE_HALTED))
> 1770                         return 1;
> 1771 
> 1772         return 0;
> 1773 }
> 
> In fact, there is a typo on that comment. It needs this patch:
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 4e8c3cf..a8bbacb 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1759,7 +1759,7 @@ static int xhci_requires_manual_halt_cleanup(struct xhci_hcd *xhci,
>  	if (trb_comp_code == COMP_TX_ERR ||
>  			trb_comp_code == COMP_BABBLE ||
>  			trb_comp_code == COMP_SPLIT_ERR)
> -		/* The 0.96 spec says a babbling control endpoint
> +		/* The 0.95 spec says a babbling control endpoint
>  		 * is not halted. The 0.96 spec says it is.  Some HW
>  		 * claims to be 0.95 compliant, but it halts the control
>  		 * endpoint anyway.  Check if a babble halted the
> 
>>>> 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.
> 
> alright.
> 
>>>> 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 into EP_STATE_HALT.
>>>
>>> The only thing we should be discussing here, is proper placement for
>>> xhci_cleanup_halted_endpoint().
>>
>> 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 ?
> 
> Something like below (it won't build, just trying to illustrate the
> situation):
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 4e8c3cf..a1dac2c 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2814,6 +2814,12 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
>  		return -EINVAL;
>  	case EP_STATE_HALTED:
>  		xhci_dbg(xhci, "WARN halted endpoint, queueing URB anyway.\n");
> +		if (!usb_endpoint_xfer_control(&td->urb->ep->desc)) {
> +			xhci_dbg(xhci, "Resetting endpoint.\n");
> +			xhci_cleanup_halted_endpoint(xhci,
> +					slot_id, ep_index, ep_ring->stream_id,
> +					td, event_trb);
> +		}
>  	case EP_STATE_STOPPED:
>  	case EP_STATE_RUNNING:
>  		break;
> 
> 
> 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 ?
> 
> What do you think ?
> 

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.

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:

https://bugzilla.kernel.org/show_bug.cgi?id=75521

-Mathias

(1) xhci 1.0 section 4.8.3:

Note:

A STALL detected on any stage (Setup, Data, or Status) of a Default Control Endpoint request
shall transition the Endpoint Context to the Halted state. A Default Control Endpoint STALL
condition is cleared by a Reset Endpoint Command which transitions the endpoint from the Halted
to the Stopped state. The Default Control Endpoint shall return to the Running state when the
Doorbell is rung for the next Setup Stage TD sent to the endpoint.
Section 8.5.3.4 of the USB2 spec and section 8.12.2.3 of the USB3 spec state of Control pipes,
“Unlike the case of a functional stall, protocol stall does not indicate an error with the device.” The
xHC treats a functional stall and protocol stall identically, by Halting the endpoint and requiring
software to clear the condition by issuing a Reset Endpoint Command







--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Nov. 13, 2014, 3:11 p.m. UTC | #2 
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.
> > 	"
> > 
> >> 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 codes that may require a manual halt cleanup */
> > 1759         if (trb_comp_code == COMP_TX_ERR ||
> > 1760                         trb_comp_code == COMP_BABBLE ||
> > 1761                         trb_comp_code == COMP_SPLIT_ERR)
> > 1762                 /* The 0.96 spec says a babbling control endpoint
> > 1763                  * is not halted. The 0.96 spec says it is.  Some HW
> > 1764                  * claims to be 0.95 compliant, but it halts the control
> > 1765                  * endpoint anyway.  Check if a babble halted the
> > 1766                  * endpoint.
> > 1767                  */
> > 1768                 if ((ep_ctx->ep_info & cpu_to_le32(EP_STATE_MASK)) ==
> > 1769                     cpu_to_le32(EP_STATE_HALTED))
> > 1770                         return 1;
> > 1771 
> > 1772         return 0;
> > 1773 }
> > 
> > In fact, there is a typo on that comment. It needs this patch:
> > 
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 4e8c3cf..a8bbacb 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -1759,7 +1759,7 @@ static int xhci_requires_manual_halt_cleanup(struct xhci_hcd *xhci,
> >  	if (trb_comp_code == COMP_TX_ERR ||
> >  			trb_comp_code == COMP_BABBLE ||
> >  			trb_comp_code == COMP_SPLIT_ERR)
> > -		/* The 0.96 spec says a babbling control endpoint
> > +		/* The 0.95 spec says a babbling control endpoint
> >  		 * is not halted. The 0.96 spec says it is.  Some HW
> >  		 * claims to be 0.95 compliant, but it halts the control
> >  		 * endpoint anyway.  Check if a babble halted the
> > 
> >>>> 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.
> > 
> > alright.
> > 
> >>>> 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 into EP_STATE_HALT.
> >>>
> >>> The only thing we should be discussing here, is proper placement for
> >>> xhci_cleanup_halted_endpoint().
> >>
> >> 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 ?
> > 
> > Something like below (it won't build, just trying to illustrate the
> > situation):
> > 
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 4e8c3cf..a1dac2c 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -2814,6 +2814,12 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
> >  		return -EINVAL;
> >  	case EP_STATE_HALTED:
> >  		xhci_dbg(xhci, "WARN halted endpoint, queueing URB anyway.\n");
> > +		if (!usb_endpoint_xfer_control(&td->urb->ep->desc)) {
> > +			xhci_dbg(xhci, "Resetting endpoint.\n");
> > +			xhci_cleanup_halted_endpoint(xhci,
> > +					slot_id, ep_index, ep_ring->stream_id,
> > +					td, event_trb);
> > +		}
> >  	case EP_STATE_STOPPED:
> >  	case EP_STATE_RUNNING:
> >  		break;
> > 
> > 
> > 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 ?
> > 
> > What do you think ?
> > 
> 
> 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

this works too. Can you just make sure to Cc: stable for v3.14+ or
older, if you know which is the earliest kernel that needs this fix.

You also want to update the comments on that function so they match
what's done now.

> 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.
> 
> 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:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=75521

alright. Thanks
Mathias Nyman Nov. 13, 2014, 3:30 p.m. UTC | #3 
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 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 codes that may require a manual halt cleanup */
>>> 1759         if (trb_comp_code == COMP_TX_ERR ||
>>> 1760                         trb_comp_code == COMP_BABBLE ||
>>> 1761                         trb_comp_code == COMP_SPLIT_ERR)
>>> 1762                 /* The 0.96 spec says a babbling control endpoint
>>> 1763                  * is not halted. The 0.96 spec says it is.  Some HW
>>> 1764                  * claims to be 0.95 compliant, but it halts the control
>>> 1765                  * endpoint anyway.  Check if a babble halted the
>>> 1766                  * endpoint.
>>> 1767                  */
>>> 1768                 if ((ep_ctx->ep_info & cpu_to_le32(EP_STATE_MASK)) ==
>>> 1769                     cpu_to_le32(EP_STATE_HALTED))
>>> 1770                         return 1;
>>> 1771 
>>> 1772         return 0;
>>> 1773 }
>>>
>>> In fact, there is a typo on that comment. It needs this patch:
>>>
>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>>> index 4e8c3cf..a8bbacb 100644
>>> --- a/drivers/usb/host/xhci-ring.c
>>> +++ b/drivers/usb/host/xhci-ring.c
>>> @@ -1759,7 +1759,7 @@ static int xhci_requires_manual_halt_cleanup(struct xhci_hcd *xhci,
>>>  	if (trb_comp_code == COMP_TX_ERR ||
>>>  			trb_comp_code == COMP_BABBLE ||
>>>  			trb_comp_code == COMP_SPLIT_ERR)
>>> -		/* The 0.96 spec says a babbling control endpoint
>>> +		/* The 0.95 spec says a babbling control endpoint
>>>  		 * is not halted. The 0.96 spec says it is.  Some HW
>>>  		 * claims to be 0.95 compliant, but it halts the control
>>>  		 * endpoint anyway.  Check if a babble halted the
>>>
>>>>>> 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.
>>>
>>> alright.
>>>
>>>>>> 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 into EP_STATE_HALT.
>>>>>
>>>>> The only thing we should be discussing here, is proper placement for
>>>>> xhci_cleanup_halted_endpoint().
>>>>
>>>> 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 ?
>>>
>>> Something like below (it won't build, just trying to illustrate the
>>> situation):
>>>
>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>>> index 4e8c3cf..a1dac2c 100644
>>> --- a/drivers/usb/host/xhci-ring.c
>>> +++ b/drivers/usb/host/xhci-ring.c
>>> @@ -2814,6 +2814,12 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
>>>  		return -EINVAL;
>>>  	case EP_STATE_HALTED:
>>>  		xhci_dbg(xhci, "WARN halted endpoint, queueing URB anyway.\n");
>>> +		if (!usb_endpoint_xfer_control(&td->urb->ep->desc)) {
>>> +			xhci_dbg(xhci, "Resetting endpoint.\n");
>>> +			xhci_cleanup_halted_endpoint(xhci,
>>> +					slot_id, ep_index, ep_ring->stream_id,
>>> +					td, event_trb);
>>> +		}
>>>  	case EP_STATE_STOPPED:
>>>  	case EP_STATE_RUNNING:
>>>  		break;
>>>
>>>
>>> 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 ?
>>>
>>> What do you think ?
>>>
>>
>> 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
> 
> this works too. Can you just make sure to Cc: stable for v3.14+ or
> older, if you know which is the earliest kernel that needs this fix.
> 
> You also want to update the comments on that function so they match
> what's done now.

Sure, will do. 
I'll remove the queueing of a reset endpoint command in the .endpoint_reset
callback as well.

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  

-Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern Nov. 13, 2014, 3:31 p.m. UTC | #4 
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 stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern Nov. 13, 2014, 3:45 p.m. UTC | #5 
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 stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Nov. 13, 2014, 3:47 p.m. UTC | #6 
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.
Alan Stern Nov. 13, 2014, 4:03 p.m. UTC | #7 
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 stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mathias Nyman Nov. 13, 2014, 5:58 p.m. UTC | #8 
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 stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mathias Nyman Nov. 13, 2014, 6:16 p.m. UTC | #9 
> 
> 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 stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Nov. 13, 2014, 6:31 p.m. UTC | #10 
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.
Felipe Balbi Nov. 13, 2014, 6:46 p.m. UTC | #11 
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 <balbi@ti.com>

But please also add proper fixes and Cc: stable, so older kernels can
use those.
Mathias Nyman Nov. 14, 2014, 8:39 a.m. UTC | #12 
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 <balbi@ti.com>
> 
> 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 stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Nov. 14, 2014, 3:04 p.m. UTC | #13 
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 <balbi@ti.com>
> > 
> > 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.
Felipe Balbi Nov. 14, 2014, 3:49 p.m. UTC | #14 
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 <balbi@ti.com>
> > > 
> > > 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: <stable@vger.kernel.org> # 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
Mathias Nyman Nov. 14, 2014, 4:37 p.m. UTC | #15 
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 <balbi@ti.com>
>>>>
>>>> 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: <stable@vger.kernel.org> # 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 stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Nov. 14, 2014, 5 p.m. UTC | #16 
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 <balbi@ti.com>
> >>>>
> >>>> 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: <stable@vger.kernel.org> # 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).
diff mbox

Patch

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 4e8c3cf..a8bbacb 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1759,7 +1759,7 @@  static int xhci_requires_manual_halt_cleanup(struct xhci_hcd *xhci,
 	if (trb_comp_code == COMP_TX_ERR ||
 			trb_comp_code == COMP_BABBLE ||
 			trb_comp_code == COMP_SPLIT_ERR)
-		/* The 0.96 spec says a babbling control endpoint
+		/* The 0.95 spec says a babbling control endpoint
 		 * is not halted. The 0.96 spec says it is.  Some HW
 		 * claims to be 0.95 compliant, but it halts the control
 		 * endpoint anyway.  Check if a babble halted the

> > > 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.

alright.

> > > 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 into EP_STATE_HALT.
> > 
> > The only thing we should be discussing here, is proper placement for
> > xhci_cleanup_halted_endpoint().
> 
> 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 ?

Something like below (it won't build, just trying to illustrate the
situation):

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 4e8c3cf..a1dac2c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2814,6 +2814,12 @@  static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
 		return -EINVAL;
 	case EP_STATE_HALTED:
 		xhci_dbg(xhci, "WARN halted endpoint, queueing URB anyway.\n");
+		if (!usb_endpoint_xfer_control(&td->urb->ep->desc)) {
+			xhci_dbg(xhci, "Resetting endpoint.\n");
+			xhci_cleanup_halted_endpoint(xhci,
+					slot_id, ep_index, ep_ring->stream_id,
+					td, event_trb);
+		}
 	case EP_STATE_STOPPED:
 	case EP_STATE_RUNNING:
 		break;