linux-kernel - Re: [PATCH 3/3] media: uvcvideo: Remove stream->is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <adb22c62-94c8-4ab5-8aea-cc204affba3c@xs4all.nl>
Date: Mon, 2 Jun 2025 09:57:44 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Ricardo Ribalda <ribalda@...omium.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Hans de Goede <hdegoede@...hat.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] media: uvcvideo: Remove stream->is_streaming field

On 22/05/2025 19:58, Ricardo Ribalda wrote:
> The is_streaming field is used by modular PM to know if the device is
> currently streaming or not.
> 
> With the transition to vb2 and fop helpers, we can use vb2 functions for
> the same functionality.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 12 +++++-------
>  drivers/media/usb/uvc/uvcvideo.h |  1 -
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 7a5ecbefa32c0a6b74c85d7f77a25b433598471e..51419f443f2c43dfd17a9782352bd2cde1094732 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -617,7 +617,8 @@ static int uvc_v4l2_release(struct file *file)
>  
>  	uvc_ctrl_cleanup_fh(handle);
>  
> -	if (handle->is_streaming)
> +	if (stream->queue.queue.owner == file->private_data &&

Use vb2_queue_is_busy(&stream->queue) instead of directly accessing the owner field.
But see below, since this can be dropped altogether.

> +	    uvc_queue_streaming(&stream->queue))
>  		uvc_pm_put(stream->dev);

I think patch 1/3 can be improved, which likely makes this patch obsolete.

The uvc_pm_get/put should be placed in the start/stop_streaming callbacks. That's
where you need them, and it avoids all these is_streaming tests. And it allows you to
use the vb2_ioctl_streamon/off helpers in patch 2, since the streamon/off functions
no longer mess with the uvc_pm_get/put functions.

Regards,

	Hans

>  
>  	/* Release the file handle. */
> @@ -684,7 +685,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>  	struct uvc_streaming *stream = handle->stream;
>  	int ret;
>  
> -	if (handle->is_streaming)
> +	if (uvc_queue_streaming(&stream->queue))
>  		return 0;
>  
>  	ret = uvc_pm_get(stream->dev);
> @@ -697,8 +698,6 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>  		return ret;
>  	}
>  
> -	handle->is_streaming = true;
> -
>  	return 0;
>  }
>  
> @@ -707,16 +706,15 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
>  {
>  	struct uvc_fh *handle = fh;
>  	struct uvc_streaming *stream = handle->stream;
> +	bool was_streaming = uvc_queue_streaming(&stream->queue);
>  	int ret;
>  
>  	ret = vb2_ioctl_streamoff(file, fh, type);
>  	if (ret)
>  		return ret;
>  
> -	if (handle->is_streaming) {
> -		handle->is_streaming = false;
> +	if (was_streaming)
>  		uvc_pm_put(stream->dev);
> -	}
>  
>  	return 0;
>  }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 3ddbf065a2cbae40ee48cb06f84ca8f0052990c4..f895f690f7cdc1af942d5f3a5f10e9dd1c956a35 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -626,7 +626,6 @@ struct uvc_fh {
>  	struct uvc_video_chain *chain;
>  	struct uvc_streaming *stream;
>  	unsigned int pending_async_ctrls;
> -	bool is_streaming;
>  };
>  
>  /* ------------------------------------------------------------------------
>