Messages in this thread | | | Subject | Re: [PATCH RFC v2 5/6] staging: r8188eu: add error handling of rtw_read32 | From | Pavel Skripkin <> | Date | Tue, 24 Aug 2021 10:01:23 +0300 |
| |
On 8/24/21 9:58 AM, Dan Carpenter wrote: > On Sun, Aug 22, 2021 at 05:36:01PM +0300, Pavel Skripkin wrote: >> -static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr) >> +static int usb_read32(struct intf_hdl *pintfhdl, u32 addr, u32 *data) >> { >> u8 requesttype; >> u16 wvalue; >> u16 len; >> - __le32 data; >> + int res; >> + __le32 tmp; >> + >> + if (WARN_ON(unlikely(!data))) >> + return -EINVAL; >> >> requesttype = 0x01;/* read_in */ >> >> wvalue = (u16)(addr & 0x0000ffff); >> len = 4; >> >> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype); >> + res = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype); >> + if (res < 0) { >> + dev_err(dvobj_to_dev(pintfhdl->pintf_dev), "Failed to read 32 bytes: %d\n", res); > > Add a return here. Try to keep the success path and the failure path > as separate as possible. Try to keep the success path indented at one > tab so the code looks like this: > > success(); > success(); > if (fail) > handle_failure(); > success(); > success(); > > Try to deal with exceptions as quickly as possible so that the reader > has less to remember. > >> + } else { >> + /* Noone cares about positive return value */ > > Ugh... That's unfortunate. We should actually care. The > usbctrl_vendorreq() has an information leak where it copies len (4) > bytes of data even if usb_control_msg() is not able to read len bytes. > > The best fix would be to remove the information leak and make > usbctrl_vendorreq() return zero on success. In other words something > like: > > status = usb_control_msg(); > if (status < 0) > return status; > if (status != len) > return -EIO; > status = 0; >
I see, thank you for reviewing, will fix in v3! I fully forgot, that usb_control_msg() can receive only part of the message :)
With regards, Pavel Skripkin
| |