Hi Alan, On Mon, Jul 02, 2018 at 04:03:34PM -0400, Alan Stern wrote: > On Mon, 2 Jul 2018, Laurent Pinchart wrote: > > > Hi Alan, > > > > On Monday, 2 July 2018 18:13:27 EEST Alan Stern wrote: > > > On Mon, 2 Jul 2018, Paul Elder wrote: > > > > Hello Felipe, > > > > > > > > We have discovered an issue in MUSB gadget when receiving control OUT > > > > transfers. I have specifically observed it very frequently on the second > > > > SET_CUR UVC request when trying to stream video with yavta from a UVC > > > > gadget. > > > > > > > > What happens is that in the DATA phase, the controller copies the > > > > incoming data to FIFO which generates an interrupt (as per the docs), > > > > then the interrupt handler (actually ep0_rxstate) sends a stall because > > > > there is no usb_request to put the data into. > > > > > > > > Currently this usb_request is supposed to come from userspace by ioctl > > > > UVCIOC_SEND_RESPONSE, which means that (more often than not) if the > > > > interrupt corresponding to the DATA phase comes before userspace has a > > > > chance to call the ioctl (or before the ioctl handler has a chance to > > > > finish), then it will send a stall (usbmon ends with EPIPE). > > > > > > Shouldn't the UDC send a NAK install of a STALL under these > > > circumstances? In fact, shouldn't the controller not even bother to > > > copy the incoming data to its FIFO? > > > > > > I realize the MUSB hardware has a number of limitations, but this seems > > > a little peculiar. > > > > Those are questions I've asked myself when trying to help Paul with this > > issue. I realized that I couldn't tell whether MUSB was doing the right thing, > > as I didn't know what the UDC API required. > > > > When validating a control OUT request we need to validate both the data part > > of the SETUP phase (bRequest, bRequestType, wValue, wIndex) and the data part > > of the DATA phase. In the case of the UVC gadget, both validation occurs in > > userspace, so the SETUP validation can't be performed synchronously with the > > function's .setup() handler. > > > > In case of control writes, the STALL handshake can be returned in either the > > DATA phase or the STATUS phase (as far as I understand the USB specification > > doesn't indicate in which use cases it is appropriate to return a STALL > > handshake in the DATA or STATUS phase). As the data validation can obviously > > only be performed after the data is received, I assume that signaling a data > > error through a STALL handshake can only be performed in the STATUS stage. > > Signaling a setup error, on the other hand, could be done in either phase. > > That's right. However, see below. > > > The question is thus what options does the USB gadget API offer function > > drivers to STALL the control transfer in the DATA or STATUS phase. As far as I > > can tell, this can be done by returning an error from the .setup() handler (in > > which case I suppose the DATA stage will be STALLed), or calling > > usb_ep_set_halt() before the request completion handler for the DATA stage > > returns. Is that right ? > > Yes. > > > Are there other mechanisms ? If there are no other > > mechanism, I won't be able to validate the DATA stage in userspace as I can > > only send the data to userspace in the DATA stage request completion handler, > > and have to then return before receiving a response from userspace. > > Correct. Even if userspace wasn't involved, you still wouldn't be able > to check the data and halt the endpoint in time, because the UDC will > automatically queue a positive (i.e., zlp) response for the STATUS > stage when the DATA stage is complete. Laurent and I have theorized that in the case of MUSB it might be possible to do this by delaying the completion of the DATA phase (and by extension delay the host moving to the STATUS phase). I still need to test to see if this theory will work, plus we still need some way to allow userspace to tell the driver that it should reply with STALL or not :/ > This is a weakness in the gadget API, and there has been at least one > discussion about changing it. In particular, Felipe recommended that > STATUS stage packets should be queued explicitly by gadget drivers, not > sent automatically by UDCs. But as far as I know, this suggestion has > not been implemented. It would require changing pretty much every UDC > driver and every gadget driver. I'm wondering if gadget drivers explicitly queueing STATUS stage packets would help, because what I see in the MUSB spec is that the controller automatically ACKs (or STALLs if that bit in the register is set) the STATUS phase, so the driver isn't involved in the STATUS reply except for setting the STALL bit. (If it is indeed possible to delay the completion of the DATA phase and then have an ioctl actually complete the DATA phase and set the STALL bit if necessary then it might work...) Also, the interrupt signaling the STATUS phase doesn't always happen and sometimes gets coalesced with the next request: (in musb_g_ep0_irq, in drivers/usb/musb/musb_gadget_ep0.c) /* * This state is typically (but not always) indiscernible * from the status states since the corresponding interrupts * tend to happen within too little period of time (with only * a zero-length packet in between) and so get coalesced... */ I guess this isn't too much of an issue but just wanted to point out that we can't expect to do anything in response to a STATUS phase interrupt if its arrival isn't reliable. > The current situation is that currently the only way for a gadget > driver to send a STALL to the host in response to a control-OUT > transfer is to do so before queuing the DATA-stage request. In other > words, the decision has to be made before the data has been received. > > The situation for control-IN transfers is better. The gadget driver > has no reason to stall the STATUS stage once it has sent the data to > the host, so it doesn't matter that there is currently no mechanism for > doing so. > > > After clarifying the API (and possibly extending it...), let's move to the > > MUSB driver and see how to fix it. > > Well, that's a separate issue. If an endpoint is enabled but there are > no outstanding requests queued for it, then any packet received for > that endpoint should get a STALL response if and only if the endpoint > is halted, and a NAK response otherwise. I think this is a hardware issue. Nowhere in the MUSB spec (nor code) can I find anything about how to send NAK (or ACK). It seems that it's either STALL or auto-ACK. Plus, the controller has no reason to NAK, since its FIFO has space to accept the data; it neither knows nor cares that the software doesn't have its usb_request ready to accept the data. Paul -- 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
