[v2,2/3] usb: gadget: uvc: remove delay usb status phase

[v2,2/3] usb: gadget: uvc: remove delay usb status phase

[Paul Elder]

The completion of the usb status phase doesn't need to be delayed
from uvc_function_set_alt to uvc_v4l2_streamon/off.
Remove USB_GADGET_DELAYED_STATUS and usb_composite_setup_delay from
these two, respectively.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
Changes in v2:
	1. Remove delay usb status phase

 drivers/usb/gadget/function/f_uvc.c    | 3 ++-
 drivers/usb/gadget/function/uvc_v4l2.c | 6 ------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 9b63b28a1ee3..fa34dcbe1197 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -361,7 +361,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
 		memset(&v4l2_event, 0, sizeof(v4l2_event));
 		v4l2_event.type = UVC_EVENT_STREAMON;
 		v4l2_event_queue(&uvc->vdev, &v4l2_event);
-		return USB_GADGET_DELAYED_STATUS;
+
+		return 0;
 
 	default:
 		return -EINVAL;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index fdf02b6987c0..138d95b3b8d1 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -206,12 +206,6 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 
 	uvc->state = UVC_STATE_STREAMING;
 
-	/*
-	 * Complete the alternate setting selection setup phase now that
-	 * userspace is ready to provide video frames.
-	 */
-	uvc_function_setup_continue(uvc);
-
 	return 0;
 }
 

[v2,2/3] usb: gadget: uvc: remove delay usb status phase

[Roger Quadros]

Hi Paul,

On 24/04/18 23:59, Paul Elder wrote:
> The completion of the usb status phase doesn't need to be delayed
> from uvc_function_set_alt to uvc_v4l2_streamon/off.
> Remove USB_GADGET_DELAYED_STATUS and usb_composite_setup_delay from
> these two, respectively.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> Changes in v2:
> 	1. Remove delay usb status phase
> 
>  drivers/usb/gadget/function/f_uvc.c    | 3 ++-
>  drivers/usb/gadget/function/uvc_v4l2.c | 6 ------
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 9b63b28a1ee3..fa34dcbe1197 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -361,7 +361,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
>  		memset(&v4l2_event, 0, sizeof(v4l2_event));
>  		v4l2_event.type = UVC_EVENT_STREAMON;
>  		v4l2_event_queue(&uvc->vdev, &v4l2_event);
> -		return USB_GADGET_DELAYED_STATUS;
> +
> +		return 0;
>  
>  	default:
>  		return -EINVAL;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index fdf02b6987c0..138d95b3b8d1 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -206,12 +206,6 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  
>  	uvc->state = UVC_STATE_STREAMING;
>  
> -	/*
> -	 * Complete the alternate setting selection setup phase now that
> -	 * userspace is ready to provide video frames.
> -	 */
> -	uvc_function_setup_continue(uvc);
> -

We should also get rid of definition of uvc_function_setup_continue() as nobody else uses it.

>  	return 0;
>  }
>  
>

[v2,2/3] usb: gadget: uvc: remove delay usb status phase

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tuesday, 24 April 2018 23:59:35 EEST Paul Elder wrote:
> The completion of the usb status phase doesn't need to be delayed
> from uvc_function_set_alt to uvc_v4l2_streamon/off.
> Remove USB_GADGET_DELAYED_STATUS and usb_composite_setup_delay from
> these two, respectively.

Did you mean uvc_function_setup_continue() ?

In addition to Roger's comment regarding the uvc_function_setup_continue() 
function that can now be removed (don't forget the header files), I think the 
commit message would benefit from more details. How about the following ?

    usb: gadget: uvc: Don't delay the status phase of SET_INTERFACE requests
    
    Reception of a SET_INTERFACE request with a non-zero alternate setting
    signals the start of the video stream. The gadget has to enable the
    video streaming endpoint and to signal stream start to userspace, in
    order to start receiving video frames to transmit over USB. As userspace
    can be slow to react, the UVC function driver delays the status phase of
    the SET_INTERFACE control transfer until userspace is ready.
    
    The status phase is delayed by returning USB_GADGET_DELAYED_STATUS from
    the function's .set_alt() handler. This creates a race condition as the
    userspace application could process the stream start event before the
    composite layer processes the USB_GADGET_DELAYED_STATUS return value.
    The race has been observed in practice, and can't be solved without a
    change to the USB_GADGET_DELAYED_STATUS API.
    
    Fortunately the UVC function driver doesn't strictly require delaying
    the status phase, as the only requirement from a USB point of view is
    that the streaming endpoint must be enabled before the status phase
    completes, and that is already guaranteed by the current code. We can
    thus complete the status phase synchronously, removing the race
    condition at the same time.
    
    Without delaying the status phase the host will likely start issuing
    isochronous transfers before we queue the first USB requests. The UDC
    will reply with NAKs which should be handled properly by the host. If
    this ends up causing issues another option will be to modify the status
    phase delay API to fix the race condition.

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> Changes in v2:
> 	1. Remove delay usb status phase
> 
>  drivers/usb/gadget/function/f_uvc.c    | 3 ++-
>  drivers/usb/gadget/function/uvc_v4l2.c | 6 ------
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c index 9b63b28a1ee3..fa34dcbe1197
> 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -361,7 +361,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned
> interface, unsigned alt) memset(&v4l2_event, 0, sizeof(v4l2_event));
>  		v4l2_event.type = UVC_EVENT_STREAMON;
>  		v4l2_event_queue(&uvc->vdev, &v4l2_event);
> -		return USB_GADGET_DELAYED_STATUS;
> +
> +		return 0;
> 
>  	default:
>  		return -EINVAL;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
> b/drivers/usb/gadget/function/uvc_v4l2.c index fdf02b6987c0..138d95b3b8d1
> 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -206,12 +206,6 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type)
> 
>  	uvc->state = UVC_STATE_STREAMING;
> 
> -	/*
> -	 * Complete the alternate setting selection setup phase now that
> -	 * userspace is ready to provide video frames.
> -	 */
> -	uvc_function_setup_continue(uvc);
> -
>  	return 0;
>  }