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. 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. 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. 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
