diff mbox series

[v6] media: uvcvideo: Set V4L2_CTRL_FLAG_DISABLED during queryctrl errors

Message ID 20250310-uvc-eaccess-v6-1-bf4562f7cabd@chromium.org
State New
Headers show
Series [v6] media: uvcvideo: Set V4L2_CTRL_FLAG_DISABLED during queryctrl errors | expand

Commit Message

Ricardo Ribalda March 10, 2025, 1:21 p.m. UTC 
To implement VIDIOC_QUERYCTRL, we need to know the minimum, maximum,
step and flags of the control. For some of the controls, this involves
querying the actual hardware.

Some non-compliant cameras produce errors when we query them. These
error can be triggered every time, sometimes, or when other controls do
not have the "right value". Right now, we populate that error to userspace.
When an error happens, the v4l2 framework does not copy the v4l2_queryctrl
struct to userspace. Also, userspace apps are not ready to handle any
other error than -EINVAL.

One of the main usecases of VIDIOC_QUERYCTRL is enumerating the controls
of a device. This is done using the V4L2_CTRL_FLAG_NEXT_CTRL flag. In
that usecase, a non-compliant control will make it almost impossible to
enumerate all controls of the device.

A control with an invalid max/min/step/flags is better than non being
able to enumerate the rest of the controls.

This patch:
- Retries for an extra attempt to read the control, to avoid spurious
  errors. More attempts do not seem to produce better results in the
  tested hardware.
- Makes VIDIOC_QUERYCTRL return 0 in all the error cases different than
  -EINVAL.
- Introduces a warning in dmesg so we can have a trace of what has happened
  and sets the V4L2_CTRL_FLAG_DISABLED.
- Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
  attempts to query that control (other operations have the same
  functionality as now).

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Hi 2*Hans and Laurent!

I came around a device that was listing just a couple of controls when
it should be listing much more.

Some debugging latter I found that the device was returning -EIO when
all the focal controls were read.

Lots of good arguments in favor/against this patch in the v1. Please
check!

Without this patch:
$ v4l2-ctl --list-ctrls
                  auto_exposure 0x009a0901 (menu)   : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
         exposure_time_absolute 0x009a0902 (int)    : min=50 max=10000 step=1 default=166 value=166 flags=inactive
     exposure_dynamic_framerate 0x009a0903 (bool)   : default=0 value=0
region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1

With this patch:
$ v4l2-ctl --list-ctrls
                  auto_exposure 0x009a0901 (menu)   : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
         exposure_time_absolute 0x009a0902 (int)    : min=50 max=10000 step=1 default=166 value=166 flags=inactive
     exposure_dynamic_framerate 0x009a0903 (bool)   : default=0 value=0
error 5 getting ext_ctrl Focus, Absolute
error 5 getting ext_ctrl Focus, Automatic Continuous
   region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
--
---
Changes in v6:
- Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
- Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org

Changes in v5:
- Explain the retry in the commit message (Thanks Laurent).
- Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org

Changes in v4:
- Display control name (Thanks Hans)
- Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org

Changes in v3:
- Add a retry mechanism during error.
- Set V4L2_CTRL_FLAG_DISABLED flag.
- Link to v2: https://lore.kernel.org/r/20241219-uvc-eaccess-v2-1-bf6520c8b86d@chromium.org

Changes in v2:
- Never return error, even if we are not enumerating the controls
- Improve commit message.
- Link to v1: https://lore.kernel.org/r/20241213-uvc-eaccess-v1-1-62e0b4fcc634@chromium.org
---
 drivers/media/usb/uvc/uvc_ctrl.c | 46 +++++++++++++++++++++++++++++++++-------
 drivers/media/usb/uvc/uvcvideo.h |  2 ++
 2 files changed, 40 insertions(+), 8 deletions(-)


---
base-commit: c2b96a6818159fba8a3bcc38262da9e77f9b3ec7
change-id: 20241213-uvc-eaccess-755cc061a360

