[PATCH] media: uvcvideo: Create a specific id namespace for output entities

[PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Ricardo Ribalda]

Nothing can be connected from an output entity. Which means that no
other entity can reference an output entity as baSourceId.

Use this fact to move all the output entities to a different namespace
id.

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.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.

Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
---
 drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
 	return NULL;
 }
 
+#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
+
 static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
 {
 	struct uvc_streaming *stream;
 
+	id = ENTITY_HARDWARE_ID(id);
+
 	list_for_each_entry(stream, &dev->streams, list) {
 		if (stream->header.bTerminalLink == id)
 			return stream;
@@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
+		dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
+			ENTITY_HARDWARE_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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
 			return 0;
 		}
 
+		/*
+		 * Nothing can be connected from an output terminal. To avoid
+		 * entity-id's collisions in devices with invalid USB
+		 * descriptors, move the output terminal id to its own
+		 * namespace.
+		 */
+		id = buffer[3] | 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);
 

---
base-commit: ea299a2164262ff787c9d33f46049acccd120672
change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Laurent Pinchart]

On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> Nothing can be connected from an output entity. Which means that no

s/output entity/output terminal. Same below.

Did you mean s/from an/to an/ ?

> other entity can reference an output entity as baSourceId.
> 

Some output terminals have controls, so we need to preserve their ID.
That's why my proposal only set the UVC_TERM_OUTPUT bit for the
*streaming* output terminals, not for all output terminals.

> Use this fact to move all the output entities to a different namespace
> id.
> 
> 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.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> 
> Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
>  	return NULL;
>  }
>  
> +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)

This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
also & 0xff, as the UVC descriptors store IDs in 8-bit fields.

> +
>  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
>  {
>  	struct uvc_streaming *stream;
>  
> +	id = ENTITY_HARDWARE_ID(id);
> +
>  	list_for_each_entry(stream, &dev->streams, list) {
>  		if (stream->header.bTerminalLink == id)
>  			return stream;
> @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> +		dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> +			ENTITY_HARDWARE_ID(id));

It's not an error anymore if there's no collision of the full 16-bit ID,
right ? Should it be demoted to a dev_warn() ?

> +
> +	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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>  			return 0;
>  		}
>  
> +		/*
> +		 * Nothing can be connected from an output terminal. To avoid
> +		 * entity-id's collisions in devices with invalid USB
> +		 * descriptors, move the output terminal id to its own
> +		 * namespace.
> +		 */
> +		id = buffer[3] | 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);
>  
> 
> ---
> base-commit: ea299a2164262ff787c9d33f46049acccd120672
> change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Ricardo Ribalda]

On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > Nothing can be connected from an output entity. Which means that no
>
> s/output entity/output terminal. Same below.
>
> Did you mean s/from an/to an/ ?
>
> > other entity can reference an output entity as baSourceId.
> >
>
> Some output terminals have controls, so we need to preserve their ID.
> That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> *streaming* output terminals, not for all output terminals.
>
> > Use this fact to move all the output entities to a different namespace
> > id.
> >
> > 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.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> >
> > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> >       return NULL;
> >  }
> >
> > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
>
> This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
>
> > +
> >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> >  {
> >       struct uvc_streaming *stream;
> >
> > +     id = ENTITY_HARDWARE_ID(id);
> > +
> >       list_for_each_entry(stream, &dev->streams, list) {
> >               if (stream->header.bTerminalLink == id)
> >                       return stream;
> > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > +                     ENTITY_HARDWARE_ID(id));
>
> It's not an error anymore if there's no collision of the full 16-bit ID,
> right ? Should it be demoted to a dev_warn() ?

if it is OK with you I'd rather keep the dev_err(). If an ISP
manufacturer tests their camera in Linux I want them to really notice
that there is an error.
Besides that, I have implemented all your proposed changes.

I cannot test it until tomorrow in real hardware. But the changes are
trivial, let me know if I shall send the v2 right now or wait til it
is tested.

Regards

>
> > +
> > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> >                       return 0;
> >               }
> >
> > +             /*
> > +              * Nothing can be connected from an output terminal. To avoid
> > +              * entity-id's collisions in devices with invalid USB
> > +              * descriptors, move the output terminal id to its own
> > +              * namespace.
> > +              */
> > +             id = buffer[3] | 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);
> >
> >
> > ---
> > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Laurent Pinchart]

On Wed, Oct 22, 2025 at 02:29:15PM +0200, Ricardo Ribalda wrote:
> On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > > Nothing can be connected from an output entity. Which means that no
> >
> > s/output entity/output terminal. Same below.
> >
> > Did you mean s/from an/to an/ ?
> >
> > > other entity can reference an output entity as baSourceId.
> > >
> >
> > Some output terminals have controls, so we need to preserve their ID.
> > That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> > *streaming* output terminals, not for all output terminals.
> >
> > > Use this fact to move all the output entities to a different namespace
> > > id.
> > >
> > > 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.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> > >
> > > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> > >       return NULL;
> > >  }
> > >
> > > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
> >
> > This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> > also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
> >
> > > +
> > >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > >  {
> > >       struct uvc_streaming *stream;
> > >
> > > +     id = ENTITY_HARDWARE_ID(id);
> > > +

Another comment, I would have done this in the (single) caller, to keep
operating on real ids in this function. Or we could pass a struct
uvc_entity instead of an int id and rename the function to
uvc_stream_for_terminal(), which could better encapsulate the purpose.

> > >       list_for_each_entry(stream, &dev->streams, list) {
> > >               if (stream->header.bTerminalLink == id)
> > >                       return stream;
> > > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > > +                     ENTITY_HARDWARE_ID(id));
> >
> > It's not an error anymore if there's no collision of the full 16-bit ID,
> > right ? Should it be demoted to a dev_warn() ?
> 
> if it is OK with you I'd rather keep the dev_err(). If an ISP
> manufacturer tests their camera in Linux I want them to really notice
> that there is an error.

Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
that prints a message to the kernel log and posts messages on social
networks to blame the hardware manufacturer.

> Besides that, I have implemented all your proposed changes.
> 
> I cannot test it until tomorrow in real hardware. But the changes are
> trivial, let me know if I shall send the v2 right now or wait til it
> is tested.

Up to you, I don't mind either way.

If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
entity" first, do you plan to revert it to get this patch merged ?

> > > +
> > > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> > >                       return 0;
> > >               }
> > >
> > > +             /*
> > > +              * Nothing can be connected from an output terminal. To avoid
> > > +              * entity-id's collisions in devices with invalid USB
> > > +              * descriptors, move the output terminal id to its own
> > > +              * namespace.
> > > +              */
> > > +             id = buffer[3] | 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);
> > >
> > >
> > > ---
> > > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Ricardo Ribalda]

Hi Laurent

