All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains
@ 2021-09-17  7:18 Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 01/19] staging: r8188eu: remove usb_{read,write}_mem() Fabio M. De Francesco
                   ` (19 more replies)
  0 siblings, 20 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M. De Francesco

io_ops abstraction is useless in this driver, since there is only one ops
registration. Without io_ops we can get rid of indirect calls mess and
shorten the calls chain.

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.

Shorten the calls chain of rtw_write8/16/32() down to the actual writes.
For this purpose unify the four usb_write8/16/32/N() into the new
usb_write(); make the latter parameterizable with 'size'; embed most of
the code of usbctrl_vendorreq() into usb_write() and use in it the new
usb_control_msg_send() API of USB Core.

The code with the modifications was thoroughly tested by Pavel Skripkin
using a TP-Link TL-WN722N v2 / v3 [Realtek RTL8188EUS] and by Fabio M.
De Francesco using a ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano].

Changelog:

v6->v7:
	- 1-14: 
		Add Dan Carpenter's "Reviewed-by" tag. No code changes;
	- 15:
		Add a list of clean-up changes to the commit messages
		and restore an empty line inadvertently deleted in a
		previous version;
	- 16:
		Add a list of clean-up changes to the commit messages;
	- 17-18:
		No changes;
	- 19:
		Delete an unnecessary test.	

v5->v6:
        - 1-14:
                Minimal changes to patch subjects to achieve consistent
                style;
        - 15:
                Fix a mistake in the checks of values returned by
                usb_control_msg();
        - 16-19:
                No changes.

v4->v5:
        - 1-14:
                No changes;
        - 15-16:
                Fix minor formatting issues and use "Reverse Xmas Tree" style,
                according to suggestions by David Laight and Dan Carpenter;
        - 17-18:
                Pavel Skripkin found logical errors in the checks of the
                values returned by usb_control_msg_{recv,send}(), so fix them;
                Dan Carpenter suggested to do error handling before success
                handling, so change the code accordingly;
        - 19:
                Add this patch in order to get rid of the shared buffer in
                usb_read() and usb_write() and remove this field from struct
                "dvobj_priv".

v3->v4:
        - 1-14:
                Split a patch into fourteen;
        - 15-16:
                Add these patches for clean-ups of the code that is going to be
                reused in 17-18/18;
        - 17-18:
                Make some changes according to a first review of Greg
                Kroah-Hartman; furthermore, remove the unnecessary while loop
                and a couple of if' test; handle the errors returned by
                usb_control_msg_recv().

v2->v3:
        - 1-2:
                No changes;
        - 3:
                Fix the version number of the patch.

v1->v2:
        - 1-2:
                No changes;
        - 3:
                Replace parameter REALTEK_USB_VENQT_READ with REALTEK_USB_VENQT_WRITE
                in usb_control_msg_send().

v1: https://lore.kernel.org/lkml/20210904150447.14659-1-fmdefrancesco@gmail.com/
v2: https://lore.kernel.org/lkml/20210904212719.11426-1-fmdefrancesco@gmail.com/
v3: https://lore.kernel.org/lkml/20210904220048.12822-1-fmdefrancesco@gmail.com/
v4: https://lore.kernel.org/lkml/20210913181002.16651-1-fmdefrancesco@gmail.com/
v5: https://lore.kernel.org/lkml/20210915124149.27543-1-fmdefrancesco@gmail.com/
v6: https://lore.kernel.org/lkml/20210915211103.18001-1-fmdefrancesco@gmail.com/

Fabio M. De Francesco (4):
  staging: r8188eu: clean up usbctrl_vendorreq()
  staging: r8188eu: clean up rtw_read*() and rtw_write*()
  staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()
  staging: r8188eu: shorten calls chain of rtw_write{8,16,32,N}()

Pavel Skripkin (15):
  staging: r8188eu: remove usb_{read,write}_mem()
  staging: r8188eu: remove the helpers of rtw_read8()
  staging: r8188eu: remove the helpers of rtw_read16()
  staging: r8188eu: remove the helpers of rtw_read32()
  staging: r8188eu: remove the helpers of usb_write8()
  staging: r8188eu: remove the helpers of usb_write16()
  staging: r8188eu: remove the helpers of usb_write32()
  staging: r8188eu: remove the helpers of usb_writeN()
  staging: r8188eu: remove the helpers of usb_read_port()
  staging: r8188eu: remove the helpers of usb_write_port()
  staging: r8188eu: remove the helpers of usb_read_port_cancel()
  staging: r8188eu: remove the helpers of usb_write_port_cancel()
  staging: r8188eu: remove core/rtw_io.c
  staging: r8188eu: remove struct _io_ops
  staging: r8188eu: remove shared buffer for usb requests

 drivers/staging/r8188eu/Makefile              |   1 -
 drivers/staging/r8188eu/core/rtw_io.c         | 299 ------------------
 drivers/staging/r8188eu/hal/usb_halinit.c     |   6 +-
 drivers/staging/r8188eu/hal/usb_ops_linux.c   | 261 ++++++++-------
 drivers/staging/r8188eu/include/drv_types.h   |   5 -
 drivers/staging/r8188eu/include/rtw_io.h      |  89 +-----
 drivers/staging/r8188eu/include/usb_ops.h     |   2 -
 .../staging/r8188eu/include/usb_ops_linux.h   |   8 -
 drivers/staging/r8188eu/os_dep/usb_intf.c     |  48 +--
 .../staging/r8188eu/os_dep/usb_ops_linux.c    |  20 +-
 10 files changed, 167 insertions(+), 572 deletions(-)
 delete mode 100644 drivers/staging/r8188eu/core/rtw_io.c

-- 
2.33.0


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v7 01/19] staging: r8188eu: remove usb_{read,write}_mem()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 02/19] staging: r8188eu: remove the helpers of rtw_read8() Fabio M. De Francesco
                   ` (18 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove usb_{read,write}_mem() because they are unused.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c         | 29 -------------------
 drivers/staging/r8188eu/hal/usb_ops_linux.c   |  2 --
 drivers/staging/r8188eu/include/rtw_io.h      |  4 ---
 .../staging/r8188eu/include/usb_ops_linux.h   |  3 --
 .../staging/r8188eu/os_dep/usb_ops_linux.c    |  8 -----
 5 files changed, 46 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index cde0205816b1..e6f377377ab2 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -175,35 +175,6 @@ int _rtw_write32_async(struct adapter *adapter, u32 addr, u32 val)
 	return RTW_STATUS_CODE(ret);
 }
 
-void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem)
-{
-	void (*_read_mem)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *pmem);
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-
-
-	if (adapter->bDriverStopped || adapter->bSurpriseRemoved)
-	     return;
-	_read_mem = pintfhdl->io_ops._read_mem;
-	_read_mem(pintfhdl, addr, cnt, pmem);
-
-}
-
-void _rtw_write_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem)
-{
-	void (*_write_mem)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *pmem);
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-
-
-
-	_write_mem = pintfhdl->io_ops._write_mem;
-
-	_write_mem(pintfhdl, addr, cnt, pmem);
-
-
-}
-
 void _rtw_read_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem)
 {
 	u32 (*_read_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *pmem);
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 065b0d8e030a..7f30b00b3ce6 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -541,13 +541,11 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 	pops->_read8 = &usb_read8;
 	pops->_read16 = &usb_read16;
 	pops->_read32 = &usb_read32;
-	pops->_read_mem = &usb_read_mem;
 	pops->_read_port = &usb_read_port;
 	pops->_write8 = &usb_write8;
 	pops->_write16 = &usb_write16;
 	pops->_write32 = &usb_write32;
 	pops->_writeN = &usb_writeN;
-	pops->_write_mem = &usb_write_mem;
 	pops->_write_port = &usb_write_port;
 	pops->_read_port_cancel = &usb_read_port_cancel;
 	pops->_write_port_cancel = &usb_write_port_cancel;
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 263a37d49b6e..5ef89c72cc83 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -270,8 +270,6 @@ void _rtw_write_port_cancel(struct adapter *adapter);
 #define rtw_read8(adapter, addr) _rtw_read8((adapter), (addr))
 #define rtw_read16(adapter, addr) _rtw_read16((adapter), (addr))
 #define rtw_read32(adapter, addr) _rtw_read32((adapter), (addr))
-#define rtw_read_mem(adapter, addr, cnt, mem)				\
-	_rtw_read_mem((adapter), (addr), (cnt), (mem))
 #define rtw_read_port(adapter, addr, cnt, mem)				\
 	_rtw_read_port((adapter), (addr), (cnt), (mem))
 #define rtw_read_port_cancel(adapter) _rtw_read_port_cancel((adapter))
@@ -290,8 +288,6 @@ void _rtw_write_port_cancel(struct adapter *adapter);
 	_rtw_write16_async((adapter), (addr), (val))
 #define rtw_write32_async(adapter, addr, val)				\
 	_rtw_write32_async((adapter), (addr), (val))
-#define rtw_write_mem(adapter, addr, cnt, mem)				\
-	_rtw_write_mem((adapter), (addr), (cnt), (mem))
 #define rtw_write_port(adapter, addr, cnt, mem)				\
 	_rtw_write_port((adapter), (addr), (cnt), (mem))
 #define rtw_write_port_and_wait(adapter, addr, cnt, mem, timeout_ms)	\
diff --git a/drivers/staging/r8188eu/include/usb_ops_linux.h b/drivers/staging/r8188eu/include/usb_ops_linux.h
index c357a3b1560e..37e0614fd15c 100644
--- a/drivers/staging/r8188eu/include/usb_ops_linux.h
+++ b/drivers/staging/r8188eu/include/usb_ops_linux.h
@@ -28,9 +28,6 @@
 
 unsigned int ffaddr2pipehdl(struct dvobj_priv *pdvobj, u32 addr);
 
-void usb_read_mem(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *rmem);
-void usb_write_mem(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *wmem);
-
 void usb_read_port_cancel(struct intf_hdl *pintfhdl);
 
 u32 usb_write_port(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *wmem);
diff --git a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
index 928730158450..9afb4df71969 100644
--- a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
@@ -31,14 +31,6 @@ struct zero_bulkout_context {
 	void *padapter;
 };
 
-void usb_read_mem(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *rmem)
-{
-}
-
-void usb_write_mem(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *wmem)
-{
-}
-
 void usb_read_port_cancel(struct intf_hdl *pintfhdl)
 {
 	int i;
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 02/19] staging: r8188eu: remove the helpers of rtw_read8()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 01/19] staging: r8188eu: remove usb_{read,write}_mem() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 03/19] staging: r8188eu: remove the helpers of rtw_read16() Fabio M. De Francesco
                   ` (17 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_read8() and usb_read8() and embed their
code into the caller (i.e., rtw_read8()).

_rtw_read8() is a mere redefinition of rtw_read8() and it is unneeded.
usb_read8() was the only functions assigned to (*_usb_read8) pointer,
so we can simply remove it and make a direct call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c       | 14 --------------
 drivers/staging/r8188eu/hal/usb_ops_linux.c |  5 +++--
 drivers/staging/r8188eu/include/rtw_io.h    |  3 +--
 3 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index e6f377377ab2..4c43b6d00178 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -34,20 +34,6 @@ jackson@realtek.com.tw
 #define rtw_cpu_to_le16(val)		cpu_to_le16(val)
 #define rtw_cpu_to_le32(val)		cpu_to_le32(val)
 
-u8 _rtw_read8(struct adapter *adapter, u32 addr)
-{
-	u8 r_val;
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl *pintfhdl = &pio_priv->intf;
-	u8 (*_read8)(struct intf_hdl *pintfhdl, u32 addr);
-
-
-	_read8 = pintfhdl->io_ops._read8;
-	r_val = _read8(pintfhdl, addr);
-
-	return r_val;
-}
-
 u16 _rtw_read16(struct adapter *adapter, u32 addr)
 {
 	u16 r_val;
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 7f30b00b3ce6..8389deeb1182 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -97,8 +97,10 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 	return status;
 }
 
-static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)
+u8 rtw_read8(struct adapter *adapter, u32 addr)
 {
+	struct io_priv *pio_priv = &adapter->iopriv;
+	struct intf_hdl *pintfhdl = &pio_priv->intf;
 	u16 wvalue = (u16)(addr & 0x0000ffff);
 	u8 data;
 
@@ -538,7 +540,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 {
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
-	pops->_read8 = &usb_read8;
 	pops->_read16 = &usb_read16;
 	pops->_read32 = &usb_read32;
 	pops->_read_port = &usb_read_port;
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 5ef89c72cc83..9dc32f7bcae8 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -245,7 +245,7 @@ void unregister_intf_hdl(struct intf_hdl *pintfhdl);
 void _rtw_attrib_read(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
-u8 _rtw_read8(struct adapter *adapter, u32 addr);
+u8 rtw_read8(struct adapter *adapter, u32 addr);
 u16 _rtw_read16(struct adapter *adapter, u32 addr);
 u32 _rtw_read32(struct adapter *adapter, u32 addr);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
@@ -267,7 +267,6 @@ u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt,
 			     u8 *pmem, int timeout_ms);
 void _rtw_write_port_cancel(struct adapter *adapter);
 
-#define rtw_read8(adapter, addr) _rtw_read8((adapter), (addr))
 #define rtw_read16(adapter, addr) _rtw_read16((adapter), (addr))
 #define rtw_read32(adapter, addr) _rtw_read32((adapter), (addr))
 #define rtw_read_port(adapter, addr, cnt, mem)				\
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 03/19] staging: r8188eu: remove the helpers of rtw_read16()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 01/19] staging: r8188eu: remove usb_{read,write}_mem() Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 02/19] staging: r8188eu: remove the helpers of rtw_read8() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 04/19] staging: r8188eu: remove the helpers of rtw_read32() Fabio M. De Francesco
                   ` (16 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_read16() and usb_read16() and embed their
code into the caller (i.e., rtw_read16()).

_rtw_read16() is a mere redefinition of rtw_read16() and it is unneeded.
usb_read16() was the only functions assigned to the (*_usb_read16) pointer,
so we can simply remove it and make a direct call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c       | 14 --------------
 drivers/staging/r8188eu/hal/usb_ops_linux.c |  5 +++--
 drivers/staging/r8188eu/include/rtw_io.h    |  3 +--
 3 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index 4c43b6d00178..b5d1c8e52b22 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -34,20 +34,6 @@ jackson@realtek.com.tw
 #define rtw_cpu_to_le16(val)		cpu_to_le16(val)
 #define rtw_cpu_to_le32(val)		cpu_to_le32(val)
 
-u16 _rtw_read16(struct adapter *adapter, u32 addr)
-{
-	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);
-
-	_read16 = pintfhdl->io_ops._read16;
-
-	r_val = _read16(pintfhdl, addr);
-
-	return r_val;
-}
-
 u32 _rtw_read32(struct adapter *adapter, u32 addr)
 {
 	u32 r_val;
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 8389deeb1182..8b4fc014d93a 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -109,8 +109,10 @@ u8 rtw_read8(struct adapter *adapter, u32 addr)
 	return data;
 }
 
