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 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.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html