Re: [PATCH] media: uvcvideo: Use indexed loops in uvc_ctrl_init_ctrl()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent

it definitely looks nicer :)

Thanks!

On Tue, 19 Jul 2022 at 00:28, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> As shown by the bug introduced in commit 86f7ef773156 ("media: uvcvideo:
> Add support for per-device control mapping overrides"), the loop style
> used by uvc_ctrl_init_ctrl() is error-prone. Rewrite the loops to use
> indices instead.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>

> ---
> This patch depends on https://lore.kernel.org/linux-media/20220718121219.16079-1-laurent.pinchart@xxxxxxxxxxxxxxxx
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 34 ++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 8c208db9600b..5c33b0b7ef9a 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2411,10 +2411,9 @@ static void uvc_ctrl_prune_entity(struct uvc_device *dev,
>  static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>                                struct uvc_control *ctrl)
>  {
> -       const struct uvc_control_info *info = uvc_ctrls;
> -       const struct uvc_control_info *iend = info + ARRAY_SIZE(uvc_ctrls);
> -       const struct uvc_control_mapping *mapping;
> -       const struct uvc_control_mapping *mend;
> +       const struct uvc_control_mapping *mappings;
> +       unsigned int num_mappings;
> +       unsigned int i;
>
>         /*
>          * XU controls initialization requires querying the device for control
> @@ -2425,7 +2424,9 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>         if (UVC_ENTITY_TYPE(ctrl->entity) == UVC_VC_EXTENSION_UNIT)
>                 return;
>
> -       for (; info < iend; ++info) {
> +       for (i = 0; i < ARRAY_SIZE(uvc_ctrls); ++i) {
> +               const struct uvc_control_info *info = &uvc_ctrls[i];
> +
>                 if (uvc_entity_match_guid(ctrl->entity, info->entity) &&
>                     ctrl->index == info->index) {
>                         uvc_ctrl_add_info(chain->dev, ctrl, info);
> @@ -2452,9 +2453,11 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>          */
>         if (chain->dev->info->mappings) {
>                 bool custom = false;
> -               unsigned int i;
>
> -               for (i = 0; (mapping = chain->dev->info->mappings[i]); ++i) {
> +               for (i = 0; chain->dev->info->mappings[i]; ++i) {
> +                       const struct uvc_control_mapping *mapping =
> +                               chain->dev->info->mappings[i];
> +
>                         if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
>                             ctrl->info.selector == mapping->selector) {
>                                 __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> @@ -2467,10 +2470,9 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>         }
>
>         /* Process common mappings next. */
> -       mapping = uvc_ctrl_mappings;
> -       mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings);
> +       for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); ++i) {
> +               const struct uvc_control_mapping *mapping = &uvc_ctrl_mappings[i];
>
> -       for (; mapping < mend; ++mapping) {
>                 if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
>                     ctrl->info.selector == mapping->selector)
>                         __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> @@ -2478,14 +2480,16 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>
>         /* Finally process version-specific mappings. */
>         if (chain->dev->uvc_version < 0x0150) {
> -               mapping = uvc_ctrl_mappings_uvc11;
> -               mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings_uvc11);
> +               mappings = uvc_ctrl_mappings_uvc11;
> +               num_mappings = ARRAY_SIZE(uvc_ctrl_mappings_uvc11);
>         } else {
> -               mapping = uvc_ctrl_mappings_uvc15;
> -               mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings_uvc15);
> +               mappings = uvc_ctrl_mappings_uvc15;
> +               num_mappings = ARRAY_SIZE(uvc_ctrl_mappings_uvc15);
>         }
>
> -       for (; mapping < mend; ++mapping) {
> +       for (i = 0; i < num_mappings; ++i) {
> +               const struct uvc_control_mapping *mapping = &mappings[i];
> +
>                 if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
>                     ctrl->info.selector == mapping->selector)
>                         __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> --
> Regards,
>
> Laurent Pinchart
>


--
Ricardo Ribalda



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux