On 8/26/21 1:50 PM, Greg KH wrote:

On Tue, Aug 24, 2021 at 10:27:35AM +0300, Pavel Skripkin wrote:

quoted

_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 <redacted>
---
 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