diff mbox series

[v2,05/15] usb: misc: emi26: update to use usb_control_msg_send()

Message ID 20201130012847.2579463-1-anant.thazhemadam@gmail.com (mailing list archive)
State New, archived
Headers show
Series drivers: usb: misc: update to use usb_control_msg_{send|recv}() | expand

Commit Message

Anant Thazhemadam Nov. 30, 2020, 1:28 a.m. UTC 
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 instance of usb_control_msg() has been replaced with
usb_control_msg_send() appropriately.

Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
---
 drivers/usb/misc/emi26.c | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

Comments

kernel test robot Nov. 30, 2020, 5:37 a.m. UTC | #1 
Hi Anant,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on balbi-usb/testing/next peter.chen-usb/ci-for-usb-next v5.10-rc6 next-20201127]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anant-Thazhemadam/drivers-usb-misc-update-to-use-usb_control_msg_-send-recv/20201130-093816
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/bd85eb79b555200026380c4f93e83c4a667564e5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anant-Thazhemadam/drivers-usb-misc-update-to-use-usb_control_msg_-send-recv/20201130-093816
        git checkout bd85eb79b555200026380c4f93e83c4a667564e5
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/usb/misc/emi26.c: In function 'emi26_load_firmware':
>> drivers/usb/misc/emi26.c:201:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
     201 | }
         | ^

vim +201 drivers/usb/misc/emi26.c

