Re: [PATCH v4 15/18] staging: r8188eu: hal: Clean up usbctrl_vendorreq()

[Dan Carpenter]

On Mon, Sep 13, 2021 at 08:09:59PM +0200, Fabio M. De Francesco wrote:
> Clean up usbctrl_vendorreq () in usb_ops_linux.c because some
> of its code will be reused in this series. This cleanup is in
> preparation for shortening the call chains of rtw_read{8,16,32}()
> and rtw_write{8,16,32,N}(). More insights about the reasons why at
> https://lore.kernel.org/lkml/5319192.FrU0QrjFp7@localhost.localdomain/
> 

This commit message is quite bad.

This patch has nothing to do with reusing the code or shortening call
chains.

Don't use a link like that in the commit message especially when it's a
link to an email you wrote.  If it's someone else's email you can say,
somethink like "As <name> points out in <his/her> email <url>.  Blah
blah blah."  That way you give credit to the other person but all the
information is in the commit message.

A better commit message would be:

    Clean up usbctrl_vendorreq().

    1) Remove impossible NULL check.
    2) Rename variables:
        pintfhdl => intfhdl
        pdata => data
        pIo_buf => io_buf
    3) Edit the comments.

> Co-developed-by: Pavel Skripkin <paskripkin@xxxxxxxxx>
> Signed-off-by: Pavel Skripkin <paskripkin@xxxxxxxxx>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> ---
> 
> v3->v4:
>         Make this patch.
> v2->v3:
>         No changes.
> v1->v2:
>         No changes.
> 
>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 62 +++++++++------------
>  1 file changed, 27 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index 117213c9f984..2c7e92085a6e 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -8,15 +8,15 @@
>  #include "../include/recv_osdep.h"
>  #include "../include/rtl8188e_hal.h"
>  
> -static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, u16 len, u8 requesttype)
> +static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u16 len, u8 requesttype)
>  {
> -	struct adapter	*adapt = pintfhdl->padapter;
> -	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
> -	struct usb_device *udev = dvobjpriv->pusbdev;
> -	unsigned int pipe;
>  	int status = 0;
> -	u8 *pIo_buf;
> +	u8 *io_buf;
> +	unsigned int pipe;
>  	int vendorreq_times = 0;
> +	struct adapter *adapt = intfhdl->padapter;
> +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> +	struct usb_device *udev = dvobjpriv->pusbdev;

I don't understand why you moved these from the top to the bottom.
But the original was better.  In networking code declarations are
normally written in Reverse Christmas Tree format, longest to shortest,
like this:

	long long long_name;
	medium name;
	u8 short;

>  
>  	if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) {
>  		status = -EPERM;
> @@ -32,51 +32,44 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>  	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
>  
>  	/*  Acquire IO memory for vendorreq */
> -	pIo_buf = dvobjpriv->usb_vendor_req_buf;
> -
> -	if (!pIo_buf) {
> -		DBG_88E("[%s] pIo_buf == NULL\n", __func__);
> -		status = -ENOMEM;
> -		goto release_mutex;
> -	}
> +	io_buf = dvobjpriv->usb_vendor_req_buf;
>  
>  	if (requesttype == REALTEK_USB_VENQT_READ)
> -		pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
> +		/* read in */
> +		pipe = usb_rcvctrlpipe(udev, 0);
>  	else
> -		pipe = usb_sndctrlpipe(udev, 0);/* write_out */
> +		/* write out */
> +		pipe = usb_sndctrlpipe(udev, 0);

Use {} curly braces around multi-line indents.  But in this case the
comments are worthless.  Just delete them.


>  
>  	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
>  		if (requesttype == REALTEK_USB_VENQT_READ)
> -			memset(pIo_buf, 0, len);
> +			memset(io_buf, 0, len);
>  		else
> -			memcpy(pIo_buf, pdata, len);
> +			memcpy(io_buf, data, len);
>  
>  		status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
>  					 requesttype, value, REALTEK_USB_VENQT_CMD_IDX,
> -					 pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT);
> +					 io_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT);
>  
> -		if (status == len) {   /*  Success this control transfer. */
> +		if (status == len) {
> +			/*  success */

Delete this comment.

>  			rtw_reset_continual_urb_error(dvobjpriv);
>  			if (requesttype == REALTEK_USB_VENQT_READ)
> -				memcpy(pdata, pIo_buf,  len);
> -		} else { /*  error cases */
> -			DBG_88E("reg 0x%x, usb %s %u fail, status:%d value=0x%x, vendorreq_times:%d\n",
> -				value, (requesttype == REALTEK_USB_VENQT_READ) ? "read" : "write",
> -				len, status, *(u32 *)pdata, vendorreq_times);
> -
> +				memcpy(data, io_buf,  len);
> +		} else {
> +			/* errors */

Delete.


>  			if (status < 0) {
> -				if (status == (-ESHUTDOWN) || status == -ENODEV) {
> +				if (status == (-ESHUTDOWN || -ENODEV)) {
>  					adapt->bSurpriseRemoved = true;
>  				} else {
> -					struct hal_data_8188e	*haldata = GET_HAL_DATA(adapt);
> +					struct hal_data_8188e *haldata = GET_HAL_DATA(adapt);
>  					haldata->srestpriv.wifi_error_status = USB_VEN_REQ_CMD_FAIL;
>  				}
> -			} else { /*  status != len && status >= 0 */
> +			} else {
> +				/*  status != len && status >= 0 */

Delete.

>  				if (status > 0) {
> -					if (requesttype == REALTEK_USB_VENQT_READ) {
> -						/*  For Control read transfer, we have to copy the read data from pIo_buf to pdata. */
> -						memcpy(pdata, pIo_buf,  len);
> -					}
> +					if (requesttype == REALTEK_USB_VENQT_READ)
> +						memcpy(data, io_buf,  len);
>  				}
>  			}
>  

regards,
dan carpenter