On Wed, 22 Oct 2025 at 14:49, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Oct 22, 2025 at 02:29:15PM +0200, Ricardo Ribalda wrote:
> > On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > > On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > > > Nothing can be connected from an output entity. Which means that no
> > >
> > > s/output entity/output terminal. Same below.
> > >
> > > Did you mean s/from an/to an/ ?
> > >
> > > > other entity can reference an output entity as baSourceId.
> > > >
> > >
> > > Some output terminals have controls, so we need to preserve their ID.
> > > That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> > > *streaming* output terminals, not for all output terminals.
> > >
> > > > Use this fact to move all the output entities to a different namespace
> > > > id.
> > > >
> > > > 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.
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> > > >
> > > > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> > > >       return NULL;
> > > >  }
> > > >
> > > > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
> > >
> > > This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> > > also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
> > >
> > > > +
> > > >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > > >  {
> > > >       struct uvc_streaming *stream;
> > > >
> > > > +     id = ENTITY_HARDWARE_ID(id);
> > > > +
>
> Another comment, I would have done this in the (single) caller, to keep
> operating on real ids in this function. Or we could pass a struct
> uvc_entity instead of an int id and rename the function to
> uvc_stream_for_terminal(), which could better encapsulate the purpose.

Like the second option better.

>
> > > >       list_for_each_entry(stream, &dev->streams, list) {
> > > >               if (stream->header.bTerminalLink == id)
> > > >                       return stream;
> > > > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > > > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > > > +                     ENTITY_HARDWARE_ID(id));
> > >
> > > It's not an error anymore if there's no collision of the full 16-bit ID,
> > > right ? Should it be demoted to a dev_warn() ?
> >
> > if it is OK with you I'd rather keep the dev_err(). If an ISP
> > manufacturer tests their camera in Linux I want them to really notice
> > that there is an error.
>
> Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
> that prints a message to the kernel log and posts messages on social
> networks to blame the hardware manufacturer.
>
> > Besides that, I have implemented all your proposed changes.
> >
> > I cannot test it until tomorrow in real hardware. But the changes are
> > trivial, let me know if I shall send the v2 right now or wait til it
> > is tested.
>
> Up to you, I don't mind either way.
>
> If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
> entity" first, do you plan to revert it to get this patch merged ?

I think they solve two different issues:

- Output terminal id collides with another entity id.
- Incorrect bTerminalLink


 We can have the two patches in.  If there is any conflict because we
land one and then the other I can send a v3 fixing the conflict. Or a
maintainer can do that, they should be trivial conflicts.

Regards



>
> > > > +
> > > > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> > > >                       return 0;
> > > >               }
> > > >
> > > > +             /*
> > > > +              * Nothing can be connected from an output terminal. To avoid
> > > > +              * entity-id's collisions in devices with invalid USB
> > > > +              * descriptors, move the output terminal id to its own
> > > > +              * namespace.
> > > > +              */
> > > > +             id = buffer[3] | 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);
> > > >
> > > >
> > > > ---
> > > > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > > > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Laurent Pinchart]

On Wed, Oct 22, 2025 at 03:08:58PM +0200, Ricardo Ribalda wrote:
> On Wed, 22 Oct 2025 at 14:49, Laurent Pinchart wrote:
> > On Wed, Oct 22, 2025 at 02:29:15PM +0200, Ricardo Ribalda wrote:
> > > On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > > > On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > > > > Nothing can be connected from an output entity. Which means that no
> > > >
> > > > s/output entity/output terminal. Same below.
> > > >
> > > > Did you mean s/from an/to an/ ?
> > > >
> > > > > other entity can reference an output entity as baSourceId.
> > > > >
> > > >
> > > > Some output terminals have controls, so we need to preserve their ID.
> > > > That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> > > > *streaming* output terminals, not for all output terminals.
> > > >
> > > > > Use this fact to move all the output entities to a different namespace
> > > > > id.
> > > > >
> > > > > 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.
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> > > > >
> > > > > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> > > > >       return NULL;
> > > > >  }
> > > > >
> > > > > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
> > > >
> > > > This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> > > > also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
> > > >
> > > > > +
> > > > >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > > > >  {
> > > > >       struct uvc_streaming *stream;
> > > > >
> > > > > +     id = ENTITY_HARDWARE_ID(id);
> > > > > +
> >
> > Another comment, I would have done this in the (single) caller, to keep
> > operating on real ids in this function. Or we could pass a struct
> > uvc_entity instead of an int id and rename the function to
> > uvc_stream_for_terminal(), which could better encapsulate the purpose.
> 
> Like the second option better.

I think I do too.

> > > > >       list_for_each_entry(stream, &dev->streams, list) {
> > > > >               if (stream->header.bTerminalLink == id)
> > > > >                       return stream;
> > > > > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > > > > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > > > > +                     ENTITY_HARDWARE_ID(id));
> > > >
> > > > It's not an error anymore if there's no collision of the full 16-bit ID,
> > > > right ? Should it be demoted to a dev_warn() ?
> > >
> > > if it is OK with you I'd rather keep the dev_err(). If an ISP
> > > manufacturer tests their camera in Linux I want them to really notice
> > > that there is an error.
> >
> > Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
> > that prints a message to the kernel log and posts messages on social
> > networks to blame the hardware manufacturer.
> >
> > > Besides that, I have implemented all your proposed changes.
> > >
> > > I cannot test it until tomorrow in real hardware. But the changes are
> > > trivial, let me know if I shall send the v2 right now or wait til it
> > > is tested.
> >
> > Up to you, I don't mind either way.
> >
> > If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
> > entity" first, do you plan to revert it to get this patch merged ?
> 
> I think they solve two different issues:
> 
> - Output terminal id collides with another entity id.
> - Incorrect bTerminalLink

Do we know of any device affected by that issue ?

>  We can have the two patches in.  If there is any conflict because we
> land one and then the other I can send a v3 fixing the conflict. Or a
> maintainer can do that, they should be trivial conflicts.
> 
> > > > > +
> > > > > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> > > > >                       return 0;
> > > > >               }
> > > > >
> > > > > +             /*
> > > > > +              * Nothing can be connected from an output terminal. To avoid
> > > > > +              * entity-id's collisions in devices with invalid USB
> > > > > +              * descriptors, move the output terminal id to its own
> > > > > +              * namespace.
> > > > > +              */
> > > > > +             id = buffer[3] | 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);
> > > > >
> > > > >
> > > > > ---
> > > > > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > > > > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Ricardo Ribalda]

