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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 ?

-- 
balbi

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]