[PATCH v3 0/3] staging: r8188eu: Shorten and simplify calls chain

[PATCH v3 0/3] staging: r8188eu: Shorten and simplify calls chain

[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]

Fabio M. De Francesco (2):
  staging: r8188eu: Shorten calls chain of rtw_read8/16/32()
  staging: r8188eu: Shorten calls chain of rtw_write8/16/32/N()

Pavel Skripkin (1):
  staging: r8188eu: remove _io_ops structure

 drivers/staging/r8188eu/core/rtw_io.c         | 241 +----------------
 drivers/staging/r8188eu/hal/usb_halinit.c     |   6 +-
 drivers/staging/r8188eu/hal/usb_ops_linux.c   | 242 +++++++++++-------
 drivers/staging/r8188eu/include/rtw_io.h      |  76 +-----
 drivers/staging/r8188eu/include/usb_ops.h     |   2 -
 .../staging/r8188eu/include/usb_ops_linux.h   |   8 -
 drivers/staging/r8188eu/os_dep/usb_intf.c     |   2 +-
 .../staging/r8188eu/os_dep/usb_ops_linux.c    |  40 +--
 8 files changed, 174 insertions(+), 443 deletions(-)

-- 
2.33.0

[PATCH v3 1/3] staging: r8188eu: remove _io_ops structure

[Fabio M. De Francesco]

From: Pavel Skripkin <paskripkin@gmail.com>

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.

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

v2->v3: No changes.

v1->v2: No changes.

 drivers/staging/r8188eu/core/rtw_io.c         | 241 +-----------------
 drivers/staging/r8188eu/hal/usb_halinit.c     |   6 +-
 drivers/staging/r8188eu/hal/usb_ops_linux.c   |  70 ++---
 drivers/staging/r8188eu/include/rtw_io.h      |  76 +-----
 drivers/staging/r8188eu/include/usb_ops.h     |   2 -
 .../staging/r8188eu/include/usb_ops_linux.h   |   8 -
 drivers/staging/r8188eu/os_dep/usb_intf.c     |   2 +-
 .../staging/r8188eu/os_dep/usb_ops_linux.c    |  40 +--
 8 files changed, 68 insertions(+), 377 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_io.c b/drivers/staging/r8188eu/core/rtw_io.c
index cde0205816b1..2d5cc080bf1d 100644
--- a/drivers/staging/r8188eu/core/rtw_io.c
+++ b/drivers/staging/r8188eu/core/rtw_io.c
@@ -34,224 +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;
-	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;
-	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;
-	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;
-	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;
-	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;
-	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;
-	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);
-}
-
-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);
-	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);
-	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(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;
@@ -261,7 +43,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);
@@ -269,31 +51,12 @@ 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))
+void rtw_init_io_priv(struct adapter *padapter)
 {
 	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/hal/usb_halinit.c b/drivers/staging/r8188eu/hal/usb_halinit.c
index 147c51255878..33147cbc55bb 100644
--- a/drivers/staging/r8188eu/hal/usb_halinit.c
+++ b/drivers/staging/r8188eu/hal/usb_halinit.c
@@ -1052,11 +1052,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;
 
@@ -1065,7 +1061,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 0cf69033c529..a87b0d2e87d0 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;
 
@@ -107,8 +109,10 @@ static u8 usb_read8(struct intf_hdl *pintfhdl, 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;
 
@@ -117,8 +121,10 @@ static u16 usb_read16(struct intf_hdl *pintfhdl, 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;
 
@@ -127,40 +133,59 @@ static u32 usb_read32(struct intf_hdl *pintfhdl, 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;
 
-	return usbctrl_vendorreq(pintfhdl, wvalue, &val, 1, REALTEK_USB_VENQT_WRITE);
+	ret = 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)
+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;
+
+	ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_WRITE);
 
-	return 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)
+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)
+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;
 
 	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)
@@ -431,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;
@@ -534,26 +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));
-	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;
-
-}
-
 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 4b41c7b03972..9722b76533dc 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 {
@@ -245,58 +220,34 @@ 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);
-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);
-void _rtw_read_port_cancel(struct adapter *adapter);
+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_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_write32(struct adapter *adapter, u32 addr, u32 val);
-int _rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *pdata);
+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_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(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_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))
-
-#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)				\
-	_rtw_write32((adapter), (addr), (val))
-#define  rtw_writeN(adapter, addr, length, data)			\
-	_rtw_writeN((adapter), (addr), (length), (data))
+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_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)	\
 	_rtw_write_port_and_wait((adapter), (addr), (cnt), (mem), (timeout_ms))
-#define rtw_write_port_cancel(adapter) _rtw_write_port_cancel((adapter))
 
 void rtw_write_scsi(struct adapter *adapter, u32 cnt, u8 *pmem);
 
@@ -340,8 +291,7 @@ 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));
+void rtw_init_io_priv(struct adapter *padapter);
 
 uint alloc_io_queue(struct adapter *adapter);
 void free_io_queue(struct adapter *adapter);
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
diff --git a/drivers/staging/r8188eu/include/usb_ops_linux.h b/drivers/staging/r8188eu/include/usb_ops_linux.h
index c357a3b1560e..641f059ffaf7 100644
--- a/drivers/staging/r8188eu/include/usb_ops_linux.h
+++ b/drivers/staging/r8188eu/include/usb_ops_linux.h
@@ -28,12 +28,4 @@
 
 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);
-void usb_write_port_cancel(struct intf_hdl *pintfhdl);
-
 #endif
diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
index bb85ab77fd26..78c91b14e637 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -596,7 +596,7 @@ 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);
+	rtw_init_io_priv(padapter);
 
 	/* step read_chip_version */
 	rtw_hal_read_chip_version(padapter);
diff --git a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
index 62dd4a131534..f1440a837b97 100644
--- a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
@@ -31,24 +31,14 @@ 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)
+void rtw_read_port_cancel(struct adapter *adapter)
 {
 	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 *) adapter->recvpriv.precv_buf;
 
 	DBG_88E("%s\n", __func__);
 