-static u16 usb_read16(struct intf_hdl *pintfhdl, u32 addr)
+u16 rtw_read16(struct adapter *adapter, u32 addr)
 {
+	struct io_priv *pio_priv = &adapter->iopriv;
+	struct intf_hdl *pintfhdl = &pio_priv->intf;
 	u16 wvalue = (u16)(addr & 0x0000ffff);
 	__le32 data;
 
@@ -540,7 +542,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 {
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
-	pops->_read16 = &usb_read16;
 	pops->_read32 = &usb_read32;
 	pops->_read_port = &usb_read_port;
 	pops->_write8 = &usb_write8;
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 9dc32f7bcae8..527503d3ecc8 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -246,7 +246,7 @@ void _rtw_attrib_read(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
 u8 rtw_read8(struct adapter *adapter, u32 addr);
-u16 _rtw_read16(struct adapter *adapter, u32 addr);
+u16 rtw_read16(struct adapter *adapter, u32 addr);
 u32 _rtw_read32(struct adapter *adapter, u32 addr);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_read_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
@@ -267,7 +267,6 @@ u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt,
 			     u8 *pmem, int timeout_ms);
 void _rtw_write_port_cancel(struct adapter *adapter);
 
-#define rtw_read16(adapter, addr) _rtw_read16((adapter), (addr))
 #define rtw_read32(adapter, addr) _rtw_read32((adapter), (addr))
 #define rtw_read_port(adapter, addr, cnt, mem)				\
 	_rtw_read_port((adapter), (addr), (cnt), (mem))
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 04/19] staging: r8188eu: remove the helpers of rtw_read32()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (2 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 03/19] staging: r8188eu: remove the helpers of rtw_read16() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 05/19] staging: r8188eu: remove the helpers of usb_write8() Fabio M. De Francesco
                   ` (15 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_readr32() and usb_read32() and embed their
code into the caller (i.e., rtw_read32()).

_rtw_read32() is a mere redefinition of rtw_read32() and it is unneeded.
usb_read32() was the only functions assigned to the (*_usb_read32) pointer,
so we can simply remove it and make a direct call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c       | 14 --------------
 drivers/staging/r8188eu/hal/usb_ops_linux.c |  5 +++--
 drivers/staging/r8188eu/include/rtw_io.h    |  3 +--
 3 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index b5d1c8e52b22..cb24500cbc6e 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -34,20 +34,6 @@ jackson@realtek.com.tw
 #define rtw_cpu_to_le16(val)		cpu_to_le16(val)
 #define rtw_cpu_to_le32(val)		cpu_to_le32(val)
 
-u32 _rtw_read32(struct adapter *adapter, u32 addr)
-{
-	u32 r_val;
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-	u32	(*_read32)(struct intf_hdl *pintfhdl, u32 addr);
-
-	_read32 = pintfhdl->io_ops._read32;
-
-	r_val = _read32(pintfhdl, addr);
-
-	return r_val;
-}
-
 int _rtw_write8(struct adapter *adapter, u32 addr, u8 val)
 {
 	struct io_priv *pio_priv = &adapter->iopriv;
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 8b4fc014d93a..39fd9994787d 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -121,8 +121,10 @@ u16 rtw_read16(struct adapter *adapter, u32 addr)
 	return (u16)(le32_to_cpu(data) & 0xffff);
 }
 
-static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
+u32 rtw_read32(struct adapter *adapter, u32 addr)
 {
+	struct io_priv *pio_priv = &adapter->iopriv;
+	struct intf_hdl *pintfhdl = &pio_priv->intf;
 	u16 wvalue = (u16)(addr & 0x0000ffff);
 	__le32 data;
 
@@ -542,7 +544,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 {
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
-	pops->_read32 = &usb_read32;
 	pops->_read_port = &usb_read_port;
 	pops->_write8 = &usb_write8;
 	pops->_write16 = &usb_write16;
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 527503d3ecc8..c53d9c8bd9a7 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -247,7 +247,7 @@ void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
 u8 rtw_read8(struct adapter *adapter, u32 addr);
 u16 rtw_read16(struct adapter *adapter, u32 addr);
-u32 _rtw_read32(struct adapter *adapter, u32 addr);
+u32 rtw_read32(struct adapter *adapter, u32 addr);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_read_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_read_port_cancel(struct adapter *adapter);
@@ -267,7 +267,6 @@ u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt,
 			     u8 *pmem, int timeout_ms);
 void _rtw_write_port_cancel(struct adapter *adapter);
 
-#define rtw_read32(adapter, addr) _rtw_read32((adapter), (addr))
 #define rtw_read_port(adapter, addr, cnt, mem)				\
 	_rtw_read_port((adapter), (addr), (cnt), (mem))
 #define rtw_read_port_cancel(adapter) _rtw_read_port_cancel((adapter))
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 05/19] staging: r8188eu: remove the helpers of usb_write8()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (3 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 04/19] staging: r8188eu: remove the helpers of rtw_read32() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 06/19] staging: r8188eu: remove the helpers of usb_write16() Fabio M. De Francesco
                   ` (14 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_write8() and usb_write8() and embed their
code into the caller (i.e., rtw_write8()).

_rtw_write8() is a mere redefinition of rtw_write8() and it is unneeded.
usb_write8() was the only functions assigned to the (*_usb_write8) pointer,
so we can simply remove it and make a direct call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c       | 15 ---------------
 drivers/staging/r8188eu/hal/usb_ops_linux.c | 10 +++++++---
 drivers/staging/r8188eu/include/rtw_io.h    |  4 +---
 3 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index cb24500cbc6e..69ab6e24a4b7 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -34,21 +34,6 @@ jackson@realtek.com.tw
 #define rtw_cpu_to_le16(val)		cpu_to_le16(val)
 #define rtw_cpu_to_le32(val)		cpu_to_le32(val)
 
-int _rtw_write8(struct adapter *adapter, u32 addr, u8 val)
-{
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-	int (*_write8)(struct intf_hdl *pintfhdl, u32 addr, u8 val);
-	int ret;
-
-	_write8 = pintfhdl->io_ops._write8;
-
-	ret = _write8(pintfhdl, addr, val);
-
-
-	return RTW_STATUS_CODE(ret);
-}
-
 int _rtw_write16(struct adapter *adapter, u32 addr, u16 val)
 {
 	struct io_priv *pio_priv = &adapter->iopriv;
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 39fd9994787d..69e64863a5d1 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -133,11 +133,16 @@ u32 rtw_read32(struct adapter *adapter, u32 addr)
 	return le32_to_cpu(data);
 }
 
-static int usb_write8(struct intf_hdl *pintfhdl, u32 addr, u8 val)
+int rtw_write8(struct adapter *adapter, u32 addr, u8 val)
 {
+	struct io_priv *pio_priv = &adapter->iopriv;
+	struct intf_hdl *pintfhdl = &pio_priv->intf;
 	u16 wvalue = (u16)(addr & 0x0000ffff);
+	int ret;
+
+	ret = usbctrl_vendorreq(pintfhdl, wvalue, &val, 1, REALTEK_USB_VENQT_WRITE);
 
-	return usbctrl_vendorreq(pintfhdl, wvalue, &val, 1, REALTEK_USB_VENQT_WRITE);
+	return RTW_STATUS_CODE(ret);
 }
 
 static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
@@ -545,7 +550,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
 	pops->_read_port = &usb_read_port;
-	pops->_write8 = &usb_write8;
 	pops->_write16 = &usb_write16;
 	pops->_write32 = &usb_write32;
 	pops->_writeN = &usb_writeN;
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index c53d9c8bd9a7..3b229027f094 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -252,7 +252,7 @@ void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_read_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_read_port_cancel(struct adapter *adapter);
 
-int _rtw_write8(struct adapter *adapter, u32 addr, u8 val);
+int rtw_write8(struct adapter *adapter, u32 addr, u8 val);
 int _rtw_write16(struct adapter *adapter, u32 addr, u16 val);
 int _rtw_write32(struct adapter *adapter, u32 addr, u32 val);
 int _rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata);
@@ -271,8 +271,6 @@ void _rtw_write_port_cancel(struct adapter *adapter);
 	_rtw_read_port((adapter), (addr), (cnt), (mem))
 #define rtw_read_port_cancel(adapter) _rtw_read_port_cancel((adapter))
 
-#define  rtw_write8(adapter, addr, val)					\
-	_rtw_write8((adapter), (addr), (val))
 #define  rtw_write16(adapter, addr, val)				\
 	_rtw_write16((adapter), (addr), (val))
 #define  rtw_write32(adapter, addr, val)				\
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 06/19] staging: r8188eu: remove the helpers of usb_write16()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (4 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 05/19] staging: r8188eu: remove the helpers of usb_write8() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 07/19] staging: r8188eu: remove the helpers of usb_write32() Fabio M. De Francesco
                   ` (13 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_write16() and usb_write16() and embed their
code into the caller (i.e., rtw_write16()).

_rtw_write16() is a mere redefinition of rtw_write16() and it is unneeded.
usb_write16() was the only functions assigned to the (*_usb_write16)
pointer, so we can simply remove it and make a direct call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c       | 14 --------------
 drivers/staging/r8188eu/hal/usb_ops_linux.c | 10 +++++++---
 drivers/staging/r8188eu/include/rtw_io.h    |  4 +---
 3 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index 69ab6e24a4b7..7547e25d870b 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -34,20 +34,6 @@ jackson@realtek.com.tw
 #define rtw_cpu_to_le16(val)		cpu_to_le16(val)
 #define rtw_cpu_to_le32(val)		cpu_to_le32(val)
 
-int _rtw_write16(struct adapter *adapter, u32 addr, u16 val)
-{
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-	int (*_write16)(struct intf_hdl *pintfhdl, u32 addr, u16 val);
-	int ret;
-
-	_write16 = pintfhdl->io_ops._write16;
-
-	ret = _write16(pintfhdl, addr, val);
-
-
-	return RTW_STATUS_CODE(ret);
-}
 int _rtw_write32(struct adapter *adapter, u32 addr, u32 val)
 {
 	struct io_priv *pio_priv = &adapter->iopriv;
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 69e64863a5d1..128cdb178abc 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -145,12 +145,17 @@ int rtw_write8(struct adapter *adapter, u32 addr, u8 val)
 	return RTW_STATUS_CODE(ret);
 }
 
-static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
+int rtw_write16(struct adapter *adapter, u32 addr, u16 val)
 {
+	struct io_priv *pio_priv = &adapter->iopriv;
+	struct intf_hdl *pintfhdl = &pio_priv->intf;
 	u16 wvalue = (u16)(addr & 0x0000ffff);
 	__le32 data = cpu_to_le32(val & 0x0000ffff);
+	int ret;
 
-	return usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_WRITE);
+	ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_WRITE);
+
+	return RTW_STATUS_CODE(ret);
 }
 
 static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
@@ -550,7 +555,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
 	pops->_read_port = &usb_read_port;
-	pops->_write16 = &usb_write16;
 	pops->_write32 = &usb_write32;
 	pops->_writeN = &usb_writeN;
 	pops->_write_port = &usb_write_port;
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 3b229027f094..0cb1f626a549 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -253,7 +253,7 @@ void _rtw_read_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_read_port_cancel(struct adapter *adapter);
 
 int rtw_write8(struct adapter *adapter, u32 addr, u8 val);
-int _rtw_write16(struct adapter *adapter, u32 addr, u16 val);
+int rtw_write16(struct adapter *adapter, u32 addr, u16 val);
 int _rtw_write32(struct adapter *adapter, u32 addr, u32 val);
 int _rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata);
 
@@ -271,8 +271,6 @@ void _rtw_write_port_cancel(struct adapter *adapter);
 	_rtw_read_port((adapter), (addr), (cnt), (mem))
 #define rtw_read_port_cancel(adapter) _rtw_read_port_cancel((adapter))
 
-#define  rtw_write16(adapter, addr, val)				\
-	_rtw_write16((adapter), (addr), (val))
 #define  rtw_write32(adapter, addr, val)				\
 	_rtw_write32((adapter), (addr), (val))
 #define  rtw_writeN(adapter, addr, length, data)			\
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 07/19] staging: r8188eu: remove the helpers of usb_write32()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (5 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 06/19] staging: r8188eu: remove the helpers of usb_write16() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 08/19] staging: r8188eu: remove the helpers of usb_writeN() Fabio M. De Francesco
                   ` (12 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_write32() and usb_write32() and embed their
code into the caller (i.e., rtw_write32()).

_rtw_write32() is a mere redefinition of rtw_write32() and it is unneeded.
usb_write32() was the only functions assigned to the (*_usb_write32)
pointer, so we can simply remove it and make a direct call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c       | 15 ---------------
 drivers/staging/r8188eu/hal/usb_ops_linux.c | 10 +++++++---
 drivers/staging/r8188eu/include/rtw_io.h    |  4 +---
 3 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index 7547e25d870b..98c9823fe53b 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -34,21 +34,6 @@ jackson@realtek.com.tw
 #define rtw_cpu_to_le16(val)		cpu_to_le16(val)
 #define rtw_cpu_to_le32(val)		cpu_to_le32(val)
 
-int _rtw_write32(struct adapter *adapter, u32 addr, u32 val)
-{
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-	int (*_write32)(struct intf_hdl *pintfhdl, u32 addr, u32 val);
-	int ret;
-
-	_write32 = pintfhdl->io_ops._write32;
-
-	ret = _write32(pintfhdl, addr, val);
-
-
-	return RTW_STATUS_CODE(ret);
-}
-
 int _rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata)
 {
 	struct io_priv *pio_priv = &adapter->iopriv;
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 128cdb178abc..73f90b21364e 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -158,12 +158,17 @@ int rtw_write16(struct adapter *adapter, u32 addr, u16 val)
 	return RTW_STATUS_CODE(ret);
 }
 
-static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
+int rtw_write32(struct adapter *adapter, u32 addr, u32 val)
 {
+	struct io_priv *pio_priv = &adapter->iopriv;
+	struct intf_hdl *pintfhdl = &pio_priv->intf;
 	u16 wvalue = (u16)(addr & 0x0000ffff);
 	__le32 data = cpu_to_le32(val);
+	int ret;
 
-	return usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_WRITE);
+	ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_WRITE);
+
+	return RTW_STATUS_CODE(ret);
 }
 
 static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
@@ -555,7 +560,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
 	pops->_read_port = &usb_read_port;
-	pops->_write32 = &usb_write32;
 	pops->_writeN = &usb_writeN;
 	pops->_write_port = &usb_write_port;
 	pops->_read_port_cancel = &usb_read_port_cancel;
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 0cb1f626a549..83e2ed13b667 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -254,7 +254,7 @@ void _rtw_read_port_cancel(struct adapter *adapter);
 
 int rtw_write8(struct adapter *adapter, u32 addr, u8 val);
 int rtw_write16(struct adapter *adapter, u32 addr, u16 val);
-int _rtw_write32(struct adapter *adapter, u32 addr, u32 val);
+int rtw_write32(struct adapter *adapter, u32 addr, u32 val);
 int _rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata);
 
 int _rtw_write8_async(struct adapter *adapter, u32 addr, u8 val);
@@ -271,8 +271,6 @@ void _rtw_write_port_cancel(struct adapter *adapter);
 	_rtw_read_port((adapter), (addr), (cnt), (mem))
 #define rtw_read_port_cancel(adapter) _rtw_read_port_cancel((adapter))
 
