On Thu, 20 Aug 2015, Felipe Balbi wrote:
> > > > - Test 13 will fail due to there is pending IN request (f_sourcesink
> > > > will queue a request unconditionally at its completion), and udc driver
> > > > will run out error if that. udc driver must do that if it wants to
> > >
> > > wait, what ? test 13 works just fine here. (I'll try again in a few
> > > minutes just to make sure)
> >
> > It may depend on what UDC and what test device you use.
>
> Why ? Why would SET_FEATURE(HALT) fail ? I can only see it as a bug on
> the UDC driver. A bug which should be fixed.
Here's what the kerneldoc for usb_ep_set_halt() says:
* Returns zero, or a negative error code. On success, this call sets
* underlying hardware state that blocks data transfers.
* Attempts to halt IN endpoints will fail (returning -EAGAIN) if any
* transfer requests are still queued, or if the controller hardware
* (usually a FIFO) still holds bytes that the host hasn't collected.
That's talking about IN endpoints only, not OUT -- but Peter's original
email mentioned that Test 13 fails if there's a pending IN usb_request.
> usbtest's halt set/clear
> test is pretty simple:
>
> 1. move data on EP to verify it's not halted
> 2. set halt
> 3. move data on EP to verify STALL is returned
> 4. clear halt
> 5. move data on EP to verify it's not halted
>
> I did fix that months ago on DWC3 because there was a bug on DWC3. MUSB
> was fixed years back too for a similar bug.
According to the kerneldoc above, step 2 will fail if the gadget driver
automatically submits a new IN request after step 1 completes. Since
gadget zero (either loopback or source-sink mode) does resubmit IN
requests, you can understand Peter's problem.
> > > > pass USB CV2.0 MSC TEST. (othwerwise, "Command Set Test - Device Configured"
> > > > will fail)
> > >
> > > Why would a pending struct usb_request in your queue fail USB CV ?
> >
> > _Lack_ of a pending request can cause the USB CV to fail. In Peter's
> > example, if you're testing a Mass-Storage gadget, USB CV requires that
> > there be a pending Bulk-OUT request when the gadget is idle (so that
> > the next SCSI command can be sent out).
> >
> > But if there's a pending usb_request on a bulk-OUT endpoint, will a UDC
> > driver be able to carry out a Set-Halt-Feature request from the host?
> > The answer isn't clear. And it's even worse for bulk-IN.
>
> how can USB CV even test that there is a pending request on the UDC's
> side ? Short of actually moving data on that pipe, there's not way.
That's exactly what the USB CV does -- it tries to send data. If it
can't, the test fails. (At least, that's what I remember; it was a
while ago that I looked at this.)
However, in this respect Peter was a little inconsistent. The USB CV
MSC test requires a pending bulk-OUT request, but the Set-Halt problem
affects bulk-IN endpoints.
> Also, when the Halt request comes, UDC must take care of doing whatever
> necessary to make halt work. If that means cancelling transfers on a
> particular EP, so be it. Give them back with -ESHUTDOWN or -EPIPE or
> whatever. Just look at usb_ep_set_halt()'s documentation which states
> that "drivers may need to empty the endpoint's request queue first".
Exactly. As far as I know, UDCs _don't_ bother to cancel transfers on
an endpoint when the host sends Set-Halt. If you want to say that this
is a bug in the UDC drivers, I won't argue.
> On top of that, we have the stall=0 flag for cases where the UDC really
> can't handle that halt request correctly.
True. As far as I know, f_mass_storage is the only code which uses
that flag (because no protocols other than Mass Storage require stalls
on endpoints other than ep0).
> > > > - When using pattern = 1 as module parameters to compare the data, the
> > > > packet size must be same between host and device's.
> > >
> > > why ?
> >
> > The gadget stores the pattern data starting from 0 for each packet it
> > sends. But the host tests the pattern data starting from 0 only at the
> > beginning of the transfer; the host expects the pattern to continue
> > without resetting at packet boundaries (if the transfer length is
> > larger than the maxpacket size).
>
> then that's another bug which needs to be fixed :-)
That at least should be relatively simple. A few changes to
simple_fill_buf(), simple_check_buf(), and alloc_sglist() should do it.
(One extra complication is that these routines will need to know the
maxpacket size, but currently they don't.)
> > I suppose the right thing is for the UDC to temporarily disable the
> > endpoint, set the HALT feature, and then re-enable the endpoint (along
> > with the pending request). But changing a bunch of UDC drivers for
> > such a minor thing doesn't seem worthwhile.
>
> we can move that into udc-core.c:
>
> int usb_ep_set_halt(struct usb_ep *ep)
> {
> if (WARN_ON(!ep))
> return -EINVAL;
>
> if (!usb_ep_queue_is_empty(ep))
> usb_ep_empty_queue(ep, -EPIPE);
>
> return __usb_ep_set_halt(ep);
> }
>
> or something similar.
It's a good idea, but I'm not sure it will work as is. Somehow you
have to prevent the gadget driver from submitting any more requests to
the endpoint in between the usb_ep_empty_queue and __usb_ep_set_halt
calls.
Also, the Set-Halt request is generally handled in interrupt context
(the ep0 completion routine). But cancelling a pending request
requires that the caller be able to sleep.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
