| Message ID | 8eb606d30e33ccee7256a329f9d4a31793864e29.1725285495.git.hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: vb2: prepare for vb2_ops_wait_prepare/finish removal | expand |
Hi Hans, Thank you for the patch. On Mon, Sep 02, 2024 at 04:04:54PM +0200, Hans Verkuil wrote: > Add two new checks: > > 1) wait_prepare and wait_finish callbacks are either both present or > both unset, you can't mix. > 2) if lock == NULL, then wait_prepare (and due to check 1 also > wait_finish) must be present. > > These checks should prevent the case where lock == NULL, but there > is no way to release/reacquire whatever lock is used when waiting > for a buffer to arrive in VIDIOC_DQBUF. Don't we use the video_device lock when the queue lock is NULL ? Shouldn't it be valid to not set wait_prepare and wait_finish in that case ? > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 29a8d876e6c2..6335ac7b771a 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -2644,6 +2644,14 @@ int vb2_core_queue_init(struct vb2_queue *q) > if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers)) > return -EINVAL; > > + /* Either both or none are set */ > + if (WARN_ON(!q->ops->wait_prepare ^ !q->ops->wait_finish)) > + return -EINVAL; > + > + /* Warn if q->lock is NULL and no custom wait_prepare is provided */ > + if (WARN_ON(!q->lock && !q->ops->wait_prepare)) > + return -EINVAL; > + > INIT_LIST_HEAD(&q->queued_list); > INIT_LIST_HEAD(&q->done_list); > spin_lock_init(&q->done_lock);
On 09/09/2024 16:59, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Mon, Sep 02, 2024 at 04:04:54PM +0200, Hans Verkuil wrote: >> Add two new checks: >> >> 1) wait_prepare and wait_finish callbacks are either both present or >> both unset, you can't mix. >> 2) if lock == NULL, then wait_prepare (and due to check 1 also >> wait_finish) must be present. >> >> These checks should prevent the case where lock == NULL, but there >> is no way to release/reacquire whatever lock is used when waiting >> for a buffer to arrive in VIDIOC_DQBUF. > > Don't we use the video_device lock when the queue lock is NULL ? > Shouldn't it be valid to not set wait_prepare and wait_finish in that > case ? If q->lock is NULL, then vb2 doesn't know which mutex needs to be unlocked while waiting for a buffer to arrive (and reacquired afterwards). So in that case you need to provide a function. Remember that vb2 doesn't know about the video_device lock. It is the driver that normally fills in q->lock, and often (or even always?) that is indeed the video_device lock. If q->lock is NULL, and these functions are not provided, then that is almost certainly a bug since there is almost certainly some serialization mutex. And if you don't unlock that while waiting, then you can't issue other ioctls. In the unlikely event that there really is no mutex involved, then you have to supply empty functions. There is no driver like that, though. Regards, Hans > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> drivers/media/common/videobuf2/videobuf2-core.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index 29a8d876e6c2..6335ac7b771a 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -2644,6 +2644,14 @@ int vb2_core_queue_init(struct vb2_queue *q) >> if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers)) >> return -EINVAL; >> >> + /* Either both or none are set */ >> + if (WARN_ON(!q->ops->wait_prepare ^ !q->ops->wait_finish)) >> + return -EINVAL; >> + >> + /* Warn if q->lock is NULL and no custom wait_prepare is provided */ >> + if (WARN_ON(!q->lock && !q->ops->wait_prepare)) >> + return -EINVAL; >> + >> INIT_LIST_HEAD(&q->queued_list); >> INIT_LIST_HEAD(&q->done_list); >> spin_lock_init(&q->done_lock); >
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 29a8d876e6c2..6335ac7b771a 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -2644,6 +2644,14 @@ int vb2_core_queue_init(struct vb2_queue *q) if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers)) return -EINVAL; + /* Either both or none are set */ + if (WARN_ON(!q->ops->wait_prepare ^ !q->ops->wait_finish)) + return -EINVAL; + + /* Warn if q->lock is NULL and no custom wait_prepare is provided */ + if (WARN_ON(!q->lock && !q->ops->wait_prepare)) + return -EINVAL; + INIT_LIST_HEAD(&q->queued_list); INIT_LIST_HEAD(&q->done_list); spin_lock_init(&q->done_lock);
Add two new checks: 1) wait_prepare and wait_finish callbacks are either both present or both unset, you can't mix. 2) if lock == NULL, then wait_prepare (and due to check 1 also wait_finish) must be present. These checks should prevent the case where lock == NULL, but there is no way to release/reacquire whatever lock is used when waiting for a buffer to arrive in VIDIOC_DQBUF. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/common/videobuf2/videobuf2-core.c | 8 ++++++++ 1 file changed, 8 insertions(+)