| 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 |
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
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
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
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 --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; }
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(-)