On Wed, 22 Oct 2025 at 15:12, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Oct 22, 2025 at 03:08:58PM +0200, Ricardo Ribalda wrote:
> > On Wed, 22 Oct 2025 at 14:49, Laurent Pinchart wrote:
> > > On Wed, Oct 22, 2025 at 02:29:15PM +0200, Ricardo Ribalda wrote:
> > > > On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > > > > On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > > > > > Nothing can be connected from an output entity. Which means that no
> > > > >
> > > > > s/output entity/output terminal. Same below.
> > > > >
> > > > > Did you mean s/from an/to an/ ?
> > > > >
> > > > > > other entity can reference an output entity as baSourceId.
> > > > > >
> > > > >
> > > > > Some output terminals have controls, so we need to preserve their ID.
> > > > > That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> > > > > *streaming* output terminals, not for all output terminals.
> > > > >
> > > > > > Use this fact to move all the output entities to a different namespace
> > > > > > id.
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> > > > > >
> > > > > > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > > > > > ---
> > > > > >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> > > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> > > > > >       return NULL;
> > > > > >  }
> > > > > >
> > > > > > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
> > > > >
> > > > > This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> > > > > also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
> > > > >
> > > > > > +
> > > > > >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > > > > >  {
> > > > > >       struct uvc_streaming *stream;
> > > > > >
> > > > > > +     id = ENTITY_HARDWARE_ID(id);
> > > > > > +
> > >
> > > Another comment, I would have done this in the (single) caller, to keep
> > > operating on real ids in this function. Or we could pass a struct
> > > uvc_entity instead of an int id and rename the function to
> > > uvc_stream_for_terminal(), which could better encapsulate the purpose.
> >
> > Like the second option better.
>
> I think I do too.
>
> > > > > >       list_for_each_entry(stream, &dev->streams, list) {
> > > > > >               if (stream->header.bTerminalLink == id)
> > > > > >                       return stream;
> > > > > > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > > > > > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > > > > > +                     ENTITY_HARDWARE_ID(id));
> > > > >
> > > > > It's not an error anymore if there's no collision of the full 16-bit ID,
> > > > > right ? Should it be demoted to a dev_warn() ?
> > > >
> > > > if it is OK with you I'd rather keep the dev_err(). If an ISP
> > > > manufacturer tests their camera in Linux I want them to really notice
> > > > that there is an error.
> > >
> > > Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
> > > that prints a message to the kernel log and posts messages on social
> > > networks to blame the hardware manufacturer.
> > >
> > > > Besides that, I have implemented all your proposed changes.
> > > >
> > > > I cannot test it until tomorrow in real hardware. But the changes are
> > > > trivial, let me know if I shall send the v2 right now or wait til it
> > > > is tested.
> > >
> > > Up to you, I don't mind either way.
> > >
> > > If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
> > > entity" first, do you plan to revert it to get this patch merged ?
> >
> > I think they solve two different issues:
> >
> > - Output terminal id collides with another entity id.
> > - Incorrect bTerminalLink
>
> Do we know of any device affected by that issue ?

I bet you there is one :)

>
> >  We can have the two patches in.  If there is any conflict because we
> > land one and then the other I can send a v3 fixing the conflict. Or a
> > maintainer can do that, they should be trivial conflicts.
> >
> > > > > > +
> > > > > > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> > > > > >                       return 0;
> > > > > >               }
> > > > > >
> > > > > > +             /*
> > > > > > +              * Nothing can be connected from an output terminal. To avoid
> > > > > > +              * entity-id's collisions in devices with invalid USB
> > > > > > +              * descriptors, move the output terminal id to its own
> > > > > > +              * namespace.
> > > > > > +              */
> > > > > > +             id = buffer[3] | 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);
> > > > > >
> > > > > >
> > > > > > ---
> > > > > > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > > > > > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Laurent Pinchart]

On Wed, Oct 22, 2025 at 03:14:09PM +0200, Ricardo Ribalda wrote:
> On Wed, 22 Oct 2025 at 15:12, Laurent Pinchart wrote:
> > On Wed, Oct 22, 2025 at 03:08:58PM +0200, Ricardo Ribalda wrote:
> > > On Wed, 22 Oct 2025 at 14:49, Laurent Pinchart wrote:
> > > > On Wed, Oct 22, 2025 at 02:29:15PM +0200, Ricardo Ribalda wrote:
> > > > > On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > > > > > On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > > > > > > Nothing can be connected from an output entity. Which means that no
> > > > > >
> > > > > > s/output entity/output terminal. Same below.
> > > > > >
> > > > > > Did you mean s/from an/to an/ ?
> > > > > >
> > > > > > > other entity can reference an output entity as baSourceId.
> > > > > > >
> > > > > >
> > > > > > Some output terminals have controls, so we need to preserve their ID.
> > > > > > That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> > > > > > *streaming* output terminals, not for all output terminals.
> > > > > >
> > > > > > > Use this fact to move all the output entities to a different namespace
> > > > > > > id.
> > > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> > > > > > >
> > > > > > > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > > > > > > ---
> > > > > > >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> > > > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> > > > > > >       return NULL;
> > > > > > >  }
> > > > > > >
> > > > > > > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
> > > > > >
> > > > > > This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> > > > > > also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
> > > > > >
> > > > > > > +
> > > > > > >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > > > > > >  {
> > > > > > >       struct uvc_streaming *stream;
> > > > > > >
> > > > > > > +     id = ENTITY_HARDWARE_ID(id);
> > > > > > > +
> > > >
> > > > Another comment, I would have done this in the (single) caller, to keep
> > > > operating on real ids in this function. Or we could pass a struct
> > > > uvc_entity instead of an int id and rename the function to
> > > > uvc_stream_for_terminal(), which could better encapsulate the purpose.
> > >
> > > Like the second option better.
> >
> > I think I do too.
> >
> > > > > > >       list_for_each_entry(stream, &dev->streams, list) {
> > > > > > >               if (stream->header.bTerminalLink == id)
> > > > > > >                       return stream;
> > > > > > > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > > > > > > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > > > > > > +                     ENTITY_HARDWARE_ID(id));
> > > > > >
> > > > > > It's not an error anymore if there's no collision of the full 16-bit ID,
> > > > > > right ? Should it be demoted to a dev_warn() ?
> > > > >
> > > > > if it is OK with you I'd rather keep the dev_err(). If an ISP
> > > > > manufacturer tests their camera in Linux I want them to really notice
> > > > > that there is an error.
> > > >
> > > > Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
> > > > that prints a message to the kernel log and posts messages on social
> > > > networks to blame the hardware manufacturer.
> > > >
> > > > > Besides that, I have implemented all your proposed changes.
> > > > >
> > > > > I cannot test it until tomorrow in real hardware. But the changes are
> > > > > trivial, let me know if I shall send the v2 right now or wait til it
> > > > > is tested.
> > > >
> > > > Up to you, I don't mind either way.
> > > >
> > > > If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
> > > > entity" first, do you plan to revert it to get this patch merged ?
> > >
> > > I think they solve two different issues:
> > >
> > > - Output terminal id collides with another entity id.
> > > - Incorrect bTerminalLink
> >
> > Do we know of any device affected by that issue ?
> 
> I bet you there is one :)