^1da177e4c3f415 Linus Torvalds     2005-04-16   60  
^1da177e4c3f415 Linus Torvalds     2005-04-16   61  static int emi26_load_firmware (struct usb_device *dev)
^1da177e4c3f415 Linus Torvalds     2005-04-16   62  {
ae93a55bf948753 David Woodhouse    2008-05-30   63  	const struct firmware *loader_fw = NULL;
ae93a55bf948753 David Woodhouse    2008-05-30   64  	const struct firmware *bitstream_fw = NULL;
ae93a55bf948753 David Woodhouse    2008-05-30   65  	const struct firmware *firmware_fw = NULL;
ae93a55bf948753 David Woodhouse    2008-05-30   66  	const struct ihex_binrec *rec;
b412284b9698456 Greg Kroah-Hartman 2012-04-20   67  	int err = -ENOMEM;
^1da177e4c3f415 Linus Torvalds     2005-04-16   68  	int i;
^1da177e4c3f415 Linus Torvalds     2005-04-16   69  	__u32 addr;	/* Address to write */
bd85eb79b555200 Anant Thazhemadam  2020-11-30   70  	__u8 buf[FW_LOAD_SIZE];
^1da177e4c3f415 Linus Torvalds     2005-04-16   71  
ae93a55bf948753 David Woodhouse    2008-05-30   72  	err = request_ihex_firmware(&loader_fw, "emi26/loader.fw", &dev->dev);
ae93a55bf948753 David Woodhouse    2008-05-30   73  	if (err)
ae93a55bf948753 David Woodhouse    2008-05-30   74  		goto nofw;
ae93a55bf948753 David Woodhouse    2008-05-30   75  
ae93a55bf948753 David Woodhouse    2008-05-30   76  	err = request_ihex_firmware(&bitstream_fw, "emi26/bitstream.fw",
ae93a55bf948753 David Woodhouse    2008-05-30   77  				    &dev->dev);
ae93a55bf948753 David Woodhouse    2008-05-30   78  	if (err)
ae93a55bf948753 David Woodhouse    2008-05-30   79  		goto nofw;
ae93a55bf948753 David Woodhouse    2008-05-30   80  
ae93a55bf948753 David Woodhouse    2008-05-30   81  	err = request_ihex_firmware(&firmware_fw, "emi26/firmware.fw",
ae93a55bf948753 David Woodhouse    2008-05-30   82  				    &dev->dev);
ae93a55bf948753 David Woodhouse    2008-05-30   83  	if (err) {
ae93a55bf948753 David Woodhouse    2008-05-30   84  	nofw:
fd3f1917e345d85 Greg Kroah-Hartman 2008-08-14   85  		dev_err(&dev->dev, "%s - request_firmware() failed\n",
fd3f1917e345d85 Greg Kroah-Hartman 2008-08-14   86  			__func__);
ae93a55bf948753 David Woodhouse    2008-05-30   87  		goto wraperr;
ae93a55bf948753 David Woodhouse    2008-05-30   88  	}
ae93a55bf948753 David Woodhouse    2008-05-30   89  
^1da177e4c3f415 Linus Torvalds     2005-04-16   90  	/* Assert reset (stop the CPU in the EMI) */
^1da177e4c3f415 Linus Torvalds     2005-04-16   91  	err = emi26_set_reset(dev,1);
b412284b9698456 Greg Kroah-Hartman 2012-04-20   92  	if (err < 0)
^1da177e4c3f415 Linus Torvalds     2005-04-16   93  		goto wraperr;
^1da177e4c3f415 Linus Torvalds     2005-04-16   94  
ae93a55bf948753 David Woodhouse    2008-05-30   95  	rec = (const struct ihex_binrec *)loader_fw->data;
^1da177e4c3f415 Linus Torvalds     2005-04-16   96  	/* 1. We need to put the loader for the FPGA into the EZ-USB */
ae93a55bf948753 David Woodhouse    2008-05-30   97  	while (rec) {
ae93a55bf948753 David Woodhouse    2008-05-30   98  		err = emi26_writememory(dev, be32_to_cpu(rec->addr),
ae93a55bf948753 David Woodhouse    2008-05-30   99  					rec->data, be16_to_cpu(rec->len),
ae93a55bf948753 David Woodhouse    2008-05-30  100  					ANCHOR_LOAD_INTERNAL);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  101  		if (err < 0)
^1da177e4c3f415 Linus Torvalds     2005-04-16  102  			goto wraperr;
ae93a55bf948753 David Woodhouse    2008-05-30  103  		rec = ihex_next_binrec(rec);
^1da177e4c3f415 Linus Torvalds     2005-04-16  104  	}
^1da177e4c3f415 Linus Torvalds     2005-04-16  105  
^1da177e4c3f415 Linus Torvalds     2005-04-16  106  	/* De-assert reset (let the CPU run) */
^1da177e4c3f415 Linus Torvalds     2005-04-16  107  	err = emi26_set_reset(dev,0);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  108  	if (err < 0)
cf4cf0bb89cbff9 Oliver Neukum      2007-10-25  109  		goto wraperr;
16c23f7d88cbcce Monty              2006-05-09  110  	msleep(250);	/* let device settle */
^1da177e4c3f415 Linus Torvalds     2005-04-16  111  
^1da177e4c3f415 Linus Torvalds     2005-04-16  112  	/* 2. We upload the FPGA firmware into the EMI
^1da177e4c3f415 Linus Torvalds     2005-04-16  113  	 * Note: collect up to 1023 (yes!) bytes and send them with
^1da177e4c3f415 Linus Torvalds     2005-04-16  114  	 * a single request. This is _much_ faster! */
ae93a55bf948753 David Woodhouse    2008-05-30  115  	rec = (const struct ihex_binrec *)bitstream_fw->data;
^1da177e4c3f415 Linus Torvalds     2005-04-16  116  	do {
^1da177e4c3f415 Linus Torvalds     2005-04-16  117  		i = 0;
ae93a55bf948753 David Woodhouse    2008-05-30  118  		addr = be32_to_cpu(rec->addr);
^1da177e4c3f415 Linus Torvalds     2005-04-16  119  
^1da177e4c3f415 Linus Torvalds     2005-04-16  120  		/* intel hex records are terminated with type 0 element */
ae93a55bf948753 David Woodhouse    2008-05-30  121  		while (rec && (i + be16_to_cpu(rec->len) < FW_LOAD_SIZE)) {
bd85eb79b555200 Anant Thazhemadam  2020-11-30  122  			memcpy(&buf[i], rec->data, be16_to_cpu(rec->len));
ae93a55bf948753 David Woodhouse    2008-05-30  123  			i += be16_to_cpu(rec->len);
ae93a55bf948753 David Woodhouse    2008-05-30  124  			rec = ihex_next_binrec(rec);
^1da177e4c3f415 Linus Torvalds     2005-04-16  125  		}
bd85eb79b555200 Anant Thazhemadam  2020-11-30  126  		err = emi26_writememory(dev, addr, &buf, i, ANCHOR_LOAD_FPGA);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  127  		if (err < 0)
^1da177e4c3f415 Linus Torvalds     2005-04-16  128  			goto wraperr;
327d74f6b65ddc8 Marcin Slusarz     2009-01-04  129  	} while (rec);
^1da177e4c3f415 Linus Torvalds     2005-04-16  130  
^1da177e4c3f415 Linus Torvalds     2005-04-16  131  	/* Assert reset (stop the CPU in the EMI) */
^1da177e4c3f415 Linus Torvalds     2005-04-16  132  	err = emi26_set_reset(dev,1);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  133  	if (err < 0)
^1da177e4c3f415 Linus Torvalds     2005-04-16  134  		goto wraperr;
^1da177e4c3f415 Linus Torvalds     2005-04-16  135  
^1da177e4c3f415 Linus Torvalds     2005-04-16  136  	/* 3. We need to put the loader for the firmware into the EZ-USB (again...) */
ae93a55bf948753 David Woodhouse    2008-05-30  137  	for (rec = (const struct ihex_binrec *)loader_fw->data;
ae93a55bf948753 David Woodhouse    2008-05-30  138  	     rec; rec = ihex_next_binrec(rec)) {
ae93a55bf948753 David Woodhouse    2008-05-30  139  		err = emi26_writememory(dev, be32_to_cpu(rec->addr),
ae93a55bf948753 David Woodhouse    2008-05-30  140  					rec->data, be16_to_cpu(rec->len),
ae93a55bf948753 David Woodhouse    2008-05-30  141  					ANCHOR_LOAD_INTERNAL);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  142  		if (err < 0)
^1da177e4c3f415 Linus Torvalds     2005-04-16  143  			goto wraperr;
^1da177e4c3f415 Linus Torvalds     2005-04-16  144  	}
16c23f7d88cbcce Monty              2006-05-09  145  	msleep(250);	/* let device settle */
^1da177e4c3f415 Linus Torvalds     2005-04-16  146  
^1da177e4c3f415 Linus Torvalds     2005-04-16  147  	/* De-assert reset (let the CPU run) */
^1da177e4c3f415 Linus Torvalds     2005-04-16  148  	err = emi26_set_reset(dev,0);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  149  	if (err < 0)
^1da177e4c3f415 Linus Torvalds     2005-04-16  150  		goto wraperr;
^1da177e4c3f415 Linus Torvalds     2005-04-16  151  
^1da177e4c3f415 Linus Torvalds     2005-04-16  152  	/* 4. We put the part of the firmware that lies in the external RAM into the EZ-USB */
ae93a55bf948753 David Woodhouse    2008-05-30  153  
ae93a55bf948753 David Woodhouse    2008-05-30  154  	for (rec = (const struct ihex_binrec *)firmware_fw->data;
ae93a55bf948753 David Woodhouse    2008-05-30  155  	     rec; rec = ihex_next_binrec(rec)) {
ae93a55bf948753 David Woodhouse    2008-05-30  156  		if (!INTERNAL_RAM(be32_to_cpu(rec->addr))) {
ae93a55bf948753 David Woodhouse    2008-05-30  157  			err = emi26_writememory(dev, be32_to_cpu(rec->addr),
ae93a55bf948753 David Woodhouse    2008-05-30  158  						rec->data, be16_to_cpu(rec->len),
ae93a55bf948753 David Woodhouse    2008-05-30  159  						ANCHOR_LOAD_EXTERNAL);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  160  			if (err < 0)
^1da177e4c3f415 Linus Torvalds     2005-04-16  161  				goto wraperr;
^1da177e4c3f415 Linus Torvalds     2005-04-16  162  		}
^1da177e4c3f415 Linus Torvalds     2005-04-16  163  	}
^1da177e4c3f415 Linus Torvalds     2005-04-16  164  
^1da177e4c3f415 Linus Torvalds     2005-04-16  165  	/* Assert reset (stop the CPU in the EMI) */
^1da177e4c3f415 Linus Torvalds     2005-04-16  166  	err = emi26_set_reset(dev,1);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  167  	if (err < 0)
^1da177e4c3f415 Linus Torvalds     2005-04-16  168  		goto wraperr;
^1da177e4c3f415 Linus Torvalds     2005-04-16  169  
ae93a55bf948753 David Woodhouse    2008-05-30  170  	for (rec = (const struct ihex_binrec *)firmware_fw->data;
ae93a55bf948753 David Woodhouse    2008-05-30  171  	     rec; rec = ihex_next_binrec(rec)) {
ae93a55bf948753 David Woodhouse    2008-05-30  172  		if (INTERNAL_RAM(be32_to_cpu(rec->addr))) {
ae93a55bf948753 David Woodhouse    2008-05-30  173  			err = emi26_writememory(dev, be32_to_cpu(rec->addr),
ae93a55bf948753 David Woodhouse    2008-05-30  174  						rec->data, be16_to_cpu(rec->len),
ae93a55bf948753 David Woodhouse    2008-05-30  175  						ANCHOR_LOAD_INTERNAL);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  176  			if (err < 0)
^1da177e4c3f415 Linus Torvalds     2005-04-16  177  				goto wraperr;
^1da177e4c3f415 Linus Torvalds     2005-04-16  178  		}
^1da177e4c3f415 Linus Torvalds     2005-04-16  179  	}
^1da177e4c3f415 Linus Torvalds     2005-04-16  180  
^1da177e4c3f415 Linus Torvalds     2005-04-16  181  	/* De-assert reset (let the CPU run) */
^1da177e4c3f415 Linus Torvalds     2005-04-16  182  	err = emi26_set_reset(dev,0);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  183  	if (err < 0)
^1da177e4c3f415 Linus Torvalds     2005-04-16  184  		goto wraperr;
16c23f7d88cbcce Monty              2006-05-09  185  	msleep(250);	/* let device settle */
^1da177e4c3f415 Linus Torvalds     2005-04-16  186  
^1da177e4c3f415 Linus Torvalds     2005-04-16  187  	/* return 1 to fail the driver inialization
^1da177e4c3f415 Linus Torvalds     2005-04-16  188  	 * and give real driver change to load */
^1da177e4c3f415 Linus Torvalds     2005-04-16  189  	err = 1;
^1da177e4c3f415 Linus Torvalds     2005-04-16  190  
^1da177e4c3f415 Linus Torvalds     2005-04-16  191  wraperr:
b412284b9698456 Greg Kroah-Hartman 2012-04-20  192  	if (err < 0)
b412284b9698456 Greg Kroah-Hartman 2012-04-20  193  		dev_err(&dev->dev,"%s - error loading firmware: error = %d\n",
b412284b9698456 Greg Kroah-Hartman 2012-04-20  194  			__func__, err);
b412284b9698456 Greg Kroah-Hartman 2012-04-20  195  
ae93a55bf948753 David Woodhouse    2008-05-30  196  	release_firmware(loader_fw);
ae93a55bf948753 David Woodhouse    2008-05-30  197  	release_firmware(bitstream_fw);
ae93a55bf948753 David Woodhouse    2008-05-30  198  	release_firmware(firmware_fw);
ae93a55bf948753 David Woodhouse    2008-05-30  199  
^1da177e4c3f415 Linus Torvalds     2005-04-16  200  	return err;
^1da177e4c3f415 Linus Torvalds     2005-04-16 @201  }
^1da177e4c3f415 Linus Torvalds     2005-04-16  202  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Johan Hovold Dec. 4, 2020, 2:41 p.m. UTC | #2 
On Mon, Nov 30, 2020 at 06:58:47AM +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,

