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"
> 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)?
> Signed-off-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> 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.
Also, would it look better if the type of explicit_status was bool
instead of unsigned int (both here and in the structure)?
Either way, once this change is made:
Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> + 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,
>
