Re: [PATCH v4 5/6] usb: musb: gadget: implement optional explicit status stage

From: Alan Stern
Date: Tue Jan 08 2019 - 10:32:08 EST


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