Best regards,

Comments

Ricardo Ribalda April 3, 2025, 7:59 a.m. UTC | #1 
On Thu, 3 Apr 2025 at 09:01, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 10/03/2025 14:21, Ricardo Ribalda wrote:
> > To implement VIDIOC_QUERYCTRL, we need to know the minimum, maximum,
> > step and flags of the control. For some of the controls, this involves
> > querying the actual hardware.
> >
> > Some non-compliant cameras produce errors when we query them. These
> > error can be triggered every time, sometimes, or when other controls do
> > not have the "right value". Right now, we populate that error to userspace.
> > When an error happens, the v4l2 framework does not copy the v4l2_queryctrl
> > struct to userspace. Also, userspace apps are not ready to handle any
> > other error than -EINVAL.
> >
> > One of the main usecases of VIDIOC_QUERYCTRL is enumerating the controls
> > of a device. This is done using the V4L2_CTRL_FLAG_NEXT_CTRL flag. In
> > that usecase, a non-compliant control will make it almost impossible to
> > enumerate all controls of the device.
> >
> > A control with an invalid max/min/step/flags is better than non being
> > able to enumerate the rest of the controls.
> >
> > This patch:
> > - Retries for an extra attempt to read the control, to avoid spurious
> >   errors. More attempts do not seem to produce better results in the
> >   tested hardware.
> > - Makes VIDIOC_QUERYCTRL return 0 in all the error cases different than
> >   -EINVAL.
> > - Introduces a warning in dmesg so we can have a trace of what has happened
> >   and sets the V4L2_CTRL_FLAG_DISABLED.
> > - Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
> >   attempts to query that control (other operations have the same
> >   functionality as now).
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > Hi 2*Hans and Laurent!
> >
> > I came around a device that was listing just a couple of controls when
> > it should be listing much more.
> >
> > Some debugging latter I found that the device was returning -EIO when
> > all the focal controls were read.
> >
> > Lots of good arguments in favor/against this patch in the v1. Please
> > check!
> >
> > Without this patch:
> > $ v4l2-ctl --list-ctrls
> >                   auto_exposure 0x009a0901 (menu)   : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
> >          exposure_time_absolute 0x009a0902 (int)    : min=50 max=10000 step=1 default=166 value=166 flags=inactive
> >      exposure_dynamic_framerate 0x009a0903 (bool)   : default=0 value=0
> > region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
> >
> > With this patch:
> > $ v4l2-ctl --list-ctrls
> >                   auto_exposure 0x009a0901 (menu)   : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
> >          exposure_time_absolute 0x009a0902 (int)    : min=50 max=10000 step=1 default=166 value=166 flags=inactive
> >      exposure_dynamic_framerate 0x009a0903 (bool)   : default=0 value=0
> > error 5 getting ext_ctrl Focus, Absolute
> > error 5 getting ext_ctrl Focus, Automatic Continuous
> >    region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
> > region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
>
> This output still refers to the result from the v1 patch (I think).
> Can you redo this test with this v6 applied? You probably want to update these
> comments anyway.

I do not have access to the broken device right now. But I will fake a
-EIO error.

