| 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: <fe7bc889-9ba3-4621-8257-e81ba02db9d4@kernel.org>
Date: Mon, 8 Sep 2025 13:31:41 +0200
From: Hans de Goede <hansg@...nel.org>
To: Ricardo Ribalda <ribalda@...omium.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Hans Verkuil
<hverkuil@...all.nl>, Sakari Ailus <sakari.ailus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Linus Walleij
<linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>,
"Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-acpi@...r.kernel.org
Subject: Re: [PATCH v2 10/12] media: uvcvideo: Add get_* functions to
uvc_entity
Hi,
On 16-Jul-25 12:32, Ricardo Ribalda wrote:
> On Tue, 15 Jul 2025 at 21:35, Laurent Pinchart
> <laurent.pinchart@...asonboard.com> wrote:
...
>> As for the minimum and maximum, they are currently set to 0 if the
>> corresponding operations are not supported. I wonder if we should set
>> them to the current value instead for read-only controls (as in controls
>> whose flags report support for GET_CUR only)..
>
> I am not sure that I like that approach IMO the code looks worse...
> but if you prefer that, we can go that way
>
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index ec472e111248..47224437018b 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -35,6 +35,8 @@
> /* ------------------------------------------------------------------------
> * Controls
> */
> +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> + struct uvc_control *ctrl);
>
> static const struct uvc_control_info uvc_ctrls[] = {
> {
> @@ -1272,6 +1274,13 @@ static int uvc_ctrl_populate_cache(struct
> uvc_video_chain *chain,
> uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF));
> if (ret < 0)
> return ret;
> + } else if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) {
> + ret = __uvc_ctrl_load_cur(chain, ctrl);
> + if (!ret) {
> + memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> + ctrl->info.size);
> + }
> }
>
> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) {
Interesting change. Note you also need to check for
(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) being true,
__uvc_ctrl_load_cur() will return a 0 filled buffer
and success if that is not set.
I wonder why not do something like this instead though:
if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) &&
(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) &&
__uvc_ctrl_load_cur(chain, ctrl) == 0) {
/* Read-only control, set def / min / max to cur */
memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
ctrl->info.size);
memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN),
uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
ctrl->info.size);
memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX),
uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
ctrl->info.size);
}
IOW why bother to make the GET_DEF, etc. calls at all for a
read-only control (even if they are supported) ?
Generally speaking making less calls into the hw seems better?
Although maybe replace the:
if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) &&
(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) &&
part of the check with a flag in ctrl->info indicating to do
this and do this for specific controls like the new
rotation and orientation controls ?
...
> @@ -1541,11 +1573,8 @@ static int __uvc_queryctrl_boundaries(struct
> uvc_video_chain *chain,
> return ret;
> }
>
> - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF)
> v4l2_ctrl->default_value = uvc_mapping_get_s32(mapping,
> UVC_GET_DEF, uvc_ctrl_data(ctrl,
> UVC_CTRL_DATA_DEF));
> - else
> - v4l2_ctrl->default_value = 0;
>
> switch (mapping->v4l2_type) {
> case V4L2_CTRL_TYPE_MENU:
> @@ -1576,23 +1605,14 @@ static int __uvc_queryctrl_boundaries(struct
> uvc_video_chain *chain,
> break;
> }
>
> - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN)
> - v4l2_ctrl->minimum = uvc_mapping_get_s32(mapping, UVC_GET_MIN,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
> - else
> - v4l2_ctrl->minimum = 0;
> + v4l2_ctrl->minimum = uvc_mapping_get_s32(mapping, UVC_GET_MIN,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
>
> - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX)
> - v4l2_ctrl->maximum = uvc_mapping_get_s32(mapping, UVC_GET_MAX,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
> - else
> - v4l2_ctrl->maximum = 0;
> + v4l2_ctrl->maximum = uvc_mapping_get_s32(mapping, UVC_GET_MAX,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
>
> - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES)
> - v4l2_ctrl->step = uvc_mapping_get_s32(mapping, UVC_GET_RES,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
> - else
> - v4l2_ctrl->step = 0;
> + v4l2_ctrl->step = uvc_mapping_get_s32(mapping, UVC_GET_RES,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
>
> return 0;
> }
I agree with Laurent that thee changes are nice, but please split them into
a separate patch.
Regards,
Hans
Powered by blists - more mailing lists