linux-kernel - Re: [PATCH v4 15/18] staging: r8188eu: hal: Clean up usbctrl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 14 Sep 2021 12:24:05 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Michael Straube <straube.linux@...il.com>,
        Pavel Skripkin <paskripkin@...il.com>
Subject: Re: [PATCH v4 15/18] staging: r8188eu: hal: Clean up
 usbctrl_vendorreq()

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@...il.com>
> Signed-off-by: Pavel Skripkin <paskripkin@...il.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> ---
> 
> 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