I'd rather be cautious and address that if the issue arises. Enabling
non-compliant behaviour has the drawback of making issues less visible
to vendors, so I would prefer not working around problems unless we know
they exist.

> > >  We can have the two patches in.  If there is any conflict because we
> > > land one and then the other I can send a v3 fixing the conflict. Or a
> > > maintainer can do that, they should be trivial conflicts.
> > >
> > > > > > > +
> > > > > > > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> > > > > > >                       return 0;
> > > > > > >               }
> > > > > > >
> > > > > > > +             /*
> > > > > > > +              * Nothing can be connected from an output terminal. To avoid
> > > > > > > +              * entity-id's collisions in devices with invalid USB
> > > > > > > +              * descriptors, move the output terminal id to its own
> > > > > > > +              * namespace.
> > > > > > > +              */
> > > > > > > +             id = buffer[3] | 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);
> > > > > > >
> > > > > > >
> > > > > > > ---
> > > > > > > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > > > > > > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Ricardo Ribalda]

On Thu, 23 Oct 2025 at 13:25, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Oct 22, 2025 at 03:14:09PM +0200, Ricardo Ribalda wrote:
> > On Wed, 22 Oct 2025 at 15:12, Laurent Pinchart wrote:
> > > On Wed, Oct 22, 2025 at 03:08:58PM +0200, Ricardo Ribalda wrote:
> > > > On Wed, 22 Oct 2025 at 14:49, Laurent Pinchart wrote:
> > > > > On Wed, Oct 22, 2025 at 02:29:15PM +0200, Ricardo Ribalda wrote:
> > > > > > On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > > > > > > On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > > > > > > > Nothing can be connected from an output entity. Which means that no
> > > > > > >
> > > > > > > s/output entity/output terminal. Same below.
> > > > > > >
> > > > > > > Did you mean s/from an/to an/ ?
> > > > > > >
> > > > > > > > other entity can reference an output entity as baSourceId.
> > > > > > > >
> > > > > > >
> > > > > > > Some output terminals have controls, so we need to preserve their ID.
> > > > > > > That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> > > > > > > *streaming* output terminals, not for all output terminals.
> > > > > > >
> > > > > > > > Use this fact to move all the output entities to a different namespace
> > > > > > > > id.
> > > > > > > >
> > > > > > > > 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.
> > > > > > > >
> > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> > > > > > > >
> > > > > > > > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > > > > > > > ---
> > > > > > > >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> > > > > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> > > > > > > >       return NULL;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
> > > > > > >
> > > > > > > This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> > > > > > > also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
> > > > > > >
> > > > > > > > +
> > > > > > > >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > > > > > > >  {
> > > > > > > >       struct uvc_streaming *stream;
> > > > > > > >
> > > > > > > > +     id = ENTITY_HARDWARE_ID(id);
> > > > > > > > +
> > > > >
> > > > > Another comment, I would have done this in the (single) caller, to keep
> > > > > operating on real ids in this function. Or we could pass a struct
> > > > > uvc_entity instead of an int id and rename the function to
> > > > > uvc_stream_for_terminal(), which could better encapsulate the purpose.
> > > >
> > > > Like the second option better.
> > >
> > > I think I do too.
> > >
> > > > > > > >       list_for_each_entry(stream, &dev->streams, list) {
> > > > > > > >               if (stream->header.bTerminalLink == id)
> > > > > > > >                       return stream;
> > > > > > > > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > > > > > > > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > > > > > > > +                     ENTITY_HARDWARE_ID(id));
> > > > > > >
> > > > > > > It's not an error anymore if there's no collision of the full 16-bit ID,
> > > > > > > right ? Should it be demoted to a dev_warn() ?
> > > > > >
> > > > > > if it is OK with you I'd rather keep the dev_err(). If an ISP
> > > > > > manufacturer tests their camera in Linux I want them to really notice
> > > > > > that there is an error.
> > > > >
> > > > > Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
> > > > > that prints a message to the kernel log and posts messages on social
> > > > > networks to blame the hardware manufacturer.
> > > > >
> > > > > > Besides that, I have implemented all your proposed changes.
> > > > > >
> > > > > > I cannot test it until tomorrow in real hardware. But the changes are
> > > > > > trivial, let me know if I shall send the v2 right now or wait til it
> > > > > > is tested.
> > > > >
> > > > > Up to you, I don't mind either way.
> > > > >
> > > > > If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
> > > > > entity" first, do you plan to revert it to get this patch merged ?
> > > >
> > > > I think they solve two different issues:
> > > >
> > > > - Output terminal id collides with another entity id.
> > > > - Incorrect bTerminalLink
> > >
> > > Do we know of any device affected by that issue ?
> >
> > I bet you there is one :)
>
> I'd rather be cautious and address that if the issue arises. Enabling
> non-compliant behaviour has the drawback of making issues less visible
> to vendors, so I would prefer not working around problems unless we know
> they exist.

My main goal right now is to solve the regression.

If you think that this approach is best, please add your review-by and
the following tags to the patch. And let's start landing into upstream
asap.

We can discuss later if we want an extra patch or if we want to wait
for another report.


Reported-by: Angel4005 <ooara1337@gmail.com>
Closes: https://lore.kernel.org/linux-media/CAOzBiVuS7ygUjjhCbyWg-KiNx+HFTYnqH5+GJhd6cYsNLT=DaA@mail.gmail.com/
Fixes: 0e2ee70291e6 ("media: uvcvideo: Mark invalid entities with id
UVC_INVALID_ENTITY_ID")

Regards!



>
> > > >  We can have the two patches in.  If there is any conflict because we
> > > > land one and then the other I can send a v3 fixing the conflict. Or a
> > > > maintainer can do that, they should be trivial conflicts.
> > > >
> > > > > > > > +
> > > > > > > > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> > > > > > > >                       return 0;
> > > > > > > >               }
> > > > > > > >
> > > > > > > > +             /*
> > > > > > > > +              * Nothing can be connected from an output terminal. To avoid
> > > > > > > > +              * entity-id's collisions in devices with invalid USB
> > > > > > > > +              * descriptors, move the output terminal id to its own
> > > > > > > > +              * namespace.
> > > > > > > > +              */
> > > > > > > > +             id = buffer[3] | 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);
> > > > > > > >
> > > > > > > >
> > > > > > > > ---
> > > > > > > > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > > > > > > > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b
>
> --
> Regards,
>
> Laurent Pinchart



--
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Laurent Pinchart]

