Re: [PATCH v3 1/1] media: uvcvideo: require entities to have a non-zero unique ID

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

On Thu, Jan 09, 2025 at 07:47:31AM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Wed, Jan 08, 2025 at 11:14:28PM +0100, Tomasz Sikora wrote:
> > Hello,
> > you right
> > I have in dmsg (line 1228):
> > [   12.981124] usb 3-2: Failed to create links for entity 5
> > [   12.981126] usb 3-2: Failed to register entities (-22).
> > 
> > full output in my log.
> 
> Thanks, Tomasz.
> 
> Can you test the attached fix? It should still keep the warning about the
> multiple units with the same ID, but now it would not return an error nor
> warn when registering the entities.
> 
> Cascardo.

> From f771f5c4657ed25ae36784bf13992ddbee3161e6 Mon Sep 17 00:00:00 2001
> From: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> Date: Thu, 9 Jan 2025 07:37:41 -0300
> Subject: [PATCH RFC] media: uvcvideo: restore support for non-compliant devices
> 
> Some real-world devices have multiple units with the same ID. When creating
> their media entities, it would lead to warnings and failure to create such
> entities. However, the V4L2 devices would still be created and work.
> 
> Restore their support, but still warn about the multiple units with the
> same ID. Avoid the failure in navigating through the chain by storing

What's "the failure" here ?

> pointers to the entities instead of only their IDs.

Missing SoB and Fixes tags.

The commit message should explain why this is better than reverting
3dd075fe8ebb ("media: uvcvideo: Require entities to have a non-zero
unique ID"). I'm wondering if a revert with a clean fix on top may not
be easier to review.

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 16 +++++++++++-----
>  drivers/media/usb/uvc/uvc_entity.c |  4 +++-
>  drivers/media/usb/uvc/uvcvideo.h   |  1 +
>  3 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 1a22364f7da9..dd81067f8d30 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -791,10 +791,8 @@ 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->udev->dev, "Found multiple Units with ID %u\n", id);
> -		return ERR_PTR(-EINVAL);
> -	}
> +	if (uvc_entity_by_id(dev, id))
> +		dev_warn(&dev->udev->dev, "Found multiple Units with ID %u\n", id);
>  
>  	extra_size = roundup(extra_size, sizeof(*entity->pads));
>  	if (num_pads)
> @@ -802,7 +800,7 @@ static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
>  	else
>  		num_inputs = 0;
>  	size = sizeof(*entity) + extra_size + sizeof(*entity->pads) * num_pads
> -	     + num_inputs;
> +	     + num_inputs + sizeof(struct uvc_entity *) * num_inputs;
>  	entity = kzalloc(size, GFP_KERNEL);
>  	if (entity == NULL)
>  		return ERR_PTR(-ENOMEM);
> @@ -840,6 +838,7 @@ static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
>  
>  	entity->bNrInPins = num_inputs;
>  	entity->baSourceID = (u8 *)(&entity->pads[num_pads]);
> +	entity->source_entities = (struct uvc_entity **)(&entity->baSourceID[num_inputs]);
>  
>  	return entity;
>  }
> @@ -1503,6 +1502,7 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
>  				}
>  
>  				forward->baSourceID[0] = source->id;
> +				forward->source_entities[0] = source;
>  			}
>  
>  			list_add_tail(&forward->chain, &chain->entities);
> @@ -1586,6 +1586,8 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
>  				return -EINVAL;
>  			}
>  
> +			entity->source_entities[i] = term;
> +
>  			uvc_dbg_cont(PROBE, " %d", term->id);
>  
>  			list_add_tail(&term->chain, &chain->entities);
> @@ -1620,6 +1622,8 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
>  		return -EINVAL;
>  	}
>  
> +	(*_entity)->source_entities[0] = entity;
> +
>  	*_entity = entity;
>  	return 0;
>  }
> @@ -1783,6 +1787,7 @@ static int uvc_scan_fallback(struct uvc_device *dev)
>  			goto error;
>  
>  		prev->baSourceID[0] = entity->id;
> +		prev->source_entities[0] = entity;
>  		prev = entity;
>  	}
>  
> @@ -1790,6 +1795,7 @@ static int uvc_scan_fallback(struct uvc_device *dev)
>  		goto error;
>  
>  	prev->baSourceID[0] = iterm->id;
> +	prev->source_entities[0] = iterm;
>  
>  	list_add_tail(&chain->list, &dev->chains);
>  
> diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
> index cc68dd24eb42..7f42292b7fde 100644
> --- a/drivers/media/usb/uvc/uvc_entity.c
> +++ b/drivers/media/usb/uvc/uvc_entity.c
> @@ -36,7 +36,9 @@ static int uvc_mc_create_links(struct uvc_video_chain *chain,
>  		if (!(entity->pads[i].flags & MEDIA_PAD_FL_SINK))
>  			continue;
>  
> -		remote = uvc_entity_by_id(chain->dev, entity->baSourceID[i]);
> +		remote = entity->source_entities[i];
> +		if (remote == NULL)
> +			remote = uvc_entity_by_id(chain->dev, entity->baSourceID[i]);

That looks worrying. Why would source_entities[i] be NULL ?

Devices with bad descriptors can lead to crashes, and it's important to
harden the code. Just warning about duplicate ideas and adding a
source_entities array that may or may not point to the right source (and
could point to NULL) doesn't seem to go in the right direction.

Other options include adding a device-specific quirk that overrides the
incorrect entity IDs, or, possibly better, implementing a heuristic to
fix those automatically.

>  		if (remote == NULL || remote->num_pads == 0)
>  			return -EINVAL;
>  
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 07f9921d83f2..a4ee79e4e85b 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -239,6 +239,7 @@ struct uvc_entity {
>  
>  	u8 bNrInPins;
>  	u8 *baSourceID;
> +	struct uvc_entity **source_entities;
>  
>  	int (*get_info)(struct uvc_device *dev, struct uvc_entity *entity,
>  			u8 cs, u8 *caps);

-- 
Regards,

Laurent Pinchart