On Tue, 8 Jan 2019, Paul Elder wrote: > On Sun, Jan 06, 2019 at 03:03:09PM -0500, Alan Stern wrote: > > On Sun, 6 Jan 2019, Paul Elder wrote: > > > > > Implement the mechanism for optional explicit status stage for the MUSB > > > driver. This allows a function driver to specify what to reply for the > > > status stage. The functionality for an implicit status stage is > > > retained. > > > > > > Signed-off-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx> > > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > > v1 Acked-by: Bin Liu <b-liu@xxxxxx> > > > --- > > > No change from v3 > > > > > > Changes from v2: > > > - update call to usb_gadget_control_complete to include status > > > - since sending STALL from the function driver is now done with > > > usb_ep_set_halt, there is no need for the internal ep0_send_response to > > > take a stall/ack parameter; remove the parameter and make the function > > > only send ack, and remove checking for the status reply in the > > > usb_request for the status stage > > > > > > Changes from v1: > > > - obvious change to implement v2 mechanism laid out by 4/6 of this > > > series (send_response, and musb_g_ep0_send_response function has > > > been removed, call to usb_gadget_control_complete has been added) > > > - ep0_send_response's ack argument has been changed from stall > > > - last_packet flag in ep0_rxstate has been removed, since it is equal to > > > req != NULL > > > > > > drivers/usb/musb/musb_gadget.c | 2 ++ > > > drivers/usb/musb/musb_gadget_ep0.c | 23 +++++++++++++++++++++++ > > > 2 files changed, 25 insertions(+) > > > > > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c > > > index d3f33f449445..a7a992ab0c9d 100644 > > > --- a/drivers/usb/musb/musb_gadget.c > > > +++ b/drivers/usb/musb/musb_gadget.c > > > @@ -145,6 +145,8 @@ __acquires(ep->musb->lock) > > > > > > trace_musb_req_gb(req); > > > usb_gadget_giveback_request(&req->ep->end_point, &req->request); > > > + usb_gadget_control_complete(&musb->g, request->explicit_status, > > > + request->status); > > > > I haven't paid much attention to this part of the patch series, not > > knowing much about musb. Still, it's clear that > > usb_gadget_control_complete should be called only for transfers on > > ep0. You need to test the endpoint value. > > Oh oops, yeah, you're right. > > > Another problem: the completion handler may deallocate the request. > > Dereferencing request->expicit_status and request->status would then > > cause errors. Would it be preferable to call > > usb_gadget_control_complete before usb_gadget_giveback_request? If > > it gets done that way then the arguments could be simplified: we could > > pass a pointer to the request instead of the separate explicit_status > > and status values. > > I thought that usb_gadget_control_complete needs to check the status of > the request that was given back. Doesn't that mean that > usb_gadget_giveback_request must be called first? You misunderstand. req->status has already been determined by the time usb_gadget_giveback_request is called. It does not get set by that call. > I was thinking that maybe we could save explicit_status before calling > usb_gadget_giveback_request, and if request is still valid then we can > pull status from it otherwise use 0, but then would that be considered > adding complexity to UDCs that want to implement optional status stage > delay? Or add a wrapper function? > > On the other hand, if we do put usb_gadget_control_complete before > usb_gadget_giveback_request, then the control transfer would complete > before the function driver has a chance to complete, but if the function > driver wanted to intervene/determine the status stage then it would have > gone through the new mechanism that we're making here. So it could be > fine to switch the order. My tests for it work too, so I guess we'll go > with usb_gadget_control_complete before usb_gadget_giveback_request > then. In that case usb_gadget_control_complete doesn't need to check the > status of the request, since there isn't any, right? Wrong -- the status has already been set by that time. Alan Stern
