linux-kernel - Re: [PATCH v3 1/1] media: uvcvideo: UVC minimum relative pan/tilt/zoom speed fix.

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiDSCs2MLqdNuZtcQEHhE6c8XXc=4AKNcuHNNgqJKnc9da3Hg@mail.gmail.com>
Date: Wed, 19 Jun 2024 08:15:26 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Gergo Koteles <soyer@....hu>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, John Bauer <johnebgood@...uritylive.com>, linh.tp.vu@...il.com, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/1] media: uvcvideo: UVC minimum relative
 pan/tilt/zoom speed fix.

Hi Gergo

Thanks for your your patch.

On Tue, 18 Jun 2024 at 18:33, Gergo Koteles <soyer@....hu> wrote:
>
> From: John Bauer <johnebgood@...uritylive.com>
>
> The minimum UVC control value for the relative pan/tilt/zoom speeds
> cannot be probed as the implementation condenses the pan and tilt
> direction and speed into two 16 bit values. The minimum cannot be
> set at probe time because it is probed first and the maximum is not
> yet known. With this fix if a relative speed control is queried
> or set the minimum is set and checked based on the additive inverse of
> the maximum at that time.
>
> Signed-off-by: John Bauer <johnebgood@...uritylive.com>
> Signed-off-by: Gergo Koteles <soyer@....hu>

Reviewed-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 38 +++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 4b685f883e4d..93ed2462e90b 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -441,7 +441,6 @@ static s32 uvc_ctrl_get_rel_speed(struct uvc_control_mapping *mapping,
>                 return (rel == 0) ? 0 : (rel > 0 ? data[first+1]
>                                                  : -data[first+1]);
>         case UVC_GET_MIN:
> -               return -data[first+1];
>         case UVC_GET_MAX:
>         case UVC_GET_RES:
>         case UVC_GET_DEF:
> @@ -1233,6 +1232,17 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
>         return ~0;
>  }
>
> +static bool is_relative_ptz_ctrl(__u32 ctrl_id)
> +{
> +       switch (ctrl_id) {
> +       case V4L2_CID_ZOOM_CONTINUOUS:
> +       case V4L2_CID_PAN_SPEED:
> +       case V4L2_CID_TILT_SPEED:
> +               return true;
> +       }
> +       return false;
> +}
> +
>  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>         struct uvc_control *ctrl,
>         struct uvc_control_mapping *mapping,
> @@ -1322,14 +1332,23 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>                 break;
>         }
>
> -       if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN)
> -               v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN,
> -                                    uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
> -
>         if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX)
>                 v4l2_ctrl->maximum = mapping->get(mapping, UVC_GET_MAX,
>                                      uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
>
> +       if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) {
> +               /*
> +                * For the relative speed implementation the minimum
> +                * value cannot be probed so it becomes the additive
> +                * inverse of maximum.
> +                */
> +               if (is_relative_ptz_ctrl(v4l2_ctrl->id))
> +                       v4l2_ctrl->minimum = -v4l2_ctrl->maximum;
> +               else
> +                       v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN,
> +                                               uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
> +       }
> +
>         if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES)
>                 v4l2_ctrl->step = mapping->get(mapping, UVC_GET_RES,
>                                   uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
> @@ -1916,6 +1935,15 @@ int uvc_ctrl_set(struct uvc_fh *handle,
>                                    uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
>                 max = mapping->get(mapping, UVC_GET_MAX,
>                                    uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
> +
> +               /*
> +                * For the relative speed implementation the minimum
> +                * value cannot be probed so it becomes the additive
> +                * inverse of maximum.
> +                */
> +               if (is_relative_ptz_ctrl(xctrl->id))
> +                       min = -max;
> +

nit: The following would probably be more correct but less clear:

if  (is_relative_ptz_ctrl(xctrl->id))
    min = -max;
else
    min = mapping->get(mapping, UVC_GET_MIN,...)

So up to you what do you/Laurent what is better ;)

>                 step = mapping->get(mapping, UVC_GET_RES,
>                                     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
>                 if (step == 0)
> --
> 2.45.2
>


-- 
Ricardo Ribalda