Message-ID: <20181221090438.GA2140@emerald.amanokami.net>
Date:   Fri, 21 Dec 2018 03:04:38 -0600
From:   Paul Elder <paul.elder@...asonboard.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     laurent.pinchart@...asonboard.com, kieran.bingham@...asonboard.com,
        b-liu@...com, rogerq@...com, balbi@...nel.org,
        gregkh@...uxfoundation.org, USB list <linux-usb@...r.kernel.org>,
        Kernel development list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/6] usb: gadget: add mechanism to specify an explicit
 status stage

On Wed, Dec 19, 2018 at 11:01:52AM -0500, Alan Stern wrote:
> On Wed, 19 Dec 2018, Paul Elder wrote:
> 
> > A usb gadget function driver may or may not want to delay the status
> > stage of a control OUT request. An instance it might want to is to
> ---------------------------------------------^
> Typo: missing "where"

ack

> 
> > asynchronously validate the data of a class-specific request.
> > 
> > A function driver that wants an explicit status stage should set the
> > newly added explicit_status flag of the usb_request corresponding to the
> > data stage. Later on the function driver can explicitly complete the
> > status stage by enqueueing a usb_request for ACK, or calling
> > usb_ep_set_halt() for STALL.
> > 
> > To support both explicit and implicit status stages, a UDC driver must
> > call the newly added usb_gadget_control_complete function right after
> > calling usb_gadget_giveback_request. The status of the request that was
> > just given back must be fed into usb_gadget_control_complete. To support
> > the explicit status stage, it might then check what stage the
> > usb_request was queued in, and send an ACK.
> 
> I don't really understand that last sentence.  Perhaps what you mean is
> that depending on the direction of the control transfer, the driver
> should either ACK the host's 0-length data packet (control-IN) or send
> a 0-length DATA1 packet (control-OUT)?

Yes, that is what I meant, though for MUSB I've only implemented and
tested the control OUT case.

> > Signed-off-by: Paul Elder <paul.elder@...asonboard.com>
> > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > ---
> > Changes from v2:
> > 
> > Add status parameter to usb_gadget_control_complete, so that a
> > usb_request is not queued if the status of the just given back request
> > is nonzero.
> > 
> > Changes from v1:
> > 
> > Complete change of API. Now we use a flag that should be set in the
> > usb_request that is queued for the data stage to signal to the UDC that
> > we want to delay the status stage (as opposed to setting a flag in the
> > UDC itself, that persists across all requests). We now also provide a
> > function for UDC drivers to very easily allow implicit status stages, to
> > mitigate the need to convert all function drivers to this new API at
> > once, and to make it easier for UDC drivers to convert.
> > 
> >  drivers/usb/gadget/udc/core.c | 34 ++++++++++++++++++++++++++++++++++
> >  include/linux/usb/gadget.h    | 10 ++++++++++
> >  2 files changed, 44 insertions(+)
> > 
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index af88b48c1cea..0a0ccd2b7639 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -894,6 +894,40 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
> >  
> >  /* ------------------------------------------------------------------------- */
> >  
> > +/**
> > + * usb_gadget_control_complete - complete the status stage of a control
> > + *	request, or delay it
> > + * Context: in_interrupt()
> > + *
> > + * @gadget: gadget whose control request's status stage should be completed
> > + * @explicit_status: true to delay status stage, false to complete here
> > + * @status: completion code of previously completed request
> > + *
> > + * This is called by device controller drivers after returning the completed
> > + * request back to the gadget layer, to either complete or delay the status
> > + * stage.
> > + */
> > +void usb_gadget_control_complete(struct usb_gadget *gadget,
> > +		unsigned int explicit_status, int status)
> > +{
> > +	struct usb_request *req;
> > +
> > +	if (explicit_status || status)
> > +		return;
> > +
> > +	/* Send an implicit status-stage request for ep0 */
> > +	req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
> > +	if (req) {
> > +		req->length = 0;
> > +		req->explicit_status = 0;
> 
> Oops!  I should have spotted this in the previous version, sorry. The
> implicit status-stage request should have its ->explicit_status set, so
> that we don't try to submit another status request when this one
> completes.

No, it's fine; the previous version had it set to 1 :)

I didn't think it mattered, but the way you describe it makes it seem
like it would matter depending on the UDC driver implementation, so I'll
put it back in.

> Also, would it look better if the type of explicit_status was bool 
> instead of unsigned int (both here and in the structure)?

Yes, it would; I'll make the change.

> Either way, once this change is made:
> 
> Acked-by: Alan Stern <stern@...land.harvard.edu>

Thank you :)

> > +		req->complete = usb_ep_free_request;
> > +		usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(usb_gadget_control_complete);
> > +
> > +/* ------------------------------------------------------------------------- */
> > +
> >  /**
> >   * gadget_find_ep_by_name - returns ep whose name is the same as sting passed
> >   *	in second parameter or NULL if searched endpoint not found
> > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> > index e5cd84a0f84a..1e3a23637468 100644
> > --- a/include/linux/usb/gadget.h
> > +++ b/include/linux/usb/gadget.h
> > @@ -73,6 +73,7 @@ struct usb_ep;
> >   *	Note that for writes (IN transfers) some data bytes may still
> >   *	reside in a device-side FIFO when the request is reported as
> >   *	complete.
> > + * @explicit_status: If true, delays the status stage
> >   *
> >   * These are allocated/freed through the endpoint they're used with.  The
> >   * hardware's driver can add extra per-request data to the memory it returns,
> > @@ -114,6 +115,8 @@ struct usb_request {
> >  
> >  	int			status;
> >  	unsigned		actual;
> > +
> > +	unsigned		explicit_status:1;
> >  };
> >  
> >  /*-------------------------------------------------------------------------*/
> > @@ -850,6 +853,13 @@ extern void usb_gadget_giveback_request(struct usb_ep *ep,
> >  
> >  /*-------------------------------------------------------------------------*/
> >  
> > +/* utility to complete or delay status stage */
> > +
> > +void usb_gadget_control_complete(struct usb_gadget *gadget,
> > +		unsigned int explicit_status, int status);
> > +
> > +/*-------------------------------------------------------------------------*/
> > +
> >  /* utility to find endpoint by name */
> >  
> >  extern struct usb_ep *gadget_find_ep_by_name(struct usb_gadget *g,
> > 
> 

Paul Elder

Powered by blists - more mailing lists