* [PATCH] media: usbtv: Remove useless locks in usbtv_video_free()
@ 2024-02-20 8:06 Benjamin Gaignard
2024-02-20 10:42 ` Tomasz Figa
2024-02-28 8:45 ` Hans Verkuil
0 siblings, 2 replies; 4+ messages in thread
From: Benjamin Gaignard @ 2024-02-20 8:06 UTC (permalink / raw
To: mchehab, hverkuil-cisco, tfiga
Cc: linux-media, linux-kernel, kernel, Benjamin Gaignard
Remove locks calls in usbtv_video_free() because
are useless and may led to a deadlock as reported here:
https://syzkaller.appspot.com/x/bisect.txt?x=166dc872180000
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
---
drivers/media/usb/usbtv/usbtv-video.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 62a583040cd4..96276358d116 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -963,15 +963,9 @@ int usbtv_video_init(struct usbtv *usbtv)
void usbtv_video_free(struct usbtv *usbtv)
{
- mutex_lock(&usbtv->vb2q_lock);
- mutex_lock(&usbtv->v4l2_lock);
-
usbtv_stop(usbtv);
vb2_video_unregister_device(&usbtv->vdev);
v4l2_device_disconnect(&usbtv->v4l2_dev);
- mutex_unlock(&usbtv->v4l2_lock);
- mutex_unlock(&usbtv->vb2q_lock);
-
v4l2_device_put(&usbtv->v4l2_dev);
}
--
2.40.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] media: usbtv: Remove useless locks in usbtv_video_free()
2024-02-20 8:06 [PATCH] media: usbtv: Remove useless locks in usbtv_video_free() Benjamin Gaignard
@ 2024-02-20 10:42 ` Tomasz Figa
2024-02-28 8:45 ` Hans Verkuil
1 sibling, 0 replies; 4+ messages in thread
From: Tomasz Figa @ 2024-02-20 10:42 UTC (permalink / raw
To: Benjamin Gaignard
Cc: mchehab, hverkuil-cisco, linux-media, linux-kernel, kernel
On Tue, Feb 20, 2024 at 5:06 PM Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
>
> Remove locks calls in usbtv_video_free() because
> are useless and may led to a deadlock as reported here:
> https://syzkaller.appspot.com/x/bisect.txt?x=166dc872180000
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
> ---
> drivers/media/usb/usbtv/usbtv-video.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
> index 62a583040cd4..96276358d116 100644
> --- a/drivers/media/usb/usbtv/usbtv-video.c
> +++ b/drivers/media/usb/usbtv/usbtv-video.c
> @@ -963,15 +963,9 @@ int usbtv_video_init(struct usbtv *usbtv)
>
> void usbtv_video_free(struct usbtv *usbtv)
> {
> - mutex_lock(&usbtv->vb2q_lock);
> - mutex_lock(&usbtv->v4l2_lock);
> -
> usbtv_stop(usbtv);
> vb2_video_unregister_device(&usbtv->vdev);
> v4l2_device_disconnect(&usbtv->v4l2_dev);
>
> - mutex_unlock(&usbtv->v4l2_lock);
> - mutex_unlock(&usbtv->vb2q_lock);
> -
> v4l2_device_put(&usbtv->v4l2_dev);
> }
> --
> 2.40.1
>
From looking at the code in related vb2 functions and how the uvc
driver handles the cleanup, these locks shouldn't be necessary indeed,
but it would be good if someone could test this.
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: usbtv: Remove useless locks in usbtv_video_free()
2024-02-20 8:06 [PATCH] media: usbtv: Remove useless locks in usbtv_video_free() Benjamin Gaignard
2024-02-20 10:42 ` Tomasz Figa
@ 2024-02-28 8:45 ` Hans Verkuil
2024-02-28 11:33 ` Hans Verkuil
1 sibling, 1 reply; 4+ messages in thread
From: Hans Verkuil @ 2024-02-28 8:45 UTC (permalink / raw
To: Benjamin Gaignard, mchehab, tfiga; +Cc: linux-media, linux-kernel, kernel
On 20/02/2024 09:06, Benjamin Gaignard wrote:
> Remove locks calls in usbtv_video_free() because
> are useless and may led to a deadlock as reported here:
> https://syzkaller.appspot.com/x/bisect.txt?x=166dc872180000
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
Hmm, the code was really always wrong, it is just that before this commit
it would only fail if you did a disconnect while streaming, and now it also
fails if you disconnect when you are not streaming, so it is much more
noticeable now.
That should probably be explained better in the commit log. You could add a
second Fixes line:
Fixes: f3d27f34fdd7 ("[media] usbtv: Add driver for Fushicai USBTV007 video frame grabber")
but I am not sure how useful that is.
> ---
> drivers/media/usb/usbtv/usbtv-video.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
> index 62a583040cd4..96276358d116 100644
> --- a/drivers/media/usb/usbtv/usbtv-video.c
> +++ b/drivers/media/usb/usbtv/usbtv-video.c
> @@ -963,15 +963,9 @@ int usbtv_video_init(struct usbtv *usbtv)
>
> void usbtv_video_free(struct usbtv *usbtv)
> {
> - mutex_lock(&usbtv->vb2q_lock);
> - mutex_lock(&usbtv->v4l2_lock);
> -
> usbtv_stop(usbtv);
This call should also be dropped...
> vb2_video_unregister_device(&usbtv->vdev);
...since this function will call stop_streaming if streaming is in progress,
which will call usbtv_stop with all the correct locks held.
Regards,
Hans
> v4l2_device_disconnect(&usbtv->v4l2_dev);
>
> - mutex_unlock(&usbtv->v4l2_lock);
> - mutex_unlock(&usbtv->vb2q_lock);
> -
> v4l2_device_put(&usbtv->v4l2_dev);
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: usbtv: Remove useless locks in usbtv_video_free()
2024-02-28 8:45 ` Hans Verkuil
@ 2024-02-28 11:33 ` Hans Verkuil
0 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2024-02-28 11:33 UTC (permalink / raw
To: Benjamin Gaignard, mchehab, tfiga; +Cc: linux-media, linux-kernel, kernel
On 28/02/2024 09:45, Hans Verkuil wrote:
> On 20/02/2024 09:06, Benjamin Gaignard wrote:
>> Remove locks calls in usbtv_video_free() because
>> are useless and may led to a deadlock as reported here:
>> https://syzkaller.appspot.com/x/bisect.txt?x=166dc872180000
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")
>
> Hmm, the code was really always wrong, it is just that before this commit
> it would only fail if you did a disconnect while streaming, and now it also
> fails if you disconnect when you are not streaming, so it is much more
> noticeable now.
>
> That should probably be explained better in the commit log. You could add a
> second Fixes line:
>
> Fixes: f3d27f34fdd7 ("[media] usbtv: Add driver for Fushicai USBTV007 video frame grabber")
>
> but I am not sure how useful that is.
>
>> ---
>> drivers/media/usb/usbtv/usbtv-video.c | 6 ------
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
>> index 62a583040cd4..96276358d116 100644
>> --- a/drivers/media/usb/usbtv/usbtv-video.c
>> +++ b/drivers/media/usb/usbtv/usbtv-video.c
>> @@ -963,15 +963,9 @@ int usbtv_video_init(struct usbtv *usbtv)
>>
>> void usbtv_video_free(struct usbtv *usbtv)
>> {
>> - mutex_lock(&usbtv->vb2q_lock);
>> - mutex_lock(&usbtv->v4l2_lock);
>> -
>> usbtv_stop(usbtv);
>
> This call should also be dropped...
>
>> vb2_video_unregister_device(&usbtv->vdev);
>
> ...since this function will call stop_streaming if streaming is in progress,
> which will call usbtv_stop with all the correct locks held.
With that change you can add my:
Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Regards,
Hans
>
> Regards,
>
> Hans
>
>> v4l2_device_disconnect(&usbtv->v4l2_dev);
>>
>> - mutex_unlock(&usbtv->v4l2_lock);
>> - mutex_unlock(&usbtv->vb2q_lock);
>> -
>> v4l2_device_put(&usbtv->v4l2_dev);
>> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-02-28 11:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-20 8:06 [PATCH] media: usbtv: Remove useless locks in usbtv_video_free() Benjamin Gaignard
2024-02-20 10:42 ` Tomasz Figa
2024-02-28 8:45 ` Hans Verkuil
2024-02-28 11:33 ` Hans Verkuil
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.