On Thu, Oct 23, 2025 at 01:47:39PM +0200, Ricardo Ribalda wrote:
> On Thu, 23 Oct 2025 at 13:25, Laurent Pinchart wrote:
> > On Wed, Oct 22, 2025 at 03:14:09PM +0200, Ricardo Ribalda wrote:
> > > On Wed, 22 Oct 2025 at 15:12, Laurent Pinchart wrote:
> > > > On Wed, Oct 22, 2025 at 03:08:58PM +0200, Ricardo Ribalda wrote:
> > > > > On Wed, 22 Oct 2025 at 14:49, Laurent Pinchart wrote:
> > > > > > On Wed, Oct 22, 2025 at 02:29:15PM +0200, Ricardo Ribalda wrote:
> > > > > > > On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > > > > > > > On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > > > > > > > > Nothing can be connected from an output entity. Which means that no
> > > > > > > >
> > > > > > > > s/output entity/output terminal. Same below.
> > > > > > > >
> > > > > > > > Did you mean s/from an/to an/ ?
> > > > > > > >
> > > > > > > > > other entity can reference an output entity as baSourceId.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Some output terminals have controls, so we need to preserve their ID.
> > > > > > > > That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> > > > > > > > *streaming* output terminals, not for all output terminals.
> > > > > > > >
> > > > > > > > > Use this fact to move all the output entities to a different namespace
> > > > > > > > > id.
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> > > > > > > > >
> > > > > > > > > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > > > > > > > > ---
> > > > > > > > >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> > > > > > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> > > > > > > > >       return NULL;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
> > > > > > > >
> > > > > > > > This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> > > > > > > > also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > > > > > > > >  {
> > > > > > > > >       struct uvc_streaming *stream;
> > > > > > > > >
> > > > > > > > > +     id = ENTITY_HARDWARE_ID(id);
> > > > > > > > > +
> > > > > >
> > > > > > Another comment, I would have done this in the (single) caller, to keep
> > > > > > operating on real ids in this function. Or we could pass a struct
> > > > > > uvc_entity instead of an int id and rename the function to
> > > > > > uvc_stream_for_terminal(), which could better encapsulate the purpose.
> > > > >
> > > > > Like the second option better.
> > > >
> > > > I think I do too.
> > > >
> > > > > > > > >       list_for_each_entry(stream, &dev->streams, list) {
> > > > > > > > >               if (stream->header.bTerminalLink == id)
> > > > > > > > >                       return stream;
> > > > > > > > > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > > > > > > > > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > > > > > > > > +                     ENTITY_HARDWARE_ID(id));
> > > > > > > >
> > > > > > > > It's not an error anymore if there's no collision of the full 16-bit ID,
> > > > > > > > right ? Should it be demoted to a dev_warn() ?
> > > > > > >
> > > > > > > if it is OK with you I'd rather keep the dev_err(). If an ISP
> > > > > > > manufacturer tests their camera in Linux I want them to really notice
> > > > > > > that there is an error.
> > > > > >
> > > > > > Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
> > > > > > that prints a message to the kernel log and posts messages on social
> > > > > > networks to blame the hardware manufacturer.
> > > > > >
> > > > > > > Besides that, I have implemented all your proposed changes.
> > > > > > >
> > > > > > > I cannot test it until tomorrow in real hardware. But the changes are
> > > > > > > trivial, let me know if I shall send the v2 right now or wait til it
> > > > > > > is tested.
> > > > > >
> > > > > > Up to you, I don't mind either way.
> > > > > >
> > > > > > If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
> > > > > > entity" first, do you plan to revert it to get this patch merged ?
> > > > >
> > > > > I think they solve two different issues:
> > > > >
> > > > > - Output terminal id collides with another entity id.
> > > > > - Incorrect bTerminalLink
> > > >
> > > > Do we know of any device affected by that issue ?
> > >
> > > I bet you there is one :)
> >
> > I'd rather be cautious and address that if the issue arises. Enabling
> > non-compliant behaviour has the drawback of making issues less visible
> > to vendors, so I would prefer not working around problems unless we know
> > they exist.
> 
> My main goal right now is to solve the regression.
> 
> If you think that this approach is best, please add your review-by and
> the following tags to the patch. And let's start landing into upstream
> asap.

I've done so in v2. Could you check if you're OK with the proposed
changes for the commit message and comment ? I'll then send a v3 with
all the tags (or you can do so yourself to ack the proposed changes),
and Hans or Mauro can pick the patch up as a fix.

Thanks for testing the proposed change, I appreciate it.

> We can discuss later if we want an extra patch or if we want to wait
> for another report.
> 
> Reported-by: Angel4005 <ooara1337@gmail.com>
> Closes: https://lore.kernel.org/linux-media/CAOzBiVuS7ygUjjhCbyWg-KiNx+HFTYnqH5+GJhd6cYsNLT=DaA@mail.gmail.com/
> Fixes: 0e2ee70291e6 ("media: uvcvideo: Mark invalid entities with id
> UVC_INVALID_ENTITY_ID")
> 
> > > > >  We can have the two patches in.  If there is any conflict because we
> > > > > land one and then the other I can send a v3 fixing the conflict. Or a
> > > > > maintainer can do that, they should be trivial conflicts.
> > > > >
> > > > > > > > > +
> > > > > > > > > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> > > > > > > > >                       return 0;
> > > > > > > > >               }
> > > > > > > > >
> > > > > > > > > +             /*
> > > > > > > > > +              * Nothing can be connected from an output terminal. To avoid
> > > > > > > > > +              * entity-id's collisions in devices with invalid USB
> > > > > > > > > +              * descriptors, move the output terminal id to its own
> > > > > > > > > +              * namespace.
> > > > > > > > > +              */
> > > > > > > > > +             id = buffer[3] | 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);
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > ---
> > > > > > > > > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > > > > > > > > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Ricardo Ribalda]

Hi Laurent