-#define  rtw_write32(adapter, addr, val)				\
-	_rtw_write32((adapter), (addr), (val))
 #define  rtw_writeN(adapter, addr, length, data)			\
 	_rtw_writeN((adapter), (addr), (length), (data))
 #define rtw_write8_async(adapter, addr, val)				\
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 08/19] staging: r8188eu: remove the helpers of usb_writeN()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (6 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 07/19] staging: r8188eu: remove the helpers of usb_write32() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 09/19] staging: r8188eu: remove the helpers of usb_read_port() Fabio M. De Francesco
                   ` (11 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_writeN() and usb_writeN() and embed their
code into the caller (i.e., rtw_writeN()).

_rtw_writeN() is a mere redefinition of rtw_writeN() and it is unneeded.
usb_writeN() was the only functions assigned to the (*_usb_writeN) pointer,
so we can simply remove it and make a direct call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c       | 14 --------------
 drivers/staging/r8188eu/hal/usb_ops_linux.c | 11 +++++++----
 drivers/staging/r8188eu/include/rtw_io.h    |  4 +---
 3 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index 98c9823fe53b..b2e41fa48d81 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -34,20 +34,6 @@ jackson@realtek.com.tw
 #define rtw_cpu_to_le16(val)		cpu_to_le16(val)
 #define rtw_cpu_to_le32(val)		cpu_to_le32(val)
 
-int _rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata)
-{
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl *pintfhdl = (struct intf_hdl *)(&pio_priv->intf);
-	int (*_writeN)(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata);
-	int ret;
-
-	_writeN = pintfhdl->io_ops._writeN;
-
-	ret = _writeN(pintfhdl, addr, length, pdata);
-
-
-	return RTW_STATUS_CODE(ret);
-}
 int _rtw_write8_async(struct adapter *adapter, u32 addr, u8 val)
 {
 	struct io_priv *pio_priv = &adapter->iopriv;
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 73f90b21364e..90fb46c75159 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -171,17 +171,21 @@ int rtw_write32(struct adapter *adapter, u32 addr, u32 val)
 	return RTW_STATUS_CODE(ret);
 }
 
-static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
+int rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata)
 {
+	struct io_priv *pio_priv = &adapter->iopriv;
+	struct intf_hdl *pintfhdl = &pio_priv->intf;
 	u16 wvalue = (u16)(addr & 0x0000ffff);
 	u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
+	int ret;
 
 	if (length > VENDOR_CMD_MAX_DATA_LEN)
-		return -EINVAL;
+		return _FAIL;
 
 	memcpy(buf, pdata, length);
+	ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
 
-	return usbctrl_vendorreq(pintfhdl, wvalue, buf, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
+	return RTW_STATUS_CODE(ret);
 }
 
 static void interrupt_handler_8188eu(struct adapter *adapt, u16 pkt_len, u8 *pbuf)
@@ -560,7 +564,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
 	pops->_read_port = &usb_read_port;
-	pops->_writeN = &usb_writeN;
 	pops->_write_port = &usb_write_port;
 	pops->_read_port_cancel = &usb_read_port_cancel;
 	pops->_write_port_cancel = &usb_write_port_cancel;
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 83e2ed13b667..1123017cac65 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -255,7 +255,7 @@ void _rtw_read_port_cancel(struct adapter *adapter);
 int rtw_write8(struct adapter *adapter, u32 addr, u8 val);
 int rtw_write16(struct adapter *adapter, u32 addr, u16 val);
 int rtw_write32(struct adapter *adapter, u32 addr, u32 val);
-int _rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata);
+int rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata);
 
 int _rtw_write8_async(struct adapter *adapter, u32 addr, u8 val);
 int _rtw_write16_async(struct adapter *adapter, u32 addr, u16 val);
@@ -271,8 +271,6 @@ void _rtw_write_port_cancel(struct adapter *adapter);
 	_rtw_read_port((adapter), (addr), (cnt), (mem))
 #define rtw_read_port_cancel(adapter) _rtw_read_port_cancel((adapter))
 
-#define  rtw_writeN(adapter, addr, length, data)			\
-	_rtw_writeN((adapter), (addr), (length), (data))
 #define rtw_write8_async(adapter, addr, val)				\
 	_rtw_write8_async((adapter), (addr), (val))
 #define rtw_write16_async(adapter, addr, val)				\
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 09/19] staging: r8188eu: remove the helpers of usb_read_port()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (7 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 08/19] staging: r8188eu: remove the helpers of usb_writeN() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 10/19] staging: r8188eu: remove the helpers of usb_write_port() Fabio M. De Francesco
                   ` (10 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_read_port() and usb_read_port() and embed their
code into the caller (i.e., rtw_read_port()).

_rtw_read_port() is a mere redefinition of rtw_read_port() and it is
unneeded. usb_read_port() was the only functions assigned to the
(*_usb_read_port) pointer, so we can simply remove it and make a direct
call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c       | 18 ------------------
 drivers/staging/r8188eu/hal/usb_halinit.c   |  6 +-----
 drivers/staging/r8188eu/hal/usb_ops_linux.c |  4 +---
 drivers/staging/r8188eu/include/rtw_io.h    |  4 +---
 4 files changed, 3 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index b2e41fa48d81..ac72f894da75 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -75,24 +75,6 @@ int _rtw_write32_async(struct adapter *adapter, u32 addr, u32 val)
 	return RTW_STATUS_CODE(ret);
 }
 
-void _rtw_read_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem)
-{
-	u32 (*_read_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *pmem);
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-
-
-
-	if (adapter->bDriverStopped || adapter->bSurpriseRemoved)
-	     return;
-
-	_read_port = pintfhdl->io_ops._read_port;
-
-	_read_port(pintfhdl, addr, cnt, pmem);
-
-
-}
-
 void _rtw_read_port_cancel(struct adapter *adapter)
 {
 	void (*_read_port_cancel)(struct intf_hdl *pintfhdl);
diff --git a/drivers/staging/r8188eu/hal/usb_halinit.c b/drivers/staging/r8188eu/hal/usb_halinit.c
index dfaa0195dbc3..5133d533674f 100644
--- a/drivers/staging/r8188eu/hal/usb_halinit.c
+++ b/drivers/staging/r8188eu/hal/usb_halinit.c
@@ -1033,11 +1033,7 @@ static unsigned int rtl8188eu_inirp_init(struct adapter *Adapter)
 	u8 i;
 	struct recv_buf *precvbuf;
 	uint	status;
-	struct intf_hdl *pintfhdl = &Adapter->iopriv.intf;
 	struct recv_priv *precvpriv = &Adapter->recvpriv;
-	u32 (*_read_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *pmem);
-
-	_read_port = pintfhdl->io_ops._read_port;
 
 	status = _SUCCESS;
 
@@ -1046,7 +1042,7 @@ static unsigned int rtl8188eu_inirp_init(struct adapter *Adapter)
 	/* issue Rx irp to receive data */
 	precvbuf = (struct recv_buf *)precvpriv->precv_buf;
 	for (i = 0; i < NR_RECVBUFF; i++) {
-		if (!_read_port(pintfhdl, precvpriv->ff_hwaddr, 0, (unsigned char *)precvbuf)) {
+		if (!rtw_read_port(Adapter, precvpriv->ff_hwaddr, 0, (unsigned char *)precvbuf)) {
 			status = _FAIL;
 			goto exit;
 		}
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 90fb46c75159..a104e3fac7d1 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -456,11 +456,10 @@ static void usb_read_port_complete(struct urb *purb, struct pt_regs *regs)
 	}
 }
 
-static u32 usb_read_port(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *rmem)
+u32 rtw_read_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *rmem)
 {
 	struct urb *purb = NULL;
 	struct recv_buf	*precvbuf = (struct recv_buf *)rmem;
-	struct adapter		*adapter = pintfhdl->padapter;
 	struct dvobj_priv	*pdvobj = adapter_to_dvobj(adapter);
 	struct recv_priv	*precvpriv = &adapter->recvpriv;
 	struct usb_device	*pusbd = pdvobj->pusbdev;
@@ -563,7 +562,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 {
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
-	pops->_read_port = &usb_read_port;
 	pops->_write_port = &usb_write_port;
 	pops->_read_port_cancel = &usb_read_port_cancel;
 	pops->_write_port_cancel = &usb_write_port_cancel;
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 1123017cac65..600c6e7a375b 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -249,7 +249,7 @@ u8 rtw_read8(struct adapter *adapter, u32 addr);
 u16 rtw_read16(struct adapter *adapter, u32 addr);
 u32 rtw_read32(struct adapter *adapter, u32 addr);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
-void _rtw_read_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
+u32 rtw_read_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_read_port_cancel(struct adapter *adapter);
 
 int rtw_write8(struct adapter *adapter, u32 addr, u8 val);
@@ -267,8 +267,6 @@ u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt,
 			     u8 *pmem, int timeout_ms);
 void _rtw_write_port_cancel(struct adapter *adapter);
 
-#define rtw_read_port(adapter, addr, cnt, mem)				\
-	_rtw_read_port((adapter), (addr), (cnt), (mem))
 #define rtw_read_port_cancel(adapter) _rtw_read_port_cancel((adapter))
 
 #define rtw_write8_async(adapter, addr, val)				\
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 10/19] staging: r8188eu: remove the helpers of usb_write_port()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (8 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 09/19] staging: r8188eu: remove the helpers of usb_read_port() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 11/19] staging: r8188eu: remove the helpers of usb_read_port_cancel() Fabio M. De Francesco
                   ` (9 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_write_port() and usb_write_port() and embed
their code into the caller (i.e., rtw_write_port()).

_rtw_write_port() is a mere redefinition of rtw_write_port() and it is
unneeded. usb_write_port() was the only functions assigned to the
(*_usb_write_port) pointer, so we can simply remove it and make a direct
call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c         | 20 +------------------
 drivers/staging/r8188eu/hal/usb_ops_linux.c   |  1 -
 drivers/staging/r8188eu/include/rtw_io.h      |  4 +---
 .../staging/r8188eu/include/usb_ops_linux.h   |  1 -
 .../staging/r8188eu/os_dep/usb_ops_linux.c    |  3 +--
 5 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index ac72f894da75..3a5e9dbfcb12 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -87,24 +87,6 @@ void _rtw_read_port_cancel(struct adapter *adapter)
 		_read_port_cancel(pintfhdl);
 }
 
-u32 _rtw_write_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem)
-{
-	u32 (*_write_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *pmem);
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-	u32 ret = _SUCCESS;
-
-
-
-	_write_port = pintfhdl->io_ops._write_port;
-
-	ret = _write_port(pintfhdl, addr, cnt, pmem);
-
-
-
-	return ret;
-}
-
 u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem, int timeout_ms)
 {
 	int ret = _SUCCESS;
@@ -114,7 +96,7 @@ u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt, u8 *pme
 	rtw_sctx_init(&sctx, timeout_ms);
 	pxmitbuf->sctx = &sctx;
 
-	ret = _rtw_write_port(adapter, addr, cnt, pmem);
+	ret = rtw_write_port(adapter, addr, cnt, pmem);
 
 	if (ret == _SUCCESS)
 		ret = rtw_sctx_wait(&sctx);
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index a104e3fac7d1..4fea21c0f7af 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -562,7 +562,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 {
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
-	pops->_write_port = &usb_write_port;
 	pops->_read_port_cancel = &usb_read_port_cancel;
 	pops->_write_port_cancel = &usb_write_port_cancel;
 
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 600c6e7a375b..f2b1978b6e80 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -262,7 +262,7 @@ int _rtw_write16_async(struct adapter *adapter, u32 addr, u16 val);
 int _rtw_write32_async(struct adapter *adapter, u32 addr, u32 val);
 
 void _rtw_write_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
-u32 _rtw_write_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
+u32 rtw_write_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt,
 			     u8 *pmem, int timeout_ms);
 void _rtw_write_port_cancel(struct adapter *adapter);
@@ -275,8 +275,6 @@ void _rtw_write_port_cancel(struct adapter *adapter);
 	_rtw_write16_async((adapter), (addr), (val))
 #define rtw_write32_async(adapter, addr, val)				\
 	_rtw_write32_async((adapter), (addr), (val))
-#define rtw_write_port(adapter, addr, cnt, mem)				\
-	_rtw_write_port((adapter), (addr), (cnt), (mem))
 #define rtw_write_port_and_wait(adapter, addr, cnt, mem, timeout_ms)	\
 	_rtw_write_port_and_wait((adapter), (addr), (cnt), (mem), (timeout_ms))
 #define rtw_write_port_cancel(adapter) _rtw_write_port_cancel((adapter))
diff --git a/drivers/staging/r8188eu/include/usb_ops_linux.h b/drivers/staging/r8188eu/include/usb_ops_linux.h
index 37e0614fd15c..bdc596fe5854 100644
--- a/drivers/staging/r8188eu/include/usb_ops_linux.h
+++ b/drivers/staging/r8188eu/include/usb_ops_linux.h
@@ -30,7 +30,6 @@ unsigned int ffaddr2pipehdl(struct dvobj_priv *pdvobj, u32 addr);
 
 void usb_read_port_cancel(struct intf_hdl *pintfhdl);
 
-u32 usb_write_port(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *wmem);
 void usb_write_port_cancel(struct intf_hdl *pintfhdl);
 
 #endif
diff --git a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
index 9afb4df71969..36ef06f88fdd 100644
--- a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
@@ -124,14 +124,13 @@ static void usb_write_port_complete(struct urb *purb, struct pt_regs *regs)
 
 }
 
-u32 usb_write_port(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *wmem)
+u32 rtw_write_port(struct adapter *padapter, u32 addr, u32 cnt, u8 *wmem)
 {
 	unsigned long irqL;
 	unsigned int pipe;
 	int status;
 	u32 ret = _FAIL;
 	struct urb *purb = NULL;
-	struct adapter *padapter = (struct adapter *)pintfhdl->padapter;
 	struct dvobj_priv	*pdvobj = adapter_to_dvobj(padapter);
 	struct xmit_priv	*pxmitpriv = &padapter->xmitpriv;
 	struct xmit_buf *pxmitbuf = (struct xmit_buf *)wmem;
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 11/19] staging: r8188eu: remove the helpers of usb_read_port_cancel()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (9 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 10/19] staging: r8188eu: remove the helpers of usb_write_port() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 12/19] staging: r8188eu: remove the helpers of usb_write_port_cancel() Fabio M. De Francesco
                   ` (8 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_read_port_cancel() and usb_read_port_cancel()
and embed their code into the caller (i.e., rtw_read_port_cancel()).

_rtw_read_port_cancel() is a mere redefinition of rtw_read_port_cancel()
and it is unneeded. usb_read_port_cancel() was the only functions assigned
to the (*_usb_read_port_cancel) pointer, so we can simply remove it and
make a direct call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c           | 11 -----------
 drivers/staging/r8188eu/hal/usb_ops_linux.c     |  1 -
 drivers/staging/r8188eu/include/rtw_io.h        |  3 +--
 drivers/staging/r8188eu/include/usb_ops_linux.h |  2 --
 drivers/staging/r8188eu/os_dep/usb_ops_linux.c  |  6 ++----
 5 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index 3a5e9dbfcb12..a57742057a65 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -75,17 +75,6 @@ int _rtw_write32_async(struct adapter *adapter, u32 addr, u32 val)
 	return RTW_STATUS_CODE(ret);
 }
 
-void _rtw_read_port_cancel(struct adapter *adapter)
-{
-	void (*_read_port_cancel)(struct intf_hdl *pintfhdl);
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct intf_hdl *pintfhdl = &pio_priv->intf;
-
-	_read_port_cancel = pintfhdl->io_ops._read_port_cancel;
-
-	if (_read_port_cancel)
-		_read_port_cancel(pintfhdl);
-}
 
 u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem, int timeout_ms)
 {
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 4fea21c0f7af..1865a26142bc 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -562,7 +562,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 {
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
-	pops->_read_port_cancel = &usb_read_port_cancel;
 	pops->_write_port_cancel = &usb_write_port_cancel;
 
 }
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index f2b1978b6e80..56e17e2a7ee2 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -250,7 +250,7 @@ u16 rtw_read16(struct adapter *adapter, u32 addr);
 u32 rtw_read32(struct adapter *adapter, u32 addr);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 u32 rtw_read_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
-void _rtw_read_port_cancel(struct adapter *adapter);
+void rtw_read_port_cancel(struct adapter *adapter);
 
 int rtw_write8(struct adapter *adapter, u32 addr, u8 val);
 int rtw_write16(struct adapter *adapter, u32 addr, u16 val);
@@ -267,7 +267,6 @@ u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt,
 			     u8 *pmem, int timeout_ms);
 void _rtw_write_port_cancel(struct adapter *adapter);
 
-#define rtw_read_port_cancel(adapter) _rtw_read_port_cancel((adapter))
 
 #define rtw_write8_async(adapter, addr, val)				\
 	_rtw_write8_async((adapter), (addr), (val))
diff --git a/drivers/staging/r8188eu/include/usb_ops_linux.h b/drivers/staging/r8188eu/include/usb_ops_linux.h
index bdc596fe5854..186c6b7628dc 100644
--- a/drivers/staging/r8188eu/include/usb_ops_linux.h
+++ b/drivers/staging/r8188eu/include/usb_ops_linux.h
@@ -28,8 +28,6 @@
 
 unsigned int ffaddr2pipehdl(struct dvobj_priv *pdvobj, u32 addr);
 
-void usb_read_port_cancel(struct intf_hdl *pintfhdl);
-
 void usb_write_port_cancel(struct intf_hdl *pintfhdl);
 
 #endif
diff --git a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
index 36ef06f88fdd..a98ffdf92ed4 100644
--- a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
@@ -31,12 +31,10 @@ struct zero_bulkout_context {
 	void *padapter;
 };
 