-	padapter->bReadPortCancel = true;
+	adapter->bReadPortCancel = true;
 
 	for (i = 0; i < NR_RECVBUFF; i++) {
 		precvbuf->reuse = true;
@@ -134,22 +124,21 @@ 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 *adapter, 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 dvobj_priv	*pdvobj = adapter_to_dvobj(adapter);
+	struct xmit_priv	*pxmitpriv = &adapter->xmitpriv;
 	struct xmit_buf *pxmitbuf = (struct xmit_buf *)wmem;
 	struct xmit_frame *pxmitframe = (struct xmit_frame *)pxmitbuf->priv_data;
 	struct usb_device *pusbd = pdvobj->pusbdev;
 
-	if ((padapter->bDriverStopped) || (padapter->bSurpriseRemoved) ||
-	    (padapter->pwrctrlpriv.pnp_bstop_trx)) {
+	if ((adapter->bDriverStopped) || (adapter->bSurpriseRemoved) ||
+	    (adapter->pwrctrlpriv.pnp_bstop_trx)) {
 		rtw_sctx_done_err(&pxmitbuf->sctx, RTW_SCTX_DONE_TX_DENY);
 		goto exit;
 	}
@@ -196,7 +185,7 @@ u32 usb_write_port(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *wmem)
 
 	status = usb_submit_urb(purb, GFP_ATOMIC);
 	if (!status) {
-		struct hal_data_8188e	*haldata = GET_HAL_DATA(padapter);
+		struct hal_data_8188e	*haldata = GET_HAL_DATA(adapter);
 
 		haldata->srestpriv.last_tx_time = jiffies;
 	} else {
@@ -205,7 +194,7 @@ u32 usb_write_port(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *wmem)
 
 		switch (status) {
 		case -ENODEV:
-			padapter->bDriverStopped = true;
+			adapter->bDriverStopped = true;
 			break;
 		default:
 			break;
@@ -224,15 +213,14 @@ u32 usb_write_port(struct intf_hdl *pintfhdl, u32 addr, u32 cnt, u8 *wmem)
 	return ret;
 }
 
-void usb_write_port_cancel(struct intf_hdl *pintfhdl)
+void rtw_write_port_cancel(struct adapter *adapter)
 {
 	int i, j;
-	struct adapter	*padapter = pintfhdl->padapter;
-	struct xmit_buf *pxmitbuf = (struct xmit_buf *)padapter->xmitpriv.pxmitbuf;
+	struct xmit_buf *pxmitbuf = (struct xmit_buf *) adapter->xmitpriv.pxmitbuf;
 
 	DBG_88E("%s\n", __func__);
 
-	padapter->bWritePortCancel = true;
+	adapter->bWritePortCancel = true;
 
 	for (i = 0; i < NR_XMITBUFF; i++) {
 		for (j = 0; j < 8; j++) {
@@ -242,7 +230,7 @@ void usb_write_port_cancel(struct intf_hdl *pintfhdl)
 		pxmitbuf++;
 	}
 
-	pxmitbuf = (struct xmit_buf *)padapter->xmitpriv.pxmit_extbuf;
+	pxmitbuf = (struct xmit_buf *) adapter->xmitpriv.pxmit_extbuf;
 	for (i = 0; i < NR_XMIT_EXTBUFF; i++) {
 		for (j = 0; j < 8; j++) {
 			if (pxmitbuf->pxmit_urb[j])
-- 
2.33.0

Re: [PATCH v3 1/3] staging: r8188eu: remove _io_ops structure

[Greg Kroah-Hartman]

On Sun, Sep 05, 2021 at 12:00:46AM +0200, Fabio M. De Francesco wrote:
> -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);
> -
> -}

This is odd, in that it resolves down to usb_read_mem which does
nothing at all.

And then no one calls this at all either?

How about removing the io ops that are not used at all first, one at a
time, making it obvious what is happening, and then convert the ones
that are used one at a time, and when all is done, then removing the
structure?

That makes it obvious what is happening and much much easier to review
for correctness.

thanks,

greg k-h

Re: [PATCH v3 1/3] staging: r8188eu: remove _io_ops structure

[Pavel Skripkin]

On 9/6/21 4:56 PM, Greg Kroah-Hartman wrote:
> On Sun, Sep 05, 2021 at 12:00:46AM +0200, Fabio M. De Francesco wrote:
>> -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);
>> -
>> -}
> 
> This is odd, in that it resolves down to usb_read_mem which does
> nothing at all.
> 
> And then no one calls this at all either?
> 

Yep, there is no caller of this function... Idk why this was added :)

> How about removing the io ops that are not used at all first, one at a
> time, making it obvious what is happening, and then convert the ones
> that are used one at a time, and when all is done, then removing the
> structure?
> 

Ok, sounds like a good idea. Will fix in v4, thank you

> That makes it obvious what is happening and much much easier to review
> for correctness.
> 

Agree


With regards,
Pavel Skripkin

Re: [PATCH v3 1/3] staging: r8188eu: remove _io_ops structure

[Greg Kroah-Hartman]

On Mon, Sep 06, 2021 at 05:01:09PM +0300, Pavel Skripkin wrote:
> On 9/6/21 4:56 PM, Greg Kroah-Hartman wrote:
> > On Sun, Sep 05, 2021 at 12:00:46AM +0200, Fabio M. De Francesco wrote:
> > > -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);
> > > -
> > > -}
> > 
> > This is odd, in that it resolves down to usb_read_mem which does
> > nothing at all.
> > 
> > And then no one calls this at all either?
> > 
> 
> Yep, there is no caller of this function... Idk why this was added :)

PCI devices require a read after a write to ensure that the write has
completed.  Odds are this is left over from that.

thanks,

greg k-h

Re: [PATCH v3 1/3] staging: r8188eu: remove _io_ops structure

[Pavel Skripkin]

On 9/6/21 16:56, Greg Kroah-Hartman wrote:
> On Sun, Sep 05, 2021 at 12:00:46AM +0200, Fabio M. De Francesco wrote:
>> -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);
>> -
>> -}
> 
> This is odd, in that it resolves down to usb_read_mem which does
> nothing at all.
> 
> And then no one calls this at all either?
> 
> How about removing the io ops that are not used at all first, one at a
> time, making it obvious what is happening, and then convert the ones
> that are used one at a time, and when all is done, then removing the
> structure?
> 

