[snip] > > >> > Another thing we should do is give function drivers a way to send a > > >> > STALL response for the status stage. Currently there's no way to do > > >> > it, if a data stage is present. > > >> > > >> Status stage can only be stalled if host tries to move data on the wrong > > >> direction. > > > > > > The USB-2 spec disagrees. See Table 8-7 in section 8.5.3.1 and the > > > following paragraphs. (Although, I can't see why a function would ever > > > fail to complete the command sequence for a control-IN transfer after > > > the data had already been sent.) > > > > I can't see how we could ever STALL after returning the data requested > > by the host. Seems like that wasn't well thought out. > > Yes, it doesn't make a lot of sense. However, STALLing the status > stage of a control-OUT transfer does make sense, so we should be able > to do it. The obvious approach is to call ep0's set_halt() method > instead of submitting an explicit status request. Exactly, that's what we want to be able to do. > > > Checking the length isn't enough. A data stage can have 0 length. > > > > apologies, I meant wLength, like so: > > > > len = le16_to_cpu(ctrl->wLength); > > if (!len) { > > dwc->three_stage_setup = false; > > dwc->ep0_expect_in = false; > > dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS; > > } else { > > dwc->three_stage_setup = true; > > dwc->ep0_expect_in = !!(ctrl->bRequestType & USB_DIR_IN); > > dwc->ep0_next_event = DWC3_EP0_NRDY_DATA; > > } > > Presumably you invert the value of ep0_expect_in and set ep0_next_event > to DWC3_EP0_NRDY_STATUS when the next (data-stage) request is submitted > for ep0. Okay. > > > special return values would be rendered uncessary if there's agreement > > that status stage is always explicit. Why would need > > USB_GADGET_DELAYED_STATUS if every case returns that? > > Agreed. > > > >> > There's also the fact that requests can specify a completion handler, but only > > >> > the data stage request would see its completion handler called (unless we > > >> > require UDCs to call completion requests at the completion of the status > > >> > stage, but I'm not sure that all UDCs can report the event to the driver, and > > >> > that would likely be useless as nobody needs that feature). > > >> > > >> you still wanna know if the host actually processed your status > > >> stage. udc-core can (and should) provide a generic status stage > > >> completion function which, at a minimum, aids with some tracepoints. > > > > > > Helping with tracepoints is fine. However, I don't think function > > > drivers really need to know whether the status stage was processed by > > > the host. Can you point out any examples where such information would > > > be useful? > > > > If you know your STATUS stage completed, you have a guarantee that your > > previous control transfer is complete. It's a very clear signal that you > > should prepare for more control transfers. > > That doesn't seem to make sense, because in reality you don't have > this guarantee. Consider the following scenario: The host starts a > control-IN transfer. You send the data-stage request succesfully and > then submit the status-stage request. That request will complete > before the host receives the ACK for its 0-length status OUT > transaction. In fact, the host may never receive that ACK and so the > transfer may never complete. > > Besides, you don't need a signal (clear or otherwise) to prepare for > more control transfers. You should start preparing as soon as the > status-stage request has been submitted. At that point, what else is > there for you to do? > > For that matter, you should be prepared to receive a new setup callback > at any time. The host doesn't have to wait for an old control transfer > to complete before starting a new one. > > I just don't see any value in knowing the completion code of a > status-stage request. I agree. > > >> > To simplify function drivers, do you think the above proposal of adding a flag > > >> > to the (data stage) request to request an automatic transition to the status > > >> > stage is a good idea ? We could even possibly invert the logic and transition > > >> > > >> no, I don't think so. Making the status phase always explicit is far > > >> better. UDCs won't have to check flags, or act on magic return > > >> values. It just won't do anything until a request is queued. > > > > > > I don't agree. This would be a simple test in a localized area (the > > > completion callback for control requests). It could even be > > > implemented by a library routine; the UDC driver would simply have to > > > call this routine immediately after invoking the callback. > > > > I don't follow what you mean here. > > Suppose we have a core library routine like this: > > void usb_gadget_control_complete(struct usb_gadget *gadget, > unsigned int no_implicit_status, int status) > { > struct usb_request *req; > > if (no_implicit_status || status != 0) > return; > > /* Send an implicit status-stage request for ep0 */ > req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC); > if (req) { > req->length = 0; > req->no_implicit_status = 1; > req->complete = /* req's deallocation routine */ > usb_ep_queue(gadget->ep0, req, GFP_ATOMIC); > } > } > > Then all a UDC driver would need to do is call > usb_gadget_control_complete() after invoking a control request's > completion handler. The no_implicit_status and status arguments would > be taken from the request that was just completed. > > With this one call added to each UDC, all the existing function drivers > would work correctly. Even though they don't explicitly queue > status-stage requests, the new routine will do so for them, > transparently. Function drivers that want to handle their own > status-stage requests explicitly will merely have to set the > req->no_implicit_status bit. I think this is a good idea. We still get the benefits of explicit status stage without being overly intrusive in the conversion, and we maintain the queue-based API. Would it be fine for me to proceed in this direction for a v2? > (We might or might not need to watch out for 0-length control-OUT > transfers. Function drivers _do_ queue status-stage requests for > those.) Thanks, Paul Elder