>
> > --
> > ---
> > Changes in v6:
> > - Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
> > - Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org
> >
> > Changes in v5:
> > - Explain the retry in the commit message (Thanks Laurent).
> > - Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org
> >
> > Changes in v4:
> > - Display control name (Thanks Hans)
> > - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org
> >
> > Changes in v3:
> > - Add a retry mechanism during error.
> > - Set V4L2_CTRL_FLAG_DISABLED flag.
> > - Link to v2: https://lore.kernel.org/r/20241219-uvc-eaccess-v2-1-bf6520c8b86d@chromium.org
> >
> > Changes in v2:
> > - Never return error, even if we are not enumerating the controls
> > - Improve commit message.
> > - Link to v1: https://lore.kernel.org/r/20241213-uvc-eaccess-v1-1-62e0b4fcc634@chromium.org
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 46 +++++++++++++++++++++++++++++++++-------
> >  drivers/media/usb/uvc/uvcvideo.h |  2 ++
> >  2 files changed, 40 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 4e58476d305efddac331417feda8cb064e340a13..4b282ac714220b26581fe468d9ecb1109a28483f 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1280,6 +1280,8 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
> >       return ~0;
> >  }
> >
> > +#define MAX_QUERY_RETRIES 2
>
> As you mentioned in the commit log, trying more times didn't make a difference.
> Add that as a comment here as well, it is good to have that documented in the code.
>
> > +
> >  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> >       struct uvc_control *ctrl,
> >       struct uvc_control_mapping *mapping,
> > @@ -1305,19 +1307,44 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> >               __uvc_find_control(ctrl->entity, mapping->master_id,
> >                                  &master_map, &master_ctrl, 0);
> >       if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> > +             unsigned int retries;
> >               s32 val;
> > -             int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> > -             if (ret < 0)
> > -                     return ret;
> > +             int ret;
> >
> > -             if (val != mapping->master_manual)
> > -                             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > +             for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> > +                     ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
> > +                                          &val);
> > +                     if (ret >= 0)
> > +                             break;
>
> This retries regardless of the error code. Isn't a retry only needed for -EIO?
> Are there other error codes that need a retry? Or, perhaps easier, are there
> errors codes for which a retry is *not* appropriate? E.g. -EACCESS, -ERANGE.

So far I have only seen -EIO, so i will start with -EIO, if we see
other behaviour we can fix this later.