Just have started to cut one big patch to smaller ones and does it make 
sense to group changes like: one for usb_read*, one for usb_write* and 
one for usb_port*? I think, it would be cleaner and series won't be too big.


What do you think?




With regards,
Pavel Skripkin

Re: [PATCH v3 1/3] staging: r8188eu: remove _io_ops structure

[Greg Kroah-Hartman]

On Mon, Sep 06, 2021 at 08:19:05PM +0300, Pavel Skripkin wrote:
> On 9/6/21 16:56, Greg Kroah-Hartman wrote:
> > On Sun, Sep 05, 2021 at 12:00:46AM +0200, Fabio M. De Francesco wrote:
> > > -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);
> > > -
> > > -}
> > 
> > This is odd, in that it resolves down to usb_read_mem which does
> > nothing at all.
> > 
> > And then no one calls this at all either?
> > 
> > How about removing the io ops that are not used at all first, one at a
> > time, making it obvious what is happening, and then convert the ones
> > that are used one at a time, and when all is done, then removing the
> > structure?
> > 
> 
> Just have started to cut one big patch to smaller ones and does it make
> sense to group changes like: one for usb_read*, one for usb_write* and one
> for usb_port*? I think, it would be cleaner and series won't be too big.
> 
> 
> What do you think?

I will not know until I see the patches, so no need to ask :)

[PATCH v3 2/3] staging: r8188eu: Shorten calls chain of rtw_read8/16/32()

[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>
---

v2->v3: No changes.

v1->v2: No changes.

 drivers/staging/r8188eu/hal/usb_ops_linux.c | 92 +++++++++++++++++----
 1 file changed, 78 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index a87b0d2e87d0..f9c4fd5a2c53 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -97,38 +97,102 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 	return status;
 }
 
+static int usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
+{
+	u16 value = (u16)(addr & 0x0000ffff);
+	struct adapter *adapt = intfhdl->padapter;
+	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
+	struct usb_device *udev = dvobjpriv->pusbdev;
+	int status;
+	u8 *io_buf;
+	int vendorreq_times = 0;
+
+	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx) {
+		status = -EPERM;
+		goto exit;
+	}
+
+	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
+
+	/*  Acquire IO memory for vendorreq */
+	io_buf = dvobjpriv->usb_vendor_req_buf;
+
+	if (!io_buf) {
+		DBG_88E("[%s] io_buf == NULL\n", __func__);
+		status = -ENOMEM;
+		goto release_mutex;
+	}
+
+	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
+		status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
+					      REALTEK_USB_VENQT_READ, value,
+					      REALTEK_USB_VENQT_CMD_IDX, io_buf,
+					      size, RTW_USB_CONTROL_MSG_TIMEOUT,
+					      GFP_KERNEL);
+		if (!status) {   /*  Success this control transfer. */
+			rtw_reset_continual_urb_error(dvobjpriv);
+			memcpy(data, io_buf, size);
+		} else { /*  error cases */
+			DBG_88E("reg 0x%x, usb %s %u fail, status:%d vendorreq_times:%d\n",
+				value, "read", size, status, vendorreq_times);
+
+			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;
+			}
+
+			if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
+				adapt->bSurpriseRemoved = true;
+				break;
+			}
+		}
+
+		/*  firmware download is checksummed, don't retry */
+		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || !status)
+			break;
+	}
+
+release_mutex:
+	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
+exit:
+	return status;
+}
+
 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 value = (u16)(addr & 0x0000ffff);
 	u8 data;
 
-	usbctrl_vendorreq(pintfhdl, wvalue, &data, 1, REALTEK_USB_VENQT_READ);
+	usb_read(intfhdl, value, &data, 1);
 
 	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 value = (u16)(addr & 0x0000ffff);
+	__le16 data;
 
-	usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_READ);
+	usb_read(intfhdl, value, &data, 2);
 
-	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 value = (u16)(addr & 0x0000ffff);
 	__le32 data;
 
-	usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_READ);
+	usb_read(intfhdl, value, &data, 4);
 
 	return le32_to_cpu(data);
 }
-- 
2.33.0

Re: [PATCH v3 2/3] staging: r8188eu: Shorten calls chain of rtw_read8/16/32()

[Greg Kroah-Hartman]

On Sun, Sep 05, 2021 at 12:00:47AM +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>
> ---
> 
> v2->v3: No changes.
> 
> v1->v2: No changes.
> 
>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 92 +++++++++++++++++----
>  1 file changed, 78 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index a87b0d2e87d0..f9c4fd5a2c53 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -97,38 +97,102 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>  	return status;
>  }
>  
> +static int usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
> +{
> +	u16 value = (u16)(addr & 0x0000ffff);

Why not just pass in the address as a 16bit value?


> +	struct adapter *adapt = intfhdl->padapter;
> +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> +	struct usb_device *udev = dvobjpriv->pusbdev;
> +	int status;
> +	u8 *io_buf;
> +	int vendorreq_times = 0;
> +
> +	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx) {
> +		status = -EPERM;
> +		goto exit;

This is "interesting" to see if it's really even working as they think
it does, but let's leave it alone for now...

> +	}
> +
> +	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
> +
> +	/*  Acquire IO memory for vendorreq */
> +	io_buf = dvobjpriv->usb_vendor_req_buf;
> +
> +	if (!io_buf) {
> +		DBG_88E("[%s] io_buf == NULL\n", __func__);

How can this buffer ever be NULL?

> +		status = -ENOMEM;
> +		goto release_mutex;
> +	}

Why share a buffer at all anyway?

> +	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> +		status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> +					      REALTEK_USB_VENQT_READ, value,
> +					      REALTEK_USB_VENQT_CMD_IDX, io_buf,
> +					      size, RTW_USB_CONTROL_MSG_TIMEOUT,
> +					      GFP_KERNEL);
> +		if (!status) {   /*  Success this control transfer. */

Comments go on the next line.

> +			rtw_reset_continual_urb_error(dvobjpriv);
> +			memcpy(data, io_buf, size);
> +		} else { /*  error cases */

Again, next line for the comment.

> +			DBG_88E("reg 0x%x, usb %s %u fail, status:%d vendorreq_times:%d\n",
> +				value, "read", size, status, vendorreq_times);

These should be removed eventually...

> +
> +			if (status == (-ESHUTDOWN) || status == -ENODEV) {
> +				adapt->bSurpriseRemoved = true;

Odd, but ok...

> +			} else {
> +				struct hal_data_8188e *haldata = GET_HAL_DATA(adapt);
> +
> +				haldata->srestpriv.wifi_error_status = USB_VEN_REQ_CMD_FAIL;

Why are we not saying the command failed even if the device was removed?

But if we do say an error happened, why are we trying to send this out
again?  What would happen to make it work the second time?

> +			}
> +
> +			if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
> +				adapt->bSurpriseRemoved = true;

So we try to see if the device was removed again?

> +				break;
> +			}
> +		}
> +
> +		/*  firmware download is checksummed, don't retry */
> +		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || !status)
> +			break;