On Fri, 24 Oct 2025 at 12:54, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Thu, Oct 23, 2025 at 01:47:39PM +0200, Ricardo Ribalda wrote:
> > On Thu, 23 Oct 2025 at 13:25, Laurent Pinchart wrote:
> > > On Wed, Oct 22, 2025 at 03:14:09PM +0200, Ricardo Ribalda wrote:
> > > > On Wed, 22 Oct 2025 at 15:12, Laurent Pinchart wrote:
> > > > > On Wed, Oct 22, 2025 at 03:08:58PM +0200, Ricardo Ribalda wrote:
> > > > > > On Wed, 22 Oct 2025 at 14:49, Laurent Pinchart wrote:
> > > > > > > On Wed, Oct 22, 2025 at 02:29:15PM +0200, Ricardo Ribalda wrote:
> > > > > > > > On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > > > > > > > > On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > > > > > > > > > Nothing can be connected from an output entity. Which means that no
> > > > > > > > >
> > > > > > > > > s/output entity/output terminal. Same below.
> > > > > > > > >
> > > > > > > > > Did you mean s/from an/to an/ ?
> > > > > > > > >
> > > > > > > > > > other entity can reference an output entity as baSourceId.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Some output terminals have controls, so we need to preserve their ID.
> > > > > > > > > That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> > > > > > > > > *streaming* output terminals, not for all output terminals.
> > > > > > > > >
> > > > > > > > > > Use this fact to move all the output entities to a different namespace
> > > > > > > > > > id.
> > > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> > > > > > > > > >
> > > > > > > > > > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > > > > > > > > > ---
> > > > > > > > > >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> > > > > > > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > > > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> > > > > > > > > >       return NULL;
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
> > > > > > > > >
> > > > > > > > > This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> > > > > > > > > also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
> > > > > > > > >
> > > > > > > > > > +
> > > > > > > > > >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > > > > > > > > >  {
> > > > > > > > > >       struct uvc_streaming *stream;
> > > > > > > > > >
> > > > > > > > > > +     id = ENTITY_HARDWARE_ID(id);
> > > > > > > > > > +
> > > > > > >
> > > > > > > Another comment, I would have done this in the (single) caller, to keep
> > > > > > > operating on real ids in this function. Or we could pass a struct
> > > > > > > uvc_entity instead of an int id and rename the function to
> > > > > > > uvc_stream_for_terminal(), which could better encapsulate the purpose.
> > > > > >
> > > > > > Like the second option better.
> > > > >
> > > > > I think I do too.
> > > > >
> > > > > > > > > >       list_for_each_entry(stream, &dev->streams, list) {
> > > > > > > > > >               if (stream->header.bTerminalLink == id)
> > > > > > > > > >                       return stream;
> > > > > > > > > > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > > > > > > > > > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > > > > > > > > > +                     ENTITY_HARDWARE_ID(id));
> > > > > > > > >
> > > > > > > > > It's not an error anymore if there's no collision of the full 16-bit ID,
> > > > > > > > > right ? Should it be demoted to a dev_warn() ?
> > > > > > > >
> > > > > > > > if it is OK with you I'd rather keep the dev_err(). If an ISP
> > > > > > > > manufacturer tests their camera in Linux I want them to really notice
> > > > > > > > that there is an error.
> > > > > > >
> > > > > > > Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
> > > > > > > that prints a message to the kernel log and posts messages on social
> > > > > > > networks to blame the hardware manufacturer.
> > > > > > >
> > > > > > > > Besides that, I have implemented all your proposed changes.
> > > > > > > >
> > > > > > > > I cannot test it until tomorrow in real hardware. But the changes are
> > > > > > > > trivial, let me know if I shall send the v2 right now or wait til it
> > > > > > > > is tested.
> > > > > > >
> > > > > > > Up to you, I don't mind either way.
> > > > > > >
> > > > > > > If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
> > > > > > > entity" first, do you plan to revert it to get this patch merged ?
> > > > > >
> > > > > > I think they solve two different issues:
> > > > > >
> > > > > > - Output terminal id collides with another entity id.
> > > > > > - Incorrect bTerminalLink
> > > > >
> > > > > Do we know of any device affected by that issue ?
> > > >
> > > > I bet you there is one :)
> > >
> > > I'd rather be cautious and address that if the issue arises. Enabling
> > > non-compliant behaviour has the drawback of making issues less visible
> > > to vendors, so I would prefer not working around problems unless we know
> > > they exist.
> >
> > My main goal right now is to solve the regression.
> >
> > If you think that this approach is best, please add your review-by and
> > the following tags to the patch. And let's start landing into upstream
> > asap.
>
> I've done so in v2. Could you check if you're OK with the proposed
> changes for the commit message and comment ? I'll then send a v3 with
> all the tags (or you can do so yourself to ack the proposed changes),
> and Hans or Mauro can pick the patch up as a fix.

I am fine with you sending the v3.

HansV has already landed the other patch in /fixes

So I think the best approach here is:

1) Wait until the fix is in media-commiters/next (*)
2) Add a revert in uvc/next
3) Add this patch in uvc/next


(*) If after 1 you have not posted v3 I will do it with your changes and tags.

Regards!

>
> Thanks for testing the proposed change, I appreciate it.
>
> > We can discuss later if we want an extra patch or if we want to wait
> > for another report.
> >
> > Reported-by: Angel4005 <ooara1337@gmail.com>
> > Closes: https://lore.kernel.org/linux-media/CAOzBiVuS7ygUjjhCbyWg-KiNx+HFTYnqH5+GJhd6cYsNLT=DaA@mail.gmail.com/
> > Fixes: 0e2ee70291e6 ("media: uvcvideo: Mark invalid entities with id
> > UVC_INVALID_ENTITY_ID")
> >
> > > > > >  We can have the two patches in.  If there is any conflict because we
> > > > > > land one and then the other I can send a v3 fixing the conflict. Or a
> > > > > > maintainer can do that, they should be trivial conflicts.
> > > > > >
> > > > > > > > > > +
> > > > > > > > > > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> > > > > > > > > >                       return 0;
> > > > > > > > > >               }
> > > > > > > > > >
> > > > > > > > > > +             /*
> > > > > > > > > > +              * Nothing can be connected from an output terminal. To avoid
> > > > > > > > > > +              * entity-id's collisions in devices with invalid USB
> > > > > > > > > > +              * descriptors, move the output terminal id to its own
> > > > > > > > > > +              * namespace.
> > > > > > > > > > +              */
> > > > > > > > > > +             id = buffer[3] | 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);
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > > > > > > > > > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Laurent Pinchart]

