Re: [PATCH] Revert "media: uvcvideo: Require entities to have a non-zero unique ID"

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

Hi Thadeu,

Thank you for the patch.

On Tue, Jan 14, 2025 at 05:00:45PM -0300, Thadeu Lima de Souza Cascardo wrote:
> This reverts commit 3dd075fe8ebbc6fcbf998f81a75b8c4b159a6195.
> 
> Tomasz has reported that his device, Generalplus Technology Inc. 808 Camera,
> with ID 1b3f:2002, stopped being detected:
> 
> $ ls -l /dev/video*
> zsh: no matches found: /dev/video*
> [    7.230599] usb 3-2: Found multiple Units with ID 5
> 
> This particular device is non-compliant, having both the Output Terminal
> and Processing Unit with ID 5. uvc_scan_fallback, though, is able to build
> a chain. However, when media elements are added and uvc_mc_create_links
> call uvc_entity_by_id, it will get the incorrect entity,
> media_create_pad_link will WARN, and it will fail to register the entities.
> 
> In order to reinstate support for such devices in a timely fashion,
> reverting the fix for these warnings is appropriate. A proper fix that
> considers the existence of such non-compliant devices will be submitted in
> a later development cycle.
> 
> Reported-by: Tomasz Sikora <sikora.tomus@gmail.com>
> Fixes: 3dd075fe8ebb ("media: uvcvideo: Require entities to have a non-zero unique ID")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

Cc: stable@vger.kernel.org
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 70 ++++++++++++------------------
>  1 file changed, 27 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index b3c8411dc05c..9febd2375636 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -775,27 +775,14 @@ static const u8 uvc_media_transport_input_guid[16] =
>  	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
>  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
>  
> -static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> -					       u16 id, unsigned int num_pads,
> -					       unsigned int extra_size)
> +static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
> +		unsigned int num_pads, unsigned int extra_size)
>  {
>  	struct uvc_entity *entity;
>  	unsigned int num_inputs;
>  	unsigned int size;
>  	unsigned int i;
>  
> -	/* Per UVC 1.1+ spec 3.7.2, the ID should be non-zero. */
> -	if (id == 0) {
> -		dev_err(&dev->udev->dev, "Found Unit with invalid ID 0.\n");
> -		return ERR_PTR(-EINVAL);
> -	}
> -
> -	/* 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);
> -	}
> -
>  	extra_size = roundup(extra_size, sizeof(*entity->pads));
>  	if (num_pads)
>  		num_inputs = type & UVC_TERM_OUTPUT ? num_pads : num_pads - 1;
> @@ -805,7 +792,7 @@ static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
>  	     + num_inputs;
>  	entity = kzalloc(size, GFP_KERNEL);
>  	if (entity == NULL)
> -		return ERR_PTR(-ENOMEM);
> +		return NULL;
>  
>  	entity->id = id;
>  	entity->type = type;
> @@ -917,10 +904,10 @@ static int uvc_parse_vendor_control(struct uvc_device *dev,
>  			break;
>  		}
>  
> -		unit = uvc_alloc_new_entity(dev, UVC_VC_EXTENSION_UNIT,
> -					    buffer[3], p + 1, 2 * n);
> -		if (IS_ERR(unit))
> -			return PTR_ERR(unit);
> +		unit = uvc_alloc_entity(UVC_VC_EXTENSION_UNIT, buffer[3],
> +					p + 1, 2*n);
> +		if (unit == NULL)
> +			return -ENOMEM;
>  
>  		memcpy(unit->guid, &buffer[4], 16);
>  		unit->extension.bNumControls = buffer[20];
> @@ -1029,10 +1016,10 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>  			return -EINVAL;
>  		}
>  
> -		term = uvc_alloc_new_entity(dev, type | UVC_TERM_INPUT,
> -					    buffer[3], 1, n + p);
> -		if (IS_ERR(term))
> -			return PTR_ERR(term);
> +		term = uvc_alloc_entity(type | UVC_TERM_INPUT, buffer[3],
> +					1, n + p);
> +		if (term == NULL)
> +			return -ENOMEM;
>  
>  		if (UVC_ENTITY_TYPE(term) == UVC_ITT_CAMERA) {
>  			term->camera.bControlSize = n;
> @@ -1088,10 +1075,10 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>  			return 0;
>  		}
>  
> -		term = uvc_alloc_new_entity(dev, type | UVC_TERM_OUTPUT,
> -					    buffer[3], 1, 0);
> -		if (IS_ERR(term))
> -			return PTR_ERR(term);
> +		term = uvc_alloc_entity(type | UVC_TERM_OUTPUT, buffer[3],
> +					1, 0);
> +		if (term == NULL)
> +			return -ENOMEM;
>  
>  		memcpy(term->baSourceID, &buffer[7], 1);
>  
> @@ -1110,10 +1097,9 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>  			return -EINVAL;
>  		}
>  
> -		unit = uvc_alloc_new_entity(dev, buffer[2], buffer[3],
> -					    p + 1, 0);
> -		if (IS_ERR(unit))
> -			return PTR_ERR(unit);
> +		unit = uvc_alloc_entity(buffer[2], buffer[3], p + 1, 0);
> +		if (unit == NULL)
> +			return -ENOMEM;
>  
>  		memcpy(unit->baSourceID, &buffer[5], p);
>  
> @@ -1133,9 +1119,9 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>  			return -EINVAL;
>  		}
>  
> -		unit = uvc_alloc_new_entity(dev, buffer[2], buffer[3], 2, n);
> -		if (IS_ERR(unit))
> -			return PTR_ERR(unit);
> +		unit = uvc_alloc_entity(buffer[2], buffer[3], 2, n);
> +		if (unit == NULL)
> +			return -ENOMEM;
>  
>  		memcpy(unit->baSourceID, &buffer[4], 1);
>  		unit->processing.wMaxMultiplier =
> @@ -1162,10 +1148,9 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>  			return -EINVAL;
>  		}
>  
> -		unit = uvc_alloc_new_entity(dev, buffer[2], buffer[3],
> -					    p + 1, n);
> -		if (IS_ERR(unit))
> -			return PTR_ERR(unit);
> +		unit = uvc_alloc_entity(buffer[2], buffer[3], p + 1, n);
> +		if (unit == NULL)
> +			return -ENOMEM;
>  
>  		memcpy(unit->guid, &buffer[4], 16);
>  		unit->extension.bNumControls = buffer[20];
> @@ -1305,10 +1290,9 @@ static int uvc_gpio_parse(struct uvc_device *dev)
>  		return dev_err_probe(&dev->udev->dev, irq,
>  				     "No IRQ for privacy GPIO\n");
>  
> -	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> -				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
> -	if (IS_ERR(unit))
> -		return PTR_ERR(unit);
> +	unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> +	if (!unit)
> +		return -ENOMEM;
>  
>  	unit->gpio.gpio_privacy = gpio_privacy;
>  	unit->gpio.irq = irq;

-- 
Regards,

Laurent Pinchart