Nothing like a special case for firmware magic.

Those calls should just use a different write function entirely,
eventually, to remove this...

Ok, I know you are just moving code around, this is fine, just pointing
out things that should be fixed up eventually...

thanks,

greg k-h

Re: [PATCH v3 2/3] staging: r8188eu: Shorten calls chain of rtw_read8/16/32()

[Pavel Skripkin]

On 9/6/21 5:07 PM, Greg Kroah-Hartman wrote:
> On Sun, Sep 05, 2021 at 12:00:47AM +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>
>> ---
>> 
>> v2->v3: No changes.
>> 
>> v1->v2: No changes.
>> 
>>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 92 +++++++++++++++++----
>>  1 file changed, 78 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> index a87b0d2e87d0..f9c4fd5a2c53 100644
>> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> @@ -97,38 +97,102 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>>  	return status;
>>  }
>>  
>> +static int usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
>> +{
>> +	u16 value = (u16)(addr & 0x0000ffff);
> 
> Why not just pass in the address as a 16bit value?
> 
> 
>> +	struct adapter *adapt = intfhdl->padapter;
>> +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
>> +	struct usb_device *udev = dvobjpriv->pusbdev;
>> +	int status;
>> +	u8 *io_buf;
>> +	int vendorreq_times = 0;
>> +
>> +	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx) {
>> +		status = -EPERM;
>> +		goto exit;
> 
> This is "interesting" to see if it's really even working as they think
> it does, but let's leave it alone for now...
> 
>> +	}
>> +
>> +	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
>> +
>> +	/*  Acquire IO memory for vendorreq */
>> +	io_buf = dvobjpriv->usb_vendor_req_buf;
>> +
>> +	if (!io_buf) {
>> +		DBG_88E("[%s] io_buf == NULL\n", __func__);
> 
> How can this buffer ever be NULL?
> 
>> +		status = -ENOMEM;
>> +		goto release_mutex;
>> +	}
> 
> Why share a buffer at all anyway?
> 
>> +	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
>> +		status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
>> +					      REALTEK_USB_VENQT_READ, value,
>> +					      REALTEK_USB_VENQT_CMD_IDX, io_buf,
>> +					      size, RTW_USB_CONTROL_MSG_TIMEOUT,
>> +					      GFP_KERNEL);
>> +		if (!status) {   /*  Success this control transfer. */
> 
> Comments go on the next line.
> 
>> +			rtw_reset_continual_urb_error(dvobjpriv);
>> +			memcpy(data, io_buf, size);
>> +		} else { /*  error cases */
> 
> Again, next line for the comment.
> 
>> +			DBG_88E("reg 0x%x, usb %s %u fail, status:%d vendorreq_times:%d\n",
>> +				value, "read", size, status, vendorreq_times);
> 
> These should be removed eventually...
> 
>> +
>> +			if (status == (-ESHUTDOWN) || status == -ENODEV) {
>> +				adapt->bSurpriseRemoved = true;
> 
> Odd, but ok...
> 
>> +			} else {
>> +				struct hal_data_8188e *haldata = GET_HAL_DATA(adapt);
>> +
>> +				haldata->srestpriv.wifi_error_status = USB_VEN_REQ_CMD_FAIL;
> 
> Why are we not saying the command failed even if the device was removed?
> 
> But if we do say an error happened, why are we trying to send this out
> again?  What would happen to make it work the second time?
> 
>> +			}
>> +
>> +			if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
>> +				adapt->bSurpriseRemoved = true;
> 
> So we try to see if the device was removed again?
> 
>> +				break;
>> +			}
>> +		}
>> +
>> +		/*  firmware download is checksummed, don't retry */
>> +		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || !status)
>> +			break;
> 
> Nothing like a special case for firmware magic.
> 
> Those calls should just use a different write function entirely,
> eventually, to remove this...
> 
> Ok, I know you are just moving code around, this is fine, just pointing
> out things that should be fixed up eventually...
> 

I agree with all statements, and I've asked maintainers about some of 
them before. It's 100% should be fixed, but we (me and Fabio) want to 
fix them step by step to not rebase every time new clean up goes in.

Our plan is: (Fabio, please, correct me, if I am wrong here)

1. ops removal + shorten call chain (this patch series)
2. my error handling series [1]
3. Clean ups for rtw_* functions (old usb_*)
4. Remove dead proc code + introduce new sysfs interface, based on old 
proc code.


We can prepare them all at once, but, IMO:

1. It will be really hard to review
2. A lot of rebasing through new versions, since there are a lot
clean ups appear every day.


I believe, our step-by-step plan should be more comfortable for all of us :)


Thank you!


[1] 
https://lore.kernel.org/linux-staging/cover.1629479152.git.paskripkin@gmail.com/



With regards,
Pavel Skripkin

Re: [PATCH v3 2/3] staging: r8188eu: Shorten calls chain of rtw_read8/16/32()

[Fabio M. De Francesco]

