| 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
| ||
|
Message-ID: <CANiDSCvkTQeTsFwRiJF=9s8jHsRtWjb46=JcwLWPPK0VLqkbEA@mail.gmail.com>
Date: Mon, 14 Jul 2025 10:21:20 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Guennadi Liakhovetski <guennadi.liakhovetski@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Hans de Goede <hansg@...nel.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org
Subject: Re: [PATCH v8 5/5] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META
On Mon, 14 Jul 2025 at 10:06, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Mon, Jul 14, 2025 at 09:46:45AM +0200, Ricardo Ribalda wrote:
> > On Fri, 11 Jul 2025 at 21:58, Laurent Pinchart wrote:
> > > On Mon, Jul 07, 2025 at 06:34:05PM +0000, Ricardo Ribalda wrote:
> > > > If the camera supports the MSXU_CONTROL_METADATA control, auto set the
> > > > MSXU_META quirk.
> > > >
> > > > Reviewed-by: Hans de Goede <hansg@...nel.org>
> > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > ---
> > > > drivers/media/usb/uvc/uvc_driver.c | 7 +++-
> > > > drivers/media/usb/uvc/uvc_metadata.c | 75 +++++++++++++++++++++++++++++++++++-
> > > > drivers/media/usb/uvc/uvcvideo.h | 2 +-
> > > > include/linux/usb/uvc.h | 3 ++
> > > > 4 files changed, 84 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index 56ea20eeb7b9d5d92f3d837c15bdf11d536e9f2d..9de5abb43e19d9e876cddc5d7124592953db89ac 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -2315,7 +2315,12 @@ static int uvc_probe(struct usb_interface *intf,
> > > > goto error;
> > > > }
> > > >
> > > > - uvc_meta_init(dev);
> > > > + ret = uvc_meta_init(dev);
> > > > + if (ret < 0) {
> > > > + dev_err(&dev->udev->dev,
> > > > + "Error initializing the metadata formats (%d)\n", ret);
> > > > + goto error;
> > > > + }
> > > >
> > > > if (dev->quirks & UVC_QUIRK_NO_RESET_RESUME)
> > > > udev->quirks &= ~USB_QUIRK_RESET_RESUME;
> > > > diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> > > > index 77e03273d3cf6b00cac6ebb9b29b941f1cbfd9f7..59bb133baf9a73ef6a30fa8ead85aa90653d60f4 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>
> > > > @@ -166,6 +167,71 @@ static const struct v4l2_file_operations uvc_meta_fops = {
> > > > .mmap = vb2_fop_mmap,
> > > > };
> > > >
> > > > +static struct uvc_entity *uvc_meta_find_msxu(struct uvc_device *dev)
> > > > +{
> > > > + static const u8 uvc_msxu_guid[16] = UVC_GUID_MSXU_1_5;
> > > > + struct uvc_entity *entity;
> > > > +
> > > > + list_for_each_entry(entity, &dev->entities, list) {
> > > > + if (!memcmp(entity->guid, uvc_msxu_guid, sizeof(entity->guid)))
> > > > + return entity;
> > > > + }
> > > > +
> > > > + return NULL;
> > > > +}
> > > > +
> > > > +#define MSXU_CONTROL_METADATA 0x9
> > > > +static int uvc_meta_detect_msxu(struct uvc_device *dev)
> > > > +{
> > > > + u32 *data __free(kfree) = NULL;
> > > > + struct uvc_entity *entity;
> > > > + int ret;
> > > > +
> > > > + entity = uvc_meta_find_msxu(dev);
> > > > + if (!entity)
> > > > + return 0;
> > > > +
> > > > + /*
> > > > + * USB requires buffers aligned in a special way, simplest way is to
> > > > + * make sure that query_ctrl will work is to kmalloc() them.
> > > > + */
> > > > + data = kmalloc(sizeof(*data), GFP_KERNEL);
> > > > + if (!data)
> > > > + return -ENOMEM;
> > > > +
> > > > + /* Check if the metadata is already enabled. */
> > > > + ret = uvc_query_ctrl(dev, UVC_GET_CUR, entity->id, dev->intfnum,
> > > > + MSXU_CONTROL_METADATA, data, sizeof(*data));
> > > > + if (ret)
> > > > + return 0;
> > > > +
> > > > + if (*data) {
> > > > + dev->quirks |= UVC_QUIRK_MSXU_META;
> > > > + return 0;
> > > > + }
> > > > +
> > > > + /*
> > > > + * We have seen devices that require 1 to enable the metadata, others
> > > > + * requiring a value != 1 and others requiring a value >1. Luckily for
> > >
> > > I'm confused here. If those are three different behaviours, then value
> > > != 1 would be value == 0 (as the third behaviour is value > 1). You test
> > > for !*data below, so 0 is not accepted as a valid value for this
> > > purpose. What am I missing ?
> >
> > There is a typo in the comment.
> >
> > Some devices require 1, some devices any value !=0, and other value=MAX.
> > I will fix it in a follow-up patch.
>
> The documentation of the control states that MSXU_CONTROL_METADATA
> reports the maximum size of the MS metadata generated by the device in
> kB, and the control should be set to the value reported by GET_MAX to
> enable metadata generation. That's what you're doing in this patch, so
> you can update the comment to document there.
Some vendors assumed the kB was a typo in MS spec and used bytes instead :)
In this favor I must say that kB here sounds like a mistake, I haven't
seen more than 300 bytes of metadata.
Will send a patch with the updated documentation.
>
> Some devices also don't support SET_CUR, in which case they should
> report GET_MIN == GET_DEF == GET_MAX. I assume GET_CUR should then also
> report the same value. Please also update the previous comment to
> explain this, the GET_CUR value check above is more about handling
> devices that always produce metadata than devices for which a driver has
> enabled metadata production.
>
> This leads to another question: should we enable metadata generation
> only when the metadata capture device is streaming ?
This will lead to a lot of issues with very little benefit.
We have no control over the order that the user will enable streamon
in the metadata and "normal" video devices. And the device will not
like that MSXU_CONTROL_METADATA changes during streamon.
I have not seen any performance penalty by the 300 new bytes
transferred when the metadata is on.
>
> > > > + * us, the value from GET_MAX seems to work all the time.
> > > > + */
> > > > + ret = uvc_query_ctrl(dev, UVC_GET_MAX, entity->id, dev->intfnum,
> > > > + MSXU_CONTROL_METADATA, data, sizeof(*data));
> > > > + if (ret || !*data)
> > > > + return 0;
> > > > +
> > > > + /*
> > > > + * If we can set MSXU_CONTROL_METADATA, the device will report
> > > > + * metadata.
> > > > + */
> > > > + 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;
> > > > +}
> > > > +
> > > > int uvc_meta_register(struct uvc_streaming *stream)
> > > > {
> > > > struct uvc_device *dev = stream->dev;
> > > > @@ -185,9 +251,14 @@ int uvc_meta_register(struct uvc_streaming *stream)
> > > > &uvc_meta_fops, &uvc_meta_ioctl_ops);
> > > > }
> > > >
> > > > -void uvc_meta_init(struct uvc_device *dev)
> > > > +int uvc_meta_init(struct uvc_device *dev)
> > > > {
> > > > unsigned int i = 0;
> > > > + int ret;
> > > > +
> > > > + ret = uvc_meta_detect_msxu(dev);
> > > > + if (ret)
> > > > + return ret;
> > > >
> > > > dev->meta_formats[i++] = V4L2_META_FMT_UVC;
> > > >
> > > > @@ -201,4 +272,6 @@ void uvc_meta_init(struct uvc_device *dev)
> > > >
> > > > /* IMPORTANT: for new meta-formats update UVC_MAX_META_DATA_FORMATS. */
> > > > dev->meta_formats[i++] = 0;
> > > > +
> > > > + return 0;
> > > > }
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index 616adc417c62a58686beccbc440a5dfac0a2d588..a4c064c5e046f2a4adba742c8777a10619569606 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -757,7 +757,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > void uvc_video_clock_update(struct uvc_streaming *stream,
> > > > struct vb2_v4l2_buffer *vbuf,
> > > > struct uvc_buffer *buf);
> > > > -void uvc_meta_init(struct uvc_device *dev);
> > > > +int uvc_meta_init(struct uvc_device *dev);
> > > > int uvc_meta_register(struct uvc_streaming *stream);
> > > >
> > > > int uvc_register_video_device(struct uvc_device *dev,
> > > > 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, \
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
Powered by blists - more mailing lists