| 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
| ||
|
Message-ID: <663123d4-9ac4-4c8d-bc88-d4e197786199@jjverkuil.nl>
Date: Mon, 2 Jun 2025 15:47:50 +0200
From: Hans Verkuil <hans@...erkuil.nl>
To: Ricardo Ribalda <ribalda@...omium.org>
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 v2 3/3] media: uvcvideo: Remove stream->is_streaming field
On 02/06/2025 15:33, Ricardo Ribalda wrote:
> Hi Hans
>
> On Mon, 2 Jun 2025 at 15:23, Hans Verkuil <hans@...erkuil.nl> wrote:
>>
>> On 02/06/2025 14:59, 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. The great benefit is that vb2 already takes
>>> track of the streaming state for us.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
>>> ---
>>> drivers/media/usb/uvc/uvc_queue.c | 11 ++++++++-
>>> drivers/media/usb/uvc/uvc_v4l2.c | 51 ++-------------------------------------
>>> drivers/media/usb/uvc/uvcvideo.h | 1 -
>>> 3 files changed, 12 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
>>> index 72c5494dee9f46ff61072e7d293bfaddda40e615..dff93bec204428b8aebc09332e0322fa68823fa4 100644
>>> --- a/drivers/media/usb/uvc/uvc_queue.c
>>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>>> @@ -165,12 +165,18 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>>
>>> lockdep_assert_irqs_enabled();
>>>
>>> + ret = uvc_pm_get(stream->dev);
>>> + if (ret)
>>> + return ret;
>>> +
>>> queue->buf_used = 0;
>>>
>>> ret = uvc_video_start_streaming(stream);
>>
>> I'm not sure this is correct. See comments below.
>>
>>> if (ret == 0)
>>> return 0;
>>>
>>> + uvc_pm_put(stream->dev);
>>> +
>>> spin_lock_irq(&queue->irqlock);
>>> uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
>>> spin_unlock_irq(&queue->irqlock);
>>> @@ -181,11 +187,14 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>> static void uvc_stop_streaming(struct vb2_queue *vq)
>>> {
>>> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>> + struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>>>
>>> lockdep_assert_irqs_enabled();
>>>
>>> - if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
>>> + if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) {
>>> + uvc_pm_put(stream->dev);
>>
>> This doesn't look right, for both video and metadata uvc_pm_get is called,
>> but only for video is put called.
>
> Please take a look at
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_queue.c#n195
>
> start_streaming is not called for metadata nodes, only for video nodes.
So when you start streaming metadata and no video is streaming, then nothing
happens. I noticed this before, in fact. Only after you also start to stream
video will the metadata start to arrive. And it stops again as soon as you
stop streaming video.
That's not really how it is supposed to work: whoever starts streaming first
is the one that calls uvc_video_start_streaming. And only when nobody is streaming
should uvc_video_stop_streaming be called. That's how it works in other drivers
(e.g. those that stream both video and vbi, or even more different types of data).
Fixing this would change the behavior of uvc, and I'm not sure if this is
something we want. I leave that to Laurent and Hans.
If this isn't fixed, then at least add a comment explaining why you test for
!= V4L2_BUF_TYPE_META_CAPTURE before calling uvc_pm_put. It's not obvious.
Regards,
Hans
>
>
>
>>
>>> uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>>
>> And this is odd too.
>>
>>> + }
>>
>> My assumption is that uvc_video_start_streaming and uvc_video_stop_streaming
>> are valid for both video and meta: i.e. the first time you start streaming
>> (either video or meta) you call uvc_video_start_streaming. If you were already
>> streaming for e.g. video, then start streaming metadata (or vice versa), then
>> you don't need to do anything in start_streaming.
>>
>> Same for stop_streaming: only if both video and metadata stopped streaming
>> is uvc_video_stop_streaming called.
>>
>> Please correct me if I am wrong.
>>
>> In any case, if I am right, then you have to rework this code accordingly.
>>
>> Regardless, you need to test various sequences of streaming video and metadata
>> in different orders and make sure this is handled correctly.
>
> I have tried streaming and getting frames. After some seconds the
> device turns off as expected.
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>> spin_lock_irq(&queue->irqlock);
>>> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>> index 7a5ecbefa32c0a6b74c85d7f77a25b433598471e..d4bee0d4334b764c0cf02363b573b55fb44eb228 100644
>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>> @@ -617,9 +617,6 @@ static int uvc_v4l2_release(struct file *file)
>>>
>>> uvc_ctrl_cleanup_fh(handle);
>>>
>>> - if (handle->is_streaming)
>>> - uvc_pm_put(stream->dev);
>>> -
>>> /* Release the file handle. */
>>> vb2_fop_release(file);
>>> file->private_data = NULL;
>>> @@ -677,50 +674,6 @@ static int uvc_ioctl_try_fmt(struct file *file, void *fh,
>>> return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
>>> }
>>>
>>> -static int uvc_ioctl_streamon(struct file *file, void *fh,
>>> - enum v4l2_buf_type type)
>>> -{
>>> - struct uvc_fh *handle = fh;
>>> - struct uvc_streaming *stream = handle->stream;
>>> - int ret;
>>> -
>>> - if (handle->is_streaming)
>>> - return 0;
>>> -
>>> - ret = uvc_pm_get(stream->dev);
>>> - if (ret)
>>> - return ret;
>>> -
>>> - ret = vb2_ioctl_streamon(file, fh, type);
>>> - if (ret) {
>>> - uvc_pm_put(stream->dev);
>>> - return ret;
>>> - }
>>> -
>>> - handle->is_streaming = true;
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static int uvc_ioctl_streamoff(struct file *file, void *fh,
>>> - enum v4l2_buf_type type)
>>> -{
>>> - struct uvc_fh *handle = fh;
>>> - struct uvc_streaming *stream = handle->stream;
>>> - int ret;
>>> -
>>> - ret = vb2_ioctl_streamoff(file, fh, type);
>>> - if (ret)
>>> - return ret;
>>> -
>>> - if (handle->is_streaming) {
>>> - handle->is_streaming = false;
>>> - uvc_pm_put(stream->dev);
>>> - }
>>> -
>>> - return 0;
>>> -}
>>> -
>>> static int uvc_ioctl_enum_input(struct file *file, void *fh,
>>> struct v4l2_input *input)
>>> {
>>> @@ -1323,8 +1276,8 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>>> .vidioc_expbuf = vb2_ioctl_expbuf,
>>> .vidioc_dqbuf = vb2_ioctl_dqbuf,
>>> .vidioc_create_bufs = vb2_ioctl_create_bufs,
>>> - .vidioc_streamon = uvc_ioctl_streamon,
>>> - .vidioc_streamoff = uvc_ioctl_streamoff,
>>> + .vidioc_streamon = vb2_ioctl_streamon,
>>> + .vidioc_streamoff = vb2_ioctl_streamoff,
>>> .vidioc_enum_input = uvc_ioctl_enum_input,
>>> .vidioc_g_input = uvc_ioctl_g_input,
>>> .vidioc_s_input = uvc_ioctl_s_input,
>>> 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;
>>> };
>>>
>>> /* ------------------------------------------------------------------------
>>>
>>
>
>
Powered by blists - more mailing lists