[PATCH] staging: most: video: add comments to mutex and spinlock definitions

[PATCH] staging: most: video: add comments to mutex and spinlock definitions

[Muhammad Israr]

Add comments describing what the list_lock spinlock and lock mutex
in struct most_video_dev protect, per checkpatch.pl's
"definition without comment" check.

list_lock protects the pending_mbos list. The mutex is registered
as vdev->lock and is used by the V4L2 core to serialize
video_device ioctl calls; it is not locked directly in this file.

Signed-off-by: Muhammad Israr <7israr.work@gmail.com>
---
 drivers/staging/most/video/video.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index 3a0445ff62f4..41b617b553aa 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -33,6 +33,7 @@ struct most_video_dev {
 	bool mute;
 
 	struct list_head pending_mbos;
+	/* protects pending_mbos */
 	spinlock_t list_lock;
 
 	struct v4l2_device v4l2_dev;
@@ -40,6 +41,7 @@ struct most_video_dev {
 	struct video_device *vdev;
 	unsigned int ctrl_input;
 
+	/* registered as vdev->lock; serializes video_device ioctls */
 	struct mutex lock;
 
 	wait_queue_head_t wait_data;
-- 
2.55.0

Re: [PATCH] staging: most: video: add comments to mutex and spinlock definitions

[Dan Carpenter]

On Thu, Sep 10, 2026 at 05:44:03PM +0500, Muhammad Israr wrote:
> Add comments describing what the list_lock spinlock and lock mutex
> in struct most_video_dev protect, per checkpatch.pl's
> "definition without comment" check.
> 
> list_lock protects the pending_mbos list. The mutex is registered
> as vdev->lock and is used by the V4L2 core to serialize
> video_device ioctl calls; it is not locked directly in this file.
> 
> Signed-off-by: Muhammad Israr <7israr.work@gmail.com>
> ---
>  drivers/staging/most/video/video.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> index 3a0445ff62f4..41b617b553aa 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -33,6 +33,7 @@ struct most_video_dev {
>  	bool mute;
>  
>  	struct list_head pending_mbos;
> +	/* protects pending_mbos */
>  	spinlock_t list_lock;

It's supposed to but it is buggy...  What prevents multiple
threads from reading comp_vdev_read() at the same time?
I prefer to keep the warning around until someone fixes the
code.

I didn't read the other change.

regards,
dan carpenter

Re: [PATCH] staging: most: video: add comments to mutex and spinlock definitions

[Muhammad Israr]

On Thu, Sep 12, 2026 at 12:19:00AM +0000, Dan Carpenter wrote:
> It's supposed to but it is buggy...  What prevents multiple
> threads from reading comp_vdev_read() at the same time?
> I prefer to keep the warning around until someone fixes the
> code.

Thanks for pointing this out!
I traced through comp_vdev_read(): list_lock (the spinlock --
the mutex field in this struct is unrelated, it's only vdev->lock
used for V4L2 ioctl serialization) is only actually held around
the final list_del() in the read loop. data_ready() and
get_top_mbo(), both called earlier in the same function, read
pending_mbos with no lock held at all. comp_rx_data() (the
rx_completion producer) does take list_lock correctly around its
list_add_tail(), but that only protects against whatever happens
to be holding list_lock at that instant  which today is just
the list_del() call. So nothing stops two threads from both being
inside comp_vdev_read() concurrently and reading/deciding on the
same list state unprotected, which is what you were asking about.

The change I am proposing is to add a dedicated mutex to struct
most_video_dev, held across the whole read() call, so only one
thread is ever inside comp_vdev_read() at a time. list_lock still
wraps the actual list touches (checking for an empty list and
picking the head entry, and the existing list_del()), matching
what comp_rx_data() already does; copy_to_user() stays outside
any lock since it can fault. The mutex handles thread-vs-thread
serialization, list_lock keeps handling reader-vs-rx_completion
synchronization.

Does that match what you had in mind, or would you take a
different approach?

I will send the race fix on its own, and once the locking is
accurate I will send the follow-up comment patch.

regards,
Muhammad Israr

Re: [PATCH] staging: most: video: add comments to mutex and spinlock definitions

[Dan Carpenter]

On Sat, Sep 12, 2026 at 12:21:01AM +0500, Muhammad Israr wrote:
> On Thu, Sep 12, 2026 at 12:19:00AM +0000, Dan Carpenter wrote:
> > It's supposed to but it is buggy...  What prevents multiple
> > threads from reading comp_vdev_read() at the same time?
> > I prefer to keep the warning around until someone fixes the
> > code.
> 
> Thanks for pointing this out!
> I traced through comp_vdev_read(): list_lock (the spinlock --
> the mutex field in this struct is unrelated, it's only vdev->lock
> used for V4L2 ioctl serialization) is only actually held around
> the final list_del() in the read loop. data_ready() and
> get_top_mbo(), both called earlier in the same function, read
> pending_mbos with no lock held at all. comp_rx_data() (the
> rx_completion producer) does take list_lock correctly around its
> list_add_tail(), but that only protects against whatever happens
> to be holding list_lock at that instant  which today is just
> the list_del() call. So nothing stops two threads from both being
> inside comp_vdev_read() concurrently and reading/deciding on the
> same list state unprotected, which is what you were asking about.

Imagine one thread is calling get_top_mbo() which reads:

	list_first_entry(&mdev->pending_mbos, struct mbo, list);

but the other thread is calling:

	list_del(&mbo->list);

It's a race condition.  We can't delete two at the time, fine.
But we also should be trying to read from one while it's being
deleted.

regards,
dan carpenter