-void usb_read_port_cancel(struct intf_hdl *pintfhdl)
+void rtw_read_port_cancel(struct adapter *padapter)
 {
 	int i;
-	struct recv_buf *precvbuf;
-	struct adapter	*padapter = pintfhdl->padapter;
-	precvbuf = (struct recv_buf *)padapter->recvpriv.precv_buf;
+	struct recv_buf *precvbuf = (struct recv_buf *)padapter->recvpriv.precv_buf;
 
 	DBG_88E("%s\n", __func__);
 
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 12/19] staging: r8188eu: remove the helpers of usb_write_port_cancel()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (10 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 11/19] staging: r8188eu: remove the helpers of usb_read_port_cancel() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 13/19] staging: r8188eu: remove core/rtw_io.c Fabio M. De Francesco
                   ` (7 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the unnecessary _rtw_write_port_cancel() and usb_write_port_cancel()
and embed their code into the caller (i.e., rtw_write_port_cancel()).

_rtw_write_port_cancel() is a mere redefinition of rtw_write_port_cancel()
and it is unneeded. usb_write_port_cancel() was the only functions
assigned to the (*_usb_write_port_cancel) pointer, so we can simply remove
it and make a direct call.

This patch is in preparation for the _io_ops structure removal.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_io.c           | 12 ------------
 drivers/staging/r8188eu/hal/usb_ops_linux.c     |  2 --
 drivers/staging/r8188eu/include/rtw_io.h        |  3 +--
 drivers/staging/r8188eu/include/usb_ops_linux.h |  2 --
 drivers/staging/r8188eu/os_dep/usb_ops_linux.c  |  3 +--
 5 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index a57742057a65..74b02ff8e44d 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -93,18 +93,6 @@ u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt, u8 *pme
 	return ret;
 }
 
-void _rtw_write_port_cancel(struct adapter *adapter)
-{
-	void (*_write_port_cancel)(struct intf_hdl *pintfhdl);
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct intf_hdl *pintfhdl = &pio_priv->intf;
-
-	_write_port_cancel = pintfhdl->io_ops._write_port_cancel;
-
-	if (_write_port_cancel)
-		_write_port_cancel(pintfhdl);
-}
-
 int rtw_init_io_priv(struct adapter *padapter, void (*set_intf_ops)(struct _io_ops *pops))
 {
 	struct io_priv	*piopriv = &padapter->iopriv;
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 1865a26142bc..2516cfc464a9 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -562,8 +562,6 @@ void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
 {
 
 	memset((u8 *)pops, 0, sizeof(struct _io_ops));
-	pops->_write_port_cancel = &usb_write_port_cancel;
-
 }
 
 void rtl8188eu_set_hw_type(struct adapter *adapt)
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 56e17e2a7ee2..4f4678a55687 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -265,7 +265,7 @@ void _rtw_write_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 u32 rtw_write_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt,
 			     u8 *pmem, int timeout_ms);
-void _rtw_write_port_cancel(struct adapter *adapter);
+void rtw_write_port_cancel(struct adapter *adapter);
 
 
 #define rtw_write8_async(adapter, addr, val)				\
@@ -276,7 +276,6 @@ void _rtw_write_port_cancel(struct adapter *adapter);
 	_rtw_write32_async((adapter), (addr), (val))
 #define rtw_write_port_and_wait(adapter, addr, cnt, mem, timeout_ms)	\
 	_rtw_write_port_and_wait((adapter), (addr), (cnt), (mem), (timeout_ms))
-#define rtw_write_port_cancel(adapter) _rtw_write_port_cancel((adapter))
 
 /* ioreq */
 void ioreq_read8(struct adapter *adapter, u32 addr, u8 *pval);
diff --git a/drivers/staging/r8188eu/include/usb_ops_linux.h b/drivers/staging/r8188eu/include/usb_ops_linux.h
index 186c6b7628dc..641f059ffaf7 100644
--- a/drivers/staging/r8188eu/include/usb_ops_linux.h
+++ b/drivers/staging/r8188eu/include/usb_ops_linux.h
@@ -28,6 +28,4 @@
 
 unsigned int ffaddr2pipehdl(struct dvobj_priv *pdvobj, u32 addr);
 
-void usb_write_port_cancel(struct intf_hdl *pintfhdl);
-
 #endif
diff --git a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
index a98ffdf92ed4..4085f3e6067d 100644
--- a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
@@ -211,10 +211,9 @@ u32 rtw_write_port(struct adapter *padapter, u32 addr, u32 cnt, u8 *wmem)
 	return ret;
 }
 
-void usb_write_port_cancel(struct intf_hdl *pintfhdl)
+void rtw_write_port_cancel(struct adapter *padapter)
 {
 	int i, j;
-	struct adapter	*padapter = pintfhdl->padapter;
 	struct xmit_buf *pxmitbuf = (struct xmit_buf *)padapter->xmitpriv.pxmitbuf;
 
 	DBG_88E("%s\n", __func__);
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 13/19] staging: r8188eu: remove core/rtw_io.c
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (11 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 12/19] staging: r8188eu: remove the helpers of usb_write_port_cancel() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 14/19] staging: r8188eu: remove struct _io_ops Fabio M. De Francesco
                   ` (6 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

There are only unused functions and macros and one function which can be
open-coded.

So, removed core/rtw_io.c at all, removed core/rtw_io.c from Makefile
and open-coded rtw_init_io_priv

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/Makefile          |   1 -
 drivers/staging/r8188eu/core/rtw_io.c     | 111 ----------------------
 drivers/staging/r8188eu/include/rtw_io.h  |  19 +---
 drivers/staging/r8188eu/os_dep/usb_intf.c |   8 +-
 4 files changed, 8 insertions(+), 131 deletions(-)
 delete mode 100644 drivers/staging/r8188eu/core/rtw_io.c

diff --git a/drivers/staging/r8188eu/Makefile b/drivers/staging/r8188eu/Makefile
index aebaf29990fd..4ca48fe628fd 100644
--- a/drivers/staging/r8188eu/Makefile
+++ b/drivers/staging/r8188eu/Makefile
@@ -78,7 +78,6 @@ rtk_core :=				\
 		core/rtw_debug.o	\
 		core/rtw_efuse.o	\
 		core/rtw_ieee80211.o	\
-		core/rtw_io.o		\
 		core/rtw_ioctl_set.o	\
 		core/rtw_iol.o		\
 		core/rtw_led.o		\
diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
deleted file mode 100644
index 74b02ff8e44d..000000000000
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ /dev/null
@@ -1,111 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* Copyright(c) 2007 - 2011 Realtek Corporation. */
-
-/*
-
-The purpose of rtw_io.c
-
-a. provides the API
-
-b. provides the protocol engine
-
-c. provides the software interface between caller and the hardware interface
-
-Compiler Flag Option:
-
-USB:
-   a. USE_ASYNC_IRP: Both sync/async operations are provided.
-
-Only sync read/rtw_write_mem operations are provided.
-
-jackson@realtek.com.tw
-
-*/
-
-#define _RTW_IO_C_
-#include "../include/osdep_service.h"
-#include "../include/drv_types.h"
-#include "../include/rtw_io.h"
-#include "../include/osdep_intf.h"
-#include "../include/usb_ops.h"
-
-#define rtw_le16_to_cpu(val)		le16_to_cpu(val)
-#define rtw_le32_to_cpu(val)		le32_to_cpu(val)
-#define rtw_cpu_to_le16(val)		cpu_to_le16(val)
-#define rtw_cpu_to_le32(val)		cpu_to_le32(val)
-
-int _rtw_write8_async(struct adapter *adapter, u32 addr, u8 val)
-{
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-	int (*_write8_async)(struct intf_hdl *pintfhdl, u32 addr, u8 val);
-	int ret;
-
-	_write8_async = pintfhdl->io_ops._write8_async;
-
-	ret = _write8_async(pintfhdl, addr, val);
-
-
-	return RTW_STATUS_CODE(ret);
-}
-
-int _rtw_write16_async(struct adapter *adapter, u32 addr, u16 val)
-{
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-	int (*_write16_async)(struct intf_hdl *pintfhdl, u32 addr, u16 val);
-	int ret;
-
-	_write16_async = pintfhdl->io_ops._write16_async;
-	ret = _write16_async(pintfhdl, addr, val);
-
-	return RTW_STATUS_CODE(ret);
-}
-
-int _rtw_write32_async(struct adapter *adapter, u32 addr, u32 val)
-{
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
-	int (*_write32_async)(struct intf_hdl *pintfhdl, u32 addr, u32 val);
-	int ret;
-
-	_write32_async = pintfhdl->io_ops._write32_async;
-	ret = _write32_async(pintfhdl, addr, val);
-
-	return RTW_STATUS_CODE(ret);
-}
-
-
-u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem, int timeout_ms)
-{
-	int ret = _SUCCESS;
-	struct xmit_buf *pxmitbuf = (struct xmit_buf *)pmem;
-	struct submit_ctx sctx;
-
-	rtw_sctx_init(&sctx, timeout_ms);
-	pxmitbuf->sctx = &sctx;
-
-	ret = rtw_write_port(adapter, addr, cnt, pmem);
-
-	if (ret == _SUCCESS)
-		ret = rtw_sctx_wait(&sctx);
-
-	return ret;
-}
-
-int rtw_init_io_priv(struct adapter *padapter, void (*set_intf_ops)(struct _io_ops *pops))
-{
-	struct io_priv	*piopriv = &padapter->iopriv;
-	struct intf_hdl *pintf = &piopriv->intf;
-
-	if (!set_intf_ops)
-		return _FAIL;
-
-	piopriv->padapter = padapter;
-	pintf->padapter = padapter;
-	pintf->pintf_dev = adapter_to_dvobj(padapter);
-
-	set_intf_ops(&pintf->io_ops);
-
-	return _SUCCESS;
-}
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 4f4678a55687..2b9b64f1ac80 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -257,25 +257,11 @@ int rtw_write16(struct adapter *adapter, u32 addr, u16 val);
 int rtw_write32(struct adapter *adapter, u32 addr, u32 val);
 int rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata);
 
-int _rtw_write8_async(struct adapter *adapter, u32 addr, u8 val);
-int _rtw_write16_async(struct adapter *adapter, u32 addr, u16 val);
-int _rtw_write32_async(struct adapter *adapter, u32 addr, u32 val);
-
 void _rtw_write_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 u32 rtw_write_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
-u32 _rtw_write_port_and_wait(struct adapter *adapter, u32 addr, u32 cnt,
-			     u8 *pmem, int timeout_ms);
 void rtw_write_port_cancel(struct adapter *adapter);
 
-
-#define rtw_write8_async(adapter, addr, val)				\
-	_rtw_write8_async((adapter), (addr), (val))
-#define rtw_write16_async(adapter, addr, val)				\
-	_rtw_write16_async((adapter), (addr), (val))
-#define rtw_write32_async(adapter, addr, val)				\
-	_rtw_write32_async((adapter), (addr), (val))
-#define rtw_write_port_and_wait(adapter, addr, cnt, mem, timeout_ms)	\
-	_rtw_write_port_and_wait((adapter), (addr), (cnt), (mem), (timeout_ms))
+void rtw_write_scsi(struct adapter *adapter, u32 cnt, u8 *pmem);
 
 /* ioreq */
 void ioreq_read8(struct adapter *adapter, u32 addr, u8 *pval);
@@ -317,9 +303,6 @@ void async_write32(struct adapter *adapter, u32 addr, u32 val,
 void async_write_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void async_write_port(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
-int rtw_init_io_priv(struct adapter *padapter,
-		     void (*set_intf_ops)(struct _io_ops *pops));
-
 uint alloc_io_queue(struct adapter *adapter);
 void free_io_queue(struct adapter *adapter);
 void async_bus_io(struct io_queue *pio_q);
diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
index d04d2f658ce0..306325818a9a 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -448,6 +448,8 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
 	struct adapter *padapter = NULL;
 	struct net_device *pnetdev = NULL;
 	int status = _FAIL;
+	struct io_priv *piopriv;
+	struct intf_hdl *pintf;
 
 	padapter = vzalloc(sizeof(*padapter));
 	if (!padapter)
@@ -479,7 +481,11 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
 	padapter->intf_stop = &usb_intf_stop;
 
 	/* step init_io_priv */
-	rtw_init_io_priv(padapter, usb_set_intf_ops);
+	piopriv = &padapter->iopriv;
+	pintf = &piopriv->intf;
+	piopriv->padapter = padapter;
+	pintf->padapter = padapter;
+	pintf->pintf_dev = adapter_to_dvobj(padapter);
 
 	/* step read_chip_version */
 	rtl8188e_read_chip_version(padapter);
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 14/19] staging: r8188eu: remove struct _io_ops
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (12 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 13/19] staging: r8188eu: remove core/rtw_io.c Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 15/19] staging: r8188eu: clean up usbctrl_vendorreq() Fabio M. De Francesco
                   ` (5 subsequent siblings)
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Finally struct _io_ops is unused, so remove it.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/hal/usb_ops_linux.c |  6 -----
 drivers/staging/r8188eu/include/rtw_io.h    | 25 ---------------------
 drivers/staging/r8188eu/include/usb_ops.h   |  2 --
 3 files changed, 33 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 2516cfc464a9..117213c9f984 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -558,12 +558,6 @@ void rtl8188eu_xmit_tasklet(unsigned long priv)
 	}
 }
 
-void rtl8188eu_set_intf_ops(struct _io_ops	*pops)
-{
-
-	memset((u8 *)pops, 0, sizeof(struct _io_ops));
-}
-
 void rtl8188eu_set_hw_type(struct adapter *adapt)
 {
 	adapt->chip_type = RTL8188E;
diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index 2b9b64f1ac80..c6a078210eeb 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -84,30 +84,6 @@ struct intf_priv;
 struct intf_hdl;
 struct io_queue;
 
-struct _io_ops {
-	u8 (*_read8)(struct intf_hdl *pintfhdl, u32 addr);
-	u16 (*_read16)(struct intf_hdl *pintfhdl, u32 addr);
-	u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
-	int (*_write8)(struct intf_hdl *pintfhdl, u32 addr, u8 val);
-	int (*_write16)(struct intf_hdl *pintfhdl, u32 addr, u16 val);
-	int (*_write32)(struct intf_hdl *pintfhdl, u32 addr, u32 val);
-	int (*_writeN)(struct intf_hdl *pintfhdl, u32 addr, u32 length,
-		       u8 *pdata);
-	int (*_write8_async)(struct intf_hdl *pintfhdl, u32 addr, u8 val);
-	int (*_write16_async)(struct intf_hdl *pintfhdl, u32 addr, u16 val);
-	int (*_write32_async)(struct intf_hdl *pintfhdl, u32 addr, u32 val);
-	void (*_read_mem)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
-			  u8 *pmem);
-	void (*_write_mem)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
-			   u8 *pmem);
-	u32 (*_read_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
-			  u8 *pmem);
-	u32 (*_write_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
-			   u8 *pmem);
-	void (*_read_port_cancel)(struct intf_hdl *pintfhdl);
-	void (*_write_port_cancel)(struct intf_hdl *pintfhdl);
-};
-
 struct io_req {
 	struct list_head list;
 	u32	addr;
@@ -125,7 +101,6 @@ struct io_req {
 struct	intf_hdl {
 	struct adapter *padapter;
 	struct dvobj_priv *pintf_dev;
-	struct _io_ops	io_ops;
 };
 
 struct reg_protocol_rd {
diff --git a/drivers/staging/r8188eu/include/usb_ops.h b/drivers/staging/r8188eu/include/usb_ops.h
index c53cc54b6b87..1939b781b097 100644
--- a/drivers/staging/r8188eu/include/usb_ops.h
+++ b/drivers/staging/r8188eu/include/usb_ops.h
@@ -21,8 +21,6 @@
 
 void rtl8188eu_set_hw_type(struct adapter *padapter);
 #define hal_set_hw_type rtl8188eu_set_hw_type
-void rtl8188eu_set_intf_ops(struct _io_ops *pops);
-#define usb_set_intf_ops rtl8188eu_set_intf_ops
 
 /*
  * Increase and check if the continual_urb_error of this @param dvobjprivei
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 15/19] staging: r8188eu: clean up usbctrl_vendorreq()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (13 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 14/19] staging: r8188eu: remove struct _io_ops Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17 14:44   ` Greg Kroah-Hartman
  2021-09-17  7:18 ` [PATCH v7 16/19] staging: r8188eu: clean up rtw_read*() and rtw_write*() Fabio M. De Francesco
                   ` (4 subsequent siblings)
  19 siblings, 1 reply; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M. De Francesco

Clean up usbctrl_vendoreq() in usb_ops_linux.c.

List of changes:

1) Rename variables:
	pdata => data
        pio_priv => io_priv
        pintfhdl => intfhdl
        wvalue => address.
2) Reorder variables declarations according to the "Reverse Xmas Tree"
   style.
3) Remove unncecessary test for "!pIo_buf".
4) Move comments one line below code.
5) Remove unnecessary excess parentheses.
6) Remove unnecessary extra spaces.
7) Remove unnecessary comments.
8) Fix grammar errors (checksumed => checksummed).

Co-developed-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/r8188eu/hal/usb_ops_linux.c | 60 +++++++++------------
 1 file changed, 26 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 117213c9f984..2098ce935dc0 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 adapter *adapt = intfhdl->padapter;
+	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
 	struct usb_device *udev = dvobjpriv->pusbdev;
+	int vendorreq_times = 0;
 	unsigned int pipe;
 	int status = 0;
-	u8 *pIo_buf;
-	int vendorreq_times = 0;
+	u8 *io_buf;
 
 	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);
 
 	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 */
 			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 */
 			if (status < 0) {
-				if (status == (-ESHUTDOWN) || status == -ENODEV) {
+				if (status == -ESHUTDOWN || status == -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 */
 				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);
 				}
 			}
 
