Re: [PATCH] media: uvcvido: Include streaming interface number in debugfs dir name

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

Hi Torleiv,

Thank you for the patch.

In the subject line uvcvido should be written uvcvideo.

On Sun, Apr 28, 2019 at 07:21:13AM +0200, Torleiv Sundre wrote:
> uvcvideo creates a debugfs directory based on the device bus number and
> device number. If a device contains more than one uvc function, the
> creation of the second and following debugfs directories will fail and
> print an info message like this:
>   "uvcvideo: Unable to create debugfs 3-2 directory."
> 
> This patch includes the uvc streaming interface number in the debugfs
> directory name, to make sure it is unique. The directory name format is
> changed from "<busnum>-<devnum>" to "<busnum>-<devnum>-<intfnum>"

Good idea. Please see below for a few comments.

> Signed-off-by: Torleiv Sundre <torleiv@huddly.com>
> ---
>  drivers/media/usb/uvc/uvc_debugfs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_debugfs.c b/drivers/media/usb/uvc/uvc_debugfs.c
> index 77e7c2419b9b..6e244acb80ab 100644
> --- a/drivers/media/usb/uvc/uvc_debugfs.c
> +++ b/drivers/media/usb/uvc/uvc_debugfs.c
> @@ -84,7 +84,8 @@ void uvc_debugfs_init_stream(struct uvc_streaming *stream)
>  	if (uvc_debugfs_root_dir == NULL)
>  		return;
>  
> -	sprintf(dir_name, "%u-%u", udev->bus->busnum, udev->devnum);
> +	sprintf(dir_name, "%u-%u-%d", udev->bus->busnum, udev->devnum,
> +		stream->intfnum);

intfnum should never be negative, I would thus use %u instead of %d.
Furthermore, the dir_name buffer is 32 bytes long, so if something
really wrongs happen and the three variables end up being very large,
sprintf could overflow (10 chars for each value, 2 for the dashes, 1
for the ending \0). I propose extending dir_name to 33 bytes, and using
snprintf() instead of sprintf() to be on the safe side.

 {
 	struct usb_device *udev = stream->dev->udev;
 	struct dentry *dent;
-	char dir_name[32];
+	char dir_name[33];

 	if (uvc_debugfs_root_dir == NULL)
 		return;

-	sprintf(dir_name, "%u-%u", udev->bus->busnum, udev->devnum);
+	snprintf(dir_name, sizeof(dir_name), "%u-%u-%u", udev->bus->busnum,
+		 udev->devnum, stream->intfnum);

 	dent = debugfs_create_dir(dir_name, uvc_debugfs_root_dir);
 	if (IS_ERR_OR_NULL(dent)) {

With those changes,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

If you're fine with the changes there's no need to resubmit, I'll apply
the modified patch.

>  
>  	dent = debugfs_create_dir(dir_name, uvc_debugfs_root_dir);
>  	if (IS_ERR_OR_NULL(dent)) {

-- 
Regards,

Laurent Pinchart