Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3 4/6] staging: r8188eu: add error handling of rtw_read16
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Larry.Finger@lwfinger.net, phil@philpotter.co.uk,
        straube.linux@gmail.com, fmdefrancesco@gmail.com,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
References: <cover.1629789580.git.paskripkin@gmail.com>
 <d06f3173e0a4c6f5449d5551cbfb0202849332b7.1629789580.git.paskripkin@gmail.com>
 <YSdx4ixYYAyZkOzR@kroah.com>
From:   Pavel Skripkin <paskripkin@gmail.com>
Message-ID: <ce0db9b5-9222-0b84-81ba-d584b0dd5739@gmail.com>
Date:   Thu, 26 Aug 2021 13:58:32 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <YSdx4ixYYAyZkOzR@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/26/21 1:50 PM, Greg KH wrote:
> On Tue, Aug 24, 2021 at 10:27:35AM +0300, Pavel Skripkin wrote:
>> _rtw_read16 function can fail in case of usb transfer failure. But
>> previous function prototype wasn't designed to return an error to
>> caller. It can cause a lot uninit value bugs all across the driver code,
>> since rtw_read16() returns local stack variable to caller.
>> 
>> Fix it by changing the prototype of this function. Now it returns an
>> int: 0 on success, negative error value on failure and callers should pass
>> the pointer to storage location for register value.
>> 
>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
>> ---
>>  drivers/staging/r8188eu/core/rtw_debug.c      |  7 +++-
>>  drivers/staging/r8188eu/core/rtw_io.c         |  9 ++---
>>  drivers/staging/r8188eu/core/rtw_mp_ioctl.c   |  4 +-
>>  drivers/staging/r8188eu/hal/odm_interface.c   |  4 +-
>>  .../staging/r8188eu/hal/rtl8188e_hal_init.c   | 29 +++++++++++----
>>  drivers/staging/r8188eu/hal/rtl8188e_phycfg.c |  5 ++-
>>  drivers/staging/r8188eu/hal/usb_halinit.c     | 37 ++++++++++++++++---
>>  drivers/staging/r8188eu/hal/usb_ops_linux.c   | 22 +++++++++--
>>  .../staging/r8188eu/include/odm_interface.h   |  2 +-
>>  drivers/staging/r8188eu/include/rtw_io.h      |  6 +--
>>  drivers/staging/r8188eu/os_dep/ioctl_linux.c  | 28 +++++++++++---
>>  11 files changed, 115 insertions(+), 38 deletions(-)
>> 
>> diff --git a/drivers/staging/r8188eu/core/rtw_debug.c b/drivers/staging/r8188eu/core/rtw_debug.c
>> index 8b7d3eb12bd0..a41675e101ac 100644
>> --- a/drivers/staging/r8188eu/core/rtw_debug.c
>> +++ b/drivers/staging/r8188eu/core/rtw_debug.c
>> @@ -91,7 +91,12 @@ int proc_get_read_reg(char *page, char **start,
>>  				proc_get_read_addr, (u8) tmp);
>>  		break;
>>  	case 2:
>> -		len += snprintf(page + len, count - len, "rtw_read16(0x%x)=0x%x\n", proc_get_read_addr, rtw_read16(padapter, proc_get_read_addr));
>> +		error = rtw_read16(padapter, proc_get_read_addr, (u16 *) &tmp);
>> +		if (error)
>> +			return len;
>> +
>> +		len += snprintf(page + len, count - len, "rtw_read16(0x%x)=0x%x\n",
>> +				proc_get_read_addr, (u16) tmp);
>>  		break;
>>  	case 4:
>>  		len += snprintf(page + len, count - len, "rtw_read32(0x%x)=0x%x\n", proc_get_read_addr, rtw_read32(padapter, proc_get_read_addr));
>> diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
>> index 2714506c8ffb..fd64893c778d 100644
>> --- a/drivers/staging/r8188eu/core/rtw_io.c
>> +++ b/drivers/staging/r8188eu/core/rtw_io.c
>> @@ -45,18 +45,15 @@ int __must_check _rtw_read8(struct adapter *adapter, u32 addr, u8 *data)
>>  	return _read8(pintfhdl, addr, data);
>>  }
>>  
>> -u16 _rtw_read16(struct adapter *adapter, u32 addr)
>> +int __must_check _rtw_read16(struct adapter *adapter, u32 addr, u16 *data)
>>  {
>> -	u16 r_val;
>>  	struct io_priv *pio_priv = &adapter->iopriv;
>>  	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
>> -	u16 (*_read16)(struct intf_hdl *pintfhdl, u32 addr);
>> +	int (*_read16)(struct intf_hdl *pintfhdl, u32 addr, u16 *data);
>>  
>>  	_read16 = pintfhdl->io_ops._read16;
> 
> Why do you need this extra pointer abstraction anyway?  Please unwind
> that first, which will make this function go away (or get easier to
> understand at the least...)
> 
It was there before my changes :) I had a plan to clean up DBG macros 
and this abstraction after adding error handling...


If you feel, that these clean up are needed before error handling, I can 
add them to this series :) Anyway, some of them are already in my local 
tree


With regards,
Pavel Skripkin