On Monday, September 6, 2021 4:07:26 PM CEST Greg Kroah-Hartman wrote:
> On Sun, Sep 05, 2021 at 12:00:47AM +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>
> > ---
> > 
> > v2->v3: No changes.
> > 
> > v1->v2: No changes.
> > 
> >  drivers/staging/r8188eu/hal/usb_ops_linux.c | 92 +++++++++++++++++----
> >  1 file changed, 78 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/
staging/r8188eu/hal/usb_ops_linux.c
> > index a87b0d2e87d0..f9c4fd5a2c53 100644
> > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > @@ -97,38 +97,102 @@ static int usbctrl_vendorreq(struct intf_hdl 
*pintfhdl, u16 value, void *pdata,
> >  	return status;
> >  }
> >  
> > +static int usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 
size)
> > +{
> > +	u16 value = (u16)(addr & 0x0000ffff);
> 
> Why not just pass in the address as a 16bit value?

Yes, it should be better. It will be done in v4.

> 
> 
> > +	struct adapter *adapt = intfhdl->padapter;
> > +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> > +	struct usb_device *udev = dvobjpriv->pusbdev;
> > +	int status;
> > +	u8 *io_buf;
> > +	int vendorreq_times = 0;
> > +
> > +	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx) 
{
> > +		status = -EPERM;
> > +		goto exit;
> 
> This is "interesting" to see if it's really even working as they think
> it does, but let's leave it alone for now...

As you suggest, I also prefer to leave it alone for now.

> 
> > +	}
> > +
> > +	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
> > +
> > +	/*  Acquire IO memory for vendorreq */
> > +	io_buf = dvobjpriv->usb_vendor_req_buf;
> > +
> > +	if (!io_buf) {
> > +		DBG_88E("[%s] io_buf == NULL\n", __func__);
> 
> How can this buffer ever be NULL?

As you noticed a few lines below this, I moved some code around and ignored 
and left as was anything that wasn't functional for my purpose.. 

> 
> > +		status = -ENOMEM;
> > +		goto release_mutex;
> > +	}
> 
> Why share a buffer at all anyway?

Same answer as above.

> 
> > +	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> > +		status = usb_control_msg_recv(udev, 0, 
REALTEK_USB_VENQT_CMD_REQ,
> > +					      
REALTEK_USB_VENQT_READ, value,
> > +					      
REALTEK_USB_VENQT_CMD_IDX, io_buf,
> > +					      size, 
RTW_USB_CONTROL_MSG_TIMEOUT,
> > +					      GFP_KERNEL);
> > +		if (!status) {   /*  Success this control transfer. */
> 
> Comments go on the next line.

This will be fixed in v4, although I'm not sure if this comment and the next 
are really necessary. The code seems self-explanatory.

> 
> > +			rtw_reset_continual_urb_error(dvobjpriv);
> > +			memcpy(data, io_buf, size);
> > +		} else { /*  error cases */
> 
> Again, next line for the comment.

Same as above.

> 
> > +			DBG_88E("reg 0x%x, usb %s %u fail, status:
%d vendorreq_times:%d\n",
> > +				value, "read", size, status, 
vendorreq_times);
> 
> These should be removed eventually...
> 

I could use pr_debug() for now or remove it immediately. I'd prefer the 
latter but I'm not sure if it is the most appropriate thing to do. Let me 
think about it...

> > +
> > +			if (status == (-ESHUTDOWN) || status == -
ENODEV) {
> > +				adapt->bSurpriseRemoved = true;
> 
> Odd, but ok...
> 
> > +			} else {
> > +				struct hal_data_8188e *haldata = 
GET_HAL_DATA(adapt);
> > +
> > +				haldata-
>srestpriv.wifi_error_status = USB_VEN_REQ_CMD_FAIL;
> 
> Why are we not saying the command failed even if the device was removed?

This question is not clear to me. Are you talking about -ENOENT?
I suppose it should be if (status == (-ESHUTDOWN || -ENODEV || -ENOENT)) ...

> But if we do say an error happened, why are we trying to send this out
> again?  What would happen to make it work the second time?

There are some errors that are unrecoverable and the loop has no reason to 
re-iterate again and again. I'll break this loop on unrecoverable errors.

I see that usb_submit_urb() at the very end of the calls chain may fail for a 
lot of different reasons and some of them are a bit obscure to me 
(unfortunately, at the moment I have no time to go deeper into the 
architecture and the inner workings of the USB subsystem :) ) 

I hope that I won't overlook any of them - as far as it regards some of all 
possible errors I have doubts in telling whether or not they are 
unrecoverable and if some can actually happen in this code :(

> > +			}
> > +
> > +			if 
(rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
> > +				adapt->bSurpriseRemoved = true;
> 
> So we try to see if the device was removed again?

This must be changed, let me see if I can understand how. At the moment I 
don't have the whole picture.
 
> > +				break;
> > +			}
> > +		}
> > +
> > +		/*  firmware download is checksummed, don't retry */
> > +		if ((value >= FW_8188E_START_ADDRESS && value <= 
FW_8188E_END_ADDRESS) || !status)
> > +			break;
> 
> Nothing like a special case for firmware magic.

This is something that I really cannot understand, so I think I'll leave it 
as-is unless I may get some more hints... :)

> Those calls should just use a different write function entirely,
> eventually, to remove this...
> 
> Ok, I know you are just moving code around, this is fine, just pointing
> out things that should be fixed up eventually...

Yes, I'm just moving the code around. Anyway I guess that I can fix/change  
most of the things you pointed out.

Thanks very much for your review,

Fabio  

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v3 2/3] staging: r8188eu: Shorten calls chain of rtw_read8/16/32()

[Fabio M. De Francesco]

