Message-ID: <CANiDSCsB2mEf2JGUTNgkrgYo8LYgSNyD126-Hq_Ek__O3KtxTQ@mail.gmail.com>
Date: Fri, 24 Oct 2025 13:04:05 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans de Goede <hansg@...nel.org>, Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: uvcvideo: Create a specific id namespace for
 output entities

Hi Laurent

On Fri, 24 Oct 2025 at 12:50, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Wed, Oct 22, 2025 at 01:12:09PM +0000, Ricardo Ribalda wrote:
> > Nothing can be connected to an output terminal. Which means that no
> > other entity can reference an output terminal as baSourceId.
> >
> > Use this fact to move all the output streaming entities, which have no
> > controls, to a different namespace.
> >
> > The output entities are usually named after the dev_name() of the usb
> > device, so there should not be any uAPI change from this change.
> >
> > Although with this change we can handle some id collisions
> > automagically, change the logic of uvc_alloc_new_entity() to keep
> > showing a warning when a camera has invalid descriptors. Hopefully this
> > message will help vendors fix their invalid descriptors.
>
> This is one of the commits that I expect we'll go back to in the future
> when trying to remember why we namespace the streeaming terminal IDs.
> Let's make the rationale very clear, and also mention the name of the
> devices we know to be affected.
>
> ----
> Some devices, such as the Grandstream GUV3100 and the LSK Meeting Eye
> for Business & Home, exhibit entity ID collisions between units and
> streaming output terminals.
>
> The UVC specification requires unit and terminal IDs to be unique, and
> uses the ID to reference entities:
>
> - In control requests, to identify the target entity
> - In the UVC units and terminals descriptors' bSourceID field, to
>   identify source entities
> - In the UVC input header descriptor's bTerminalLink, to identify the
>   terminal associated with a streaming interface
>
> Entity ID collisions break accessing controls and make the graph
> description in the UVC descriptors ambiguous. However, collisions where
> one of the entities is a streaming output terminal and the other entity
> is not a streaming terminal are less severe. Streaming output terminals
> have no controls, and, as they are the final entity in pipelines, they
> are never referenced in descriptors as source entities. They are
> referenced by ID only from innput header descriptors, which by
> definition only reference streaming terminals.
>
> For these reasons, we can work around the collision by giving streaming
> output terminals their own ID namespace. Do so by setting bit
> UVC_TERM_OUTPUT (15) in the uvc_entity.id field, which is normally never
> set as the ID is a 8-bit value.
>
> This ID change doesn't affect the entity name in the media controller
> graph as the name isn't constructed from the ID, so there should not be
> any impact on the uAPI.
>
> Although this change handles some ID collisions automagically, keep
> printing an error in uvc_alloc_new_entity() when a camera has invalid
> descriptors. Hopefully this message will help vendors fix their invalid
> descriptors.
> ----
>
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> > Hi, this patch fixes support for some devices with invalid USB
> > descriptor.
> >
> > It is orthogonal to:
> > https://lore.kernel.org/linux-media/20251021184213.GC19043@pendragon.ideasonboard.com/T/#t
> >
> > Some devices will be fixed by the other patch, other devices will be
> > fixed by this. In my opinion is worth to land both patches.
>
> I would prefer merging this fix only (or rather, if we merge the other
> one as a quick fix, reverting it once we merge this one). The rationale
> is that enabling non-compliant behaviour should be limited to issues we
> know about, otherwise it could encourage making more non-compliant
> devices.
>
> The UVC specification states
>
>   Each Unit and Terminal within the video function is assigned a unique
>   identification number, the Unit ID (UID) or Terminal ID (TID),
>   contained in the bUnitID or bTerminalID field of the descriptor. The
>   value 0x00 is reserved for undefined ID, effectively restricting the
>   total number of addressable entities in the video function (both Units
>   and Terminals) to 255.
>
> To me it's clear that the unit and terminal IDs share the same
> namespace, but I suppose some vendors can overlook that. When combined
> with the fact that the streaming output terminals do not have controls,
> and therefore do not need to be addressable and lookup up by ID for
> anything else than matching with a streaming interface, this makes me
> think that ID collision between a streaming output terminal and another
> unit will likely be the most common case.
>
> > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > ---
> > Changes in v2:
> > - Change Macro name
> > - Apply quirk only to TT_STEAMING
> > - Add missing suggested by
> > - uvc_stream_for_terminal
> > - Note, v2 has not been tested yet in real hardware, only v1.
> > - Link to v1: https://lore.kernel.org/r/20251022-uvc-grandstream-laurent-v1-1-0925738a3484@chromium.org
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 31 ++++++++++++++++++++++++-------
> >  drivers/media/usb/uvc/uvcvideo.h   |  3 ++-
> >  2 files changed, 26 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..c0a2c05b0f13a8c3b14018c47dfb0be2614340ce 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -165,8 +165,10 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> >       return NULL;
> >  }
> >
> > -static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > +static struct uvc_streaming *uvc_stream_for_terminal(struct uvc_device *dev,
> > +                                                  struct uvc_entity *term)
> >  {
> > +     u16 id = UVC_HARDWARE_ENTITY_ID(term->id);
> >       struct uvc_streaming *stream;
> >
> >       list_for_each_entry(stream, &dev->streams, list) {
> > @@ -810,10 +812,12 @@ static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> >       }
> >
> >       /* Per UVC 1.1+ spec 3.7.2, the ID is unique. */
> > -     if (uvc_entity_by_id(dev, id)) {
> > -             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n", id);
> > +     if (uvc_entity_by_id(dev, UVC_HARDWARE_ENTITY_ID(id)))
> > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > +                     UVC_HARDWARE_ENTITY_ID(id));
> > +
> > +     if (uvc_entity_by_id(dev, id))
> >               id = UVC_INVALID_ENTITY_ID;
> > -     }
> >
> >       extra_size = roundup(extra_size, sizeof(*entity->pads));
> >       if (num_pads)
> > @@ -969,6 +973,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> >       struct usb_host_interface *alts = dev->intf->cur_altsetting;
> >       unsigned int i, n, p, len;
> >       const char *type_name;
> > +     unsigned int id;
> >       u16 type;
> >
> >       switch (buffer[2]) {
> > @@ -1107,8 +1112,20 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> >                       return 0;
> >               }
> >
> > +             id = buffer[3];
> > +
> > +             /*
> > +              * Nothing can be connected to an output terminal. To avoid
> > +              * entity-id's collisions in devices with invalid USB
> > +              * descriptors, move the output terminal id to its own
> > +              * namespace. Do this only for UVC_TT_STREAMING entities, to
> > +              * avoid changing the id of terminals with controls.
> > +              */
>
> How about expanding this too, similarly to the commit message to avoid
> digging all the information from the git history ? I've found that
> explaining hacks in more details helped me before when I had to rework
> the code years later.