On Fri, Oct 24, 2025 at 01:00:53PM +0200, Ricardo Ribalda wrote:
> On Fri, 24 Oct 2025 at 12:54, Laurent Pinchart wrote:
> > On Thu, Oct 23, 2025 at 01:47:39PM +0200, Ricardo Ribalda wrote:
> > > On Thu, 23 Oct 2025 at 13:25, Laurent Pinchart wrote:
> > > > On Wed, Oct 22, 2025 at 03:14:09PM +0200, Ricardo Ribalda wrote:
> > > > > On Wed, 22 Oct 2025 at 15:12, Laurent Pinchart wrote:
> > > > > > On Wed, Oct 22, 2025 at 03:08:58PM +0200, Ricardo Ribalda wrote:
> > > > > > > On Wed, 22 Oct 2025 at 14:49, Laurent Pinchart wrote:
> > > > > > > > On Wed, Oct 22, 2025 at 02:29:15PM +0200, Ricardo Ribalda wrote:
> > > > > > > > > On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > > > > > > > > > On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > > > > > > > > > > Nothing can be connected from an output entity. Which means that no
> > > > > > > > > >
> > > > > > > > > > s/output entity/output terminal. Same below.
> > > > > > > > > >
> > > > > > > > > > Did you mean s/from an/to an/ ?
> > > > > > > > > >
> > > > > > > > > > > other entity can reference an output entity as baSourceId.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Some output terminals have controls, so we need to preserve their ID.
> > > > > > > > > > That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> > > > > > > > > > *streaming* output terminals, not for all output terminals.
> > > > > > > > > >
> > > > > > > > > > > Use this fact to move all the output entities to a different namespace
> > > > > > > > > > > id.
> > > > > > > > > > >
> > > > > > > > > > > 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.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> > > > > > > > > > >
> > > > > > > > > > > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> > > > > > > > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > > > > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> > > > > > > > > > >       return NULL;
> > > > > > > > > > >  }
> > > > > > > > > > >
> > > > > > > > > > > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
> > > > > > > > > >
> > > > > > > > > > This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> > > > > > > > > > also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
> > > > > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > > > > > > > > > >  {
> > > > > > > > > > >       struct uvc_streaming *stream;
> > > > > > > > > > >
> > > > > > > > > > > +     id = ENTITY_HARDWARE_ID(id);
> > > > > > > > > > > +
> > > > > > > >
> > > > > > > > Another comment, I would have done this in the (single) caller, to keep
> > > > > > > > operating on real ids in this function. Or we could pass a struct
> > > > > > > > uvc_entity instead of an int id and rename the function to
> > > > > > > > uvc_stream_for_terminal(), which could better encapsulate the purpose.
> > > > > > >
> > > > > > > Like the second option better.
> > > > > >
> > > > > > I think I do too.
> > > > > >
> > > > > > > > > > >       list_for_each_entry(stream, &dev->streams, list) {
> > > > > > > > > > >               if (stream->header.bTerminalLink == id)
> > > > > > > > > > >                       return stream;
> > > > > > > > > > > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > > > > > > > > > > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > > > > > > > > > > +                     ENTITY_HARDWARE_ID(id));
> > > > > > > > > >
> > > > > > > > > > It's not an error anymore if there's no collision of the full 16-bit ID,
> > > > > > > > > > right ? Should it be demoted to a dev_warn() ?
> > > > > > > > >
> > > > > > > > > if it is OK with you I'd rather keep the dev_err(). If an ISP
> > > > > > > > > manufacturer tests their camera in Linux I want them to really notice
> > > > > > > > > that there is an error.
> > > > > > > >
> > > > > > > > Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
> > > > > > > > that prints a message to the kernel log and posts messages on social
> > > > > > > > networks to blame the hardware manufacturer.
> > > > > > > >
> > > > > > > > > Besides that, I have implemented all your proposed changes.
> > > > > > > > >
> > > > > > > > > I cannot test it until tomorrow in real hardware. But the changes are
> > > > > > > > > trivial, let me know if I shall send the v2 right now or wait til it
> > > > > > > > > is tested.
> > > > > > > >
> > > > > > > > Up to you, I don't mind either way.
> > > > > > > >
> > > > > > > > If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
> > > > > > > > entity" first, do you plan to revert it to get this patch merged ?
> > > > > > >
> > > > > > > I think they solve two different issues:
> > > > > > >
> > > > > > > - Output terminal id collides with another entity id.
> > > > > > > - Incorrect bTerminalLink
> > > > > >
> > > > > > Do we know of any device affected by that issue ?
> > > > >
> > > > > I bet you there is one :)
> > > >
> > > > I'd rather be cautious and address that if the issue arises. Enabling
> > > > non-compliant behaviour has the drawback of making issues less visible
> > > > to vendors, so I would prefer not working around problems unless we know
> > > > they exist.
> > >
> > > My main goal right now is to solve the regression.
> > >
> > > If you think that this approach is best, please add your review-by and
> > > the following tags to the patch. And let's start landing into upstream
> > > asap.
> >
> > I've done so in v2. Could you check if you're OK with the proposed
> > changes for the commit message and comment ? I'll then send a v3 with
> > all the tags (or you can do so yourself to ack the proposed changes),
> > and Hans or Mauro can pick the patch up as a fix.
> 
> I am fine with you sending the v3.
> 
> HansV has already landed the other patch in /fixes
> 
> So I think the best approach here is:
> 
> 1) Wait until the fix is in media-commiters/next (*)
> 2) Add a revert in uvc/next
> 3) Add this patch in uvc/next

The fix hasn't been merged in v6.18-rc3 yet. If it doesn't land in -rc4
we'll need a different strategy.

> (*) If after 1 you have not posted v3 I will do it with your changes and tags.

I've just posted v3.

> > Thanks for testing the proposed change, I appreciate it.
> >
> > > We can discuss later if we want an extra patch or if we want to wait
> > > for another report.
> > >
> > > Reported-by: Angel4005 <ooara1337@gmail.com>
> > > Closes: https://lore.kernel.org/linux-media/CAOzBiVuS7ygUjjhCbyWg-KiNx+HFTYnqH5+GJhd6cYsNLT=DaA@mail.gmail.com/
> > > Fixes: 0e2ee70291e6 ("media: uvcvideo: Mark invalid entities with id
> > > UVC_INVALID_ENTITY_ID")
> > >
> > > > > > >  We can have the two patches in.  If there is any conflict because we
> > > > > > > land one and then the other I can send a v3 fixing the conflict. Or a
> > > > > > > maintainer can do that, they should be trivial conflicts.
> > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> > > > > > > > > > >                       return 0;
> > > > > > > > > > >               }
> > > > > > > > > > >
> > > > > > > > > > > +             /*
> > > > > > > > > > > +              * Nothing can be connected from an output terminal. To avoid
> > > > > > > > > > > +              * entity-id's collisions in devices with invalid USB
> > > > > > > > > > > +              * descriptors, move the output terminal id to its own
> > > > > > > > > > > +              * namespace.
> > > > > > > > > > > +              */
> > > > > > > > > > > +             id = buffer[3] | 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);
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > > > > > > > > > > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

[Laurent Pinchart]

