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

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

[Peter Boström]

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));
+	}
 
 	/* 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;
-- 
2.12.2.715.g7642488e1d-goog

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

[Peter Boström]

I'll put some more info/discussion points inline. Very happy for
feedback/input here, thanks!

On Thu, Apr 6, 2017 at 1:58 PM Peter Boström <pbos@google.com> wrote:
>
> Permits distinguishing between two /dev/videoX entries from the same
> physical UVC device (that naturally share the same iProduct name).

The device under test has interface associations (and has iFunction
present, but not iInterface present). This device is enumerated as
"Camera Name: Interface Function" after this patch, instead of two
/dev/videoX entries showing up as "Camera Name" with different
functions not user visible (apart from lsusb). My tested "Logitech
Webcam C930e" shows no additional string (has only iProduct out of
these). I haven't tested any other devices (none with iInterface
present), and this device is still under development, so any
experience or input from interpretating the USB standard is
appreciated here.

> 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.

Windows only displays one of them, but I thought removing iProduct
from the string was scary. Do we want to match Windows for
consistency/keeping names short here, or is displaying both (which I
personally thinks make more sense) a good strategy here? "Should"
iFunction be expected to include the product name? Otherwise I think
it's fair to display both since something generic like "main video
feed" doesn't tie back to a specific device.

-------------------------------------------------------------------------
String descriptors present              |    Device function name used
-------------------------------------------------------------------------
iProduct                                |    iProduct
iProduct + iFunction                    |    iFunction
iProduct + iFunction + iInterface       |    iFunction
iProduct + iInterface                   |    iInterface
-------------------------------------------------------------------------


> 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));
> +       }
>
>         /* 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

Note that this expands the device name, and is used because I believe
iProduct + iFunction strings can reasonably extend the previous 32
character max.

> +
>  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;
> --
> 2.12.2.715.g7642488e1d-goog
>

Best,
- Peter

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

[Laurent Pinchart]

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