| 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: <16f45860-923b-4b5d-995e-0729d0479752@kernel.org>
Date: Mon, 22 Sep 2025 09:58:26 +0200
From: Hans Verkuil <hverkuil+cisco@...nel.org>
To: Jai Luthra <jai.luthra@...asonboard.com>,
Hans Verkuil <hverkuil@...nel.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
Jacopo Mondi <jacopo.mondi@...asonboard.com>, linux-media@...r.kernel.org
Cc: Ricardo Ribalda <ribalda@...omium.org>,
Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
Ma Ke <make24@...as.ac.cn>,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 02/10] media: v4l2-dev: Add support for try state
On 22/09/2025 09:52, Hans Verkuil wrote:
> On 19/09/2025 11:55, Jai Luthra wrote:
>> Format negotiation performed via the TRY_FMT ioctl should only affect
>> the temporary context of a specific filehandle, not the active state
>> stored in the video device structure. To support this distinction, we
>> need separate storage for try and active states.
>>
>> Introduce an enum to distinguish between these two state types and store
>> the try state in struct v4l2_fh instead of the video device structure.
>> The try state is allocated during file handle initialization in
>> v4l2_fh_init() and released in v4l2_fh_exit().
>
> There is a major difference between TRY in video devices and TRY in subdev
> devices: TRY for video devices is not persistent, i.e. it doesn't need to
> be stored anywhere since nobody will be using that information.
>
> If the plan is to change that in later patch series, then that should be
> very clearly stated. And I think I would need to know the details of those
> future plans before I can OK this.
>
> So how is this try state intended to be used in the future?
Follow-up: if there are no plans to change/enhance the VIDIOC_TRY_FMT semantics,
then I don't really see the point of this since there is no need to store this
information.
Regards,
Hans
>
> Regards,
>
> Hans
>
>>
>> Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
>> --
>> Cc: Mauro Carvalho Chehab <mchehab@...nel.org>
>> Cc: Hans Verkuil <hverkuil@...nel.org>
>> Cc: Ricardo Ribalda <ribalda@...omium.org>
>> Cc: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
>> Cc: Jai Luthra <jai.luthra@...asonboard.com>
>> Cc: Ma Ke <make24@...as.ac.cn>
>> Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>> Cc: linux-media@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> ---
>> drivers/media/v4l2-core/v4l2-dev.c | 7 +++++--
>> drivers/media/v4l2-core/v4l2-fh.c | 6 ++++++
>> include/media/v4l2-dev.h | 17 ++++++++++++++++-
>> include/media/v4l2-fh.h | 2 ++
>> 4 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 997255709448510fcd17b6de798a3df99cd7ea09..26b6b2f37ca55ce981aa17a28a875dc3cf253d9b 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -164,7 +164,8 @@ void video_device_release_empty(struct video_device *vdev)
>> EXPORT_SYMBOL(video_device_release_empty);
>>
>> struct video_device_state *
>> -__video_device_state_alloc(struct video_device *vdev)
>> +__video_device_state_alloc(struct video_device *vdev,
>> + enum video_device_state_whence which)
>> {
>> struct video_device_state *state =
>> kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
>> @@ -172,6 +173,7 @@ __video_device_state_alloc(struct video_device *vdev)
>> if (!state)
>> return ERR_PTR(-ENOMEM);
>>
>> + state->which = which;
>> state->vdev = vdev;
>>
>> return state;
>> @@ -962,7 +964,8 @@ int __video_register_device(struct video_device *vdev,
>>
>> /* state support */
>> if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
>> - vdev->state = __video_device_state_alloc(vdev);
>> + vdev->state = __video_device_state_alloc(vdev,
>> + VIDEO_DEVICE_STATE_ACTIVE);
>>
>> /* Part 1: check device type */
>> switch (type) {
>> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
>> index df3ba9d4674bd25626cfcddc2d0cb28c233e3cc3..522acc0eb8401305c6893232d96d826669ab90d5 100644
>> --- a/drivers/media/v4l2-core/v4l2-fh.c
>> +++ b/drivers/media/v4l2-core/v4l2-fh.c
>> @@ -38,6 +38,10 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>> INIT_LIST_HEAD(&fh->subscribed);
>> fh->sequence = -1;
>> mutex_init(&fh->subscribe_lock);
>> + /* state support */
>> + if (test_bit(V4L2_FL_USES_STATE, &fh->vdev->flags))
>> + fh->state = __video_device_state_alloc(vdev,
>> + VIDEO_DEVICE_STATE_TRY);
>> }
>> EXPORT_SYMBOL_GPL(v4l2_fh_init);
>>
>> @@ -84,6 +88,8 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
>> {
>> if (fh->vdev == NULL)
>> return;
>> + if (test_bit(V4L2_FL_USES_STATE, &fh->vdev->flags))
>> + kfree(fh->state);
>> v4l_disable_media_source(fh->vdev);
>> v4l2_event_unsubscribe_all(fh);
>> mutex_destroy(&fh->subscribe_lock);
>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>> index 57e4691ef467aa2b0782dd4b8357bd0670643293..5ca04a1674e0bf7016537e6fb461d790fc0a958f 100644
>> --- a/include/media/v4l2-dev.h
>> +++ b/include/media/v4l2-dev.h
>> @@ -220,15 +220,28 @@ struct v4l2_file_operations {
>> int (*release) (struct file *);
>> };
>>
>> +/**
>> + * enum video_device_state_whence - Video device state type
>> + *
>> + * @VIDEO_DEVICE_STATE_TRY: from VIDIOC_TRY_xxx, for negotiation only
>> + * @VIDEO_DEVICE_STATE_ACTIVE: from VIDIOC_S_xxx, applied to the device
>> + */
>> +enum video_device_state_whence {
>> + VIDEO_DEVICE_STATE_TRY = 0,
>> + VIDEO_DEVICE_STATE_ACTIVE = 1,
>> +};
>> +
>> /**
>> * struct video_device_state - Used for storing video device state information.
>> *
>> * @fmt: Format of the capture stream
>> * @vdev: Pointer to video device
>> + * @which: State type (from enum video_device_state_whence)
>> */
>> struct video_device_state {
>> struct v4l2_format fmt;
>> struct video_device *vdev;
>> + enum video_device_state_whence which;
>> };
>>
>> /*
>> @@ -568,13 +581,15 @@ static inline int video_is_registered(struct video_device *vdev)
>> /** __video_device_state_alloc - allocate video device state structure
>> *
>> * @vdev: pointer to struct video_device
>> + * @which: type of video device state (from enum video_device_state_whence)
>> *
>> * .. note::
>> *
>> * This function is meant to be used only inside the V4L2 core.
>> */
>> struct video_device_state *
>> -__video_device_state_alloc(struct video_device *vdev);
>> +__video_device_state_alloc(struct video_device *vdev,
>> + enum video_device_state_whence which);
>>
>> /** __video_device_state_free - free video device state structure
>> *
>> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
>> index aad4b3689d7ea191978f24ce24d24cd2e73636b6..55455704a98d0785d0a3418b8a43d7363b7c8aa2 100644
>> --- a/include/media/v4l2-fh.h
>> +++ b/include/media/v4l2-fh.h
>> @@ -28,6 +28,7 @@ struct v4l2_ctrl_handler;
>> * @vdev: pointer to &struct video_device
>> * @ctrl_handler: pointer to &struct v4l2_ctrl_handler
>> * @prio: priority of the file handler, as defined by &enum v4l2_priority
>> + * @state: try state used for format negotiation on the video device
>> *
>> * @wait: event' s wait queue
>> * @subscribe_lock: serialise changes to the subscribed list; guarantee that
>> @@ -44,6 +45,7 @@ struct v4l2_fh {
>> struct video_device *vdev;
>> struct v4l2_ctrl_handler *ctrl_handler;
>> enum v4l2_priority prio;
>> + struct video_device_state *state;
>>
>> /* Events */
>> wait_queue_head_t wait;
>>
>
>
Powered by blists - more mailing lists