@@ -86,12 +79,11 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 			}
 
 		}
-
-		/*  firmware download is checksumed, don't retry */
+		/*  firmware download is checksummed, don't retry */
 		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || status == len)
 			break;
 	}
-release_mutex:
+
 	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
 exit:
 	return status;
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 16/19] staging: r8188eu: clean up rtw_read*() and rtw_write*()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (14 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 15/19] staging: r8188eu: clean up usbctrl_vendorreq() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17 14:44   ` Greg Kroah-Hartman
  2021-09-17 14:45   ` Greg Kroah-Hartman
  2021-09-17  7:18 ` [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}() Fabio M. De Francesco
                   ` (3 subsequent siblings)
  19 siblings, 2 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M. De Francesco

Clean up rtw_read{8,16,32}() and rtw_write{8,16,32,N}() in usb_ops_linux.c.

1) Rename variables:
        length => len
        pio_priv => io_priv
        pintfhdl => intfhdl
        wvalue => address.
2) Remove unnecessary casts.
3) Fix types.  Use __le16 instead of __le32.

Co-developed-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/r8188eu/hal/usb_ops_linux.c | 67 +++++++++++----------
 1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 2098ce935dc0..2d5e9b3ba538 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -91,91 +91,92 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 
 u8 rtw_read8(struct adapter *adapter, u32 addr)
 {
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct intf_hdl *pintfhdl = &pio_priv->intf;
-	u16 wvalue = (u16)(addr & 0x0000ffff);
+	struct io_priv *io_priv = &adapter->iopriv;
+	struct intf_hdl *intfhdl = &io_priv->intf;
+	u16 address = addr & 0xffff;
 	u8 data;
 
-	usbctrl_vendorreq(pintfhdl, wvalue, &data, 1, REALTEK_USB_VENQT_READ);
+	usbctrl_vendorreq(intfhdl, address, &data, 1, REALTEK_USB_VENQT_READ);
 
 	return data;
 }
 
 u16 rtw_read16(struct adapter *adapter, u32 addr)
 {
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct intf_hdl *pintfhdl = &pio_priv->intf;
-	u16 wvalue = (u16)(addr & 0x0000ffff);
-	__le32 data;
+	struct io_priv *io_priv = &adapter->iopriv;
+	struct intf_hdl *intfhdl = &io_priv->intf;
+	u16 address = addr & 0xffff;
+	__le16 data;
 
-	usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_READ);
+	usbctrl_vendorreq(intfhdl, address, &data, 2, REALTEK_USB_VENQT_READ);
 
-	return (u16)(le32_to_cpu(data) & 0xffff);
+	return le16_to_cpu(data);
 }
 
 u32 rtw_read32(struct adapter *adapter, u32 addr)
 {
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct intf_hdl *pintfhdl = &pio_priv->intf;
-	u16 wvalue = (u16)(addr & 0x0000ffff);
+	struct io_priv *io_priv = &adapter->iopriv;
+	struct intf_hdl *intfhdl = &io_priv->intf;
+	u16 address = addr & 0xffff;
 	__le32 data;
 
-	usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_READ);
+	usbctrl_vendorreq(intfhdl, address, &data, 4, REALTEK_USB_VENQT_READ);
 
 	return le32_to_cpu(data);
 }
 
 int rtw_write8(struct adapter *adapter, u32 addr, u8 val)
 {
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct intf_hdl *pintfhdl = &pio_priv->intf;
-	u16 wvalue = (u16)(addr & 0x0000ffff);
+	struct io_priv *io_priv = &adapter->iopriv;
+	struct intf_hdl *intfhdl = &io_priv->intf;
+	u16 address = addr & 0xffff;
 	int ret;
 
-	ret = usbctrl_vendorreq(pintfhdl, wvalue, &val, 1, REALTEK_USB_VENQT_WRITE);
+	ret = usbctrl_vendorreq(intfhdl, address, &val, 1, REALTEK_USB_VENQT_WRITE);
 
 	return RTW_STATUS_CODE(ret);
 }
 
 int rtw_write16(struct adapter *adapter, u32 addr, u16 val)
 {
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct intf_hdl *pintfhdl = &pio_priv->intf;
-	u16 wvalue = (u16)(addr & 0x0000ffff);
-	__le32 data = cpu_to_le32(val & 0x0000ffff);
+	struct io_priv *io_priv = &adapter->iopriv;
+	struct intf_hdl *intfhdl = &io_priv->intf;
+	__le16 data = cpu_to_le16(val);
+	u16 address = addr & 0xffff;
 	int ret;
 
-	ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_WRITE);
+	ret = usbctrl_vendorreq(intfhdl, address, &data, 2, REALTEK_USB_VENQT_WRITE);
 
 	return RTW_STATUS_CODE(ret);
 }
 
 int rtw_write32(struct adapter *adapter, u32 addr, u32 val)
 {
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct intf_hdl *pintfhdl = &pio_priv->intf;
-	u16 wvalue = (u16)(addr & 0x0000ffff);
+	struct io_priv *io_priv = &adapter->iopriv;
+	struct intf_hdl *intfhdl = &io_priv->intf;
 	__le32 data = cpu_to_le32(val);
+	u16 address = addr & 0xffff;
 	int ret;
 
-	ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_WRITE);
+	ret = usbctrl_vendorreq(intfhdl, address, &data, 4, REALTEK_USB_VENQT_WRITE);
 
 	return RTW_STATUS_CODE(ret);
 }
 
-int rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata)
+int rtw_writeN(struct adapter *adapter, u32 addr, u32 len, u8 *data)
 {
-	struct io_priv *pio_priv = &adapter->iopriv;
-	struct intf_hdl *pintfhdl = &pio_priv->intf;
-	u16 wvalue = (u16)(addr & 0x0000ffff);
+	struct io_priv *io_priv = &adapter->iopriv;
+	struct intf_hdl *intfhdl = &io_priv->intf;
 	u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
+	u16 address = addr & 0xffff;
+	u16 length = len & 0xffff;
 	int ret;
 
 	if (length > VENDOR_CMD_MAX_DATA_LEN)
 		return _FAIL;
 
-	memcpy(buf, pdata, length);
-	ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
+	memcpy(buf, data, length);
+	ret = usbctrl_vendorreq(intfhdl, address, buf, length, REALTEK_USB_VENQT_WRITE);
 
 	return RTW_STATUS_CODE(ret);
 }
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (15 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 16/19] staging: r8188eu: clean up rtw_read*() and rtw_write*() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17 14:50   ` Greg Kroah-Hartman
  2021-09-17  7:18 ` [PATCH v7 18/19] staging: r8188eu: shorten calls chain of rtw_write{8,16,32,N}() Fabio M. De Francesco
                   ` (2 subsequent siblings)
  19 siblings, 1 reply; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M. De Francesco

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@linuxfoundation.org>
Co-developed-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/r8188eu/hal/usb_ops_linux.c | 59 +++++++++++++++++++--
 1 file changed, 56 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 2d5e9b3ba538..ef35358cf2d3 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -89,6 +89,59 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 	return status;
 }
 
+static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
+{
+	struct adapter *adapt = intfhdl->padapter;
+	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
+	struct usb_device *udev = dvobjpriv->pusbdev;
+	int status;
+	u8 *io_buf; /* Pointer to I/O buffer */
+
+	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
+		return -EPERM;
+
+	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
+
+	io_buf = dvobjpriv->usb_vendor_req_buf;
+
+	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
+				      REALTEK_USB_VENQT_READ, addr,
+				      REALTEK_USB_VENQT_CMD_IDX, io_buf,
+				      size, RTW_USB_CONTROL_MSG_TIMEOUT,
+				      GFP_KERNEL);
+
+	if (status == -ESHUTDOWN ||
+	    status == -ENODEV ||
+	    status == -ENOENT) {
+		/*
+		 * device or controller has been disabled due to
+		 * some problem that could not be worked around,
+		 * device or bus doesn’t exist, endpoint does not
+		 * exist or is not enabled.
+		 */
+		adapt->bSurpriseRemoved = true;
+		goto mutex_unlock;
+	}
+
+	if (status < 0) {
+		GET_HAL_DATA(adapt)->srestpriv.wifi_error_status =
+			USB_VEN_REQ_CMD_FAIL;
+
+		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
+			adapt->bSurpriseRemoved = true;
+
+		goto mutex_unlock;
+	}
+
+	rtw_reset_continual_urb_error(dvobjpriv);
+	memcpy(data, io_buf, size);
+
+mutex_unlock:
+	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
+
+	return status;
+}
+
 u8 rtw_read8(struct adapter *adapter, u32 addr)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
@@ -96,7 +149,7 @@ u8 rtw_read8(struct adapter *adapter, u32 addr)
 	u16 address = addr & 0xffff;
 	u8 data;
 
-	usbctrl_vendorreq(intfhdl, address, &data, 1, REALTEK_USB_VENQT_READ);
+	usb_read(intfhdl, address, &data, 1);
 
 	return data;
 }
@@ -108,7 +161,7 @@ u16 rtw_read16(struct adapter *adapter, u32 addr)
 	u16 address = addr & 0xffff;
 	__le16 data;
 
-	usbctrl_vendorreq(intfhdl, address, &data, 2, REALTEK_USB_VENQT_READ);
+	usb_read(intfhdl, address, &data, 2);
 
 	return le16_to_cpu(data);
 }
@@ -120,7 +173,7 @@ u32 rtw_read32(struct adapter *adapter, u32 addr)
 	u16 address = addr & 0xffff;
 	__le32 data;
 
-	usbctrl_vendorreq(intfhdl, address, &data, 4, REALTEK_USB_VENQT_READ);
+	usb_read(intfhdl, address, &data, 4);
 
 	return le32_to_cpu(data);
 }
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 18/19] staging: r8188eu: shorten calls chain of rtw_write{8,16,32,N}()
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (16 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17  7:18 ` [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests Fabio M. De Francesco
  2021-09-17 11:07 ` [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Dan Carpenter
  19 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M. De Francesco

Shorten the calls chain of rtw_write8/16/32() down to the actual writes.
For this purpose unify the four usb_write8/16/32/N() into the new
usb_write(); make the latter parameterizable with 'size'; embed most of
the code of usbctrl_vendorreq() into usb_write() and use in it the new
usb_control_msg_send() API of USB Core.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Co-developed-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/r8188eu/hal/usb_ops_linux.c | 112 +++++++-------------
 1 file changed, 41 insertions(+), 71 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index ef35358cf2d3..656f3a774e48 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -8,88 +8,60 @@
 #include "../include/recv_osdep.h"
 #include "../include/rtl8188e_hal.h"
 
-static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u16 len, u8 requesttype)
+static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
 {
 	struct adapter *adapt = intfhdl->padapter;
 	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
 	struct usb_device *udev = dvobjpriv->pusbdev;
-	int vendorreq_times = 0;
-	unsigned int pipe;
-	int status = 0;
-	u8 *io_buf;
-
-	if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) {
-		status = -EPERM;
-		goto exit;
-	}
+	int status;
+	u8 *io_buf; /* Pointer to I/O buffer */
 
-	if (len > MAX_VENDOR_REQ_CMD_SIZE) {
-		DBG_88E("[%s] Buffer len error ,vendor request failed\n", __func__);
-		status = -EINVAL;
-		goto exit;
-	}
+	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
+		return -EPERM;
 
 	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
 
-	/*  Acquire IO memory for vendorreq */
 	io_buf = dvobjpriv->usb_vendor_req_buf;
 
-	if (requesttype == REALTEK_USB_VENQT_READ)
-		/* read in */
-		pipe = usb_rcvctrlpipe(udev, 0);
-	else
-		/* write out */
-		pipe = usb_sndctrlpipe(udev, 0);
-
-	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
-		if (requesttype == REALTEK_USB_VENQT_READ)
-			memset(io_buf, 0, len);
-		else
-			memcpy(io_buf, data, len);
+	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
+				      REALTEK_USB_VENQT_READ, addr,
+				      REALTEK_USB_VENQT_CMD_IDX, io_buf,
+				      size, RTW_USB_CONTROL_MSG_TIMEOUT,
+				      GFP_KERNEL);
 
-		status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
-					 requesttype, value, REALTEK_USB_VENQT_CMD_IDX,
-					 io_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT);
+	if (status == -ESHUTDOWN ||
+	    status == -ENODEV ||
+	    status == -ENOENT) {
+		/*
+		 * device or controller has been disabled due to
+		 * some problem that could not be worked around,
+		 * device or bus doesn’t exist, endpoint does not
+		 * exist or is not enabled.
+		 */
+		adapt->bSurpriseRemoved = true;
+		goto mutex_unlock;
+	}
 
-		if (status == len) {
-			/*  success */
-			rtw_reset_continual_urb_error(dvobjpriv);
-			if (requesttype == REALTEK_USB_VENQT_READ)
-				memcpy(data, io_buf,  len);
-		} else {
-			/* errors */
-			if (status < 0) {
-				if (status == -ESHUTDOWN || status == -ENODEV) {
-					adapt->bSurpriseRemoved = true;
-				} else {
-					struct hal_data_8188e *haldata = GET_HAL_DATA(adapt);
-					haldata->srestpriv.wifi_error_status = USB_VEN_REQ_CMD_FAIL;
-				}
-			} else {
-				/*  status != len && status >= 0 */
-				if (status > 0) {
-					if (requesttype == REALTEK_USB_VENQT_READ)
-						memcpy(data, io_buf,  len);
-				}
-			}
+	if (status < 0) {
+		GET_HAL_DATA(adapt)->srestpriv.wifi_error_status =
+			USB_VEN_REQ_CMD_FAIL;
 
-			if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
-				adapt->bSurpriseRemoved = true;
-				break;
-			}
+		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
+			adapt->bSurpriseRemoved = true;
 
-		}
-		/*  firmware download is checksummed, don't retry */
-		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || status == len)
-			break;
+		goto mutex_unlock;
 	}
 
+	rtw_reset_continual_urb_error(dvobjpriv);
+	memcpy(data, io_buf, size);
+
+mutex_unlock:
 	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
-exit:
+
 	return status;
 }
 
-static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
+static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
 {
 	struct adapter *adapt = intfhdl->padapter;
 	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
@@ -103,9 +75,10 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
 	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
 
 	io_buf = dvobjpriv->usb_vendor_req_buf;
+	memcpy(io_buf, data, size);
 
-	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
-				      REALTEK_USB_VENQT_READ, addr,
+	status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
+				      REALTEK_USB_VENQT_WRITE, addr,
 				      REALTEK_USB_VENQT_CMD_IDX, io_buf,
 				      size, RTW_USB_CONTROL_MSG_TIMEOUT,
 				      GFP_KERNEL);
@@ -134,7 +107,6 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
 	}
 
 	rtw_reset_continual_urb_error(dvobjpriv);
-	memcpy(data, io_buf, size);
 
 mutex_unlock:
 	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
@@ -185,7 +157,7 @@ int rtw_write8(struct adapter *adapter, u32 addr, u8 val)
 	u16 address = addr & 0xffff;
 	int ret;
 
-	ret = usbctrl_vendorreq(intfhdl, address, &val, 1, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, address, &val, 1);
 
 	return RTW_STATUS_CODE(ret);
 }
@@ -198,7 +170,7 @@ int rtw_write16(struct adapter *adapter, u32 addr, u16 val)
 	u16 address = addr & 0xffff;
 	int ret;
 
-	ret = usbctrl_vendorreq(intfhdl, address, &data, 2, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, address, &data, 2);
 
 	return RTW_STATUS_CODE(ret);
 }
@@ -211,7 +183,7 @@ int rtw_write32(struct adapter *adapter, u32 addr, u32 val)
 	u16 address = addr & 0xffff;
 	int ret;
 
-	ret = usbctrl_vendorreq(intfhdl, address, &data, 4, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, address, &data, 4);
 
 	return RTW_STATUS_CODE(ret);
 }
@@ -220,7 +192,6 @@ int rtw_writeN(struct adapter *adapter, u32 addr, u32 len, u8 *data)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
-	u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
 	u16 address = addr & 0xffff;
 	u16 length = len & 0xffff;
 	int ret;
@@ -228,8 +199,7 @@ int rtw_writeN(struct adapter *adapter, u32 addr, u32 len, u8 *data)
 	if (length > VENDOR_CMD_MAX_DATA_LEN)
 		return _FAIL;
 
-	memcpy(buf, data, length);
-	ret = usbctrl_vendorreq(intfhdl, address, buf, length, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, address, data, length);
 
 	return RTW_STATUS_CODE(ret);
 }
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (17 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 18/19] staging: r8188eu: shorten calls chain of rtw_write{8,16,32,N}() Fabio M. De Francesco
@ 2021-09-17  7:18 ` Fabio M. De Francesco
  2021-09-17 14:55   ` Greg Kroah-Hartman
  2021-09-17 11:07 ` [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Dan Carpenter
  19 siblings, 1 reply; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-17  7:18 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

This driver used shared buffer for usb requests. It led to using
mutexes, i.e no usb requests can be done in parallel.

USB requests can be fired in parallel since USB Core allows it. In
order to allow them, remove usb_vendor_req_buf from dvobj_priv (since
USB I/O is the only user of it) and remove also usb_vendor_req_mutex
(since there is nothing to protect).

Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/staging/r8188eu/hal/usb_ops_linux.c | 29 ++++++++-------
 drivers/staging/r8188eu/include/drv_types.h |  5 ---
 drivers/staging/r8188eu/os_dep/usb_intf.c   | 40 ++-------------------
 3 files changed, 16 insertions(+), 58 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 656f3a774e48..0ed4e6c8b1f5 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -19,9 +19,9 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
 	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
 		return -EPERM;
 
-	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
-
-	io_buf = dvobjpriv->usb_vendor_req_buf;
+	io_buf = kmalloc(size, GFP_KERNEL);
+	if (!io_buf)
+		return -ENOMEM;
 
 	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
 				      REALTEK_USB_VENQT_READ, addr,
@@ -39,7 +39,7 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
 		 * exist or is not enabled.
 		 */
 		adapt->bSurpriseRemoved = true;
-		goto mutex_unlock;
+		goto end;
 	}
 
 	if (status < 0) {
@@ -49,15 +49,14 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
 		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
 			adapt->bSurpriseRemoved = true;
 
-		goto mutex_unlock;
+		goto end;
 	}
 
 	rtw_reset_continual_urb_error(dvobjpriv);
 	memcpy(data, io_buf, size);
 
-mutex_unlock:
-	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
-
+end:
+	kfree(io_buf);
 	return status;
 }
 
