diff mbox series

[v2,06/15] usb: misc: emi62: update to use usb_control_msg_send()

Message ID 20201130012925.2579632-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:29 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/emi62.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

Comments

kernel test robot Nov. 30, 2020, 3:54 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: h8300-randconfig-s032-20201130 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-170-g3bc348f6-dirty
        # https://github.com/0day-ci/linux/commit/a9e2333efa48de6856185ec35c82b659ff1c1215
        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 a9e2333efa48de6856185ec35c82b659ff1c1215
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=h8300 

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/emi62.c: In function 'emi62_load_firmware':
>> drivers/usb/misc/emi62.c:213:1: warning: the frame size of 1048 bytes is larger than 1024 bytes [-Wframe-larger-than=]
     213 | }
         | ^

vim +213 drivers/usb/misc/emi62.c

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

---
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:43 p.m. UTC | #2 
On Mon, Nov 30, 2020 at 06:59:25AM +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 instance of usb_control_msg() has been replaced with
> usb_control_msg_send() appropriately.
> 
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> ---

This driver looks like it shares origins with the emi26 one so the same
comments apply (and especially not put large structures on the stack).

Johan
diff mbox series

Patch

diff --git a/drivers/usb/misc/emi62.c b/drivers/usb/misc/emi62.c
index 3eea60437f56..6ac4e72d53e0 100644
--- a/drivers/usb/misc/emi62.c
+++ b/drivers/usb/misc/emi62.c
@@ -36,7 +36,7 @@ 
 #define INTERNAL_RAM(address)   (address <= MAX_INTERNAL_ADDRESS)
 
 static int emi62_writememory(struct usb_device *dev, int address,
-			     const unsigned char *data, int length,
+			     const void *data, int length,
 			     __u8 bRequest);
 static int emi62_set_reset(struct usb_device *dev, unsigned char reset_bit);
 static int emi62_load_firmware (struct usb_device *dev);
@@ -45,21 +45,11 @@  static void emi62_disconnect(struct usb_interface *intf);
 
 /* thanks to drivers/usb/serial/keyspan_pda.c code */
 static int emi62_writememory(struct usb_device *dev, int address,
-			     const unsigned char *data, int length,
+			     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 */
@@ -85,12 +75,9 @@  static int emi62_load_firmware (struct usb_device *dev)
 	int err = -ENOMEM;
 	int i;
 	__u32 addr;	/* Address to write */
-	__u8 *buf;
+	__u8 buf[FW_LOAD_SIZE];
 
 	dev_dbg(&dev->dev, "load_firmware\n");
-	buf = kmalloc(FW_LOAD_SIZE, GFP_KERNEL);
-	if (!buf)
-		goto wraperr;
 
 	err = request_ihex_firmware(&loader_fw, "emi62/loader.fw", &dev->dev);
 	if (err)
@@ -140,11 +127,11 @@  static int emi62_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 = emi62_writememory(dev, addr, buf, i, ANCHOR_LOAD_FPGA);
+		err = emi62_writememory(dev, addr, &buf, i, ANCHOR_LOAD_FPGA);
 		if (err < 0)
 			goto wraperr;
 	} while (rec);
@@ -209,8 +196,6 @@  static int emi62_load_firmware (struct usb_device *dev)
 	release_firmware(bitstream_fw);
 	release_firmware(firmware_fw);
 
-	kfree(buf);
-
 	/* return 1 to fail the driver inialization
 	 * and give real driver change to load */
 	return 1;
@@ -223,7 +208,6 @@  static int emi62_load_firmware (struct usb_device *dev)
 	release_firmware(bitstream_fw);
 	release_firmware(firmware_fw);
 
-	kfree(buf);
 	dev_err(&dev->dev, "Error\n");
 	return err;
 }