Short writes have always been treated as an error. The new send helper
only changes the return value from the transfer size to 0.

And this driver never reads.

Try to describe the motivation for changing this driver which is to
avoid the explicit kmemdup().

> data can be used off the stack, and raw usb pipes need not be
> created in the calling functions.
> For this reason, the instance of usb_control_msg() has been replaced with
> usb_control_msg_send() appropriately.
> 
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> ---
>  drivers/usb/misc/emi26.c | 31 ++++++++-----------------------
>  1 file changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/misc/emi26.c b/drivers/usb/misc/emi26.c
> index 24d841850e05..1dd024507f40 100644
> --- a/drivers/usb/misc/emi26.c
> +++ b/drivers/usb/misc/emi26.c
> @@ -27,7 +27,7 @@
>  #define INTERNAL_RAM(address)   (address <= MAX_INTERNAL_ADDRESS)
>  
>  static int emi26_writememory( struct usb_device *dev, int address,
> -			      const unsigned char *data, int length,
> +			      const void *data, int length,

Why is this needed?

>  			      __u8 bRequest);
>  static int emi26_set_reset(struct usb_device *dev, unsigned char reset_bit);
>  static int emi26_load_firmware (struct usb_device *dev);
> @@ -35,22 +35,12 @@ static int emi26_probe(struct usb_interface *intf, const struct usb_device_id *i
>  static void emi26_disconnect(struct usb_interface *intf);
>  
>  /* thanks to drivers/usb/serial/keyspan_pda.c code */
> -static int emi26_writememory (struct usb_device *dev, int address,
> -			      const unsigned char *data, int length,
> +static int emi26_writememory(struct usb_device *dev, int address,
> +			      const void *data, int length,
>  			      __u8 request)
>  {
> -	int result;
> -	unsigned char *buffer =  kmemdup(data, length, GFP_KERNEL);
> -
> -	if (!buffer) {
> -		dev_err(&dev->dev, "kmalloc(%d) failed.\n", length);
> -		return -ENOMEM;
> -	}
> -	/* Note: usb_control_msg returns negative value on error or length of the
> -	 * 		 data that was written! */
> -	result = usb_control_msg (dev, usb_sndctrlpipe(dev, 0), request, 0x40, address, 0, buffer, length, 300);
> -	kfree (buffer);
> -	return result;
> +	return usb_control_msg_send(dev, 0, request, 0x40, address, 0,
> +				    data, length, 300, GFP_KERNEL);

So you're changing the return value on success from length to 0 here.
Did you make sure that all callers can handle that?

>  }
>  
>  /* thanks to drivers/usb/serial/keyspan_pda.c code */
> @@ -77,11 +67,7 @@ static int emi26_load_firmware (struct usb_device *dev)
>  	int err = -ENOMEM;
>  	int i;
>  	__u32 addr;	/* Address to write */
> -	__u8 *buf;
> -
> -	buf = kmalloc(FW_LOAD_SIZE, GFP_KERNEL);
> -	if (!buf)
> -		goto wraperr;
> +	__u8 buf[FW_LOAD_SIZE];

As the build bots reported, you must not put large structures like this
on the stack.

>  
>  	err = request_ihex_firmware(&loader_fw, "emi26/loader.fw", &dev->dev);
>  	if (err)
> @@ -133,11 +119,11 @@ static int emi26_load_firmware (struct usb_device *dev)
>  
>  		/* intel hex records are terminated with type 0 element */
>  		while (rec && (i + be16_to_cpu(rec->len) < FW_LOAD_SIZE)) {
> -			memcpy(buf + i, rec->data, be16_to_cpu(rec->len));
> +			memcpy(&buf[i], rec->data, be16_to_cpu(rec->len));
>  			i += be16_to_cpu(rec->len);
>  			rec = ihex_next_binrec(rec);
>  		}
> -		err = emi26_writememory(dev, addr, buf, i, ANCHOR_LOAD_FPGA);
> +		err = emi26_writememory(dev, addr, &buf, i, ANCHOR_LOAD_FPGA);
>  		if (err < 0)
>  			goto wraperr;
>  	} while (rec);
> @@ -211,7 +197,6 @@ static int emi26_load_firmware (struct usb_device *dev)
>  	release_firmware(bitstream_fw);
>  	release_firmware(firmware_fw);
>  
> -	kfree(buf);
>  	return err;
>  }

