Re: [PATCH v2 1/3] usb: gadget: uvc: stop pump thread on video disable

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

Hi Michael,

Thank you for the patch.

On Mon, Sep 11, 2023 at 04:05:28PM +0200, Michael Grzeschik wrote:
> Since the uvc-video gadget driver is using the v4l2 interface,
> the streamon and streamoff can be triggered at any times. To ensure
> that the pump worker will be closed as soon the userspace is
> calling streamoff we synchronize the state of the gadget ensuring
> the pump worker to bail out.

I'm sorry but I really dislike this. Not only does the patch fail to
ensure real synchronization, as the uvcg_video_pump() function still
runs asynchronously, it messes up the usage of the state field that now
tracks the state both from a host point of view (which it was doing so
far, updating the state based on callbacks from the UDC), and from a
gadget userspace point of view. This lacks clarity and is confusing.
Furthermore, the commit message doesn't even explain what issue is being
fixed here.

Greg, I think this series has been merged too soon :-(

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> v1 -> v2: Fixed the missing uvc variable in uvcg_video_enable
> 
>  drivers/usb/gadget/function/uvc_video.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 91af3b1ef0d412..4b68a3a9815d73 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -384,13 +384,14 @@ static void uvcg_video_pump(struct work_struct *work)
>  	struct uvc_video_queue *queue = &video->queue;
>  	/* video->max_payload_size is only set when using bulk transfer */
>  	bool is_bulk = video->max_payload_size;
> +	struct uvc_device *uvc = video->uvc;
>  	struct usb_request *req = NULL;
>  	struct uvc_buffer *buf;
>  	unsigned long flags;
>  	bool buf_done;
>  	int ret;
>  
> -	while (video->ep->enabled) {
> +	while (video->ep->enabled && uvc->state == UVC_STATE_STREAMING) {
>  		/*
>  		 * Retrieve the first available USB request, protected by the
>  		 * request lock.
> @@ -488,6 +489,7 @@ static void uvcg_video_pump(struct work_struct *work)
>   */
>  int uvcg_video_enable(struct uvc_video *video, int enable)
>  {
> +	struct uvc_device *uvc = video->uvc;
>  	unsigned int i;
>  	int ret;
>  
> @@ -498,6 +500,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
>  	}
>  
>  	if (!enable) {
> +		uvc->state = UVC_STATE_CONNECTED;
> +
>  		cancel_work_sync(&video->pump);
>  		uvcg_queue_cancel(&video->queue, 0);
>  
> @@ -523,6 +527,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
>  		video->encode = video->queue.use_sg ?
>  			uvc_video_encode_isoc_sg : uvc_video_encode_isoc;
>  
> +	uvc->state = UVC_STATE_STREAMING;
> +

You're now setting the state to UVC_STATE_STREAMING both here and in
uvc_v4l2_streamon(). That's confusing.

>  	video->req_int_count = 0;
>  
>  	queue_work(video->async_wq, &video->pump);

-- 
Regards,

Laurent Pinchart