>
> > +             }
> > +
> > +             if (ret < 0) {
> > +                     dev_warn_ratelimited(&chain->dev->udev->dev,
> > +                                          "UVC non compliance: Error %d querying master control %x (%s)\n",
> > +                                          ret, master_map->id,
> > +                                          uvc_map_get_name(master_map));
>
> Shouldn't you mark this control as disabled and return here instead of
> continuing?

Do you mean marking as disabled the master control or the actual control?

I think there might be cases where the master control is "disabled",
but the actual control works.
Semantically could be similar to cameras that do not have the
master_control, and we support that usecase today.

I am marking the master_map as disabled.

>
> > +             } else if (val != mapping->master_manual) {
> > +                     v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > +             }
> >       }
> >
> >       if (!ctrl->cached) {
> > -             int ret = uvc_ctrl_populate_cache(chain, ctrl);
> > -             if (ret < 0)
> > -                     return ret;
> > +             unsigned int retries;
> > +             int ret;
> > +
> > +             for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> > +                     ret = uvc_ctrl_populate_cache(chain, ctrl);
> > +                     if (ret >= 0)
> > +                             break;
>
> Same question about the error code as above.
>
> > +             }
> > +
> > +             if (ret < 0) {
> > +                     dev_warn_ratelimited(&chain->dev->udev->dev,
> > +                                          "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
> > +                                          mapping->id,
> > +                                          uvc_map_get_name(mapping), ret);
> > +                     mapping->disabled = true;
> > +             }
> >       }
> >
> >       if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
> > @@ -1325,6 +1352,9 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> >                               uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF));
> >       }
> >
> > +     if (mapping->disabled)
> > +             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
> > +
> >       switch (mapping->v4l2_type) {
> >       case V4L2_CTRL_TYPE_MENU:
> >               v4l2_ctrl->minimum = ffs(mapping->menu_mask) - 1;
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 5e388f05f3fcaf0e4c503a34745d05837ecb0184..63687d7e97738a50d037b1f456f5215241909c13 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -129,6 +129,8 @@ struct uvc_control_mapping {
> >       s32 master_manual;
> >       u32 slave_ids[2];
> >
> > +     bool disabled;
> > +
> >       const struct uvc_control_mapping *(*filter_mapping)
> >                               (struct uvc_video_chain *chain,
> >                               struct uvc_control *ctrl);
> >
> > ---
> > base-commit: c2b96a6818159fba8a3bcc38262da9e77f9b3ec7
> > change-id: 20241213-uvc-eaccess-755cc061a360
> >
> > Best regards,
>
> Regards,
>
>         Hans


Thanks :)
Hans Verkuil April 3, 2025, 8:12 a.m. UTC | #2 
On 03/04/2025 09:59, Ricardo Ribalda wrote:
> On Thu, 3 Apr 2025 at 09:01, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 10/03/2025 14:21, Ricardo Ribalda wrote:
>>> To implement VIDIOC_QUERYCTRL, we need to know the minimum, maximum,
>>> step and flags of the control. For some of the controls, this involves
>>> querying the actual hardware.
>>>
>>> Some non-compliant cameras produce errors when we query them. These
>>> error can be triggered every time, sometimes, or when other controls do
>>> not have the "right value". Right now, we populate that error to userspace.
>>> When an error happens, the v4l2 framework does not copy the v4l2_queryctrl
>>> struct to userspace. Also, userspace apps are not ready to handle any
>>> other error than -EINVAL.
>>>
>>> One of the main usecases of VIDIOC_QUERYCTRL is enumerating the controls
>>> of a device. This is done using the V4L2_CTRL_FLAG_NEXT_CTRL flag. In
>>> that usecase, a non-compliant control will make it almost impossible to
>>> enumerate all controls of the device.
>>>
>>> A control with an invalid max/min/step/flags is better than non being
>>> able to enumerate the rest of the controls.
>>>
>>> This patch:
>>> - Retries for an extra attempt to read the control, to avoid spurious
>>>   errors. More attempts do not seem to produce better results in the
>>>   tested hardware.
>>> - Makes VIDIOC_QUERYCTRL return 0 in all the error cases different than
>>>   -EINVAL.
>>> - Introduces a warning in dmesg so we can have a trace of what has happened
>>>   and sets the V4L2_CTRL_FLAG_DISABLED.
>>> - Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
>>>   attempts to query that control (other operations have the same
>>>   functionality as now).
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>> Hi 2*Hans and Laurent!
>>>
>>> I came around a device that was listing just a couple of controls when
>>> it should be listing much more.
>>>
>>> Some debugging latter I found that the device was returning -EIO when
>>> all the focal controls were read.
>>>
>>> Lots of good arguments in favor/against this patch in the v1. Please
>>> check!
>>>
>>> Without this patch:
>>> $ v4l2-ctl --list-ctrls
>>>                   auto_exposure 0x009a0901 (menu)   : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
>>>          exposure_time_absolute 0x009a0902 (int)    : min=50 max=10000 step=1 default=166 value=166 flags=inactive
>>>      exposure_dynamic_framerate 0x009a0903 (bool)   : default=0 value=0
>>> region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
>>>
>>> With this patch:
>>> $ v4l2-ctl --list-ctrls
>>>                   auto_exposure 0x009a0901 (menu)   : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
>>>          exposure_time_absolute 0x009a0902 (int)    : min=50 max=10000 step=1 default=166 value=166 flags=inactive
>>>      exposure_dynamic_framerate 0x009a0903 (bool)   : default=0 value=0
>>> error 5 getting ext_ctrl Focus, Absolute
>>> error 5 getting ext_ctrl Focus, Automatic Continuous
>>>    region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
>>> region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
>>
>> This output still refers to the result from the v1 patch (I think).
>> Can you redo this test with this v6 applied? You probably want to update these
>> comments anyway.
> 
> I do not have access to the broken device right now. But I will fake a
> -EIO error.
> 
>>
>>> --
>>> ---
>>> Changes in v6:
>>> - Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
>>> - Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org
>>>
>>> Changes in v5:
>>> - Explain the retry in the commit message (Thanks Laurent).
>>> - Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org
>>>
>>> Changes in v4:
>>> - Display control name (Thanks Hans)
>>> - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org
>>>
>>> Changes in v3:
>>> - Add a retry mechanism during error.
>>> - Set V4L2_CTRL_FLAG_DISABLED flag.
>>> - Link to v2: https://lore.kernel.org/r/20241219-uvc-eaccess-v2-1-bf6520c8b86d@chromium.org
>>>
>>> Changes in v2:
>>> - Never return error, even if we are not enumerating the controls
>>> - Improve commit message.
>>> - Link to v1: https://lore.kernel.org/r/20241213-uvc-eaccess-v1-1-62e0b4fcc634@chromium.org
>>> ---
>>>  drivers/media/usb/uvc/uvc_ctrl.c | 46 +++++++++++++++++++++++++++++++++-------
>>>  drivers/media/usb/uvc/uvcvideo.h |  2 ++
>>>  2 files changed, 40 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>>> index 4e58476d305efddac331417feda8cb064e340a13..4b282ac714220b26581fe468d9ecb1109a28483f 100644
>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>>> @@ -1280,6 +1280,8 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
>>>       return ~0;
>>>  }
>>>
>>> +#define MAX_QUERY_RETRIES 2
>>
>> As you mentioned in the commit log, trying more times didn't make a difference.
>> Add that as a comment here as well, it is good to have that documented in the code.
>>
>>> +
>>>  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>>       struct uvc_control *ctrl,
>>>       struct uvc_control_mapping *mapping,
>>> @@ -1305,19 +1307,44 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>>               __uvc_find_control(ctrl->entity, mapping->master_id,
>>>                                  &master_map, &master_ctrl, 0);
>>>       if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
>>> +             unsigned int retries;
>>>               s32 val;
>>> -             int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
>>> -             if (ret < 0)
>>> -                     return ret;
>>> +             int ret;
>>>
>>> -             if (val != mapping->master_manual)
>>> -                             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>>> +             for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
>>> +                     ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
>>> +                                          &val);
>>> +                     if (ret >= 0)
>>> +                             break;
>>
>> This retries regardless of the error code. Isn't a retry only needed for -EIO?
>> Are there other error codes that need a retry? Or, perhaps easier, are there
>> errors codes for which a retry is *not* appropriate? E.g. -EACCESS, -ERANGE.
> 
> So far I have only seen -EIO, so i will start with -EIO, if we see
> other behaviour we can fix this later.
> 
> 
>>
>>> +             }
>>> +
>>> +             if (ret < 0) {
>>> +                     dev_warn_ratelimited(&chain->dev->udev->dev,
>>> +                                          "UVC non compliance: Error %d querying master control %x (%s)\n",
>>> +                                          ret, master_map->id,
>>> +                                          uvc_map_get_name(master_map));
>>
>> Shouldn't you mark this control as disabled and return here instead of
>> continuing?
> 
> Do you mean marking as disabled the master control or the actual control?
> 
> I think there might be cases where the master control is "disabled",
> but the actual control works.
> Semantically could be similar to cameras that do not have the
> master_control, and we support that usecase today.
> 
> I am marking the master_map as disabled.

Ah, I missed that there are two different controls here. Just ignore
my comment.

Regards,

	Hans

> 
>>
>>> +             } else if (val != mapping->master_manual) {
>>> +                     v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>>> +             }
>>>       }
>>>
>>>       if (!ctrl->cached) {
>>> -             int ret = uvc_ctrl_populate_cache(chain, ctrl);
>>> -             if (ret < 0)
>>> -                     return ret;
>>> +             unsigned int retries;
>>> +             int ret;
>>> +
>>> +             for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
>>> +                     ret = uvc_ctrl_populate_cache(chain, ctrl);
>>> +                     if (ret >= 0)
>>> +                             break;
>>
>> Same question about the error code as above.
>>
>>> +             }
>>> +
>>> +             if (ret < 0) {
>>> +                     dev_warn_ratelimited(&chain->dev->udev->dev,
>>> +                                          "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
>>> +                                          mapping->id,
>>> +                                          uvc_map_get_name(mapping), ret);
>>> +                     mapping->disabled = true;
>>> +             }
>>>       }
>>>
>>>       if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
>>> @@ -1325,6 +1352,9 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>>                               uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF));
>>>       }
>>>
>>> +     if (mapping->disabled)
>>> +             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
>>> +
>>>       switch (mapping->v4l2_type) {
>>>       case V4L2_CTRL_TYPE_MENU:
>>>               v4l2_ctrl->minimum = ffs(mapping->menu_mask) - 1;
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 5e388f05f3fcaf0e4c503a34745d05837ecb0184..63687d7e97738a50d037b1f456f5215241909c13 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -129,6 +129,8 @@ struct uvc_control_mapping {
>>>       s32 master_manual;
>>>       u32 slave_ids[2];
>>>
>>> +     bool disabled;
>>> +
>>>       const struct uvc_control_mapping *(*filter_mapping)
>>>                               (struct uvc_video_chain *chain,
>>>                               struct uvc_control *ctrl);
>>>
>>> ---
>>> base-commit: c2b96a6818159fba8a3bcc38262da9e77f9b3ec7
>>> change-id: 20241213-uvc-eaccess-755cc061a360
>>>
>>> Best regards,
>>
>> Regards,
>>
>>         Hans
> 
> 
> Thanks :)
> 
>
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 4e58476d305efddac331417feda8cb064e340a13..4b282ac714220b26581fe468d9ecb1109a28483f 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1280,6 +1280,8 @@  static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
 	return ~0;
 }
 