Looks good otherwise.

Johan
Anant Thazhemadam Jan. 7, 2021, 2:13 p.m. UTC | #3 
On 04/12/20 8:11 pm, Johan Hovold wrote:
> On Mon, Nov 30, 2020 at 06:58:47AM +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,
> Short writes have always been treated as an error. The new send helper
> only changes the return value from the transfer size to 0.
>
> And this driver never reads.
>
> Try to describe the motivation for changing this driver which is to
> avoid the explicit kmemdup().
>
Thank you. I will try and put a more appropriate commit message.
>> data can be used off the stack, and raw usb pipes need not be
>> created in the calling functions.
>> For this reason, the instance of usb_control_msg() has been replaced with
>> usb_control_msg_send() appropriately.
>>
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
>> ---
>>  drivers/usb/misc/emi26.c | 31 ++++++++-----------------------
>>  1 file changed, 8 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/usb/misc/emi26.c b/drivers/usb/misc/emi26.c
>> index 24d841850e05..1dd024507f40 100644
>> --- a/drivers/usb/misc/emi26.c
>> +++ b/drivers/usb/misc/emi26.c
>> @@ -27,7 +27,7 @@
>>  #define INTERNAL_RAM(address)   (address <= MAX_INTERNAL_ADDRESS)
>>  
>>  static int emi26_writememory( struct usb_device *dev, int address,
>> -			      const unsigned char *data, int length,
>> +			      const void *data, int length,
> Why is this needed?
>
>>  			      __u8 bRequest);
>>  static int emi26_set_reset(struct usb_device *dev, unsigned char reset_bit);
>>  static int emi26_load_firmware (struct usb_device *dev);
>> @@ -35,22 +35,12 @@ static int emi26_probe(struct usb_interface *intf, const struct usb_device_id *i
>>  static void emi26_disconnect(struct usb_interface *intf);
>>  
>>  /* thanks to drivers/usb/serial/keyspan_pda.c code */
>> -static int emi26_writememory (struct usb_device *dev, int address,
>> -			      const unsigned char *data, int length,
>> +static int emi26_writememory(struct usb_device *dev, int address,
>> +			      const void *data, int length,
>>  			      __u8 request)
>>  {
>> -	int result;
>> -	unsigned char *buffer =  kmemdup(data, length, GFP_KERNEL);
>> -
>> -	if (!buffer) {
>> -		dev_err(&dev->dev, "kmalloc(%d) failed.\n", length);
>> -		return -ENOMEM;
>> -	}
>> -	/* Note: usb_control_msg returns negative value on error or length of the
>> -	 * 		 data that was written! */
>> -	result = usb_control_msg (dev, usb_sndctrlpipe(dev, 0), request, 0x40, address, 0, buffer, length, 300);
>> -	kfree (buffer);
>> -	return result;
>> +	return usb_control_msg_send(dev, 0, request, 0x40, address, 0,
>> +				    data, length, 300, GFP_KERNEL);
> So you're changing the return value on success from length to 0 here.
> Did you make sure that all callers can handle that?

All the callers presently contain only an error checking condition for a return value < 0,
which still applies even with this change. So this wouldn't raise any issues.

>>  }
>>  
>>  /* thanks to drivers/usb/serial/keyspan_pda.c code */
>> @@ -77,11 +67,7 @@ static int emi26_load_firmware (struct usb_device *dev)
>>  	int err = -ENOMEM;
>>  	int i;
>>  	__u32 addr;	/* Address to write */
>> -	__u8 *buf;
>> -
>> -	buf = kmalloc(FW_LOAD_SIZE, GFP_KERNEL);
>> -	if (!buf)
>> -		goto wraperr;
>> +	__u8 buf[FW_LOAD_SIZE];
> As the build bots reported, you must not put large structures like this
> on the stack.

Understood. 
But I'm considering dropping this change (and the one proposed for emi62)
altogether in v3 - since these would end up requiring memory to dynamically allocated
twice for the same purpose.
However, if you still think the pros of updating this (and emi62) outweigh the cons,
please let me know, and I'll make sure to send in another version fixing it.


>>  
>>  	err = request_ihex_firmware(&loader_fw, "emi26/loader.fw", &dev->dev);
>>  	if (err)
>> @@ -133,11 +119,11 @@ static int emi26_load_firmware (struct usb_device *dev)
>>  
>>  		/* intel hex records are terminated with type 0 element */
>>  		while (rec && (i + be16_to_cpu(rec->len) < FW_LOAD_SIZE)) {
>> -			memcpy(buf + i, rec->data, be16_to_cpu(rec->len));
>> +			memcpy(&buf[i], rec->data, be16_to_cpu(rec->len));
>>  			i += be16_to_cpu(rec->len);
>>  			rec = ihex_next_binrec(rec);
>>  		}
>> -		err = emi26_writememory(dev, addr, buf, i, ANCHOR_LOAD_FPGA);
>> +		err = emi26_writememory(dev, addr, &buf, i, ANCHOR_LOAD_FPGA);
>>  		if (err < 0)
>>  			goto wraperr;
>>  	} while (rec);
>> @@ -211,7 +197,6 @@ static int emi26_load_firmware (struct usb_device *dev)
>>  	release_firmware(bitstream_fw);
>>  	release_firmware(firmware_fw);
>>  
>> -	kfree(buf);
>>  	return err;
>>  }
> Looks good otherwise.
>
> Johan

