linux-kernel - Re: [PATCH v5 4/4] media: uvcvideo: Auto-set UVC_QUIRK_MSXU

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250526140209.GP17743@pendragon.ideasonboard.com>
Date: Mon, 26 May 2025 16:02:09 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Hans de Goede <hdegoede@...hat.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Guennadi Liakhovetski <guennadi.liakhovetski@...el.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 4/4] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META

Hi Ricardo,

Thank you for the patch.

On Fri, Apr 04, 2025 at 06:37:37AM +0000, Ricardo Ribalda wrote:
> If the camera supports the MSXU_CONTROL_METADATA control, auto set the
> MSXU_META quirk.

Ah, that's why you introduce a quirk in 3/4.

I would prefer if you could instead add a metadata format field in the
uvc_device structure (I'd put it right after the info field, and while
at it you could move the quirks field to that section too). The metadata
format would be initialized from dev->info (when available) or set to
the UVC format, and overridden when the MSXU is detected.

> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
>  drivers/media/usb/uvc/uvc_metadata.c | 54 ++++++++++++++++++++++++++++++++++++
>  include/linux/usb/uvc.h              |  3 ++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> index fe2678fc795d7fd5a64e8113199012f34c419176..776d280f34afad515594a873acf075acf0438304 100644
> --- a/drivers/media/usb/uvc/uvc_metadata.c
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -10,6 +10,7 @@
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/usb.h>
> +#include <linux/usb/uvc.h>
>  #include <linux/videodev2.h>
>  
>  #include <media/v4l2-ioctl.h>
> @@ -187,11 +188,64 @@ static const struct v4l2_file_operations uvc_meta_fops = {
>  	.mmap = vb2_fop_mmap,
>  };
>  
> +static const u8 uvc_msxu_guid[16] = UVC_GUID_MSXU_1_5;
> +
> +#define MSXU_CONTROL_METADATA 0x9
> +static int uvc_enable_msxu(struct uvc_device *dev)

uvc_meta_detect_msxu()

> +{
> +	u32 *data __free(kfree) = NULL;
> +	struct uvc_entity *entity;
> +
> +	list_for_each_entry(entity, &dev->entities, list) {
> +		int ret;
> +
> +		if (memcmp(entity->guid, uvc_msxu_guid, sizeof(entity->guid)))
> +			continue;
> +
> +		if (!data)
> +			data = kmalloc(sizeof(*data), GFP_KERNEL);
> +		if (!data)
> +			return -ENOMEM;
> +

A comment here to explain how the control operates would be useful.
Reading the code I assume that GET_MAX will indicate if the MS metadata
format is supported by reporting a value different than 0 (is it always
1, or can it take other values), and SET_CUR will enable metadata
generation. I suppose the first GET_CUR call catches the case where it
has already been enabled, are there also cameras where it can't be
disabled, and where SET_CUR would fail ?

> +		ret = uvc_query_ctrl(dev, UVC_GET_CUR, entity->id,
> +				     dev->intfnum, MSXU_CONTROL_METADATA,
> +				     data, sizeof(*data));
> +		if (ret)
> +			continue;

Can there be multiple MSXU instances, or can you break here (and below)
?

> +
> +		if (*data) {
> +			dev->quirks |= UVC_QUIRK_MSXU_META;
> +			return 0;
> +		}
> +
> +		ret = uvc_query_ctrl(dev, UVC_GET_MAX, entity->id,
> +				     dev->intfnum, MSXU_CONTROL_METADATA,
> +				     data, sizeof(*data));
> +		if (ret || !*data)
> +			continue;
> +
> +		ret = uvc_query_ctrl(dev, UVC_SET_CUR, entity->id,
> +				     dev->intfnum, MSXU_CONTROL_METADATA,
> +				     data, sizeof(*data));
> +		if (!ret) {
> +			dev->quirks |= UVC_QUIRK_MSXU_META;
> +			return 0;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  int uvc_meta_register(struct uvc_streaming *stream)
>  {
>  	struct uvc_device *dev = stream->dev;
>  	struct video_device *vdev = &stream->meta.vdev;
>  	struct uvc_video_queue *queue = &stream->meta.queue;
> +	int ret;
> +
> +	ret = uvc_enable_msxu(dev);
> +	if (ret)
> +		return ret;
>  
>  	stream->meta.format = V4L2_META_FMT_UVC;
>  
> diff --git a/include/linux/usb/uvc.h b/include/linux/usb/uvc.h
> index bce95153e5a65613a710d7316fc17cf5462b5bce..ee19e9f915b8370c333c426dc1ee4202c7b75c5b 100644
> --- a/include/linux/usb/uvc.h
> +++ b/include/linux/usb/uvc.h
> @@ -29,6 +29,9 @@
>  #define UVC_GUID_EXT_GPIO_CONTROLLER \
>  	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
>  	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x03}
> +#define UVC_GUID_MSXU_1_5 \
> +	{0xdc, 0x95, 0x3f, 0x0f, 0x32, 0x26, 0x4e, 0x4c, \
> +	 0x92, 0xc9, 0xa0, 0x47, 0x82, 0xf4, 0x3b, 0xc8}
>  
>  #define UVC_GUID_FORMAT_MJPEG \
>  	{ 'M',  'J',  'P',  'G', 0x00, 0x00, 0x10, 0x00, \
> 
> -- 
> 2.49.0.504.g3bcea36a83-goog
> 

-- 
Regards,

Laurent Pinchart