On Mon, Oct 27, 2025 at 04:46:50AM +0200, Laurent Pinchart wrote:
> On Fri, Oct 24, 2025 at 01:00:53PM +0200, Ricardo Ribalda wrote:
> > On Fri, 24 Oct 2025 at 12:54, Laurent Pinchart wrote:
> > > On Thu, Oct 23, 2025 at 01:47:39PM +0200, Ricardo Ribalda wrote:
> > > > On Thu, 23 Oct 2025 at 13:25, Laurent Pinchart wrote:
> > > > > On Wed, Oct 22, 2025 at 03:14:09PM +0200, Ricardo Ribalda wrote:
> > > > > > On Wed, 22 Oct 2025 at 15:12, Laurent Pinchart wrote:
> > > > > > > On Wed, Oct 22, 2025 at 03:08:58PM +0200, Ricardo Ribalda wrote:
> > > > > > > > On Wed, 22 Oct 2025 at 14:49, Laurent Pinchart wrote:
> > > > > > > > > On Wed, Oct 22, 2025 at 02:29:15PM +0200, Ricardo Ribalda wrote:
> > > > > > > > > > On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > > > > > > > > > > On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > > > > > > > > > > > Nothing can be connected from an output entity. Which means that no
> > > > > > > > > > >
> > > > > > > > > > > s/output entity/output terminal. Same below.
> > > > > > > > > > >
> > > > > > > > > > > Did you mean s/from an/to an/ ?
> > > > > > > > > > >
> > > > > > > > > > > > other entity can reference an output entity as baSourceId.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Some output terminals have controls, so we need to preserve their ID.
> > > > > > > > > > > That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> > > > > > > > > > > *streaming* output terminals, not for all output terminals.
> > > > > > > > > > >
> > > > > > > > > > > > Use this fact to move all the output entities to a different namespace
> > > > > > > > > > > > id.
> > > > > > > > > > > >
> > > > > > > > > > > > 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.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.
> > > > > > > > > > > >
> > > > > > > > > > > > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > > > > > > > > > > > ---
> > > > > > > > > > > >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> > > > > > > > > > > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > > > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > > > > > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > > > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> > > > > > > > > > > >       return NULL;
> > > > > > > > > > > >  }
> > > > > > > > > > > >
> > > > > > > > > > > > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
> > > > > > > > > > >
> > > > > > > > > > > This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> > > > > > > > > > > also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
> > > > > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > > > > > > > > > > >  {
> > > > > > > > > > > >       struct uvc_streaming *stream;
> > > > > > > > > > > >
> > > > > > > > > > > > +     id = ENTITY_HARDWARE_ID(id);
> > > > > > > > > > > > +
> > > > > > > > >
> > > > > > > > > Another comment, I would have done this in the (single) caller, to keep
> > > > > > > > > operating on real ids in this function. Or we could pass a struct
> > > > > > > > > uvc_entity instead of an int id and rename the function to
> > > > > > > > > uvc_stream_for_terminal(), which could better encapsulate the purpose.
> > > > > > > >
> > > > > > > > Like the second option better.
> > > > > > >
> > > > > > > I think I do too.
> > > > > > >
> > > > > > > > > > > >       list_for_each_entry(stream, &dev->streams, list) {
> > > > > > > > > > > >               if (stream->header.bTerminalLink == id)
> > > > > > > > > > > >                       return stream;
> > > > > > > > > > > > @@ -810,10 +814,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, ENTITY_HARDWARE_ID(id)))
> > > > > > > > > > > > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > > > > > > > > > > > +                     ENTITY_HARDWARE_ID(id));
> > > > > > > > > > >
> > > > > > > > > > > It's not an error anymore if there's no collision of the full 16-bit ID,
> > > > > > > > > > > right ? Should it be demoted to a dev_warn() ?
> > > > > > > > > >
> > > > > > > > > > if it is OK with you I'd rather keep the dev_err(). If an ISP
> > > > > > > > > > manufacturer tests their camera in Linux I want them to really notice
> > > > > > > > > > that there is an error.
> > > > > > > > >
> > > > > > > > > Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
> > > > > > > > > that prints a message to the kernel log and posts messages on social
> > > > > > > > > networks to blame the hardware manufacturer.
> > > > > > > > >
> > > > > > > > > > Besides that, I have implemented all your proposed changes.
> > > > > > > > > >
> > > > > > > > > > I cannot test it until tomorrow in real hardware. But the changes are
> > > > > > > > > > trivial, let me know if I shall send the v2 right now or wait til it
> > > > > > > > > > is tested.
> > > > > > > > >
> > > > > > > > > Up to you, I don't mind either way.
> > > > > > > > >
> > > > > > > > > If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
> > > > > > > > > entity" first, do you plan to revert it to get this patch merged ?
> > > > > > > >
> > > > > > > > I think they solve two different issues:
> > > > > > > >
> > > > > > > > - Output terminal id collides with another entity id.
> > > > > > > > - Incorrect bTerminalLink
> > > > > > >
> > > > > > > Do we know of any device affected by that issue ?
> > > > > >
> > > > > > I bet you there is one :)
> > > > >
> > > > > I'd rather be cautious and address that if the issue arises. Enabling
> > > > > non-compliant behaviour has the drawback of making issues less visible
> > > > > to vendors, so I would prefer not working around problems unless we know
> > > > > they exist.
> > > >
> > > > My main goal right now is to solve the regression.
> > > >
> > > > If you think that this approach is best, please add your review-by and
> > > > the following tags to the patch. And let's start landing into upstream
> > > > asap.
> > >
> > > I've done so in v2. Could you check if you're OK with the proposed
> > > changes for the commit message and comment ? I'll then send a v3 with
> > > all the tags (or you can do so yourself to ack the proposed changes),
> > > and Hans or Mauro can pick the patch up as a fix.
> > 
> > I am fine with you sending the v3.
> > 
> > HansV has already landed the other patch in /fixes
> > 
> > So I think the best approach here is:
> > 
> > 1) Wait until the fix is in media-commiters/next (*)
> > 2) Add a revert in uvc/next
> > 3) Add this patch in uvc/next
> 
> The fix hasn't been merged in v6.18-rc3 yet. If it doesn't land in -rc4
> we'll need a different strategy.
> 
> > (*) If after 1 you have not posted v3 I will do it with your changes and tags.
> 
> I've just posted v3.

Which I forgot to tag as v3 :-( So sorry about that.

https://lore.kernel.org/linux-media/20251027024640.7205-1-laurent.pinchart@ideasonboard.com/T/#u

> > > Thanks for testing the proposed change, I appreciate it.
> > >
> > > > We can discuss later if we want an extra patch or if we want to wait
> > > > for another report.
> > > >
> > > > Reported-by: Angel4005 <ooara1337@gmail.com>
> > > > Closes: https://lore.kernel.org/linux-media/CAOzBiVuS7ygUjjhCbyWg-KiNx+HFTYnqH5+GJhd6cYsNLT=DaA@mail.gmail.com/
> > > > Fixes: 0e2ee70291e6 ("media: uvcvideo: Mark invalid entities with id
> > > > UVC_INVALID_ENTITY_ID")
> > > >
> > > > > > > >  We can have the two patches in.  If there is any conflict because we
> > > > > > > > land one and then the other I can send a v3 fixing the conflict. Or a
> > > > > > > > maintainer can do that, they should be trivial conflicts.
> > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > +     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 +975,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 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> > > > > > > > > > > >                       return 0;
> > > > > > > > > > > >               }
> > > > > > > > > > > >
> > > > > > > > > > > > +             /*
> > > > > > > > > > > > +              * Nothing can be connected from an output terminal. To avoid
> > > > > > > > > > > > +              * entity-id's collisions in devices with invalid USB
> > > > > > > > > > > > +              * descriptors, move the output terminal id to its own
> > > > > > > > > > > > +              * namespace.
> > > > > > > > > > > > +              */
> > > > > > > > > > > > +             id = buffer[3] | 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);
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > ---
> > > > > > > > > > > > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > > > > > > > > > > > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b

-- 
Regards,

Laurent Pinchart