Re: [PATCH v3 07/12] usb: misc: isight_firmware: update to use usb_control_msg_send()

[Johan Hovold]

On Wed, Jan 27, 2021 at 12:03:58AM +0530, Anant Thazhemadam wrote:
> The newer usb_control_msg_{send|recv}() API are an improvement on the
> existing usb_control_msg() as it ensures that a short read/write is treated
> as an error, data can be used off the stack, and raw usb pipes need not be
> created in the calling functions.
> For this reason, the instances of usb_control_msg() have been replaced with
> usb_control_msg_send(), and return value checking has also been
> appropriately enforced.
> 
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@xxxxxxxxx>
> ---
>  drivers/usb/misc/isight_firmware.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/misc/isight_firmware.c b/drivers/usb/misc/isight_firmware.c
> index 4d30095d6ad2..1bd14a431f6c 100644
> --- a/drivers/usb/misc/isight_firmware.c
> +++ b/drivers/usb/misc/isight_firmware.c
> @@ -37,13 +37,10 @@ static int isight_firmware_load(struct usb_interface *intf,
>  	struct usb_device *dev = interface_to_usbdev(intf);
>  	int llen, len, req, ret = 0;
>  	const struct firmware *firmware;
> -	unsigned char *buf = kmalloc(50, GFP_KERNEL);
> +	unsigned char buf[50];

This buffer is probably large enough to not want to have it allocated on
the stack.

>  	unsigned char data[4];
>  	const u8 *ptr;
>  
> -	if (!buf)
> -		return -ENOMEM;
> -
>  	if (request_firmware(&firmware, "isight.fw", &dev->dev) != 0) {
>  		printk(KERN_ERR "Unable to load isight firmware\n");
>  		ret = -ENODEV;
> @@ -53,11 +50,11 @@ static int isight_firmware_load(struct usb_interface *intf,
>  	ptr = firmware->data;
>  
>  	buf[0] = 0x01;
> -	if (usb_control_msg
> -	    (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, 0xe600, 0, buf, 1,
> -	     300) != 1) {
> +	ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, 0xe600,
> +				   0, &buf, 1, 300, GFP_KERNEL);
> +	if (ret != 0) {
>  		printk(KERN_ERR
> -		       "Failed to initialise isight firmware loader\n");
> +			"Failed to initialise isight firmware loader\n");
>  		ret = -ENODEV;
>  		goto out;
>  	}
> @@ -82,15 +79,15 @@ static int isight_firmware_load(struct usb_interface *intf,
>  				ret = -ENODEV;
>  				goto out;
>  			}
> -			memcpy(buf, ptr, llen);
> +			memcpy(&buf, ptr, llen);
>  
>  			ptr += llen;
>  
> -			if (usb_control_msg
> -			    (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, req, 0,
> -			     buf, llen, 300) != llen) {
> +			ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, req, 0,
> +						   &buf, llen, 300, GFP_KERNEL);
> +			if (ret != 0) {
>  				printk(KERN_ERR
> -				       "Failed to load isight firmware\n");
> +					"Failed to load isight firmware\n");
>  				ret = -ENODEV;
>  				goto out;
>  			}

And here the same buffer is reused for each block of data, while the new
helpers would add an allocation and a redundant memcpy() of the data
(which was just copied a few lines above) for each iteration.

So I suggest you drop this one as well.

Johan