Re: [PATCH v3 2/3] staging: r8188eu: Shorten calls chain of rtw_read8/16/32()

[Greg Kroah-Hartman]

On Sun, Sep 05, 2021 at 12:00:47AM +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>
> ---
> 
> v2->v3: No changes.
> 
> v1->v2: No changes.
> 
>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 92 +++++++++++++++++----
>  1 file changed, 78 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index a87b0d2e87d0..f9c4fd5a2c53 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -97,38 +97,102 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>  	return status;
>  }
>  
> +static int usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
> +{
> +	u16 value = (u16)(addr & 0x0000ffff);

Why not just pass in the address as a 16bit value?


> +	struct adapter *adapt = intfhdl->padapter;
> +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> +	struct usb_device *udev = dvobjpriv->pusbdev;
> +	int status;
> +	u8 *io_buf;
> +	int vendorreq_times = 0;
> +
> +	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx) {
> +		status = -EPERM;
> +		goto exit;

This is "interesting" to see if it's really even working as they think
it does, but let's leave it alone for now...

> +	}
> +
> +	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
> +
> +	/*  Acquire IO memory for vendorreq */
> +	io_buf = dvobjpriv->usb_vendor_req_buf;
> +
> +	if (!io_buf) {
> +		DBG_88E("[%s] io_buf == NULL\n", __func__);

How can this buffer ever be NULL?

> +		status = -ENOMEM;
> +		goto release_mutex;
> +	}

Why share a buffer at all anyway?

> +	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> +		status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> +					      REALTEK_USB_VENQT_READ, value,
> +					      REALTEK_USB_VENQT_CMD_IDX, io_buf,
> +					      size, RTW_USB_CONTROL_MSG_TIMEOUT,
> +					      GFP_KERNEL);
> +		if (!status) {   /*  Success this control transfer. */

Comments go on the next line.

> +			rtw_reset_continual_urb_error(dvobjpriv);
> +			memcpy(data, io_buf, size);
> +		} else { /*  error cases */

Again, next line for the comment.

> +			DBG_88E("reg 0x%x, usb %s %u fail, status:%d vendorreq_times:%d\n",
> +				value, "read", size, status, vendorreq_times);

These should be removed eventually...

> +
> +			if (status == (-ESHUTDOWN) || status == -ENODEV) {
> +				adapt->bSurpriseRemoved = true;

Odd, but ok...

> +			} else {
> +				struct hal_data_8188e *haldata = GET_HAL_DATA(adapt);
> +
> +				haldata->srestpriv.wifi_error_status = USB_VEN_REQ_CMD_FAIL;

Why are we not saying the command failed even if the device was removed?

But if we do say an error happened, why are we trying to send this out
again?  What would happen to make it work the second time?

> +			}
> +
> +			if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
> +				adapt->bSurpriseRemoved = true;

So we try to see if the device was removed again?

> +				break;
> +			}
> +		}
> +
> +		/*  firmware download is checksummed, don't retry */
> +		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || !status)
> +			break;

Nothing like a special case for firmware magic.

Those calls should just use a different write function entirely,
eventually, to remove this...

Ok, I know you are just moving code around, this is fine, just pointing
out things that should be fixed up eventually...

thanks,

greg k-h