@@ -72,9 +71,10 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
 	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
 		return -EPERM;
 
-	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
+	io_buf = kmalloc(size, GFP_KERNEL);
+	if (!io_buf)
+		return -ENOMEM;
 
-	io_buf = dvobjpriv->usb_vendor_req_buf;
 	memcpy(io_buf, data, size);
 
 	status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
@@ -93,7 +93,7 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
 		 * exist or is not enabled.
 		 */
 		adapt->bSurpriseRemoved = true;
-		goto mutex_unlock;
+		goto end;
 	}
 
 	if (status < 0) {
@@ -103,14 +103,13 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
 		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
 			adapt->bSurpriseRemoved = true;
 
-		goto mutex_unlock;
+		goto end;
 	}
 
 	rtw_reset_continual_urb_error(dvobjpriv);
 
-mutex_unlock:
-	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
-
+end:
+	kfree(io_buf);
 	return status;
 }
 
diff --git a/drivers/staging/r8188eu/include/drv_types.h b/drivers/staging/r8188eu/include/drv_types.h
index 626c6273be6f..499b2bce8cbe 100644
--- a/drivers/staging/r8188eu/include/drv_types.h
+++ b/drivers/staging/r8188eu/include/drv_types.h
@@ -168,11 +168,6 @@ struct dvobj_priv {
 	int	ep_num[5]; /* endpoint number */
 	int	RegUsbSS;
 	struct semaphore usb_suspend_sema;
-	struct mutex  usb_vendor_req_mutex;
-
-	u8 *usb_alloc_vendor_req_buf;
-	u8 *usb_vendor_req_buf;
-
 	struct usb_interface *pusbintf;
 	struct usb_device *pusbdev;
 
diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
index 306325818a9a..47568aa10494 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -73,33 +73,9 @@ static struct rtw_usb_drv rtl8188e_usb_drv = {
 
 static struct rtw_usb_drv *usb_drv = &rtl8188e_usb_drv;
 
-static u8 rtw_init_intf_priv(struct dvobj_priv *dvobj)
-{
-	u8 rst = _SUCCESS;
-
-	mutex_init(&dvobj->usb_vendor_req_mutex);
-
-	dvobj->usb_alloc_vendor_req_buf = kzalloc(MAX_USB_IO_CTL_SIZE, GFP_KERNEL);
-	if (!dvobj->usb_alloc_vendor_req_buf) {
-		DBG_88E("alloc usb_vendor_req_buf failed... /n");
-		rst = _FAIL;
-		goto exit;
-	}
-	dvobj->usb_vendor_req_buf = (u8 *)N_BYTE_ALIGMENT((size_t)(dvobj->usb_alloc_vendor_req_buf), ALIGNMENT_UNIT);
-exit:
-	return rst;
-}
-
-static void rtw_deinit_intf_priv(struct dvobj_priv *dvobj)
-{
-	kfree(dvobj->usb_alloc_vendor_req_buf);
-	mutex_destroy(&dvobj->usb_vendor_req_mutex);
-}
-
 static struct dvobj_priv *usb_dvobj_init(struct usb_interface *usb_intf)
 {
 	int	i;
-	int	status = _FAIL;
 	struct dvobj_priv *pdvobjpriv;
 	struct usb_host_config		*phost_conf;
 	struct usb_config_descriptor	*pconf_desc;
@@ -110,7 +86,7 @@ static struct dvobj_priv *usb_dvobj_init(struct usb_interface *usb_intf)
 
 	pdvobjpriv = kzalloc(sizeof(*pdvobjpriv), GFP_KERNEL);
 	if (!pdvobjpriv)
-		goto exit;
+		return NULL;
 
 	pdvobjpriv->pusbintf = usb_intf;
 	pusbd = interface_to_usbdev(usb_intf);
@@ -158,24 +134,12 @@ static struct dvobj_priv *usb_dvobj_init(struct usb_interface *usb_intf)
 		DBG_88E("NON USB_SPEED_HIGH\n");
 	}
 
-	if (rtw_init_intf_priv(pdvobjpriv) == _FAIL)
-		goto free_dvobj;
-
 	/* 3 misc */
 	sema_init(&pdvobjpriv->usb_suspend_sema, 0);
 	rtw_reset_continual_urb_error(pdvobjpriv);
 
 	usb_get_dev(pusbd);
 
-	status = _SUCCESS;
-
-free_dvobj:
-	if (status != _SUCCESS && pdvobjpriv) {
-		usb_set_intfdata(usb_intf, NULL);
-		kfree(pdvobjpriv);
-		pdvobjpriv = NULL;
-	}
-exit:
 	return pdvobjpriv;
 }
 
@@ -200,7 +164,7 @@ static void usb_dvobj_deinit(struct usb_interface *usb_intf)
 				usb_reset_device(interface_to_usbdev(usb_intf));
 			}
 		}
-		rtw_deinit_intf_priv(dvobj);
+
 		kfree(dvobj);
 	}
 
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains
  2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (18 preceding siblings ...)
  2021-09-17  7:18 ` [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests Fabio M. De Francesco
@ 2021-09-17 11:07 ` Dan Carpenter
  19 siblings, 0 replies; 36+ messages in thread
From: Dan Carpenter @ 2021-09-17 11:07 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight

Looks good.  Thanks!

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 15/19] staging: r8188eu: clean up usbctrl_vendorreq()
  2021-09-17  7:18 ` [PATCH v7 15/19] staging: r8188eu: clean up usbctrl_vendorreq() Fabio M. De Francesco
@ 2021-09-17 14:44   ` Greg Kroah-Hartman
  2021-09-18 11:13     ` Fabio M. De Francesco
  0 siblings, 1 reply; 36+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-17 14:44 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter

On Fri, Sep 17, 2021 at 09:18:33AM +0200, Fabio M. De Francesco wrote:
> Clean up usbctrl_vendoreq() in usb_ops_linux.c.
> 
> List of changes:
> 
> 1) Rename variables:
> 	pdata => data
>         pio_priv => io_priv
>         pintfhdl => intfhdl
>         wvalue => address.
> 2) Reorder variables declarations according to the "Reverse Xmas Tree"
>    style.
> 3) Remove unncecessary test for "!pIo_buf".
> 4) Move comments one line below code.
> 5) Remove unnecessary excess parentheses.
> 6) Remove unnecessary extra spaces.
> 7) Remove unnecessary comments.
> 8) Fix grammar errors (checksumed => checksummed).

When you find yourself listing all of the different things you have done
in a single commit, that is a HUGE hint that you need to break this up
into smaller pieces.

Please do so here, this should not be just one change, as it's almost
impossible to look at this and "know" it's all still the same logic
happening here.  But if you had broken this down into 8 different
changes, then it would have been obvious and I could easily have applied
the changes.

I've taken the first 14 patches in this series, it's great work, thank
you all for doing this.  But this, and the remaining patches in here
need to be split up more to make it obvious that the changes are correct
and should be accepted.  Please feel free to start the numbering of the
patch series over now, given that the first 14 are now merged into my
tree.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 16/19] staging: r8188eu: clean up rtw_read*() and rtw_write*()
  2021-09-17  7:18 ` [PATCH v7 16/19] staging: r8188eu: clean up rtw_read*() and rtw_write*() Fabio M. De Francesco
@ 2021-09-17 14:44   ` Greg Kroah-Hartman
  2021-09-17 14:45   ` Greg Kroah-Hartman
  1 sibling, 0 replies; 36+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-17 14:44 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter

On Fri, Sep 17, 2021 at 09:18:34AM +0200, Fabio M. De Francesco wrote:
> Clean up rtw_read{8,16,32}() and rtw_write{8,16,32,N}() in usb_ops_linux.c.
> 
> 1) Rename variables:
>         length => len
>         pio_priv => io_priv
>         pintfhdl => intfhdl
>         wvalue => address.
> 2) Remove unnecessary casts.
> 3) Fix types.  Use __le16 instead of __le32.

Again, should be 3 changes.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 16/19] staging: r8188eu: clean up rtw_read*() and rtw_write*()
  2021-09-17  7:18 ` [PATCH v7 16/19] staging: r8188eu: clean up rtw_read*() and rtw_write*() Fabio M. De Francesco
  2021-09-17 14:44   ` Greg Kroah-Hartman
@ 2021-09-17 14:45   ` Greg Kroah-Hartman
  2021-09-18 11:41     ` Fabio M. De Francesco
  1 sibling, 1 reply; 36+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-17 14:45 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter

On Fri, Sep 17, 2021 at 09:18:34AM +0200, Fabio M. De Francesco wrote:
> Clean up rtw_read{8,16,32}() and rtw_write{8,16,32,N}() in usb_ops_linux.c.
> 
> 1) Rename variables:
>         length => len
>         pio_priv => io_priv
>         pintfhdl => intfhdl
>         wvalue => address.

Wait, why are you changing wvalue?  Isn't that the USB name for this
variable in the USB message sent to the device?  Check the USB spec
before changing this, that is a common field and probably should not be
changed.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()
  2021-09-17  7:18 ` [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}() Fabio M. De Francesco
@ 2021-09-17 14:50   ` Greg Kroah-Hartman
  2021-09-17 14:54     ` Pavel Skripkin
                       ` (2 more replies)
  0 siblings, 3 replies; 36+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-17 14:50 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter

On Fri, Sep 17, 2021 at 09:18:35AM +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@linuxfoundation.org>
> Co-developed-by: Pavel Skripkin <paskripkin@gmail.com>
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 59 +++++++++++++++++++--
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index 2d5e9b3ba538..ef35358cf2d3 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -89,6 +89,59 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
>  	return status;
>  }
>  
> +static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
> +{
> +	struct adapter *adapt = intfhdl->padapter;
> +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> +	struct usb_device *udev = dvobjpriv->pusbdev;
> +	int status;
> +	u8 *io_buf; /* Pointer to I/O buffer */

As you "know" size is not going to be larger than 4 (hint, you should
prboably check it), just use bytes off of the stack here, and you can
ignore this buffer entirely.  That will hopefully allow you in the
future to get rid of that buffer as odds are it will not be needed
anymore.

> +
> +	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
> +		return -EPERM;

How is it ok to check this outside of the lock?  What happens if these
values change right _after_ you check them?

Why check them at all, is this something that we even care about?

I know you are trying to make this just the same logic at is there
today, but why not just do it right the first time?

> +
> +	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
> +
> +	io_buf = dvobjpriv->usb_vendor_req_buf;
> +
> +	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> +				      REALTEK_USB_VENQT_READ, addr,
> +				      REALTEK_USB_VENQT_CMD_IDX, io_buf,
> +				      size, RTW_USB_CONTROL_MSG_TIMEOUT,
> +				      GFP_KERNEL);
> +
> +	if (status == -ESHUTDOWN ||
> +	    status == -ENODEV ||
> +	    status == -ENOENT) {
> +		/*
> +		 * device or controller has been disabled due to
> +		 * some problem that could not be worked around,
> +		 * device or bus doesn’t exist, endpoint does not
> +		 * exist or is not enabled.
> +		 */
> +		adapt->bSurpriseRemoved = true;
> +		goto mutex_unlock;
> +	}
> +
> +	if (status < 0) {
> +		GET_HAL_DATA(adapt)->srestpriv.wifi_error_status =
> +			USB_VEN_REQ_CMD_FAIL;
> +
> +		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
> +			adapt->bSurpriseRemoved = true;
> +
> +		goto mutex_unlock;
> +	}
> +
> +	rtw_reset_continual_urb_error(dvobjpriv);
> +	memcpy(data, io_buf, size);
> +
> +mutex_unlock:
> +	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
> +
> +	return status;

No one cares about this value, is that ok?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()
  2021-09-17 14:50   ` Greg Kroah-Hartman
@ 2021-09-17 14:54     ` Pavel Skripkin
  2021-09-17 15:01     ` David Laight
  2021-09-18 12:19     ` Fabio M. De Francesco
  2 siblings, 0 replies; 36+ messages in thread
From: Pavel Skripkin @ 2021-09-17 14:54 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, linux-staging, linux-kernel,
	David Laight, Dan Carpenter

On 9/17/21 17:50, Greg Kroah-Hartman wrote:
> On Fri, Sep 17, 2021 at 09:18:35AM +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@linuxfoundation.org>
>> Co-developed-by: Pavel Skripkin <paskripkin@gmail.com>
>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
>> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> ---
>>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 59 +++++++++++++++++++--
>>  1 file changed, 56 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> index 2d5e9b3ba538..ef35358cf2d3 100644
>> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> @@ -89,6 +89,59 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
>>  	return status;
>>  }
>>  
>> +static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>> +{
>> +	struct adapter *adapt = intfhdl->padapter;
>> +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
>> +	struct usb_device *udev = dvobjpriv->pusbdev;
>> +	int status;
>> +	u8 *io_buf; /* Pointer to I/O buffer */
> 
> As you "know" size is not going to be larger than 4 (hint, you should
> prboably check it), just use bytes off of the stack here, and you can
> ignore this buffer entirely.  That will hopefully allow you in the
> future to get rid of that buffer as odds are it will not be needed
> anymore.
> 
>> +
>> +	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
>> +		return -EPERM;
> 
> How is it ok to check this outside of the lock?  What happens if these
> values change right _after_ you check them?
> 
> Why check them at all, is this something that we even care about?
> 
> I know you are trying to make this just the same logic at is there
> today, but why not just do it right the first time?
> 
>> +
>> +	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
>> +
>> +	io_buf = dvobjpriv->usb_vendor_req_buf;
>> +
>> +	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
>> +				      REALTEK_USB_VENQT_READ, addr,
>> +				      REALTEK_USB_VENQT_CMD_IDX, io_buf,
>> +				      size, RTW_USB_CONTROL_MSG_TIMEOUT,
>> +				      GFP_KERNEL);
>> +
>> +	if (status == -ESHUTDOWN ||
>> +	    status == -ENODEV ||
>> +	    status == -ENOENT) {
>> +		/*
>> +		 * device or controller has been disabled due to
>> +		 * some problem that could not be worked around,
>> +		 * device or bus doesn’t exist, endpoint does not
>> +		 * exist or is not enabled.
>> +		 */
>> +		adapt->bSurpriseRemoved = true;
>> +		goto mutex_unlock;
>> +	}
>> +
>> +	if (status < 0) {
>> +		GET_HAL_DATA(adapt)->srestpriv.wifi_error_status =
>> +			USB_VEN_REQ_CMD_FAIL;
>> +
>> +		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
>> +			adapt->bSurpriseRemoved = true;
>> +
>> +		goto mutex_unlock;
>> +	}
>> +
>> +	rtw_reset_continual_urb_error(dvobjpriv);
>> +	memcpy(data, io_buf, size);
>> +
>> +mutex_unlock:
>> +	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
>> +
>> +	return status;
> 
> No one cares about this value, is that ok?
> 

