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] [day] [month] [year] [list]

Message-ID: <CANiDSCubHc+WMgdQqyyqoDLixeN=ubGm4_ejeRj7M_vA=FA_DQ@mail.gmail.com>
Date: Mon, 2 Jun 2025 10:38:40 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans Verkuil <hverkuil@...all.nl>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, Hans de Goede <hdegoede@...hat.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] media: uvcvideo: Remove stream->is_streaming field

Hi Hans

On Mon, 2 Jun 2025 at 09:57, Hans Verkuil <hverkuil@...all.nl> wrote:
>
> 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.

If it is ok with you, I'd rather not touch 1/3. It has landed already
in the uvc tree.

I am reimplemeing 3/3 using  vb2's start_streaming stop_streaming.

BTW, I see that vb2 keeps track of the streaming state, so most of the
code is gone :)


>
> 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;
> >  };
> >
> >  /* ------------------------------------------------------------------------
> >
>


-- 
Ricardo Ribalda