diff mbox series

[v7,2/8] media: uvcvideo: Add support for per-device control mapping overrides

Message ID 20220617103645.71560-3-ribalda@chromium.org
State New
Headers show
Series uvcvideo: Fix handling of power_line_frequency | expand

Commit Message

Ricardo Ribalda June 17, 2022, 10:36 a.m. UTC 
Some devices do not implement all their controls in a way that complies
with the UVC specification. This is for instance the case for several
devices that do not support the disabled mode for the power line
frequency control. Add a mechanism to allow per-device control mapping
overrides to avoid errors when accessing non-compliant controls.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 35 ++++++++++++++++++++++++++------
 drivers/media/usb/uvc/uvcvideo.h |  1 +
 2 files changed, 30 insertions(+), 6 deletions(-)

Comments

Ricardo Ribalda June 30, 2022, 11:16 p.m. UTC | #1 
Hi Dan

Thanks for your mail

On Thu, 30 Jun 2022 at 13:08, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hi Ricardo,
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Ricardo-Ribalda/uvcvideo-Fix-handling-of-power_line_frequency/20220617-185644
> base:   git://linuxtv.org/media_tree.git master
> config: microblaze-randconfig-m031-20220629 (https://download.01.org/0day-ci/archive/20220630/202206301144.r7yv0yk2-lkp@intel.com/config)
> compiler: microblaze-linux-gcc (GCC) 11.3.0
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> drivers/media/usb/uvc/uvc_ctrl.c:2459 uvc_ctrl_init_ctrl() error: uninitialized symbol 'mapping'.
>
> vim +/mapping +2459 drivers/media/usb/uvc/uvc_ctrl.c
>
> 866c6bdd5663d4 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2021-06-18  2401  static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
> 866c6bdd5663d4 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2021-06-18  2402                            struct uvc_control *ctrl)
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2403  {
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2404     const struct uvc_control_info *info = uvc_ctrls;
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2405     const struct uvc_control_info *iend = info + ARRAY_SIZE(uvc_ctrls);
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2406     const struct uvc_control_mapping *mapping;
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2407     const struct uvc_control_mapping *mend;
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2408
> 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2409     /* XU controls initialization requires querying the device for control
> 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2410      * information. As some buggy UVC devices will crash when queried
> 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2411      * repeatedly in a tight loop, delay XU controls initialization until
> 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2412      * first use.
> 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2413      */
> 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2414     if (UVC_ENTITY_TYPE(ctrl->entity) == UVC_VC_EXTENSION_UNIT)
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2415             return;
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2416
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2417     for (; info < iend; ++info) {
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2418             if (uvc_entity_match_guid(ctrl->entity, info->entity) &&
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2419                 ctrl->index == info->index) {
> 866c6bdd5663d4 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2021-06-18  2420                     uvc_ctrl_add_info(chain->dev, ctrl, info);
> 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2421                     /*
> 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2422                      * Retrieve control flags from the device. Ignore errors
> 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2423                      * and work with default flag values from the uvc_ctrl
> 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2424                      * array when the device doesn't properly implement
> 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2425                      * GET_INFO on standard controls.
> 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2426                      */
> 866c6bdd5663d4 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2021-06-18  2427                     uvc_ctrl_get_flags(chain->dev, ctrl, &ctrl->info);
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2428                     break;
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2429              }
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2430     }
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2431
> 071c8bb827c80a drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2432     if (!ctrl->initialized)
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2433             return;
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2434
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2435     /*
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2436      * First check if the device provides a custom mapping for this control,
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2437      * used to override standard mappings for non-conformant devices. Don't
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2438      * process standard mappings if a custom mapping is found. This
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2439      * mechanism doesn't support combining standard and custom mappings for
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2440      * a single control.
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2441      */
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2442     if (chain->dev->info->mappings) {
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2443             bool custom = false;
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2444             unsigned int i;
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2445
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2446             for (i = 0; (mapping = chain->dev->info->mappings[i]); ++i) {
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2447                     if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2448                         ctrl->info.selector == mapping->selector) {
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2449                             __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2450                             custom = true;
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2451                     }
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2452             }
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2453
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2454             if (custom)
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2455                     return;
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2456     }
>
> "mapping" uninitialized if chain->dev->info->mappings is NULL.
>
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2457
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2458     /* Process common mappings next. */
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17 @2459     mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings);
>                                                                                                ^^^^^^^
>
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2460     for (mapping = uvc_ctrl_mappings; mapping < mend; ++mapping) {
>
> I'm surprised this made it through testing...  Anyway, the easier way to
> to iterate through an array of structs is just say:

There is already a new version under review that fixes this:

https://patchwork.linuxtv.org/project/linux-media/patch/20220617235610.321917-3-ribalda@chromium.org/


>
>         for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); i++) {
>                 mapping = &uvc_ctrl_mappings[i];
>

I agree, and that is what I normally used, but that driver iterate this way...

Thanks again!

PS: Any idea why the test was triggered 14+ days after the patch was sent?

> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2461             if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2462                 ctrl->info.selector == mapping->selector)
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2463                     __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2464     }
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2465
> 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2466     /* Finally process version-specific mappings. */
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2467     if (chain->dev->uvc_version < 0x0150) {
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2468             mapping = uvc_ctrl_mappings_uvc11;
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2469             mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings_uvc11);
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2470     } else {
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2471             mapping = uvc_ctrl_mappings_uvc15;
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2472             mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings_uvc15);
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2473     }
> 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2474
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2475     for (; mapping < mend; ++mapping) {
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2476             if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
> 071c8bb827c80a drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2477                 ctrl->info.selector == mapping->selector)
> 866c6bdd5663d4 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2021-06-18  2478                     __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2479     }
> ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2480  }
>
> --
> 0-DAY CI Kernel Test Service
> https://01.org/lkp
>
Philip Li July 1, 2022, 2:31 a.m. UTC | #2 
On Fri, Jul 01, 2022 at 01:16:23AM +0200, Ricardo Ribalda wrote:
> Hi Dan
> 
> Thanks for your mail
> 
> On Thu, 30 Jun 2022 at 13:08, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Hi Ricardo,
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Ricardo-Ribalda/uvcvideo-Fix-handling-of-power_line_frequency/20220617-185644
> > base:   git://linuxtv.org/media_tree.git master
> > config: microblaze-randconfig-m031-20220629 (https://download.01.org/0day-ci/archive/20220630/202206301144.r7yv0yk2-lkp@intel.com/config)
> > compiler: microblaze-linux-gcc (GCC) 11.3.0
> >
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > smatch warnings:
> > drivers/media/usb/uvc/uvc_ctrl.c:2459 uvc_ctrl_init_ctrl() error: uninitialized symbol 'mapping'.
> >
> > vim +/mapping +2459 drivers/media/usb/uvc/uvc_ctrl.c
> >
> > 866c6bdd5663d4 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2021-06-18  2401  static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
> > 866c6bdd5663d4 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2021-06-18  2402                            struct uvc_control *ctrl)
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2403  {
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2404     const struct uvc_control_info *info = uvc_ctrls;
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2405     const struct uvc_control_info *iend = info + ARRAY_SIZE(uvc_ctrls);
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2406     const struct uvc_control_mapping *mapping;
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2407     const struct uvc_control_mapping *mend;
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2408
> > 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2409     /* XU controls initialization requires querying the device for control
> > 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2410      * information. As some buggy UVC devices will crash when queried
> > 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2411      * repeatedly in a tight loop, delay XU controls initialization until
> > 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2412      * first use.
> > 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2413      */
> > 52c58ad6f95ff6 drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2414     if (UVC_ENTITY_TYPE(ctrl->entity) == UVC_VC_EXTENSION_UNIT)
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2415             return;
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2416
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2417     for (; info < iend; ++info) {
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2418             if (uvc_entity_match_guid(ctrl->entity, info->entity) &&
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2419                 ctrl->index == info->index) {
> > 866c6bdd5663d4 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2021-06-18  2420                     uvc_ctrl_add_info(chain->dev, ctrl, info);
> > 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2421                     /*
> > 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2422                      * Retrieve control flags from the device. Ignore errors
> > 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2423                      * and work with default flag values from the uvc_ctrl
> > 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2424                      * array when the device doesn't properly implement
> > 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2425                      * GET_INFO on standard controls.
> > 93df48d37c3f03 drivers/media/usb/uvc/uvc_ctrl.c   Hans de Goede    2020-07-28  2426                      */
> > 866c6bdd5663d4 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2021-06-18  2427                     uvc_ctrl_get_flags(chain->dev, ctrl, &ctrl->info);
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2428                     break;
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2429              }
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2430     }
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2431
> > 071c8bb827c80a drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2432     if (!ctrl->initialized)
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2433             return;
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2434
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2435     /*
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2436      * First check if the device provides a custom mapping for this control,
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2437      * used to override standard mappings for non-conformant devices. Don't
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2438      * process standard mappings if a custom mapping is found. This
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2439      * mechanism doesn't support combining standard and custom mappings for
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2440      * a single control.
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2441      */
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2442     if (chain->dev->info->mappings) {
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2443             bool custom = false;
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2444             unsigned int i;
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2445
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2446             for (i = 0; (mapping = chain->dev->info->mappings[i]); ++i) {
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2447                     if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2448                         ctrl->info.selector == mapping->selector) {
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2449                             __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2450                             custom = true;
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2451                     }
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2452             }
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2453
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2454             if (custom)
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2455                     return;
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2456     }
> >
> > "mapping" uninitialized if chain->dev->info->mappings is NULL.
> >
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2457
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2458     /* Process common mappings next. */
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17 @2459     mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings);
> >                                                                                                ^^^^^^^
> >
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2460     for (mapping = uvc_ctrl_mappings; mapping < mend; ++mapping) {
> >
> > I'm surprised this made it through testing...  Anyway, the easier way to
> > to iterate through an array of structs is just say:
> 
> There is already a new version under review that fixes this:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/20220617235610.321917-3-ribalda@chromium.org/
> 
> 
> >
> >         for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); i++) {
> >                 mapping = &uvc_ctrl_mappings[i];
> >
> 
> I agree, and that is what I normally used, but that driver iterate this way...
> 
> Thanks again!
> 
> PS: Any idea why the test was triggered 14+ days after the patch was sent?

Thanks, the config is a random one, which is generated on 6/29 "microblaze-randconfig-m031-20220629",
which is probably the case that only this randconfig has chance to trigger the issue
and bisect successfully.

> 
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2461             if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2462                 ctrl->info.selector == mapping->selector)
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2463                     __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2464     }
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2465
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2466     /* Finally process version-specific mappings. */
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2467     if (chain->dev->uvc_version < 0x0150) {
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2468             mapping = uvc_ctrl_mappings_uvc11;
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2469             mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings_uvc11);
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2470     } else {
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2471             mapping = uvc_ctrl_mappings_uvc15;
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2472             mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings_uvc15);
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2473     }
> > 87c205087585a0 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2474
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2475     for (; mapping < mend; ++mapping) {
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2476             if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
> > 071c8bb827c80a drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-29  2477                 ctrl->info.selector == mapping->selector)
> > 866c6bdd5663d4 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2021-06-18  2478                     __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2479     }
> > ba2fa99668bb9b drivers/media/video/uvc/uvc_ctrl.c Laurent Pinchart 2010-09-20  2480  }
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://01.org/lkp
> >
> 
> 
> -- 
> Ricardo Ribalda
> _______________________________________________
> kbuild-all mailing list -- kbuild-all@lists.01.org
> To unsubscribe send an email to kbuild-all-leave@lists.01.org
Dan Carpenter July 1, 2022, 7:26 a.m. UTC | #3 
On Fri, Jul 01, 2022 at 01:16:23AM +0200, Ricardo Ribalda wrote:
> >
> > "mapping" uninitialized if chain->dev->info->mappings is NULL.
> >
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2457
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2458     /* Process common mappings next. */
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17 @2459     mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings);
> >                                                                                                ^^^^^^^
> >
> > 10bdca4191d7a8 drivers/media/usb/uvc/uvc_ctrl.c   Ricardo Ribalda  2022-06-17  2460     for (mapping = uvc_ctrl_mappings; mapping < mend; ++mapping) {
> >
> > I'm surprised this made it through testing...  Anyway, the easier way to
> > to iterate through an array of structs is just say:
> 
> There is already a new version under review that fixes this:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/20220617235610.321917-3-ribalda@chromium.org/ 
> 
> 
> >
> >         for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); i++) {
> >                 mapping = &uvc_ctrl_mappings[i];
> >
> 
> I agree, and that is what I normally used, but that driver iterate this way...

That's not really an excuse.  Imagine you were a lemming and Disney
employees were trying to chase you off a cliff!

> 
> Thanks again!
> 
> PS: Any idea why the test was triggered 14+ days after the patch was sent?

I don't know how kbuild works.  I run Smatch on linux-next so I would
have caught it eventually.  Plus the clang people would have caught it.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index a709ebbb4d69..092decfdaa62 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2403,9 +2403,8 @@  static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
 {
 	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 = uvc_ctrl_mappings;
-	const struct uvc_control_mapping *mend =
-		mapping + ARRAY_SIZE(uvc_ctrl_mappings);
+	const struct uvc_control_mapping *mapping;
+	const struct uvc_control_mapping *mend;
 
 	/* XU controls initialization requires querying the device for control
 	 * information. As some buggy UVC devices will crash when queried
@@ -2433,14 +2432,38 @@  static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
 	if (!ctrl->initialized)
 		return;
 
-	/* Process common mappings first. */
-	for (; mapping < mend; ++mapping) {
+	/*
+	 * First check if the device provides a custom mapping for this control,
+	 * used to override standard mappings for non-conformant devices. Don't
+	 * process standard mappings if a custom mapping is found. This
+	 * mechanism doesn't support combining standard and custom mappings for
+	 * a single control.
+	 */
+	if (chain->dev->info->mappings) {
+		bool custom = false;
+		unsigned int i;
+
+		for (i = 0; (mapping = chain->dev->info->mappings[i]); ++i) {
+			if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
+			    ctrl->info.selector == mapping->selector) {
+				__uvc_ctrl_add_mapping(chain, ctrl, mapping);
+				custom = true;
+			}
+		}
+
+		if (custom)
+			return;
+	}
+
+	/* Process common mappings next. */
+	mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings);
+	for (mapping = uvc_ctrl_mappings; mapping < mend; ++mapping) {
 		if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
 		    ctrl->info.selector == mapping->selector)
 			__uvc_ctrl_add_mapping(chain, ctrl, mapping);
 	}
 
-	/* And then version-specific mappings. */
+	/* Finally process version-specific mappings. */
 	if (chain->dev->uvc_version < 0x0150) {
 		mapping = uvc_ctrl_mappings_uvc11;
 		mend = mapping + ARRAY_SIZE(uvc_ctrl_mappings_uvc11);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index c5b4febd2d94..fff5c5c99a3d 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -667,6 +667,7 @@  struct uvc_device_info {
 	u32	quirks;
 	u32	meta_format;
 	u16	uvc_version;
+	const struct uvc_control_mapping **mappings;
 };
 
 struct uvc_device {