Re: [PATCH] [media] uvcvideo: Add iFunction or iInterface to device names.

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

Hi Peter,

Thank you for the patch.

On Thursday 06 April 2017 13:58:25 Peter Boström wrote:
> Permits distinguishing between two /dev/videoX entries from the same
> physical UVC device (that naturally share the same iProduct name).
> 
> This change matches current Windows behavior by prioritizing iFunction
> over iInterface, but unlike Windows it displays both iProduct and
> iFunction/iInterface strings when both are available.
> 
> Signed-off-by: Peter Boström <pbos@google.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 43 ++++++++++++++++++++++++++++-------
>  drivers/media/usb/uvc/uvcvideo.h   |  4 +++-
>  2 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 04bf35063c4c..66adf8a77e56
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1998,6 +1998,8 @@ static int uvc_probe(struct usb_interface *intf,
>  {
>  	struct usb_device *udev = interface_to_usbdev(intf);
>  	struct uvc_device *dev;
> +	char additional_name_buf[UVC_DEVICE_NAME_SIZE];
> +	const char *additional_name = NULL;
>  	int ret;
> 
>  	if (id->idVendor && id->idProduct)
> @@ -2025,13 +2027,40 @@ static int uvc_probe(struct usb_interface *intf,
>  	dev->quirks = (uvc_quirks_param == -1)
>  		    ? id->driver_info : uvc_quirks_param;
> 
> -	if (udev->product != NULL)
> -		strlcpy(dev->name, udev->product, sizeof dev->name);
> -	else
> -		snprintf(dev->name, sizeof dev->name,
> -			"UVC Camera (%04x:%04x)",
> -			le16_to_cpu(udev->descriptor.idVendor),
> -			le16_to_cpu(udev->descriptor.idProduct));
> +	/*
> +	 * Add iFunction or iInterface to names when available as additional
> +	 * distinguishers between interfaces. iFunction is prioritized over
> +	 * iInterface which matches Windows behavior at the point of writing.
> +	 */
> +	if (intf->intf_assoc && intf->intf_assoc->iFunction != 0) {
> +		usb_string(udev, intf->intf_assoc->iFunction,
> +			   additional_name_buf, sizeof(additional_name_buf));
> +		additional_name = additional_name_buf;
> +	} else if (intf->cur_altsetting->desc.iInterface != 0) {
> +		usb_string(udev, intf->cur_altsetting->desc.iInterface,
> +			   additional_name_buf, sizeof(additional_name_buf));
> +		additional_name = additional_name_buf;
> +	}
> +
> +	if (additional_name) {
> +		if (udev->product) {
> +			snprintf(dev->name, sizeof(dev->name), "%s: %s",
> +				 udev->product, additional_name);
> +		} else {
> +			snprintf(dev->name, sizeof(dev->name),
> +				 "UVC Camera: %s (%04x:%04x)",
> +				 additional_name,
> +				 le16_to_cpu(udev->descriptor.idVendor),
> +				 le16_to_cpu(udev->descriptor.idProduct));
> +		}
> +	} else if (udev->product) {
> +		strlcpy(dev->name, udev->product, sizeof(dev->name));
> +	} else {
> +		snprintf(dev->name, sizeof(dev->name),
> +			 "UVC Camera (%04x:%04x)",
> +			 le16_to_cpu(udev->descriptor.idVendor),
> +			 le16_to_cpu(udev->descriptor.idProduct));
> +	}

This makes sense to me, but I think we could come up with a version of the
same code that wouldn't require the temporary 64 bytes buffer on the stack.
How about the following (untested) code ?

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 602256ffe14d..9b90a1ac5945 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2013,6 +2013,7 @@ static int uvc_probe(struct usb_interface *intf,
 {
 	struct usb_device *udev = interface_to_usbdev(intf);
 	struct uvc_device *dev;
+	int string;
 	int ret;
 
 	if (id->idVendor && id->idProduct)
@@ -2048,6 +2049,24 @@ static int uvc_probe(struct usb_interface *intf,
 			le16_to_cpu(udev->descriptor.idVendor),
 			le16_to_cpu(udev->descriptor.idProduct));
 
+	/*
+	 * Add iFunction or iInterface to names when available as additional
+	 * distinguishers between interfaces. iFunction is prioritized over
+	 * iInterface which matches Windows behavior at the point of writing.
+	 */
+	string = intf->intf_assoc ? intf->intf_assoc->iFunction
+	       : intf->cur_altsetting->desc.iInterface;
+	if (string != 0) {
+		size_t len;
+
+		strlcat(dev->name, ": ", sizeof(dev->name));
+		len = strlen(dev->name);
+
+		/* usb_string() accounts for the terminating NULL character. */
+		usb_string(udev, string, dev->name + len,
+			   sizeof(dev->name) - len);
+	}
+
 	/* Parse the Video Class control descriptor. */
 	if (uvc_parse_control(dev) < 0) {
 		uvc_trace(UVC_TRACE_PROBE, "Unable to parse UVC "
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 15e415e32c7f..f5bb42c6b023 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -556,7 +556,7 @@ struct uvc_device {
 	unsigned long warnings;
 	__u32 quirks;
 	int intfnum;
-	char name[32];
+	char name[64];
 
 	struct mutex lock;		/* Protects users */
 	unsigned int users;

>  	/* Parse the Video Class control descriptor. */
>  	if (uvc_parse_control(dev) < 0) {
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 4205e7a423f0..0cbedaee6e19 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -541,13 +541,15 @@ struct uvc_streaming {
>  	} clock;
>  };
> 
> +#define UVC_DEVICE_NAME_SIZE	64
> +
>  struct uvc_device {
>  	struct usb_device *udev;
>  	struct usb_interface *intf;
>  	unsigned long warnings;
>  	__u32 quirks;
>  	int intfnum;
> -	char name[32];
> +	char name[UVC_DEVICE_NAME_SIZE];
> 
>  	struct mutex lock;		/* Protects users */
>  	unsigned int users;

-- 
Regards,

Laurent Pinchart