Thanks,
Anant
Johan Hovold Jan. 8, 2021, 9:20 a.m. UTC | #4 
On Thu, Jan 07, 2021 at 07:43:54PM +0530, Anant Thazhemadam wrote:
> On 04/12/20 8:11 pm, Johan Hovold wrote:
> > On Mon, Nov 30, 2020 at 06:58:47AM +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,
> > Short writes have always been treated as an error. The new send helper
> > only changes the return value from the transfer size to 0.
> >
> > And this driver never reads.
> >
> > Try to describe the motivation for changing this driver which is to
> > avoid the explicit kmemdup().

> >>  /* thanks to drivers/usb/serial/keyspan_pda.c code */
> >> @@ -77,11 +67,7 @@ static int emi26_load_firmware (struct usb_device *dev)
> >>  	int err = -ENOMEM;
> >>  	int i;
> >>  	__u32 addr;	/* Address to write */
> >> -	__u8 *buf;
> >> -
> >> -	buf = kmalloc(FW_LOAD_SIZE, GFP_KERNEL);
> >> -	if (!buf)
> >> -		goto wraperr;
> >> +	__u8 buf[FW_LOAD_SIZE];
> > As the build bots reported, you must not put large structures like this
> > on the stack.
> 
> Understood. 
> But I'm considering dropping this change (and the one proposed for
> emi62) altogether in v3 - since these would end up requiring memory to
> dynamically allocated twice for the same purpose.  However, if you
> still think the pros of updating this (and emi62) outweigh the cons,
> please let me know, and I'll make sure to send in another version
> fixing it.

