Hi,
On Thu, Aug 20, 2015 at 01:08:30PM -0400, Alan Stern wrote:
> 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.
oh right, IN endpoints are special in that sense :-)
But that's only true for some cases. When host side sends
SetFeature(HALT), that should always succeed, right ?
See commit 7a60855972f0d for example.
> > 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.
Doesn't Peter need to cope with differentiating protocol vs non-protocol
stalls ?
> > > > > 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.
right
> > 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.
see commit above.
> > 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.)
maxpacket size should be simple to fetch, however.
> > > 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.
true, it needs to be more complex than above.
--
balbi
Attachment:
signature.asc
Description: Digital signature