Hi, Greg

I haven't yet read your previous messages, I will do it right after I 
send this email.


Ignoring return value is 100% bug and i've fixed it my RFC series, that 
was posted like month ago. As you suggested, we firstly cleaning up 
usb_ops_linux and then I will rebase my series on top of this series.

Goal of my series is to add proper error handling of USB i/o all across 
the driver code.


Thank you for reviewing our patches :)




With regards,
Pavel Skripkin

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests
  2021-09-17  7:18 ` [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests Fabio M. De Francesco
@ 2021-09-17 14:55   ` Greg Kroah-Hartman
  2021-09-17 15:03     ` Pavel Skripkin
  0 siblings, 1 reply; 36+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-17 14:55 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter

On Fri, Sep 17, 2021 at 09:18:37AM +0200, Fabio M. De Francesco wrote:
> From: Pavel Skripkin <paskripkin@gmail.com>
> 
> This driver used shared buffer for usb requests. It led to using
> mutexes, i.e no usb requests can be done in parallel.
> 
> USB requests can be fired in parallel since USB Core allows it. In
> order to allow them, remove usb_vendor_req_buf from dvobj_priv (since
> USB I/O is the only user of it) and remove also usb_vendor_req_mutex
> (since there is nothing to protect).

Ah, you are removing this buffer, nice!

But, just because the USB core allows multiple messages to be sent to a
device at the same time, does NOT mean that the device itself can handle
that sort of a thing.

Keeping that lock might be a good idea, until you can prove otherwise.
You never know, maybe there's never any contention at all for it because
these accesses are all done in a serial fashion and the lock
grab/release is instant.  But if that is not the case, you might really
get a device confused here by throwing multiple control messages at it
in ways that it is not set up to handle at all.

So please do not drop the lock.

More comments below.


> 
> Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 29 ++++++++-------
>  drivers/staging/r8188eu/include/drv_types.h |  5 ---
>  drivers/staging/r8188eu/os_dep/usb_intf.c   | 40 ++-------------------
>  3 files changed, 16 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index 656f3a774e48..0ed4e6c8b1f5 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -19,9 +19,9 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>  	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
>  		return -EPERM;
>  
> -	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
> -
> -	io_buf = dvobjpriv->usb_vendor_req_buf;
> +	io_buf = kmalloc(size, GFP_KERNEL);
> +	if (!io_buf)
> +		return -ENOMEM;

Please read the docs for usb_control_msg_recv().  It can allow data off
of the stack, so no need to allocate/free the buffer like this all the
time.

Note, the usb_control_msg() call does require the data to be allocated
dynamically, like the code does today.  Which is why you probably got
confused here.

Same for usb_control_msg_send(), it can take data off of the stack.


>  
>  	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
>  				      REALTEK_USB_VENQT_READ, addr,
> @@ -39,7 +39,7 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>  		 * exist or is not enabled.
>  		 */
>  		adapt->bSurpriseRemoved = true;
> -		goto mutex_unlock;
> +		goto end;
>  	}
>  
>  	if (status < 0) {
> @@ -49,15 +49,14 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>  		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
>  			adapt->bSurpriseRemoved = true;
>  
> -		goto mutex_unlock;
> +		goto end;
>  	}
>  
>  	rtw_reset_continual_urb_error(dvobjpriv);
>  	memcpy(data, io_buf, size);
>  
> -mutex_unlock:
> -	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
> -
> +end:
> +	kfree(io_buf);
>  	return status;
>  }
>  
> @@ -72,9 +71,10 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>  	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
>  		return -EPERM;
>  
> -	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
> +	io_buf = kmalloc(size, GFP_KERNEL);
> +	if (!io_buf)
> +		return -ENOMEM;
>  
> -	io_buf = dvobjpriv->usb_vendor_req_buf;
>  	memcpy(io_buf, data, size);
>  
>  	status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> @@ -93,7 +93,7 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>  		 * exist or is not enabled.
>  		 */
>  		adapt->bSurpriseRemoved = true;
> -		goto mutex_unlock;
> +		goto end;
>  	}
>  
>  	if (status < 0) {
> @@ -103,14 +103,13 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>  		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
>  			adapt->bSurpriseRemoved = true;
>  
> -		goto mutex_unlock;
> +		goto end;
>  	}
>  
>  	rtw_reset_continual_urb_error(dvobjpriv);
>  
> -mutex_unlock:
> -	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
> -
> +end:
> +	kfree(io_buf);
>  	return status;
>  }
>  
> diff --git a/drivers/staging/r8188eu/include/drv_types.h b/drivers/staging/r8188eu/include/drv_types.h
> index 626c6273be6f..499b2bce8cbe 100644
> --- a/drivers/staging/r8188eu/include/drv_types.h
> +++ b/drivers/staging/r8188eu/include/drv_types.h
> @@ -168,11 +168,6 @@ struct dvobj_priv {
>  	int	ep_num[5]; /* endpoint number */
>  	int	RegUsbSS;
>  	struct semaphore usb_suspend_sema;
> -	struct mutex  usb_vendor_req_mutex;
> -
> -	u8 *usb_alloc_vendor_req_buf;
> -	u8 *usb_vendor_req_buf;

I do like removing these buffers, and I think that is all that this
change should be doing.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()
  2021-09-17 14:50   ` Greg Kroah-Hartman
  2021-09-17 14:54     ` Pavel Skripkin
@ 2021-09-17 15:01     ` David Laight
  2021-09-17 15:17       ` 'Greg Kroah-Hartman'
  2021-09-18 12:19     ` Fabio M. De Francesco
  2 siblings, 1 reply; 36+ messages in thread
From: David Laight @ 2021-09-17 15:01 UTC (permalink / raw)
  To: 'Greg Kroah-Hartman', Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, Dan Carpenter