On Monday, September 6, 2021 4:07:26 PM CEST Greg Kroah-Hartman wrote:
> On Sun, Sep 05, 2021 at 12:00:47AM +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>
> > ---
> >
> > [...]
> > 
> > +	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> > +		status = usb_control_msg_recv(udev, 0, 
REALTEK_USB_VENQT_CMD_REQ,
> > +					      
REALTEK_USB_VENQT_READ, value,
> > +					      
REALTEK_USB_VENQT_CMD_IDX, io_buf,
> > +					      size, 
RTW_USB_CONTROL_MSG_TIMEOUT,
> > +					      GFP_KERNEL);
> > +		if (!status) {   /*  Success this control transfer. */
> 
> Comments go on the next line.
> 
> > +			rtw_reset_continual_urb_error(dvobjpriv);
> > +			memcpy(data, io_buf, size);
> > +		} else { /*  error cases */
> 
> Again, next line for the comment.
> 
> > +			DBG_88E("reg 0x%x, usb %s %u fail, status:
%d vendorreq_times:%d\n",
> > +				value, "read", size, status, 
vendorreq_times);
> 
> These should be removed eventually...
> 
> > +
> > +			if (status == (-ESHUTDOWN) || status == -
ENODEV) {
> > +				adapt->bSurpriseRemoved = true;
> 
> Odd, but ok...

I'm not so sure that it is OK. Please correct me if I'm wrong...

The calls chain from usb_control_msg_recv() seems to be the following:

usb_control_msg_recv/send()
        -> usb_control_msg()
                -> usb_internal_control_msg()
                        -> usb_start_wait_urb()
                                -> usb_submit_urb()

Each of the above functions could fail for different reasons and if so they 
return the errors up to the first caller into "status". I can find no lines 
of code where the above-mentioned functions set and return -ESHUTDOWN.

Unless I'm missing something obvious, "status" is a non-shared variable. The 
variables that are assigned with errors in all five of the above-mentioned 
functions are also local (non shared) variables.

To summarize: how could "status" be assigned -ESHUTDOWN? Is any point in the 
chain that value assigned by a concurrent thread to a shared variable and 
then returned up to the caller (i.e., usb_control_msg_recv())?

Since the code has this "if (status == (-ESHUTDOWN) || ...)" it expects that 
sometimes it could be 'true', so I'm 100% sure that I can't see where my 
argument is not valid... :(

Can someone please help me to understand this topic?

Thanks,

Fabio



> 
> > [...]

Re: [PATCH v3 2/3] staging: r8188eu: Shorten calls chain of rtw_read8/16/32()

[Fabio M. De Francesco]

On Friday, September 10, 2021 5:19:58 PM CEST Fabio M. De Francesco wrote:
> On Monday, September 6, 2021 4:07:26 PM CEST Greg Kroah-Hartman wrote:
> > On Sun, Sep 05, 2021 at 12:00:47AM +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>
> > > ---
> > >
> > > [...]
> > > 
> > > +	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> > > +		status = usb_control_msg_recv(udev, 0, 
> REALTEK_USB_VENQT_CMD_REQ,
> > > +					      
> REALTEK_USB_VENQT_READ, value,
> > > +					      
> REALTEK_USB_VENQT_CMD_IDX, io_buf,
> > > +					      size, 
> RTW_USB_CONTROL_MSG_TIMEOUT,
> > > +					      GFP_KERNEL);
> > > +		if (!status) {   /*  Success this control transfer. */
> > 
> > Comments go on the next line.
> > 
> > > +			rtw_reset_continual_urb_error(dvobjpriv);
> > > +			memcpy(data, io_buf, size);
> > > +		} else { /*  error cases */
> > 
> > Again, next line for the comment.
> > 
> > > +			DBG_88E("reg 0x%x, usb %s %u fail, status:
> %d vendorreq_times:%d\n",
> > > +				value, "read", size, status, 
> vendorreq_times);
> > 
> > These should be removed eventually...
> > 
> > > +
> > > +			if (status == (-ESHUTDOWN) || status == -
> ENODEV) {
> > > +				adapt->bSurpriseRemoved = true;
> > 
> > Odd, but ok...
> 
> I'm not so sure that it is OK. Please correct me if I'm wrong...
> 
> The calls chain from usb_control_msg_recv() seems to be the following:
> 
> usb_control_msg_recv/send()
>         -> usb_control_msg()
>                 -> usb_internal_control_msg()
>                         -> usb_start_wait_urb()
>                                 -> usb_submit_urb()
> 
> Each of the above functions could fail for different reasons and if so they 
> return the errors up to the first caller into "status". I can find no lines 
> of code where the above-mentioned functions set and return -ESHUTDOWN.
> 
> Unless I'm missing something obvious, "status" is a non-shared variable. 
The 
> variables that are assigned with errors in all five of the above-mentioned 
> functions are also local (non shared) variables.
> 
> To summarize: how could "status" be assigned -ESHUTDOWN? Is any point in 
the 
> chain that value assigned by a concurrent thread to a shared variable and 
> then returned up to the caller (i.e., usb_control_msg_recv())?
> 
> Since the code has this "if (status == (-ESHUTDOWN) || ...)" it expects 
that 
> sometimes it could be 'true', so I'm 100% sure that I can't see where my 
> argument is not valid... :(

Sorry, please disregard my previous message.

I found that, somewhere about a couple of function deeper in the chain,  the 
-ESHUTDOWN error code can indeed be returned. I had to read again and again 
every line of the chain until I saw that.

Fabio

> Can someone please help me to understand this topic?
> 
> Thanks,
> 
> Fabio
> 
> 
> 
> > 
> > > [...]
> 
> 
> 
> 

[PATCH v3 3/3] staging: r8188eu: Shorten calls chain of rtw_write8/16/32/N()

[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>
---

v2->v3: Fix the version number of the patch.
 
v1->v2: Replace parameter REALTEK_USB_VENQT_READ with REALTEK_USB_VENQT_WRITE
in usb_control_msg_send(). More in-depth explanation at
https://lore.kernel.org/lkml/2791328.7pjKATJfGa@localhost.localdomain/T/#m1fc1ab2f7c1f463049ad88d5df5bb1b107b37260

 drivers/staging/r8188eu/hal/usb_ops_linux.c | 130 ++++++++------------
 1 file changed, 53 insertions(+), 77 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index f9c4fd5a2c53..e31d1b1fdb12 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -8,76 +8,51 @@
 #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 usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
 {
-	struct adapter	*adapt = pintfhdl->padapter;
-	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
+	u16 value = (u16)(addr & 0x0000ffff);
+	struct adapter *adapt = intfhdl->padapter;
+	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
 	struct usb_device *udev = dvobjpriv->pusbdev;
-	unsigned int pipe;
-	int status = 0;
-	u8 *pIo_buf;
+	int status;
+	u8 *io_buf;
 	int vendorreq_times = 0;
 
-	if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) {
+	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx) {
 		status = -EPERM;
 		goto exit;
 	}
 
-	if (len > MAX_VENDOR_REQ_CMD_SIZE) {
-		DBG_88E("[%s] Buffer len error ,vendor request failed\n", __func__);
-		status = -EINVAL;
-		goto exit;
-	}
-
-	_enter_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL);
+	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
 
 	/*  Acquire IO memory for vendorreq */
-	pIo_buf = dvobjpriv->usb_vendor_req_buf;
+	io_buf = dvobjpriv->usb_vendor_req_buf;
 
-	if (!pIo_buf) {
-		DBG_88E("[%s] pIo_buf == NULL\n", __func__);
+	if (!io_buf) {
+		DBG_88E("[%s] io_buf == NULL\n", __func__);
 		status = -ENOMEM;
 		goto release_mutex;
 	}
 
-	if (requesttype == REALTEK_USB_VENQT_READ)
-		pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
-	else
-		pipe = usb_sndctrlpipe(udev, 0);/* write_out */
-
 	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
-		if (requesttype == REALTEK_USB_VENQT_READ)
-			memset(pIo_buf, 0, len);
-		else
-			memcpy(pIo_buf, pdata, 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);
-
-		if (status == len) {   /*  Success this control transfer. */
+		status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
+					      REALTEK_USB_VENQT_READ, value,
+					      REALTEK_USB_VENQT_CMD_IDX, io_buf,
+					      size, RTW_USB_CONTROL_MSG_TIMEOUT,
+					      GFP_KERNEL);
+		if (!status) {   /*  Success this control transfer. */
 			rtw_reset_continual_urb_error(dvobjpriv);
-			if (requesttype == REALTEK_USB_VENQT_READ)
-				memcpy(pdata, pIo_buf,  len);
+			memcpy(data, io_buf, size);
 		} 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);
-
-			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) {
-						/*  For Control read transfer, we have to copy the read data from pIo_buf to pdata. */
-						memcpy(pdata, pIo_buf,  len);
-					}
-				}
+			DBG_88E("reg 0x%x, usb %s %u fail, status:%d vendorreq_times:%d\n",
+				value, "read", size, status, vendorreq_times);
+
+			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;
 			}
 
 			if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
@@ -87,17 +62,18 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 
 		}
 
-		/*  firmware download is checksumed, don't retry */
-		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || status == len)
+		/*  firmware download is checksummed, don't retry */
+		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || !status)
 			break;
 	}
