Re: [PATCH v2 2/3] media: uvcvideo: generalise the XU flags fixup to all controls

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Sep 01, 2026 at 08:25:52PM -0400, Michael Jordan wrote:
> uvc_ctrl_fixup_xu_info() holds a per-device table of controls whose
> GET_INFO reply is wrong, and overrides the flags for them. It only runs
> from uvc_ctrl_fill_xu_info(), so it can only correct extension unit
> controls, but standard controls suffer from the same class of firmware
> bug: a device can report a wrong capability byte for a Camera Terminal
> or Processing Unit control just as easily.
> 
> Rename it to uvc_ctrl_fixup_flags() and call it at the start of
> uvc_ctrl_get_flags(), where the flags are derived from GET_INFO for
> every control, standard and XU alike. The fixup replaces the flags
> wholesale, so when the table covers a control there is no point in
> querying a device we already know gives a wrong answer: return early
> and skip the GET_INFO request altogether. The call in
> uvc_ctrl_fill_xu_info() is dropped, as uvc_ctrl_get_flags() now handles
> the fixup for XU controls too.
> 
> No functional change for the devices already in the table: their
> entries are XU controls, matched by entity and selector before as they
> are now, and their flags come from the table either way. The only
> difference is one GET_INFO request no longer issued per fixed-up
> control.

That's a long commit message for such a simple change. Is it
LLM-generated ? If so, you need an Assisted-by tag on this patch series.

> Suggested-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: Michael Jordan <jordan.mymail@gmail.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 91 ++++++++++++++++++--------------
>  1 file changed, 50 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index aceb26310..64c90c380 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2852,6 +2852,48 @@ int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl)
>   * Dynamic controls
>   */
>  
> +static bool uvc_ctrl_fixup_flags(struct uvc_device *dev,
> +				 const struct uvc_control *ctrl,
> +				 struct uvc_control_info *info)
> +{
> +	struct uvc_ctrl_fixup {
> +		struct usb_device_id id;
> +		u8 entity;
> +		u8 selector;
> +		u8 flags;
> +	};
> +
> +	static const struct uvc_ctrl_fixup fixups[] = {
> +		{ { USB_DEVICE(0x046d, 0x08c2) }, 9, 1,
> +			UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX |
> +			UVC_CTRL_FLAG_GET_DEF | UVC_CTRL_FLAG_SET_CUR |
> +			UVC_CTRL_FLAG_AUTO_UPDATE },
> +		{ { USB_DEVICE(0x046d, 0x08cc) }, 9, 1,
> +			UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX |
> +			UVC_CTRL_FLAG_GET_DEF | UVC_CTRL_FLAG_SET_CUR |
> +			UVC_CTRL_FLAG_AUTO_UPDATE },
> +		{ { USB_DEVICE(0x046d, 0x0994) }, 9, 1,
> +			UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX |
> +			UVC_CTRL_FLAG_GET_DEF | UVC_CTRL_FLAG_SET_CUR |
> +			UVC_CTRL_FLAG_AUTO_UPDATE },
> +	};
> +
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fixups); ++i) {
> +		if (!usb_match_one_id(dev->intf, &fixups[i].id))
> +			continue;
> +
> +		if (fixups[i].entity == ctrl->entity->id &&
> +		    fixups[i].selector == info->selector) {
> +			info->flags = fixups[i].flags;
> +			return true;
> +		}
> +	}

The O(n*m) complexity isn't nice, but it's not a new issue. It can be
addressed separately.

The code change looks fine.

> +
> +	return false;
> +}
> +
>  /*
>   * Retrieve flags for a given control
>   */
> @@ -2862,6 +2904,14 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
>  	u8 *data;
>  	int ret;
>  
> +	/*
> +	 * Some devices report bogus capabilities through GET_INFO. If the
> +	 * fixup table covers this control, take the flags from the table and
> +	 * skip the query altogether.
> +	 */
> +	if (uvc_ctrl_fixup_flags(dev, ctrl, info))
> +		return 0;
> +
>  	data = kmalloc(1, GFP_KERNEL);
>  	if (data == NULL)
>  		return -ENOMEM;
> @@ -2893,45 +2943,6 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
>  	return ret;
>  }
>  
> -static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
> -	const struct uvc_control *ctrl, struct uvc_control_info *info)
> -{
> -	struct uvc_ctrl_fixup {
> -		struct usb_device_id id;
> -		u8 entity;
> -		u8 selector;
> -		u8 flags;
> -	};
> -
> -	static const struct uvc_ctrl_fixup fixups[] = {
> -		{ { USB_DEVICE(0x046d, 0x08c2) }, 9, 1,
> -			UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX |
> -			UVC_CTRL_FLAG_GET_DEF | UVC_CTRL_FLAG_SET_CUR |
> -			UVC_CTRL_FLAG_AUTO_UPDATE },
> -		{ { USB_DEVICE(0x046d, 0x08cc) }, 9, 1,
> -			UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX |
> -			UVC_CTRL_FLAG_GET_DEF | UVC_CTRL_FLAG_SET_CUR |
> -			UVC_CTRL_FLAG_AUTO_UPDATE },
> -		{ { USB_DEVICE(0x046d, 0x0994) }, 9, 1,
> -			UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX |
> -			UVC_CTRL_FLAG_GET_DEF | UVC_CTRL_FLAG_SET_CUR |
> -			UVC_CTRL_FLAG_AUTO_UPDATE },
> -	};
> -
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(fixups); ++i) {
> -		if (!usb_match_one_id(dev->intf, &fixups[i].id))
> -			continue;
> -
> -		if (fixups[i].entity == ctrl->entity->id &&
> -		    fixups[i].selector == info->selector) {
> -			info->flags = fixups[i].flags;
> -			return;
> -		}
> -	}
> -}
> -
>  /*
>   * Query control information (size and flags) for XU controls.
>   */
> @@ -2972,8 +2983,6 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>  		goto done;
>  	}
>  
> -	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
> -
>  	uvc_dbg(dev, CONTROL,
>  		"XU control %pUl/%u queried: len %u, flags { get %u set %u auto %u }\n",
>  		info->entity, info->selector, info->size,

-- 
Regards,

Laurent Pinchart