Hi, On Thu, Sep 25, 2014 at 09:57:59PM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > > > On Thu, Sep 25, 2014 at 07:08:12PM +0000, Paul Zimmerman wrote: > > > > From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx] > > > > Sent: Thursday, September 25, 2014 11:08 AM > > > > > > > > On Thu, 25 Sep 2014, Paul Zimmerman wrote: > > > > > > > > > That's why I don't understand how this can happen for IN. AFAIK, a STALL > > > > > is only sent in response to something the host sent in the CBW. At that > > > > > point, there shouldn't be any IN transfers active. > > > > > > > > The gadget may send a partial response to the CBW. The end of the > > > > response is marked with a STALL. The mass-storage gadget driver > > > > submits the partial response and then calls usb_ep_set_halt() without > > > > waiting for the IN data to be delivered. It relies on the UDC driver > > > > returning -EAGAIN if any data is still pending. > > > > > > I guess you're referring to the code under the DATA_DIR_TO_HOST case > > > in finish_reply(). > > > > > > Felipe, in our vendor driver, the ep_set_halt() and ep_set_wedge() > > > functions check the request queue for the endpoint, and if it is not > > > empty, they return -EAGAIN. > > > > is that really enough ? The request is deleted (rather, moved to another > > list) as soon as StartTransfer is issued. I can certainly check both > > lists (and already do) but I fear that we might still race with the HW > > because there's no documentation to guarantee that I won't :-) > > > > > I see your patch for the dwc3 driver does add that, in addition to the > > > FIFO empty check. Does it still work OK if you remove the FIFO empty > > > check portion? > > > > It probably does, but I can't be sure there won't be a corner case where > > the list is empty (Xfercomplete was issued and request given back) but > > host hasn't moved all the data. Synopsys databook doesn't guarantee that > > will never be the case. > > > > Can you confirm, without a shadow of a doubt, that XferComplete will > > only be issued after host has moved every single bit of data ? If that > > can be updated to the databook, then sure, I can remove the FIFO space > > check; otherwise we might leave a corner case that will take us a few > > days to debug again because it will be very difficult to trigger :-) > > From the 2.70a databook, section 8.2.2: > > "While processing TRBs, two conditions may cause the core to write out an > event and raise an interrupt line: > - TRB Complete: > - For OUT endpoints, a packet is received which reduces the > remaining byte count in the TRB buffer to zero. > - For IN endpoints, an acknowledgement is received for a > transmitted packet which reduces the remaining byte count > in the TRB buffer to zero. > - Short Packet Received: > - For OUT endpoints only. While writing to a TRB buffer, > the endpoint receives a packet that is smaller than the > endpoint's MaxPacketSize. > " > > So for an IN, the completion event doesn't come until the TRB has been > ACKed, which means all the data has been sent. a very good point indeed. I completely missed that. Well, the patch can become a lot smaller, which makes my life easier when backporting :-) I'll test that out tomorrow, for sure :-) > But, I'm not saying you *have* to remove the FIFO space check. I think > we now know the problem was caused by the missing -EAGAIN. So having sure, that's certainly what was missing. > the FIFO check there shouldn't hurt anything, it's not masking some it's also completely useless. A few register accesses for no reason :-) > other problem. And you may need it in the future for one of those > other cases the databook talks about. I'll keep that in the back of my head. Perhaps I can repurpose these FIFO space accessors to implement usb_ep_fifo_status(). I'll consider that. cheers -- balbi
Attachment:
signature.asc
Description: Digital signature
