Re: [PATCH v2 4/6] usb: gadget: add mechanism to specify an explicit status stage

[Alan Stern]

On Mon, 17 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
> 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 also with the explicit_status
> flag set, and with the zero flag set to 1 for ACK, or 0 for STALL.

This last part seems awkward.  Why not have the function driver call
usb_ep_set_halt() to indicate ep0 should send a STALL?  That's already 
part of the API; no need to add another way of doing the same thing.

> 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. To support the explicit status
> stage, it might then check what stage the usb_request was queued in, or
> the explicit_status flag, and the zero flag for what status to send.
> 
> Signed-off-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> 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 | 33 +++++++++++++++++++++++++++++++++
>  include/linux/usb/gadget.h    | 10 ++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index af88b48c1cea..e99481ef9147 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -894,6 +894,39 @@ 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
> + *
> + * 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)

This signature is incomplete.  You also need to know the status of the 
request that was just given back...

> +{
> +	struct usb_request *req;
> +
> +	if (explicit_status)
> +		return;
> +
> +	/* Send an implicit status-stage request for ep0 */
> +	req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);

... because none of this should happen if the previous request
completed unsuccessfully.

Alan Stern