From: Greg Kroah-Hartman
> Sent: 17 September 2021 15:50
...
> > +static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
> > +{
> > +	struct adapter *adapt = intfhdl->padapter;
> > +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> > +	struct usb_device *udev = dvobjpriv->pusbdev;
> > +	int status;
> > +	u8 *io_buf; /* Pointer to I/O buffer */
> 
> As you "know" size is not going to be larger than 4 (hint, you should
> prboably check it), just use bytes off of the stack here, and you can
> ignore this buffer entirely.  That will hopefully allow you in the
> future to get rid of that buffer as odds are it will not be needed
> anymore.

Isn't that likely to be the buffer that gets dma'd to/from?
In which case it can't be on-stack.
Certainly that is a common problem with usb drivers.

Give the size of the urb? structure allocated for each transfer
adding a bounce buffer area in it for short transfers would
surely be sane.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests
  2021-09-17 14:55   ` Greg Kroah-Hartman
@ 2021-09-17 15:03     ` Pavel Skripkin
  2021-09-17 15:06       ` Pavel Skripkin
  2021-09-17 15:18       ` Greg Kroah-Hartman
  0 siblings, 2 replies; 36+ messages in thread
From: Pavel Skripkin @ 2021-09-17 15:03 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Larry Finger, Phillip Potter
  Cc: linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Fabio M. De Francesco

On 9/17/21 17:55, Greg Kroah-Hartman wrote:
> On Fri, Sep 17, 2021 at 09:18:37AM +0200, Fabio M. De Francesco wrote:
>> From: Pavel Skripkin <paskripkin@gmail.com>
>> 
>> This driver used shared buffer for usb requests. It led to using
>> mutexes, i.e no usb requests can be done in parallel.
>> 
>> USB requests can be fired in parallel since USB Core allows it. In
>> order to allow them, remove usb_vendor_req_buf from dvobj_priv (since
>> USB I/O is the only user of it) and remove also usb_vendor_req_mutex
>> (since there is nothing to protect).
> 
> Ah, you are removing this buffer, nice!
> 
> But, just because the USB core allows multiple messages to be sent to a
> device at the same time, does NOT mean that the device itself can handle
> that sort of a thing.
> 
> Keeping that lock might be a good idea, until you can prove otherwise.
> You never know, maybe there's never any contention at all for it because
> these accesses are all done in a serial fashion and the lock
> grab/release is instant.  But if that is not the case, you might really
> get a device confused here by throwing multiple control messages at it
> in ways that it is not set up to handle at all.
> 
> So please do not drop the lock.
> 
> More comments below.
> 

We have tested this change. I've tested it in qemu with TP-Link 
TL-WN722N v2 / v3 [Realtek RTL8188EUS], and Fabio has tested it on his 
host for like a whole evening.

I agree, that our testing does not cover all possible cases and I can't 
say it was "good stress testing", so, I think, we need some comments 
from maintainers.

@Larry, @Phillip, does this change looks reasonable for this chip?



With regards,
Pavel Skripkin

> 
>> 
>> Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
>> ---
>>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 29 ++++++++-------
>>  drivers/staging/r8188eu/include/drv_types.h |  5 ---
>>  drivers/staging/r8188eu/os_dep/usb_intf.c   | 40 ++-------------------
>>  3 files changed, 16 insertions(+), 58 deletions(-)
>> 
>> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> index 656f3a774e48..0ed4e6c8b1f5 100644
>> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> @@ -19,9 +19,9 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>>  	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
>>  		return -EPERM;
>>  
>> -	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
>> -
>> -	io_buf = dvobjpriv->usb_vendor_req_buf;
>> +	io_buf = kmalloc(size, GFP_KERNEL);
>> +	if (!io_buf)
>> +		return -ENOMEM;
> 
> Please read the docs for usb_control_msg_recv().  It can allow data off
> of the stack, so no need to allocate/free the buffer like this all the
> time.
> 
> Note, the usb_control_msg() call does require the data to be allocated
> dynamically, like the code does today.  Which is why you probably got
> confused here.
> 
> Same for usb_control_msg_send(), it can take data off of the stack.
> 
> 
>>  
>>  	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
>>  				      REALTEK_USB_VENQT_READ, addr,
>> @@ -39,7 +39,7 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>>  		 * exist or is not enabled.
>>  		 */
>>  		adapt->bSurpriseRemoved = true;
>> -		goto mutex_unlock;
>> +		goto end;
>>  	}
>>  
>>  	if (status < 0) {
>> @@ -49,15 +49,14 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>>  		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
>>  			adapt->bSurpriseRemoved = true;
>>  
>> -		goto mutex_unlock;
>> +		goto end;
>>  	}
>>  
>>  	rtw_reset_continual_urb_error(dvobjpriv);
>>  	memcpy(data, io_buf, size);
>>  
>> -mutex_unlock:
>> -	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
>> -
>> +end:
>> +	kfree(io_buf);
>>  	return status;
>>  }
>>  
>> @@ -72,9 +71,10 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>>  	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
>>  		return -EPERM;
>>  
>> -	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
>> +	io_buf = kmalloc(size, GFP_KERNEL);
>> +	if (!io_buf)
>> +		return -ENOMEM;
>>  
>> -	io_buf = dvobjpriv->usb_vendor_req_buf;
>>  	memcpy(io_buf, data, size);
>>  
>>  	status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
>> @@ -93,7 +93,7 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>>  		 * exist or is not enabled.
>>  		 */
>>  		adapt->bSurpriseRemoved = true;
>> -		goto mutex_unlock;
>> +		goto end;
>>  	}
>>  
>>  	if (status < 0) {
>> @@ -103,14 +103,13 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>>  		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
>>  			adapt->bSurpriseRemoved = true;
>>  
>> -		goto mutex_unlock;
>> +		goto end;
>>  	}
>>  
>>  	rtw_reset_continual_urb_error(dvobjpriv);
>>  
>> -mutex_unlock:
>> -	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
>> -
>> +end:
>> +	kfree(io_buf);
>>  	return status;
>>  }
>>  
>> diff --git a/drivers/staging/r8188eu/include/drv_types.h b/drivers/staging/r8188eu/include/drv_types.h
>> index 626c6273be6f..499b2bce8cbe 100644
>> --- a/drivers/staging/r8188eu/include/drv_types.h
>> +++ b/drivers/staging/r8188eu/include/drv_types.h
>> @@ -168,11 +168,6 @@ struct dvobj_priv {
>>  	int	ep_num[5]; /* endpoint number */
>>  	int	RegUsbSS;
>>  	struct semaphore usb_suspend_sema;
>> -	struct mutex  usb_vendor_req_mutex;
>> -
>> -	u8 *usb_alloc_vendor_req_buf;
>> -	u8 *usb_vendor_req_buf;
> 
> I do like removing these buffers, and I think that is all that this
> change should be doing.
> 
> thanks,
> 
> greg k-h
> 




^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests
  2021-09-17 15:03     ` Pavel Skripkin
@ 2021-09-17 15:06       ` Pavel Skripkin
  2021-09-17 15:18       ` Greg Kroah-Hartman
  1 sibling, 0 replies; 36+ messages in thread
From: Pavel Skripkin @ 2021-09-17 15:06 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Larry Finger, Phillip Potter
  Cc: linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Fabio M. De Francesco

On 9/17/21 18:03, Pavel Skripkin wrote:
> On 9/17/21 17:55, Greg Kroah-Hartman wrote:
>> On Fri, Sep 17, 2021 at 09:18:37AM +0200, Fabio M. De Francesco wrote:
>>> From: Pavel Skripkin <paskripkin@gmail.com>
>>> 
>>> This driver used shared buffer for usb requests. It led to using
>>> mutexes, i.e no usb requests can be done in parallel.
>>> 
>>> USB requests can be fired in parallel since USB Core allows it. In
>>> order to allow them, remove usb_vendor_req_buf from dvobj_priv (since
>>> USB I/O is the only user of it) and remove also usb_vendor_req_mutex
>>> (since there is nothing to protect).
>> 
>> Ah, you are removing this buffer, nice!
>> 
>> But, just because the USB core allows multiple messages to be sent to a
>> device at the same time, does NOT mean that the device itself can handle
>> that sort of a thing.
>> 
>> Keeping that lock might be a good idea, until you can prove otherwise.
>> You never know, maybe there's never any contention at all for it because
>> these accesses are all done in a serial fashion and the lock
>> grab/release is instant.  But if that is not the case, you might really
>> get a device confused here by throwing multiple control messages at it
>> in ways that it is not set up to handle at all.
>> 
>> So please do not drop the lock.
>> 
>> More comments below.
>> 
> 
> We have tested this change. I've tested it in qemu with TP-Link
> TL-WN722N v2 / v3 [Realtek RTL8188EUS], and Fabio has tested it on his
> host for like a whole evening.
> 
> I agree, that our testing does not cover all possible cases and I can't
> say it was "good stress testing", so, I think, we need some comments
> from maintainers.
> 
> @Larry, @Phillip, does this change looks reasonable for this chip?
> 
			 ^^^^^^^^^^^

I mean mutex removal, sorry for confusion :)



With regards,
Pavel Skripkin

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()
  2021-09-17 15:01     ` David Laight
@ 2021-09-17 15:17       ` 'Greg Kroah-Hartman'
  0 siblings, 0 replies; 36+ messages in thread
From: 'Greg Kroah-Hartman' @ 2021-09-17 15:17 UTC (permalink / raw)
  To: David Laight
  Cc: Fabio M. De Francesco, Larry Finger, Phillip Potter,
	Pavel Skripkin, linux-staging, linux-kernel, Dan Carpenter

On Fri, Sep 17, 2021 at 03:01:27PM +0000, David Laight wrote:
> From: Greg Kroah-Hartman
> > Sent: 17 September 2021 15:50
> ...
> > > +static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
> > > +{
> > > +	struct adapter *adapt = intfhdl->padapter;
> > > +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> > > +	struct usb_device *udev = dvobjpriv->pusbdev;
> > > +	int status;
> > > +	u8 *io_buf; /* Pointer to I/O buffer */
> > 
> > As you "know" size is not going to be larger than 4 (hint, you should
> > prboably check it), just use bytes off of the stack here, and you can
> > ignore this buffer entirely.  That will hopefully allow you in the
> > future to get rid of that buffer as odds are it will not be needed
> > anymore.
> 
> Isn't that likely to be the buffer that gets dma'd to/from?
> In which case it can't be on-stack.
> Certainly that is a common problem with usb drivers.

Yes it was a problem, which is why the USB core function called here
does not require that and makes sure to allocate the buffer itself so
that all will be fine.

> Give the size of the urb? structure allocated for each transfer
> adding a bounce buffer area in it for short transfers would
> surely be sane.

USB speeds are slow you will never notice the difference for control
messages.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests
  2021-09-17 15:03     ` Pavel Skripkin
  2021-09-17 15:06       ` Pavel Skripkin
@ 2021-09-17 15:18       ` Greg Kroah-Hartman
  2021-09-17 15:23         ` Pavel Skripkin
  1 sibling, 1 reply; 36+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-17 15:18 UTC (permalink / raw)
  To: Pavel Skripkin
  Cc: Larry Finger, Phillip Potter, linux-staging, linux-kernel,
	David Laight, Dan Carpenter, Fabio M. De Francesco

On Fri, Sep 17, 2021 at 06:03:52PM +0300, Pavel Skripkin wrote:
> On 9/17/21 17:55, Greg Kroah-Hartman wrote:
> > On Fri, Sep 17, 2021 at 09:18:37AM +0200, Fabio M. De Francesco wrote:
> > > From: Pavel Skripkin <paskripkin@gmail.com>
> > > 
> > > This driver used shared buffer for usb requests. It led to using
> > > mutexes, i.e no usb requests can be done in parallel.
> > > 
> > > USB requests can be fired in parallel since USB Core allows it. In
> > > order to allow them, remove usb_vendor_req_buf from dvobj_priv (since
> > > USB I/O is the only user of it) and remove also usb_vendor_req_mutex
> > > (since there is nothing to protect).
> > 
> > Ah, you are removing this buffer, nice!
> > 
> > But, just because the USB core allows multiple messages to be sent to a
> > device at the same time, does NOT mean that the device itself can handle
> > that sort of a thing.
> > 
> > Keeping that lock might be a good idea, until you can prove otherwise.
> > You never know, maybe there's never any contention at all for it because
> > these accesses are all done in a serial fashion and the lock
> > grab/release is instant.  But if that is not the case, you might really
> > get a device confused here by throwing multiple control messages at it
> > in ways that it is not set up to handle at all.
> > 
> > So please do not drop the lock.
> > 
> > More comments below.
> > 
> 
> We have tested this change. I've tested it in qemu with TP-Link TL-WN722N v2
> / v3 [Realtek RTL8188EUS], and Fabio has tested it on his host for like a
> whole evening.
> 
> I agree, that our testing does not cover all possible cases and I can't say
> it was "good stress testing", so, I think, we need some comments from
> maintainers.

Ok, then make it a single patch that does nothing but remove the lock so
that we can revert it later when problems show up :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests
  2021-09-17 15:18       ` Greg Kroah-Hartman
@ 2021-09-17 15:23         ` Pavel Skripkin
  0 siblings, 0 replies; 36+ messages in thread
From: Pavel Skripkin @ 2021-09-17 15:23 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Larry Finger, Phillip Potter, linux-staging, linux-kernel,
	David Laight, Dan Carpenter, Fabio M. De Francesco

On 9/17/21 18:18, Greg Kroah-Hartman wrote:
> On Fri, Sep 17, 2021 at 06:03:52PM +0300, Pavel Skripkin wrote:
>> On 9/17/21 17:55, Greg Kroah-Hartman wrote:
>> > On Fri, Sep 17, 2021 at 09:18:37AM +0200, Fabio M. De Francesco wrote:
>> > > From: Pavel Skripkin <paskripkin@gmail.com>
>> > > 
>> > > This driver used shared buffer for usb requests. It led to using
>> > > mutexes, i.e no usb requests can be done in parallel.
>> > > 
>> > > USB requests can be fired in parallel since USB Core allows it. In
>> > > order to allow them, remove usb_vendor_req_buf from dvobj_priv (since
>> > > USB I/O is the only user of it) and remove also usb_vendor_req_mutex
>> > > (since there is nothing to protect).
>> > 
>> > Ah, you are removing this buffer, nice!
>> > 
>> > But, just because the USB core allows multiple messages to be sent to a
>> > device at the same time, does NOT mean that the device itself can handle
>> > that sort of a thing.
>> > 
>> > Keeping that lock might be a good idea, until you can prove otherwise.
>> > You never know, maybe there's never any contention at all for it because
>> > these accesses are all done in a serial fashion and the lock
>> > grab/release is instant.  But if that is not the case, you might really
>> > get a device confused here by throwing multiple control messages at it
>> > in ways that it is not set up to handle at all.
>> > 
>> > So please do not drop the lock.
>> > 
>> > More comments below.
>> > 
>> 
>> We have tested this change. I've tested it in qemu with TP-Link TL-WN722N v2
>> / v3 [Realtek RTL8188EUS], and Fabio has tested it on his host for like a
>> whole evening.
>> 
>> I agree, that our testing does not cover all possible cases and I can't say
>> it was "good stress testing", so, I think, we need some comments from
>> maintainers.
> 
> Ok, then make it a single patch that does nothing but remove the lock so
> that we can revert it later when problems show up :)
> 

Sure! Thank you again :)




With regards,
Pavel Skripkin

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 15/19] staging: r8188eu: clean up usbctrl_vendorreq()
  2021-09-17 14:44   ` Greg Kroah-Hartman
@ 2021-09-18 11:13     ` Fabio M. De Francesco
  0 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-18 11:13 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter

On Friday, September 17, 2021 4:44:17 PM CEST Greg Kroah-Hartman wrote:
> On Fri, Sep 17, 2021 at 09:18:33AM +0200, Fabio M. De Francesco wrote:
> > Clean up usbctrl_vendoreq() in usb_ops_linux.c.
> > 
> > List of changes:
> > 
> > 1) Rename variables:
> > 	pdata => data
> >         pio_priv => io_priv
> >         pintfhdl => intfhdl
> >         wvalue => address.
> > 2) Reorder variables declarations according to the "Reverse Xmas Tree"
> >    style.
> > 3) Remove unnecessary test for "!pIo_buf".
> > 4) Move comments one line below code.
> > 5) Remove unnecessary excess parentheses.
> > 6) Remove unnecessary extra spaces.
> > 7) Remove unnecessary comments.
> > 8) Fix grammar errors (checksumed => checksummed).
> 
> When you find yourself listing all of the different things you have done
> in a single commit, that is a HUGE hint that you need to break this up
> into smaller pieces.
> 
> Please do so here, this should not be just one change, as it's almost
> impossible to look at this and "know" it's all still the same logic
> happening here.  But if you had broken this down into 8 different
> changes, then it would have been obvious and I could easily have applied
> the changes.

Dear Greg,

My first thought when I read you message was to simply delete this patch 
because usbctrl_vendorreq() is going to be deleted in 18/19. But then I 
rethought of the original purpose behind this patch and (after talking with 
Pavel) we decided to do the task you asked and split this patch into 8 
smaller ones. The only reason is because, as you noticed, we "[]are moving 
code around", so, although I'm not required to clean up code in 
usbctrl_vendorreq(), I'm required to make the new usb_read() and usb_write() 
the cleaner the possible. This preventive clean up helps me a lot.

Obviously I guess that I'm required to split also the next patch of this 
series in 3 because there are also there 3 different kind of clean-ups.

So, we'll have a total of 11 clean-ups.

> 
> I've taken the first 14 patches in this series, it's great work, thank
> you all for doing this.  

Thanks to you for the "great work". We appreciated it.

> But this, and the remaining patches in here
> need to be split up more to make it obvious that the changes are correct
> and should be accepted.  Please feel free to start the numbering of the
> patch series over now, given that the first 14 are now merged into my
> tree.

We're working on this.

Thanks,

Fabio

> 
> thanks,
> 
> greg k-h
> 





^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 16/19] staging: r8188eu: clean up rtw_read*() and rtw_write*()
  2021-09-17 14:45   ` Greg Kroah-Hartman
@ 2021-09-18 11:41     ` Fabio M. De Francesco
  0 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-18 11:41 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter

On Friday, September 17, 2021 4:45:29 PM CEST Greg Kroah-Hartman wrote:
> On Fri, Sep 17, 2021 at 09:18:34AM +0200, Fabio M. De Francesco wrote:
> > Clean up rtw_read{8,16,32}() and rtw_write{8,16,32,N}() in 
usb_ops_linux.c.
> > 
> > 1) Rename variables:
> >         length => len
> >         pio_priv => io_priv
> >         pintfhdl => intfhdl
> >         wvalue => address.
> 
> Wait, why are you changing wvalue?  Isn't that the USB name for this
> variable in the USB message sent to the device?  Check the USB spec
> before changing this, that is a common field and probably should not be
> changed.

Oh, sorry. This was due to my very limited knowledge of the USB subsystems 
and its Core API. So I misunderstood the semantics of this "wvalue" argument 
and we'll change it to "value" (just to remove that unnecessary 'w', that I 
guess is for "word"). 

I had thought that the mere knowledge of C and OS kernels (from a theoretical 
perspective) would suffice to work on the code that we change in our patches. 
Now I understand that such a naive approach is clearly wrong.

I have been too lazy to open your LDD 3rd ed. for reading but now I have 
decided that it is time to do it. Furthermore, I have found one more book 
about Linux device drivers and I'm about to read also its "Linux USB device 
drivers" chapter.

I think that by this evening I'll have some basic knowledge of the USB 
subsystem, at least of what is needed to avoid future mistakes like the one 
you noticed. :) 

Thank you very much for the time you spent reviewing our code and for taking 
the first 14 patches,

Fabio

> 
> thanks,
> 
> greg k-h




^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()
  2021-09-17 14:50   ` Greg Kroah-Hartman
  2021-09-17 14:54     ` Pavel Skripkin
  2021-09-17 15:01     ` David Laight
@ 2021-09-18 12:19     ` Fabio M. De Francesco
  2 siblings, 0 replies; 36+ messages in thread
From: Fabio M. De Francesco @ 2021-09-18 12:19 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter

On Friday, September 17, 2021 4:50:10 PM CEST Greg Kroah-Hartman wrote:
> On Fri, Sep 17, 2021 at 09:18:35AM +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@linuxfoundation.org>
> > Co-developed-by: Pavel Skripkin <paskripkin@gmail.com>
> > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  drivers/staging/r8188eu/hal/usb_ops_linux.c | 59 +++++++++++++++++++--
> >  1 file changed, 56 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/
staging/r8188eu/hal/usb_ops_linux.c
> > index 2d5e9b3ba538..ef35358cf2d3 100644
> > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > @@ -89,6 +89,59 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, 
u16 value, void *data, u1
> >  	return status;
> >  }
> >  
> > +static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 
size)
> > +{
> > +	struct adapter *adapt = intfhdl->padapter;
> > +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> > +	struct usb_device *udev = dvobjpriv->pusbdev;
> > +	int status;
> > +	u8 *io_buf; /* Pointer to I/O buffer */
> 
> As you "know" size is not going to be larger than 4 (hint, you should
> probably check it), just use bytes off of the stack here, and you can
> ignore this buffer entirely.  That will hopefully allow you in the
> future to get rid of that buffer as odds are it will not be needed
> anymore.

Dear Greg,

Yes we know that in fact, for the rtw_read*() cases, we only have 1,2,4 bytes 
size embedded in the calls. It's different for rtw_write*() where we have a 
version, that is rtw_writeN(), that could pass larger sizes (checked by the 
caller to not be larger than  VENDOR_CMD_MAX_DATA_LEN).

Said that, we already get rid of the buffer in 19/19. As far as 17/19 and 
18/19 are regarded we prefer to leave the code as-is, because we have 
many other changes in this 17/19 and in the next 18/19.

> 
> > +
> > +	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
> > +		return -EPERM;
> 
> How is it ok to check this outside of the lock?  What happens if these
> values change right _after_ you check them?

Yes, this is a mistake that was in the original code and we didn't noticed. 
Anyway, we guess that moving the acquire of he mutex soon before this check 
is all that is required. I hope it is the correct fix. Isn't it?

> 
> Why check them at all, is this something that we even care about?
> 
> I know you are trying to make this just the same logic at is there
> today, but why not just do it right the first time?
> 

Yes, we are trying to make the same logic that is there today: we already 
changed a lot of other logic that was in this code. I see that 
bSurpriseRemoved is set in error cases, so for the moment we don't feel like 
removing anything related to this variable. With a high degree of probability  
we'll do this task later in future patches. 

Do you agree with the argument above?

For instance, we got rid of that while loop around the control messages 
sending/receiving API calls and we got rid of some plainly useless 'if' tests 
(such as "if ((value >= FW_8188E_START_ADDRESS && value <= 
FW_8188E_END_ADDRESS) || status == len) break;"), and much more.

> > +
> > +	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
> > +
> > +	io_buf = dvobjpriv->usb_vendor_req_buf;
> > +
> > +	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> > +				      REALTEK_USB_VENQT_READ, 
addr,
> > +				      REALTEK_USB_VENQT_CMD_IDX, 
io_buf,
> > +				      size, 
RTW_USB_CONTROL_MSG_TIMEOUT,
> > +				      GFP_KERNEL);
> > +
> > +	if (status == -ESHUTDOWN ||
> > +	    status == -ENODEV ||
> > +	    status == -ENOENT) {
> > +		/*
> > +		 * device or controller has been disabled due to
> > +		 * some problem that could not be worked around,
> > +		 * device or bus doesn’t exist, endpoint does not
> > +		 * exist or is not enabled.
> > +		 */
> > +		adapt->bSurpriseRemoved = true;
> > +		goto mutex_unlock;
> > +	}
> > +
> > +	if (status < 0) {
> > +		GET_HAL_DATA(adapt)->srestpriv.wifi_error_status =
> > +			USB_VEN_REQ_CMD_FAIL;
> > +
> > +		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
> > +			adapt->bSurpriseRemoved = true;
> > +
> > +		goto mutex_unlock;
> > +	}
> > +
> > +	rtw_reset_continual_urb_error(dvobjpriv);
> > +	memcpy(data, io_buf, size);
> > +
> > +mutex_unlock:
> > +	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
> > +
> > +	return status;
> 
> No one cares about this value, is that ok?

I'm aware of Pavel's work on changing all the callers up to the top of the 
chains in order to do proper error checking. That's why we return status 
here.

> 
> thanks,
> 
> greg k-h
> 





^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2021-09-18 12:19 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 01/19] staging: r8188eu: remove usb_{read,write}_mem() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 02/19] staging: r8188eu: remove the helpers of rtw_read8() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 03/19] staging: r8188eu: remove the helpers of rtw_read16() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 04/19] staging: r8188eu: remove the helpers of rtw_read32() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 05/19] staging: r8188eu: remove the helpers of usb_write8() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 06/19] staging: r8188eu: remove the helpers of usb_write16() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 07/19] staging: r8188eu: remove the helpers of usb_write32() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 08/19] staging: r8188eu: remove the helpers of usb_writeN() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 09/19] staging: r8188eu: remove the helpers of usb_read_port() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 10/19] staging: r8188eu: remove the helpers of usb_write_port() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 11/19] staging: r8188eu: remove the helpers of usb_read_port_cancel() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 12/19] staging: r8188eu: remove the helpers of usb_write_port_cancel() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 13/19] staging: r8188eu: remove core/rtw_io.c Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 14/19] staging: r8188eu: remove struct _io_ops Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 15/19] staging: r8188eu: clean up usbctrl_vendorreq() Fabio M. De Francesco
2021-09-17 14:44   ` Greg Kroah-Hartman
2021-09-18 11:13     ` Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 16/19] staging: r8188eu: clean up rtw_read*() and rtw_write*() Fabio M. De Francesco
2021-09-17 14:44   ` Greg Kroah-Hartman
2021-09-17 14:45   ` Greg Kroah-Hartman
2021-09-18 11:41     ` Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}() Fabio M. De Francesco
2021-09-17 14:50   ` Greg Kroah-Hartman
2021-09-17 14:54     ` Pavel Skripkin
2021-09-17 15:01     ` David Laight
2021-09-17 15:17       ` 'Greg Kroah-Hartman'
2021-09-18 12:19     ` Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 18/19] staging: r8188eu: shorten calls chain of rtw_write{8,16,32,N}() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests Fabio M. De Francesco
2021-09-17 14:55   ` Greg Kroah-Hartman
2021-09-17 15:03     ` Pavel Skripkin
2021-09-17 15:06       ` Pavel Skripkin
2021-09-17 15:18       ` Greg Kroah-Hartman
2021-09-17 15:23         ` Pavel Skripkin
2021-09-17 11:07 ` [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.