Re: [PATCH] usb: gadget: uvc: Move usb_ep_disable() to uvcg_video_enable()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dan,

Thank you for the patch.

On Thu, Jun 15, 2023 at 10:34:06AM +0100, Daniel Scally wrote:
> Currently the UVC Gadget's .set_alt() callback disables the USB
> endpoint, following which a V4L2 event is queued that closes
> down the workqueue. This ordering results in repeated calls to
> usb_ep_queue() from the workqueue whilst usb_ep_disable() is
> running - behaviour that the documentation for usb_ep_disable()
> specifically prohibits.
> 
> Move the call to usb_ep_disable() until after cancel_work_sync(),
> which will guarantee the endpoint is no longer in use when
> usb_ep_disable() is called.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  drivers/usb/gadget/function/f_uvc.c     | 3 ---
>  drivers/usb/gadget/function/uvc_video.c | 4 ++++
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 5e919fb65833..4b91bd572a83 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -337,9 +337,6 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
>  		if (uvc->state != UVC_STATE_STREAMING)
>  			return 0;
>  
> -		if (uvc->video.ep)
> -			usb_ep_disable(uvc->video.ep);
> -
>  		memset(&v4l2_event, 0, sizeof(v4l2_event));
>  		v4l2_event.type = UVC_EVENT_STREAMOFF;
>  		v4l2_event_queue(&uvc->vdev, &v4l2_event);

If we don't disable the endpoint here, should we return
USB_GADGET_DELAYED_STATUS here (and call uvc_function_setup_continue()
in uvc_v4l2_streamoff()) or is that not needed ? The uvc->state updated
should then possibly be moved to uvc_v4l2_streamoff() (checking if this
would cause any race condition would also be a nice bonus :-)).

> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 91af3b1ef0d4..c7e38fa26492 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -499,6 +499,10 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
>  
>  	if (!enable) {
>  		cancel_work_sync(&video->pump);
> +
> +		if (video->ep)
> +			usb_ep_disable(video->ep);
> +
>  		uvcg_queue_cancel(&video->queue, 0);
>  
>  		for (i = 0; i < video->uvc_num_requests; ++i)

-- 
Regards,

Laurent Pinchart