+
 release_mutex:
-	_exit_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL);
+	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
 exit:
 	return status;
 }
 
-static int usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
+static int usb_write(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
 {
 	u16 value = (u16)(addr & 0x0000ffff);
 	struct adapter *adapt = intfhdl->padapter;
@@ -123,15 +99,15 @@ static int usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
 		goto release_mutex;
 	}
 
+	memcpy(io_buf, data, size);
 	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
-		status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
-					      REALTEK_USB_VENQT_READ, value,
+		status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
+					      REALTEK_USB_VENQT_WRITE, value,
 					      REALTEK_USB_VENQT_CMD_IDX, io_buf,
 					      size, RTW_USB_CONTROL_MSG_TIMEOUT,
 					      GFP_KERNEL);
 		if (!status) {   /*  Success this control transfer. */
 			rtw_reset_continual_urb_error(dvobjpriv);
-			memcpy(data, io_buf, size);
 		} else { /*  error cases */
 			DBG_88E("reg 0x%x, usb %s %u fail, status:%d vendorreq_times:%d\n",
 				value, "read", size, status, vendorreq_times);
@@ -199,55 +175,55 @@ u32 rtw_read32(struct adapter *adapter, u32 addr)
 
 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 value = (u16)(addr & 0x0000ffff);
 	int ret;
 
-	ret = usbctrl_vendorreq(pintfhdl, wvalue, &val, 1, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, value, &val, 1);
 
 	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);
+	struct io_priv *io_priv = &adapter->iopriv;
+	struct intf_hdl *intfhdl = &io_priv->intf;
+	u16 value = (u16)(addr & 0x0000ffff);
 	__le32 data = cpu_to_le32(val & 0x0000ffff);
 	int ret;
 
-	ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, value, &data, 2);
 
 	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;
+	u16 value = (u16)(addr & 0x0000ffff);
 	__le32 data = cpu_to_le32(val);
 	int ret;
 
-	ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, value, &data, 4);
 
 	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 length, 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;
+	u16 value = (u16)(addr & 0x0000ffff);
 	u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
 	int ret;
 
 	if (length > VENDOR_CMD_MAX_DATA_LEN)
 		return -EINVAL;
 
-	memcpy(buf, pdata, length);
-	ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
+	memcpy(buf, data, length);
+	ret = usb_write(intfhdl, value, buf, (length & 0xffff));
 
 	return RTW_STATUS_CODE(ret);
 }
-- 
2.33.0

RE: [PATCH v3 3/3] staging: r8188eu: Shorten calls chain of rtw_write8/16/32/N()

[David Laight]

