Re: [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()

[Greg Kroah-Hartman]

On Fri, Sep 17, 2021 at 09:18:35AM +0200, Fabio M. De Francesco wrote:
> Shorten the calls chain of rtw_read8/16/32() down to the actual reads.
> For this purpose unify the three usb_read8/16/32 into the new
> usb_read(); make the latter parameterizable with 'size'; embed most of
> the code of usbctrl_vendorreq() into usb_read() and use in it the new
> usb_control_msg_recv() API of USB Core.
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Co-developed-by: Pavel Skripkin <paskripkin@xxxxxxxxx>
> Signed-off-by: Pavel Skripkin <paskripkin@xxxxxxxxx>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> ---
>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 59 +++++++++++++++++++--
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index 2d5e9b3ba538..ef35358cf2d3 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -89,6 +89,59 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
>  	return status;
>  }
>  
> +static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
> +{
> +	struct adapter *adapt = intfhdl->padapter;
> +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> +	struct usb_device *udev = dvobjpriv->pusbdev;
> +	int status;
> +	u8 *io_buf; /* Pointer to I/O buffer */

As you "know" size is not going to be larger than 4 (hint, you should
prboably check it), just use bytes off of the stack here, and you can
ignore this buffer entirely.  That will hopefully allow you in the
future to get rid of that buffer as odds are it will not be needed
anymore.

> +
> +	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
> +		return -EPERM;

How is it ok to check this outside of the lock?  What happens if these
values change right _after_ you check them?

Why check them at all, is this something that we even care about?

I know you are trying to make this just the same logic at is there
today, but why not just do it right the first time?

> +
> +	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
> +
> +	io_buf = dvobjpriv->usb_vendor_req_buf;
> +
> +	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> +				      REALTEK_USB_VENQT_READ, addr,
> +				      REALTEK_USB_VENQT_CMD_IDX, io_buf,
> +				      size, RTW_USB_CONTROL_MSG_TIMEOUT,
> +				      GFP_KERNEL);
> +
> +	if (status == -ESHUTDOWN ||
> +	    status == -ENODEV ||
> +	    status == -ENOENT) {
> +		/*
> +		 * device or controller has been disabled due to
> +		 * some problem that could not be worked around,
> +		 * device or bus doesn’t exist, endpoint does not
> +		 * exist or is not enabled.
> +		 */
> +		adapt->bSurpriseRemoved = true;
> +		goto mutex_unlock;
> +	}
> +
> +	if (status < 0) {
> +		GET_HAL_DATA(adapt)->srestpriv.wifi_error_status =
> +			USB_VEN_REQ_CMD_FAIL;
> +
> +		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
> +			adapt->bSurpriseRemoved = true;
> +
> +		goto mutex_unlock;
> +	}
> +
> +	rtw_reset_continual_urb_error(dvobjpriv);
> +	memcpy(data, io_buf, size);
> +
> +mutex_unlock:
> +	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
> +
> +	return status;

No one cares about this value, is that ok?

thanks,

greg k-h