The redundant memdup() is already there for the firmware buffer and
changing to usb_control_msg_send() will only make it slightly harder to
get rid of that, if anyone would bother.

But yeah, it's probably not worth switching usb_control_msg_send() for
these drivers.

Johan
diff mbox series

Patch

diff --git a/drivers/usb/misc/emi26.c b/drivers/usb/misc/emi26.c
index 24d841850e05..1dd024507f40 100644
--- a/drivers/usb/misc/emi26.c
+++ b/drivers/usb/misc/emi26.c
@@ -27,7 +27,7 @@ 
 #define INTERNAL_RAM(address)   (address <= MAX_INTERNAL_ADDRESS)
 
 static int emi26_writememory( struct usb_device *dev, int address,
-			      const unsigned char *data, int length,
+			      const void *data, int length,
 			      __u8 bRequest);
 static int emi26_set_reset(struct usb_device *dev, unsigned char reset_bit);
 static int emi26_load_firmware (struct usb_device *dev);
@@ -35,22 +35,12 @@  static int emi26_probe(struct usb_interface *intf, const struct usb_device_id *i
 static void emi26_disconnect(struct usb_interface *intf);
 
 /* thanks to drivers/usb/serial/keyspan_pda.c code */
-static int emi26_writememory (struct usb_device *dev, int address,
-			      const unsigned char *data, int length,
+static int emi26_writememory(struct usb_device *dev, int address,
+			      const void *data, int length,
 			      __u8 request)
 {
-	int result;
-	unsigned char *buffer =  kmemdup(data, length, GFP_KERNEL);
-
-	if (!buffer) {
-		dev_err(&dev->dev, "kmalloc(%d) failed.\n", length);
-		return -ENOMEM;
-	}
-	/* Note: usb_control_msg returns negative value on error or length of the
-	 * 		 data that was written! */
-	result = usb_control_msg (dev, usb_sndctrlpipe(dev, 0), request, 0x40, address, 0, buffer, length, 300);
-	kfree (buffer);
-	return result;
+	return usb_control_msg_send(dev, 0, request, 0x40, address, 0,
+				    data, length, 300, GFP_KERNEL);
 }
 
 /* thanks to drivers/usb/serial/keyspan_pda.c code */
@@ -77,11 +67,7 @@  static int emi26_load_firmware (struct usb_device *dev)
 	int err = -ENOMEM;
 	int i;
 	__u32 addr;	/* Address to write */
-	__u8 *buf;
-
-	buf = kmalloc(FW_LOAD_SIZE, GFP_KERNEL);
-	if (!buf)
-		goto wraperr;
+	__u8 buf[FW_LOAD_SIZE];
 
 	err = request_ihex_firmware(&loader_fw, "emi26/loader.fw", &dev->dev);
 	if (err)
@@ -133,11 +119,11 @@  static int emi26_load_firmware (struct usb_device *dev)
 
 		/* intel hex records are terminated with type 0 element */
 		while (rec && (i + be16_to_cpu(rec->len) < FW_LOAD_SIZE)) {
-			memcpy(buf + i, rec->data, be16_to_cpu(rec->len));
+			memcpy(&buf[i], rec->data, be16_to_cpu(rec->len));
 			i += be16_to_cpu(rec->len);
 			rec = ihex_next_binrec(rec);
 		}
-		err = emi26_writememory(dev, addr, buf, i, ANCHOR_LOAD_FPGA);
+		err = emi26_writememory(dev, addr, &buf, i, ANCHOR_LOAD_FPGA);
 		if (err < 0)
 			goto wraperr;
 	} while (rec);
@@ -211,7 +197,6 @@  static int emi26_load_firmware (struct usb_device *dev)
 	release_firmware(bitstream_fw);
 	release_firmware(firmware_fw);
 
-	kfree(buf);
 	return err;
 }