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

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

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;

> +		*data = le32_to_cpu(tmp);
> +		res = 0;
> +	}
>  
> -	return le32_to_cpu(data);
> +	return res;
>  }