I always like your comments :)

>
>                 /*
>                  * Some devices, such as the Grandstream GUV3100 and the LSK
>                  * Meeting Eye for Business & Home, exhibit entity ID collisions
I would not even Mention the LSK, they are abusing a vid:pid,.... up to you.
>                  * between units and streaming output terminals. Move streaming
>                  * output terminals to their own ID namespace by setting bit
>                  * UVC_TT_STREAMING (15), above the ID's 8-bit value. The bit is
UVC_TERM_OUTPUT
>                  * ignored in uvc_stream_for_terminal() when looking up the
>                  * streaming interface for the terminal.
>                  *
>                  * This hack is safe to enable unconditionally, asthe ID is not
as the
>                  * used for any other purpose (streaming output terminals have
>                  * no controls and are never referenced as sources in UVC
>                  * descriptors). Other types output terminals can have controls,
>                  * so limit usage of this separate namespace to streaming output
>                  * terminals.
>                  */
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
>
> > +             if (type & UVC_TT_STREAMING)
> > +                     id |= UVC_TERM_OUTPUT;
> > +
> >               term = uvc_alloc_new_entity(dev, type | UVC_TERM_OUTPUT,
> > -                                         buffer[3], 1, 0);
> > +                                         id, 1, 0);
> >               if (IS_ERR(term))
> >                       return PTR_ERR(term);
> >
> > @@ -2105,8 +2122,8 @@ static int uvc_register_terms(struct uvc_device *dev,
> >               if (UVC_ENTITY_TYPE(term) != UVC_TT_STREAMING)
> >                       continue;
> >
> > -             stream = uvc_stream_by_id(dev, term->id);
> > -             if (stream == NULL) {
> > +             stream = uvc_stream_for_terminal(dev, term);
> > +             if (!stream) {
> >                       dev_info(&dev->intf->dev,
> >                                "No streaming interface found for terminal %u.",
> >                                term->id);
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index ed7bad31f75ca474c1037d666d5310c78dd764df..3f2e832025e712585edc324afa6cad760d4edafc 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -41,7 +41,8 @@
> >  #define UVC_EXT_GPIO_UNIT            0x7ffe
> >  #define UVC_EXT_GPIO_UNIT_ID         0x100
> >
> > -#define UVC_INVALID_ENTITY_ID          0xffff
> > +#define UVC_HARDWARE_ENTITY_ID(id)   ((id) & 0xff)
> > +#define UVC_INVALID_ENTITY_ID                0xffff
> >
> >  /* ------------------------------------------------------------------------
> >   * Driver specific constants.
> >
> > ---
> > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Powered by blists - more mailing lists