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

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

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

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

On Sat, Sep 13, 2026 at 12:09:00PM +0000, Dan Carpenter wrote:
> 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.

Right, that's the exact interleaving the mutex prevents  with
read_lock held for the whole of comp_vdev_read(), a second thread
can never be inside get_top_mbo() while a first is inside
list_del(), since both calls happen inside the same
mutex-protected function body.
I'll write it up and send it as a patch.

regards,
Muhammad Israr

On Sat, Sep 13, 2026 at 01:09:00AM +0000, Dan Carpenter wrote:
> [previous]

One thing before i go further is that the changes i am proposing i
can't test it
on a real MOST hardware as i don't have it i can only do a compile
test  and i also did used LLM assistance in understanding
code especially where the race problem happens so according to greg
LLM policy i can't submit this is formal patch.

The possible according me will be use another read->lock:

static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
     size_t count, loff_t *pos)
{
...
mutex_lock(&mdev->read_lock);
...
while (count > 0) {
spin_lock_irq(&mdev->list_lock);
if (list_empty(&mdev->pending_mbos)) {
spin_unlock_irq(&mdev->list_lock);
break;
}
mbo = get_top_mbo(mdev);
list_del(&mbo->list);
spin_unlock_irq(&mdev->list_lock);
...
}
mutex_unlock(&mdev->read_lock);

}

If either of you want to take the read_lock approach
forward yourselves, you're welcome to otherwise I'll keep
working on staging drivers I can actually test on real hardware.

thanks

regards,
Muhammad Israr