Re: [PATCH RFC v2 5/6] staging: r8188eu: add error handling of rtw_read32

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 9:01:23 AM CEST Pavel Skripkin wrote:
> 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 the use of the new API I think that you don't have anymore
partial messages. usb_control_msg() returns the number of bytes 
transferred, while usb_control_msg_recv/send return only 0 if successful
otherwise a negative error number.

Regards,

Fabio

> With regards,
> Pavel Skripkin
>