Re: [PATCH 5/7] usb: gadget: uvc: only pump video data if necessary

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

Hi Michael,

Thank you for the patch.

On Thu, Sep 30, 2021 at 12:27:15PM +0200, Michael Grzeschik wrote:
> If the streaming endpoint is not enabled, the worker has nothing to do.
> In the case it still got enqueued, this patch ensueres that it will bail

s/it still got enqueued/buffers are still queued/
s/ensueres/ensures/

> out without handling any data.

Does this happen when uvc_function_set_alt() calls usb_ep_disable() ?
The current implementation will cause usb_ep_queue() (called from
uvcg_video_ep_queue(), from uvcg_video_pump()) to return an error in
that case, which will result in uvcg_queue_cancel() being called in
uvcg_video_pump(). With this patch, I believe this will still work fine
as userspace is notified of the stream off event and calls
VIDIOC_STREAMOFF, which in turn calls uvcg_video_enable(0) from
uvc_v4l2_streamoff() (or uvcg_video_enable(0) gets called from
uvc_v4l2_release() in the worst case if the application closes the
device). Could you confirm that your understanding matches this analysis
? If so,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc_video.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index ccee35177411d..152647495fa61 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -335,12 +335,12 @@ static void uvcg_video_pump(struct work_struct *work)
>  {
>  	struct uvc_video *video = container_of(work, struct uvc_video, pump);
>  	struct uvc_video_queue *queue = &video->queue;
> -	struct usb_request *req;
> +	struct usb_request *req = NULL;
>  	struct uvc_buffer *buf;
>  	unsigned long flags;
>  	int ret;
>  
> -	while (1) {
> +	while (video->ep->enabled) {
>  		/* Retrieve the first available USB request, protected by the
>  		 * request lock.
>  		 */
> @@ -390,6 +390,9 @@ static void uvcg_video_pump(struct work_struct *work)
>  		video->req_int_count++;
>  	}
>  
> +	if (!req)
> +		return;
> +
>  	spin_lock_irqsave(&video->req_lock, flags);
>  	list_add_tail(&req->list, &video->req_free);
>  	spin_unlock_irqrestore(&video->req_lock, flags);

-- 
Regards,

Laurent Pinchart