From: Fabio M. De Francesco
> Sent: 04 September 2021 23:01
>
> 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.
> 
...
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index f9c4fd5a2c53..e31d1b1fdb12 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -8,76 +8,51 @@
>  #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 usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
>  {
> -	struct adapter	*adapt = pintfhdl->padapter;
> -	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
> +	u16 value = (u16)(addr & 0x0000ffff);
> +	struct adapter *adapt = intfhdl->padapter;
> +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
>  	struct usb_device *udev = dvobjpriv->pusbdev;
> -	unsigned int pipe;
> -	int status = 0;
> -	u8 *pIo_buf;
> +	int status;
> +	u8 *io_buf;

Some of these changes are whitespace or renames.
They ought to be in a different patch.

I think you'll need 'reverse xmas tree' ordering as well.

	David

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

Re: [PATCH v3 3/3] staging: r8188eu: Shorten calls chain of rtw_write8/16/32/N()

[Pavel Skripkin]

On 9/7/21 1:10 PM, David Laight wrote:
> From: Fabio M. De Francesco
>> Sent: 04 September 2021 23:01
>>
>> 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.
>> 
> ...
>> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> index f9c4fd5a2c53..e31d1b1fdb12 100644
>> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> @@ -8,76 +8,51 @@
>>  #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 usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
>>  {
>> -	struct adapter	*adapt = pintfhdl->padapter;
>> -	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
>> +	u16 value = (u16)(addr & 0x0000ffff);
>> +	struct adapter *adapt = intfhdl->padapter;
>> +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
>>  	struct usb_device *udev = dvobjpriv->pusbdev;
>> -	unsigned int pipe;
>> -	int status = 0;
>> -	u8 *pIo_buf;
>> +	int status;
>> +	u8 *io_buf;
> 
> Some of these changes are whitespace or renames.
> They ought to be in a different patch.
> 
> I think you'll need 'reverse xmas tree' ordering as well.
> 
> 	David
> 
You are right, thank you. We will revert it in v4 in all patches in this 
series :)




With regards,
Pavel Skripkin

Re: [PATCH v3 3/3] staging: r8188eu: Shorten calls chain of rtw_write8/16/32/N()

[Fabio M. De Francesco]

On Tuesday, September 7, 2021 12:10:19 PM CEST David Laight wrote:
> From: Fabio M. De Francesco
> > Sent: 04 September 2021 23:01
> >
> > 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.
> > 
> ...
> > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/
staging/r8188eu/hal/usb_ops_linux.c
> > index f9c4fd5a2c53..e31d1b1fdb12 100644
> > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > @@ -8,76 +8,51 @@
> >  #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 usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 
size)
> >  {
> > -	struct adapter	*adapt = pintfhdl->padapter;
> > -	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
> > +	u16 value = (u16)(addr & 0x0000ffff);
> > +	struct adapter *adapt = intfhdl->padapter;
> > +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> >  	struct usb_device *udev = dvobjpriv->pusbdev;
> > -	unsigned int pipe;
> > -	int status = 0;
> > -	u8 *pIo_buf;
> > +	int status;
> > +	u8 *io_buf;
> 
> Some of these changes are whitespace or renames.
> They ought to be in a different patch.

Dear David,

No, they are not.

I guess you were misled by the structure of the patches. There is nothing I 
can do about it. Please notice that usb_read() is created in 2/3, and I'm 
free to use the name of the variables I like in new functions. Furthermore, 
usb_read() is untouched in 3/3. I can see why you thought they are renames :)

> 
> I think you'll need 'reverse xmas tree' ordering as well.
> 

I didn't know this rule, but I must agree that this style is horrible. I'll 
change the order of variables declaration for the purpose of fixing this 
ugly "reverse Xmas tree".

Thanks for your review.

Regards,

Fabio

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

RE: [PATCH v3 3/3] staging: r8188eu: Shorten calls chain of rtw_write8/16/32/N()

[David Laight]

From: Fabio M. De Francesco
> To: Larry Finger <Larry.Finger@lwfinger.net>; Phillip Potter <phil@philpotter.co.uk>; Greg Kroah-
> > ...
> > > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/
> staging/r8188eu/hal/usb_ops_linux.c
> > > index f9c4fd5a2c53..e31d1b1fdb12 100644
> > > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > > @@ -8,76 +8,51 @@
> > >  #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 usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8
> size)
> > >  {
> > > -	struct adapter	*adapt = pintfhdl->padapter;
> > > -	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
> > > +	u16 value = (u16)(addr & 0x0000ffff);
> > > +	struct adapter *adapt = intfhdl->padapter;
> > > +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> > >  	struct usb_device *udev = dvobjpriv->pusbdev;
> > > -	unsigned int pipe;
> > > -	int status = 0;
> > > -	u8 *pIo_buf;
> > > +	int status;
> > > +	u8 *io_buf;
> >
> > Some of these changes are whitespace or renames.
> > They ought to be in a different patch.
> 
> Dear David,
> 
> No, they are not.
> 
> I guess you were misled by the structure of the patches. There is nothing I
> can do about it. Please notice that usb_read() is created in 2/3, and I'm
> free to use the name of the variables I like in new functions. Furthermore,
> usb_read() is untouched in 3/3. I can see why you thought they are renames :)

Hmmm... it might be worth playing with the patches and the
order of functions so that the diffs are meaningful.

Some experimentation can be needed.

	David

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

Re: [PATCH v3 3/3] staging: r8188eu: Shorten calls chain of rtw_write8/16/32/N()

[Fabio M. De Francesco]

On Thursday, September 9, 2021 10:21:04 AM CEST David Laight wrote:
> From: Fabio M. De Francesco
> > To: Larry Finger <Larry.Finger@lwfinger.net>; Phillip Potter 
<phil@philpotter.co.uk>; Greg Kroah-
> > > ...
> > > > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/
> > staging/r8188eu/hal/usb_ops_linux.c
> > > > index f9c4fd5a2c53..e31d1b1fdb12 100644
> > > > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > > > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > > > @@ -8,76 +8,51 @@
> > > >  #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 usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, 
u8
> > size)
> > > >  {
> > > > -	struct adapter	*adapt = pintfhdl->padapter;
> > > > -	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
> > > > +	u16 value = (u16)(addr & 0x0000ffff);
> > > > +	struct adapter *adapt = intfhdl->padapter;
> > > > +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> > > >  	struct usb_device *udev = dvobjpriv->pusbdev;
> > > > -	unsigned int pipe;
> > > > -	int status = 0;
> > > > -	u8 *pIo_buf;
> > > > +	int status;
> > > > +	u8 *io_buf;
> > >
> > > Some of these changes are whitespace or renames.
> > > They ought to be in a different patch.
> > 
> > Dear David,
> > 
> > No, they are not.
> > 
> > I guess you were misled by the structure of the patches. There is nothing 
I
> > can do about it. Please notice that usb_read() is created in 2/3, and I'm
> > free to use the name of the variables I like in new functions. 
Furthermore,
> > usb_read() is untouched in 3/3. I can see why you thought they are 
renames :)
> 
> Hmmm... it might be worth playing with the patches and the
> order of functions so that the diffs are meaningful.

Yes, I agree. Readability of the diffs should be taken into account because 
it make the work of reviewers a bit easier.

I'm trying and see if some clean-up of usbctrl_vendorreq() (from which I 
copied most of the code and pasted in my new functions) can improve patches 
readability if it is carried out preventively (I mean before creating 
usb_read() in 2/3 and usb_write() in 3/3).

I hope that that preventive clean-ups would help when in 3/3 I will delete 
usbctrl_vendorreq() and the new code overlaps the lines that were used by 
that function.

Let's see... Perhaps it may solve out problem :)

Thanks,

Fabio

> 
> Some experimentation can be needed.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 
1PT, UK
> Registration No: 1397386 (Wales)
>