+#define MAX_QUERY_RETRIES 2
+
 static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 	struct uvc_control *ctrl,
 	struct uvc_control_mapping *mapping,
@@ -1305,19 +1307,44 @@  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 		__uvc_find_control(ctrl->entity, mapping->master_id,
 				   &master_map, &master_ctrl, 0);
 	if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
+		unsigned int retries;
 		s32 val;
-		int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
-		if (ret < 0)
-			return ret;
+		int ret;
 
-		if (val != mapping->master_manual)
-				v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
+		for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
+			ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
+					     &val);
+			if (ret >= 0)
+				break;
+		}
+
+		if (ret < 0) {
+			dev_warn_ratelimited(&chain->dev->udev->dev,
+					     "UVC non compliance: Error %d querying master control %x (%s)\n",
+					     ret, master_map->id,
+					     uvc_map_get_name(master_map));
+		} else if (val != mapping->master_manual) {
+			v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
+		}
 	}
 
 	if (!ctrl->cached) {
-		int ret = uvc_ctrl_populate_cache(chain, ctrl);
-		if (ret < 0)
-			return ret;
+		unsigned int retries;
+		int ret;
+
+		for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
+			ret = uvc_ctrl_populate_cache(chain, ctrl);
+			if (ret >= 0)
+				break;
+		}
+
+		if (ret < 0) {
+			dev_warn_ratelimited(&chain->dev->udev->dev,
+					     "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
+					     mapping->id,
+					     uvc_map_get_name(mapping), ret);
+			mapping->disabled = true;
+		}
 	}
 
 	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
@@ -1325,6 +1352,9 @@  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF));
 	}
 
+	if (mapping->disabled)
+		v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
+
 	switch (mapping->v4l2_type) {
 	case V4L2_CTRL_TYPE_MENU:
 		v4l2_ctrl->minimum = ffs(mapping->menu_mask) - 1;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 5e388f05f3fcaf0e4c503a34745d05837ecb0184..63687d7e97738a50d037b1f456f5215241909c13 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -129,6 +129,8 @@  struct uvc_control_mapping {
 	s32 master_manual;
 	u32 slave_ids[2];
 
+	bool disabled;
+
 	const struct uvc_control_mapping *(*filter_mapping)
 				(struct uvc_video_chain *chain,
 				struct uvc_control *ctrl);