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
