All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains
@ 2021-09-19 23:53 Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 01/19] staging: r8188eu: clean up symbols usbctrl_vendorreq() Fabio M. De Francesco
                   ` (19 more replies)
  0 siblings, 20 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

--- Preface ---

This is v8 of "shorten and simplify calls chain". The first 14 patches
have already been applied to staging-testing, so we have been requested
to reset the numbering of the remaining patches to 01/19, while discarding
from this new submission the above-mentioned 14 patches (otherwise we would 
have submitted a series containing 33 patches).

The following commit message is provided as it was in v7, both for the 
purpose of presenting the whole picture to Maintainers, Reviewers, and to 
anybody else who may be interested in knowing the entire design and the
evolution since v1 to the current v8.

--- Commit message ---

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

v7->v8 (old numbering):
	- 1-14:
		Patches applied to staging-testing, so they are dropped
		from the current v8;

	- 15-19:
		Split into 19 patches. Numbering reset to 01. After this
		reset, 15-19/19 become 01-19/19 (so we have a total of 33
		patches in this series. 

v7->v8 (new numbering for the old 15-19/19):
	- 1-15:
		Old 15/19 and 16/19 are split into 15 patches (as it is 
		required by Greg Kroah-Hartman) in order to make them
		more easily reviewable and for checking that the logic
		is not broken; no significant changes to the resulting
		code;
	- 16-17:
		Old 17/19 and 18/19 become 16/19 and 17/19: There are 
		no significant changes to the code, with the sole
		exception of moving the acquiring of a mutex before
		the test for "bSurpriseRemoved" being 'true';
	- 18-19:
		Old 19/19 is split into new 18/19 and 19/19. The changes
		are split into a first patch that remove a shared buffer
		and a second that remove the mutex protecting the receiving 
		(in usb_read()) and sending (in usb_write()) of the
		USB control messages. 
		
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().

--- List of links to previous versions of the cover letters ---

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/
v7: https://lore.kernel.org/lkml/20210917071837.10926-1-fmdefrancesco@gmail.com/

Fabio M. De Francesco (17):
  staging: r8188eu: clean up symbols usbctrl_vendorreq()
  staging: r8188eu: reorder declarations in usbctrl_vendorreq()
  staging: r8188eu: remove unnecessary test in usbctrl_vendorreq()
  staging: r8188eu: reorder comments in usbctrl_vendorreq()
  staging: r8188eu: remove unnedeed parentheses in usbctrl_vendorreq()
  staging: r8188eu: remove unnecessary space in usbctrl_vendorreq()
  staging: r8188eu: remove unnecessary comment in usbctrl_vendorreq()
  staging: r8188eu: fix grammar mistake in usbctrl_vendorreq()
  staging: r8188eu: remove unnecessary braces in usbctrl_vendorreq()
  staging: r8188eu: rename symbols in rtw_read*() and rtw_write*()
  staging: r8188eu: remove unnecessary casts from rtw_{read,write}*()
  staging: r8188eu: change the type of a variable in rtw_write16()
  staging: r8188eu: remove an unneeded buffer from rtw_writeN()
  staging: r8188eu: remove an unnecessary bit AND from rtw_writeN()
  staging: r8188eu: change the type of a variable in rtw_read16()
  staging: r8188eu: call the new usb_read() from rtw_read{8,16,32}()
  staging: r8188eu: call the new usb_write() from rtw_write{8,16,32,N}()

Pavel Skripkin (2):
  staging: r8188eu: remove shared buffer for usb requests
  staging: r8188eu: remove usb_vendor_req_mutex

 drivers/staging/r8188eu/hal/usb_ops_linux.c | 213 ++++++++++----------
 drivers/staging/r8188eu/include/drv_types.h |   3 -
 drivers/staging/r8188eu/os_dep/usb_intf.c   |  34 +---
 3 files changed, 109 insertions(+), 141 deletions(-)

-- 
2.33.0


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

* [PATCH v8 01/19] staging: r8188eu: clean up symbols usbctrl_vendorreq()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-20 11:46   ` Greg Kroah-Hartman
  2021-09-19 23:53 ` [PATCH v8 02/19] staging: r8188eu: reorder declarations in usbctrl_vendorreq() Fabio M. De Francesco
                   ` (18 subsequent siblings)
  19 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Clean up symbol names in usbctrl_vendorreq():

	pdata => data;
        pio_priv => io_priv;
        pintfhdl => intfhdl.

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 | 28 ++++++++++-----------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 117213c9f984..3bfae4bd4a1e 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -8,14 +8,14 @@
 #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;
 	unsigned int pipe;
 	int status = 0;
-	u8 *pIo_buf;
+	u8 *io_buf;
 	int vendorreq_times = 0;
 
 	if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) {
@@ -32,10 +32,10 @@ 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;
+	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;
 	}
@@ -47,22 +47,22 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 
 	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. */
 			rtw_reset_continual_urb_error(dvobjpriv);
 			if (requesttype == REALTEK_USB_VENQT_READ)
-				memcpy(pdata, pIo_buf,  len);
+				memcpy(data, io_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);
+				len, status, *(u32 *)data, vendorreq_times);
 
 			if (status < 0) {
 				if (status == (-ESHUTDOWN) || status == -ENODEV) {
@@ -74,8 +74,8 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 			} 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);
+						/*  For Control read transfer, we have to copy the read data from io_buf to data. */
+						memcpy(data, io_buf,  len);
 					}
 				}
 			}
-- 
2.33.0


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

* [PATCH v8 02/19] staging: r8188eu: reorder declarations in usbctrl_vendorreq()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 01/19] staging: r8188eu: clean up symbols usbctrl_vendorreq() Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 03/19] staging: r8188eu: remove unnecessary test " Fabio M. De Francesco
                   ` (17 subsequent siblings)
  19 siblings, 0 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Reorder variables declarations according to the "Reverse Xmas Tree"
style.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 3bfae4bd4a1e..e06aea92c8d2 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -13,10 +13,10 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 	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;
-	int vendorreq_times = 0;
 
 	if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) {
 		status = -EPERM;
-- 
2.33.0


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

* [PATCH v8 03/19] staging: r8188eu: remove unnecessary test in usbctrl_vendorreq()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 01/19] staging: r8188eu: clean up symbols usbctrl_vendorreq() Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 02/19] staging: r8188eu: reorder declarations in usbctrl_vendorreq() Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-20 11:47   ` Greg Kroah-Hartman
  2021-09-19 23:53 ` [PATCH v8 04/19] staging: r8188eu: reorder comments " Fabio M. De Francesco
                   ` (16 subsequent siblings)
  19 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Remove unnecessary test for "!io_buf" in usbctrl_vendorreq(). Remove the
no more used "release_mutex:" label.

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 | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index e06aea92c8d2..d92bdcc3716d 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -34,12 +34,6 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 	/*  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;
-	}
-
 	if (requesttype == REALTEK_USB_VENQT_READ)
 		pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
 	else
@@ -91,7 +85,7 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 		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] 39+ messages in thread

* [PATCH v8 04/19] staging: r8188eu: reorder comments in usbctrl_vendorreq()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (2 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 03/19] staging: r8188eu: remove unnecessary test " Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-20 11:48   ` Greg Kroah-Hartman
  2021-09-19 23:53 ` [PATCH v8 05/19] staging: r8188eu: remove unnedeed parentheses " Fabio M. De Francesco
                   ` (15 subsequent siblings)
  19 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Reorder comments in usbctrl_vendorreq() to follow the Linux style.

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 | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index d92bdcc3716d..9138b730490f 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -35,9 +35,11 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 	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)
@@ -49,11 +51,13 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 					 requesttype, value, REALTEK_USB_VENQT_CMD_IDX,
 					 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(data, io_buf,  len);
-		} else { /*  error cases */
+		} 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 *)data, vendorreq_times);
@@ -65,7 +69,8 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 					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 io_buf to data. */
-- 
2.33.0


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

* [PATCH v8 05/19] staging: r8188eu: remove unnedeed parentheses in usbctrl_vendorreq()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (3 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 04/19] staging: r8188eu: reorder comments " Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 06/19] staging: r8188eu: remove unnecessary space " Fabio M. De Francesco
                   ` (14 subsequent siblings)
  19 siblings, 0 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Remove unneeded parentheses around a test for -ESHUTDOWN in
usbctrl_vendorreq().

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 9138b730490f..0b48dde5657f 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -63,7 +63,7 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 				len, status, *(u32 *)data, vendorreq_times);
 
 			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);
-- 
2.33.0


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

* [PATCH v8 06/19] staging: r8188eu: remove unnecessary space in usbctrl_vendorreq()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (4 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 05/19] staging: r8188eu: remove unnedeed parentheses " Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 07/19] staging: r8188eu: remove unnecessary comment " Fabio M. De Francesco
                   ` (13 subsequent siblings)
  19 siblings, 0 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Remove unnecessary extra space in usbctrl_vendorreq().

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 0b48dde5657f..a52aeb2558ad 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -66,7 +66,7 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 				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 {
-- 
2.33.0


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

* [PATCH v8 07/19] staging: r8188eu: remove unnecessary comment in usbctrl_vendorreq()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (5 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 06/19] staging: r8188eu: remove unnecessary space " Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-20 11:48   ` Greg Kroah-Hartman
  2021-09-19 23:53 ` [PATCH v8 08/19] staging: r8188eu: fix grammar mistake " Fabio M. De Francesco
                   ` (12 subsequent siblings)
  19 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Remove an unnecessary comment in usbctrl_vendorreq().

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 | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index a52aeb2558ad..fc3da0fbf474 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -73,7 +73,6 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 				/* 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 io_buf to data. */
 						memcpy(data, io_buf,  len);
 					}
 				}
-- 
2.33.0


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

* [PATCH v8 08/19] staging: r8188eu: fix grammar mistake in usbctrl_vendorreq()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (6 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 07/19] staging: r8188eu: remove unnecessary comment " Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-20 11:49   ` Greg Kroah-Hartman
  2021-09-19 23:53 ` [PATCH v8 09/19] staging: r8188eu: remove unnecessary braces " Fabio M. De Francesco
                   ` (11 subsequent siblings)
  19 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Fix a grammar mistake in usbctrl_vendorreq(): "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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index fc3da0fbf474..3ca2959f4bcd 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -85,7 +85,7 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 
 		}
 
-		/*  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;
 	}
-- 
2.33.0


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

* [PATCH v8 09/19] staging: r8188eu: remove unnecessary braces in usbctrl_vendorreq()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (7 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 08/19] staging: r8188eu: fix grammar mistake " Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-20 11:49   ` Greg Kroah-Hartman
  2021-09-19 23:53 ` [PATCH v8 10/19] staging: r8188eu: rename symbols in rtw_read*() and rtw_write*() Fabio M. De Francesco
                   ` (10 subsequent siblings)
  19 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Remove unnecessary braces in usbctrl_vendorreq().

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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 3ca2959f4bcd..a270cb4249b5 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -72,9 +72,8 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
 			} else {
 				/* status != len && status >= 0 */
 				if (status > 0) {
-					if (requesttype == REALTEK_USB_VENQT_READ) {
+					if (requesttype == REALTEK_USB_VENQT_READ)
 						memcpy(data, io_buf,  len);
-					}
 				}
 			}
 
-- 
2.33.0


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

* [PATCH v8 10/19] staging: r8188eu: rename symbols in rtw_read*() and rtw_write*()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (8 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 09/19] staging: r8188eu: remove unnecessary braces " Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-20 11:50   ` Greg Kroah-Hartman
  2021-09-19 23:53 ` [PATCH v8 11/19] staging: r8188eu: remove unnecessary casts from rtw_{read,write}*() Fabio M. De Francesco
                   ` (9 subsequent siblings)
  19 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Rename names of variables in rtw_read{8.16.32}() and in
rtw_write{8,16,32,N}() because of unnecessary 'p' (that probably stand
for "pointer to") and 'w' (that probably stands for "word"):

        pio_priv => io_priv;
        pintfhdl => intfhdl;
        wvalue => value.

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, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index a270cb4249b5..efdf654e434a 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -96,91 +96,91 @@ 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 value = (u16)(addr & 0x0000ffff);
 	u8 data;
 
-	usbctrl_vendorreq(pintfhdl, wvalue, &data, 1, REALTEK_USB_VENQT_READ);
+	usbctrl_vendorreq(intfhdl, value, &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);
+	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, 2, REALTEK_USB_VENQT_READ);
+	usbctrl_vendorreq(intfhdl, value, &data, 2, REALTEK_USB_VENQT_READ);
 
 	return (u16)(le32_to_cpu(data) & 0xffff);
 }
 
 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);
+	usbctrl_vendorreq(intfhdl, value, &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 value = (u16)(addr & 0x0000ffff);
 	int ret;
 
-	ret = usbctrl_vendorreq(pintfhdl, wvalue, &val, 1, REALTEK_USB_VENQT_WRITE);
+	ret = usbctrl_vendorreq(intfhdl, value, &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);
+	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 = usbctrl_vendorreq(intfhdl, value, &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;
+	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 = usbctrl_vendorreq(intfhdl, value, &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 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 _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, value, buf, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
 
 	return RTW_STATUS_CODE(ret);
 }
-- 
2.33.0


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

* [PATCH v8 11/19] staging: r8188eu: remove unnecessary casts from rtw_{read,write}*()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (9 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 10/19] staging: r8188eu: rename symbols in rtw_read*() and rtw_write*() Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 12/19] staging: r8188eu: change the type of a variable in rtw_write16() Fabio M. De Francesco
                   ` (8 subsequent siblings)
  19 siblings, 0 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Remove unnecessary casts from rtw_read{8,16,32}() and from
rtw_write{8,16,32,N}().

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 | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index efdf654e434a..ba654db869f7 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -98,7 +98,7 @@ u8 rtw_read8(struct adapter *adapter, u32 addr)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
-	u16 value = (u16)(addr & 0x0000ffff);
+	u16 value = addr & 0xffff;
 	u8 data;
 
 	usbctrl_vendorreq(intfhdl, value, &data, 1, REALTEK_USB_VENQT_READ);
@@ -110,7 +110,7 @@ u16 rtw_read16(struct adapter *adapter, u32 addr)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
-	u16 value = (u16)(addr & 0x0000ffff);
+	u16 value = addr & 0xffff;
 	__le32 data;
 
 	usbctrl_vendorreq(intfhdl, value, &data, 2, REALTEK_USB_VENQT_READ);
@@ -122,7 +122,7 @@ u32 rtw_read32(struct adapter *adapter, u32 addr)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
-	u16 value = (u16)(addr & 0x0000ffff);
+	u16 value = addr & 0xffff;
 	__le32 data;
 
 	usbctrl_vendorreq(intfhdl, value, &data, 4, REALTEK_USB_VENQT_READ);
@@ -134,7 +134,7 @@ int rtw_write8(struct adapter *adapter, u32 addr, u8 val)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
-	u16 value = (u16)(addr & 0x0000ffff);
+	u16 value = addr & 0xffff;
 	int ret;
 
 	ret = usbctrl_vendorreq(intfhdl, value, &val, 1, REALTEK_USB_VENQT_WRITE);
@@ -146,7 +146,7 @@ int rtw_write16(struct adapter *adapter, u32 addr, u16 val)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
-	u16 value = (u16)(addr & 0x0000ffff);
+	u16 value = addr & 0xffff;
 	__le32 data = cpu_to_le32(val & 0x0000ffff);
 	int ret;
 
@@ -159,7 +159,7 @@ int rtw_write32(struct adapter *adapter, u32 addr, u32 val)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
-	u16 value = (u16)(addr & 0x0000ffff);
+	u16 value = addr & 0xffff;
 	__le32 data = cpu_to_le32(val);
 	int ret;
 
@@ -172,7 +172,7 @@ int rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *data)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
-	u16 value = (u16)(addr & 0x0000ffff);
+	u16 value = addr & 0xffff;
 	u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
 	int ret;
 
-- 
2.33.0


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

* [PATCH v8 12/19] staging: r8188eu: change the type of a variable in rtw_write16()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (10 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 11/19] staging: r8188eu: remove unnecessary casts from rtw_{read,write}*() Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-20 11:50   ` Greg Kroah-Hartman
  2021-09-19 23:53 ` [PATCH v8 13/19] staging: r8188eu: remove an unneeded buffer from rtw_writeN() Fabio M. De Francesco
                   ` (7 subsequent siblings)
  19 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Change the type of "data" from __le32 to __le16.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index ba654db869f7..b2a5b6ffbb46 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -147,7 +147,7 @@ int rtw_write16(struct adapter *adapter, u32 addr, u16 val)
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
 	u16 value = addr & 0xffff;
-	__le32 data = cpu_to_le32(val & 0x0000ffff);
+	__le16 data = cpu_to_le16(val);
 	int ret;
 
 	ret = usbctrl_vendorreq(intfhdl, value, &data, 2, REALTEK_USB_VENQT_WRITE);
-- 
2.33.0


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

* [PATCH v8 13/19] staging: r8188eu: remove an unneeded buffer from rtw_writeN()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (11 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 12/19] staging: r8188eu: change the type of a variable in rtw_write16() Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 14/19] staging: r8188eu: remove an unnecessary bit AND " Fabio M. De Francesco
                   ` (6 subsequent siblings)
  19 siblings, 0 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Remove the unnecessary buffer "u8 buf[VENDOR_CMD_MAX_DATA_LEN]" and
pass directly "data" to usbctrl_vendorreq().

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 | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index b2a5b6ffbb46..aa57bd7b8f10 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -173,14 +173,12 @@ int rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *data)
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
 	u16 value = addr & 0xffff;
-	u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
 	int ret;
 
 	if (length > VENDOR_CMD_MAX_DATA_LEN)
 		return _FAIL;
 
-	memcpy(buf, data, length);
-	ret = usbctrl_vendorreq(intfhdl, value, buf, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
+	ret = usbctrl_vendorreq(intfhdl, value, data, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
 
 	return RTW_STATUS_CODE(ret);
 }
-- 
2.33.0


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

* [PATCH v8 14/19] staging: r8188eu: remove an unnecessary bit AND from rtw_writeN()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (12 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 13/19] staging: r8188eu: remove an unneeded buffer from rtw_writeN() Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16() Fabio M. De Francesco
                   ` (5 subsequent siblings)
  19 siblings, 0 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Remove an unnecessary bitwise AND because "length" can never be greater
than 0xffff since VENDOR_CMD_MAX_DATA_LEN is defined as (a weird) '254'.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index aa57bd7b8f10..625b29af9410 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -178,7 +178,7 @@ int rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *data)
 	if (length > VENDOR_CMD_MAX_DATA_LEN)
 		return _FAIL;
 
-	ret = usbctrl_vendorreq(intfhdl, value, data, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
+	ret = usbctrl_vendorreq(intfhdl, value, data, length, REALTEK_USB_VENQT_WRITE);
 
 	return RTW_STATUS_CODE(ret);
 }
-- 
2.33.0


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

* [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (13 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 14/19] staging: r8188eu: remove an unnecessary bit AND " Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-20 11:51   ` Greg Kroah-Hartman
  2021-09-20 11:56   ` Dan Carpenter
  2021-09-19 23:53 ` [PATCH v8 16/19] staging: r8188eu: call the new usb_read() from rtw_read{8,16,32}() Fabio M. De Francesco
                   ` (4 subsequent siblings)
  19 siblings, 2 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Change the type of "data" from __le32 to __le16.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 625b29af9410..c378b5740353 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -111,11 +111,11 @@ u16 rtw_read16(struct adapter *adapter, u32 addr)
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intfhdl = &io_priv->intf;
 	u16 value = addr & 0xffff;
-	__le32 data;
+	__le16 data;
 
 	usbctrl_vendorreq(intfhdl, value, &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)
-- 
2.33.0


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

* [PATCH v8 16/19] staging: r8188eu: call the new usb_read() from rtw_read{8,16,32}()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (14 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16() Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 17/19] staging: r8188eu: call the new usb_write() from rtw_write{8,16,32,N}() Fabio M. De Francesco
                   ` (3 subsequent siblings)
  19 siblings, 0 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Create and call new usb_read() instead of usbctrl_vendorreq() from
inside rtw_read{8,16,32}().

In old code, rtw_read{8,16,32}() called usbctrl_vendorreq() which in
turn uses usb_control_msg() from within a "while" loop to build a control
URB, send it off and wait for completion. usbctrl_vendorreq() was used for
both receiving and sending messages, depending on the "requesttype"
argument which was passed by callers.

Compared to usbctrl_vendorreq(), which managed both reads and writes
from and to the USB endpoint, the new usb_read() manages only reads. For
this purpose it uses the newer USB Core usb_control_msg_recv() API. The
latter is preferred according both to a suggestion by Greg Kroah-Hartman
and also to its actual design.

Two noteworthy features of usb_control_msg_recv() are that (1) the data
pointer can be made to a reference on the stack because it does not have
the restriction that usb_control_msg() has where the data pointer must be
to dynamically allocated memory, and that (2) the whole message must be
properly received from the device in order for this function to be
successfuli (if a device returns less than the expected amount of data,
then the function will fail).

usbctrl_vendorreq() uses a "while" loop that we considered unnecessary
so that it is not in the new usb_read(). Furthermore, the latter has no
redundant checking, less obvious comments, and it manages errors before
success cases. All in all, usb_read() is simpler than
usbctrl_vendorreq() and uses less lines of code.

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 c378b5740353..53704b7c1059 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -94,6 +94,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 value, 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 */
+
+	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
+
+	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
+		return -EPERM;
+
+	io_buf = dvobjpriv->usb_vendor_req_buf;
+
+	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 == -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;
@@ -101,7 +154,7 @@ u8 rtw_read8(struct adapter *adapter, u32 addr)
 	u16 value = addr & 0xffff;
 	u8 data;
 
-	usbctrl_vendorreq(intfhdl, value, &data, 1, REALTEK_USB_VENQT_READ);
+	usb_read(intfhdl, value, &data, 1);
 
 	return data;
 }
@@ -113,7 +166,7 @@ u16 rtw_read16(struct adapter *adapter, u32 addr)
 	u16 value = addr & 0xffff;
 	__le16 data;
 
-	usbctrl_vendorreq(intfhdl, value, &data, 2, REALTEK_USB_VENQT_READ);
+	usb_read(intfhdl, value, &data, 2);
 
 	return le16_to_cpu(data);
 }
@@ -125,7 +178,7 @@ u32 rtw_read32(struct adapter *adapter, u32 addr)
 	u16 value = addr & 0xffff;
 	__le32 data;
 
-	usbctrl_vendorreq(intfhdl, value, &data, 4, REALTEK_USB_VENQT_READ);
+	usb_read(intfhdl, value, &data, 4);
 
 	return le32_to_cpu(data);
 }
-- 
2.33.0


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

* [PATCH v8 17/19] staging: r8188eu: call the new usb_write() from rtw_write{8,16,32,N}()
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (15 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 16/19] staging: r8188eu: call the new usb_read() from rtw_read{8,16,32}() Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 18/19] staging: r8188eu: remove shared buffer for USB requests Fabio M. De Francesco
                   ` (2 subsequent siblings)
  19 siblings, 0 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M. De Francesco

Create and call new usb_write() instead of usbctrl_vendorreq() from
inside rtw_write{8,16,32,N}().

In old code, rtw_write{8,16,32,N}() called usbctrl_vendorreq() which in
turn uses usb_control_msg() from within a "while" loop to build a control
URB, send it off and wait for completion. usbctrl_vendorreq() was used
for both receiving and sending messages, depending on the "requesttype"
argument which is passed by callers.

Compared to usbctrl_vendorreq(), which manages both reads and writes
from and to the USB endpoint, the new usb_write() manages only writes.
For this purpose it uses the newer USB Core usb_control_msg_send() API.
The latter is preferred according both to suggestions by Greg Kroah-Hartman
and also to its actual design.

A noteworthy feature of usb_control_msg_send() is that the data pointer
can be made to a reference on the stack because it does not have the
restriction that usb_control_msg() has where the data pointer must be to
dynamically allocated memory.

usbctrl_vendorreq() used a "while" loop that we considered unnecessary
so that it is not in the new usb_write(). Furthermore, the latter has no
redundant checking, less obvious comments, no debug prints, and it manages
errors before success case. All in all, usb_write() is simpler than
usbctrl_vendorreq() and uses less lines of code.

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 | 117 +++++++-------------
 1 file changed, 42 insertions(+), 75 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 53704b7c1059..2552450ab152 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -8,93 +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 value, 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;
-	}
-
-	if (len > MAX_VENDOR_REQ_CMD_SIZE) {
-		DBG_88E("[%s] Buffer len error ,vendor request failed\n", __func__);
-		status = -EINVAL;
-		goto exit;
-	}
+	int status;
+	u8 *io_buf; /* Pointer to I/O buffer */
 
 	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);
+	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
+		return -EPERM;
 
-	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
-		if (requesttype == REALTEK_USB_VENQT_READ)
-			memset(io_buf, 0, len);
-		else
-			memcpy(io_buf, data, len);
+	io_buf = dvobjpriv->usb_vendor_req_buf;
 
-		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);
+	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 == len) {
-			/*  success */
-			rtw_reset_continual_urb_error(dvobjpriv);
-			if (requesttype == REALTEK_USB_VENQT_READ)
-				memcpy(data, io_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 *)data, 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)
-						memcpy(data, io_buf,  len);
-				}
-			}
+	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 (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
-				adapt->bSurpriseRemoved = true;
-				break;
-			}
+	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;
 
-		/*  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 value, void *data, u8 size)
+static int usb_write(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 {
 	struct adapter *adapt = intfhdl->padapter;
 	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
@@ -109,8 +76,9 @@ static int usb_read(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 
 	io_buf = dvobjpriv->usb_vendor_req_buf;
 
-	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
-				      REALTEK_USB_VENQT_READ, value,
+	memcpy(io_buf, data, size);
+	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);
@@ -139,7 +107,6 @@ static int usb_read(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 	}
 
 	rtw_reset_continual_urb_error(dvobjpriv);
-	memcpy(data, io_buf, size);
 
 mutex_unlock:
 	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
@@ -190,7 +157,7 @@ int rtw_write8(struct adapter *adapter, u32 addr, u8 val)
 	u16 value = addr & 0xffff;
 	int ret;
 
-	ret = usbctrl_vendorreq(intfhdl, value, &val, 1, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, value, &val, 1);
 
 	return RTW_STATUS_CODE(ret);
 }
@@ -203,7 +170,7 @@ int rtw_write16(struct adapter *adapter, u32 addr, u16 val)
 	__le16 data = cpu_to_le16(val);
 	int ret;
 
-	ret = usbctrl_vendorreq(intfhdl, value, &data, 2, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, value, &data, 2);
 
 	return RTW_STATUS_CODE(ret);
 }
@@ -216,7 +183,7 @@ int rtw_write32(struct adapter *adapter, u32 addr, u32 val)
 	__le32 data = cpu_to_le32(val);
 	int ret;
 
-	ret = usbctrl_vendorreq(intfhdl, value, &data, 4, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, value, &data, 4);
 
 	return RTW_STATUS_CODE(ret);
 }
@@ -231,7 +198,7 @@ int rtw_writeN(struct adapter *adapter, u32 addr, u32 length, u8 *data)
 	if (length > VENDOR_CMD_MAX_DATA_LEN)
 		return _FAIL;
 
-	ret = usbctrl_vendorreq(intfhdl, value, data, length, REALTEK_USB_VENQT_WRITE);
+	ret = usb_write(intfhdl, value, data, length);
 
 	return RTW_STATUS_CODE(ret);
 }
-- 
2.33.0


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

* [PATCH v8 18/19] staging: r8188eu: remove shared buffer for USB requests
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (16 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 17/19] staging: r8188eu: call the new usb_write() from rtw_write{8,16,32,N}() Fabio M. De Francesco
@ 2021-09-19 23:53 ` Fabio M. De Francesco
  2021-09-19 23:53 ` [PATCH v8 19/19] staging: r8188eu: remove usb_vendor_req_mutex Fabio M. De Francesco
  2021-09-20 11:55 ` [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Greg Kroah-Hartman
  19 siblings, 0 replies; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-19 23:53 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Dan Carpenter,
	Martin Kaiser
  Cc: Fabio M . De Francesco

From: Pavel Skripkin <paskripkin@gmail.com>

Remove the shared buffer for USB requests because it is not necessary
and use a local on stack variable since the new Core USB API does not 
have the constraints of usb_control_msg().

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 |  8 ++------
 drivers/staging/r8188eu/include/drv_types.h |  3 ---
 drivers/staging/r8188eu/os_dep/usb_intf.c   | 14 +-------------
 3 files changed, 3 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 2552450ab152..75500c28d6e6 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -14,15 +14,13 @@ static int usb_read(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
 	struct usb_device *udev = dvobjpriv->pusbdev;
 	int status;
-	u8 *io_buf; /* Pointer to I/O buffer */
+	u8 io_buf[4];
 
 	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
 
 	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
 		return -EPERM;
 
-	io_buf = dvobjpriv->usb_vendor_req_buf;
-
 	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
 				      REALTEK_USB_VENQT_READ, value,
 				      REALTEK_USB_VENQT_CMD_IDX, io_buf,
@@ -67,15 +65,13 @@ static int usb_write(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
 	struct usb_device *udev = dvobjpriv->pusbdev;
 	int status;
-	u8 *io_buf; /* Pointer to I/O buffer */
+	u8 io_buf[VENDOR_CMD_MAX_DATA_LEN];
 
 	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
 
 	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
 		return -EPERM;
 
-	io_buf = dvobjpriv->usb_vendor_req_buf;
-
 	memcpy(io_buf, data, size);
 	status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
 				      REALTEK_USB_VENQT_WRITE, value,
diff --git a/drivers/staging/r8188eu/include/drv_types.h b/drivers/staging/r8188eu/include/drv_types.h
index 626c6273be6f..7907dd7b5bf0 100644
--- a/drivers/staging/r8188eu/include/drv_types.h
+++ b/drivers/staging/r8188eu/include/drv_types.h
@@ -170,9 +170,6 @@ struct dvobj_priv {
 	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 11a584cbe9d8..5ab42d55207f 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -75,24 +75,12 @@ 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;
+	return _SUCCESS;
 }
 
 static void rtw_deinit_intf_priv(struct dvobj_priv *dvobj)
 {
-	kfree(dvobj->usb_alloc_vendor_req_buf);
 	mutex_destroy(&dvobj->usb_vendor_req_mutex);
 }
 
-- 
2.33.0


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

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

From: Pavel Skripkin <paskripkin@gmail.com>

This mutex was used to protect shared buffer for USB requests. Since
buffer was removed in previous patch we can remove this mutex as well.

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 | 18 ++++-------------
 drivers/staging/r8188eu/os_dep/usb_intf.c   | 22 ++-------------------
 2 files changed, 6 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 75500c28d6e6..19a37fd6a4b4 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -16,8 +16,6 @@ static int usb_read(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 	int status;
 	u8 io_buf[4];
 
-	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
-
 	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
 		return -EPERM;
 
@@ -37,7 +35,7 @@ static int usb_read(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 		 * exist or is not enabled.
 		 */
 		adapt->bSurpriseRemoved = true;
-		goto mutex_unlock;
+		return status;
 	}
 
 	if (status < 0) {
@@ -47,15 +45,12 @@ static int usb_read(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
 			adapt->bSurpriseRemoved = true;
 
-		goto mutex_unlock;
+		return status;
 	}
 
 	rtw_reset_continual_urb_error(dvobjpriv);
 	memcpy(data, io_buf, size);
 
-mutex_unlock:
-	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
-
 	return status;
 }
 
@@ -67,8 +62,6 @@ static int usb_write(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 	int status;
 	u8 io_buf[VENDOR_CMD_MAX_DATA_LEN];
 
-	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
-
 	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
 		return -EPERM;
 
@@ -89,7 +82,7 @@ static int usb_write(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 		 * exist or is not enabled.
 		 */
 		adapt->bSurpriseRemoved = true;
-		goto mutex_unlock;
+		return status;
 	}
 
 	if (status < 0) {
@@ -99,14 +92,11 @@ static int usb_write(struct intf_hdl *intfhdl, u16 value, void *data, u8 size)
 		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
 			adapt->bSurpriseRemoved = true;
 
-		goto mutex_unlock;
+		return status;
 	}
 
 	rtw_reset_continual_urb_error(dvobjpriv);
 
-mutex_unlock:
-	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
-
 	return status;
 }
 
diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
index 5ab42d55207f..2e6e6070c304 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -73,21 +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)
-{
-	mutex_init(&dvobj->usb_vendor_req_mutex);
-	return _SUCCESS;
-}
-
-static void rtw_deinit_intf_priv(struct dvobj_priv *dvobj)
-{
-	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;
@@ -146,19 +134,13 @@ 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) {
+	if (pdvobjpriv) {
 		usb_set_intfdata(usb_intf, NULL);
 		kfree(pdvobjpriv);
 		pdvobjpriv = NULL;
@@ -188,7 +170,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] 39+ messages in thread

* Re: [PATCH v8 01/19] staging: r8188eu: clean up symbols usbctrl_vendorreq()
  2021-09-19 23:53 ` [PATCH v8 01/19] staging: r8188eu: clean up symbols usbctrl_vendorreq() Fabio M. De Francesco
@ 2021-09-20 11:46   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 11:46 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:38AM +0200, Fabio M. De Francesco wrote:
> Clean up symbol names in usbctrl_vendorreq():
> 
> 	pdata => data;
>         pio_priv => io_priv;
>         pintfhdl => intfhdl.

Odd mix of tabs and spaces :(

Anyway, what does intfhdl mean?

Ugh, that's a horrible structure name for a driver, but that's not your
fault.  However, isn't 'intf' a better name for this thing as that is
how the driver does use it in other structures?

This isn't windows, where "handles" are used all over the places.  We
have real structures :)

thanks,

greg k-h

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

* Re: [PATCH v8 03/19] staging: r8188eu: remove unnecessary test in usbctrl_vendorreq()
  2021-09-19 23:53 ` [PATCH v8 03/19] staging: r8188eu: remove unnecessary test " Fabio M. De Francesco
@ 2021-09-20 11:47   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 11:47 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:40AM +0200, Fabio M. De Francesco wrote:
> Remove unnecessary test for "!io_buf" in usbctrl_vendorreq(). Remove the
> no more used "release_mutex:" label.

You need to explain _why_ it is unnecessary.

thanks,

greg k-h

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

* Re: [PATCH v8 04/19] staging: r8188eu: reorder comments in usbctrl_vendorreq()
  2021-09-19 23:53 ` [PATCH v8 04/19] staging: r8188eu: reorder comments " Fabio M. De Francesco
@ 2021-09-20 11:48   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 11:48 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:41AM +0200, Fabio M. De Francesco wrote:
> Reorder comments in usbctrl_vendorreq() to follow the Linux style.
> 
> 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 | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index d92bdcc3716d..9138b730490f 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -35,9 +35,11 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
>  	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);

For single-line if statements, that's not really needed.  Just drop the
comments entirely, the code here is "obvious".

thanks,

greg k-h

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

* Re: [PATCH v8 07/19] staging: r8188eu: remove unnecessary comment in usbctrl_vendorreq()
  2021-09-19 23:53 ` [PATCH v8 07/19] staging: r8188eu: remove unnecessary comment " Fabio M. De Francesco
@ 2021-09-20 11:48   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 11:48 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:44AM +0200, Fabio M. De Francesco wrote:
> Remove an unnecessary comment in usbctrl_vendorreq().
> 
> 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 | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index a52aeb2558ad..fc3da0fbf474 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -73,7 +73,6 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
>  				/* 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 io_buf to data. */
>  						memcpy(data, io_buf,  len);
>  					}

And now you have a coding style issue :(

Don't try to fix up one and add another one please.

thanks,

greg k-h

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

* Re: [PATCH v8 08/19] staging: r8188eu: fix grammar mistake in usbctrl_vendorreq()
  2021-09-19 23:53 ` [PATCH v8 08/19] staging: r8188eu: fix grammar mistake " Fabio M. De Francesco
@ 2021-09-20 11:49   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 11:49 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:45AM +0200, Fabio M. De Francesco wrote:
> Fix a grammar mistake in usbctrl_vendorreq(): "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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index fc3da0fbf474..3ca2959f4bcd 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -85,7 +85,7 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
>  
>  		}
>  
> -		/*  firmware download is checksumed, don't retry */
> +		/*  firmware download is checksummed, don't retry */

When touching this line, also remove the leading ' ' characters in the
comment that are not needed please.

thanks,

greg k-h

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

* Re: [PATCH v8 09/19] staging: r8188eu: remove unnecessary braces in usbctrl_vendorreq()
  2021-09-19 23:53 ` [PATCH v8 09/19] staging: r8188eu: remove unnecessary braces " Fabio M. De Francesco
@ 2021-09-20 11:49   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 11:49 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:46AM +0200, Fabio M. De Francesco wrote:
> Remove unnecessary braces in usbctrl_vendorreq().
> 
> 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 | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index 3ca2959f4bcd..a270cb4249b5 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -72,9 +72,8 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
>  			} else {
>  				/* status != len && status >= 0 */
>  				if (status > 0) {
> -					if (requesttype == REALTEK_USB_VENQT_READ) {
> +					if (requesttype == REALTEK_USB_VENQT_READ)
>  						memcpy(data, io_buf,  len);
> -					}

This should have gone in the previous commit.



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

* Re: [PATCH v8 10/19] staging: r8188eu: rename symbols in rtw_read*() and rtw_write*()
  2021-09-19 23:53 ` [PATCH v8 10/19] staging: r8188eu: rename symbols in rtw_read*() and rtw_write*() Fabio M. De Francesco
@ 2021-09-20 11:50   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 11:50 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:47AM +0200, Fabio M. De Francesco wrote:
> Rename names of variables in rtw_read{8.16.32}() and in
> rtw_write{8,16,32,N}() because of unnecessary 'p' (that probably stand
> for "pointer to") and 'w' (that probably stands for "word"):
> 
>         pio_priv => io_priv;
>         pintfhdl => intfhdl;
>         wvalue => value.

All spaces, nice :)

Anyway, same 'intfhdl' issue here.

thanks,

greg k-h

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

* Re: [PATCH v8 12/19] staging: r8188eu: change the type of a variable in rtw_write16()
  2021-09-19 23:53 ` [PATCH v8 12/19] staging: r8188eu: change the type of a variable in rtw_write16() Fabio M. De Francesco
@ 2021-09-20 11:50   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 11:50 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:49AM +0200, Fabio M. De Francesco wrote:
> Change the type of "data" from __le32 to __le16.

You are saying _what_ you are doing here, but it is not obvious at all
_why_ you need to do this.

Please explain the why here when you redo this series.

thanks,

greg k-h

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

* Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()
  2021-09-19 23:53 ` [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16() Fabio M. De Francesco
@ 2021-09-20 11:51   ` Greg Kroah-Hartman
  2021-09-20 11:56   ` Dan Carpenter
  1 sibling, 0 replies; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 11:51 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:52AM +0200, Fabio M. De Francesco wrote:
> Change the type of "data" from __le32 to __le16.

Again, why?


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

* Re: [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains
  2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
                   ` (18 preceding siblings ...)
  2021-09-19 23:53 ` [PATCH v8 19/19] staging: r8188eu: remove usb_vendor_req_mutex Fabio M. De Francesco
@ 2021-09-20 11:55 ` Greg Kroah-Hartman
  2021-09-20 13:44   ` Fabio M. De Francesco
  19 siblings, 1 reply; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 11:55 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:37AM +0200, Fabio M. De Francesco wrote:
> --- Preface ---
> 
> This is v8 of "shorten and simplify calls chain". The first 14 patches
> have already been applied to staging-testing, so we have been requested
> to reset the numbering of the remaining patches to 01/19, while discarding
> from this new submission the above-mentioned 14 patches (otherwise we would 
> have submitted a series containing 33 patches).
> 
> The following commit message is provided as it was in v7, both for the 
> purpose of presenting the whole picture to Maintainers, Reviewers, and to 
> anybody else who may be interested in knowing the entire design and the
> evolution since v1 to the current v8.
> 
> --- Commit message ---
> 
> 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 ---
> 
> v7->v8 (old numbering):
> 	- 1-14:
> 		Patches applied to staging-testing, so they are dropped
> 		from the current v8;
> 
> 	- 15-19:
> 		Split into 19 patches. Numbering reset to 01. After this
> 		reset, 15-19/19 become 01-19/19 (so we have a total of 33
> 		patches in this series. 

Better, still needs a bit more work.  I took 2 of these to shorten your
load a bit :)

thanks,

greg k-h

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

* Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()
  2021-09-19 23:53 ` [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16() Fabio M. De Francesco
  2021-09-20 11:51   ` Greg Kroah-Hartman
@ 2021-09-20 11:56   ` Dan Carpenter
  2021-09-20 13:03     ` Fabio M. De Francesco
  1 sibling, 1 reply; 39+ messages in thread
From: Dan Carpenter @ 2021-09-20 11:56 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Martin Kaiser

On Mon, Sep 20, 2021 at 01:53:52AM +0200, Fabio M. De Francesco wrote:
> Change the type of "data" from __le32 to __le16.
> 

You should note in the commit message that:

The last two bytes of "data" are not initialized so the le32_to_cpu(data)
technically reads uninitialized data.  This can likely be detected by
the KASan checker as reading uninitialized data.  But because the bytes
are discarded in the end so this will not affect runtime.

regards,
dan carpenter


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

* Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()
  2021-09-20 11:56   ` Dan Carpenter
@ 2021-09-20 13:03     ` Fabio M. De Francesco
  2021-09-20 13:10       ` Dan Carpenter
  0 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-20 13:03 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Martin Kaiser

On Monday, September 20, 2021 1:56:47 PM CEST Dan Carpenter wrote:
> On Mon, Sep 20, 2021 at 01:53:52AM +0200, Fabio M. De Francesco wrote:
> > Change the type of "data" from __le32 to __le16.
> > 
> 
> You should note in the commit message that:
> 
> The last two bytes of "data" are not initialized so the le32_to_cpu(data)
> technically reads uninitialized data.  This can likely be detected by
> the KASan checker as reading uninitialized data.  But because the bytes
> are discarded in the end so this will not affect runtime.
> 
> regards,
> dan carpenter
> 

Dear Dan,

Thanks for your suggestion about this specific topic. 

We thought that, since "data" is in bitwise AND with 0xffff before being 
passed to the callee, it was enough to have reviewers know why we're doing 
that change of type with no further explanations. Actually it seems to be not 
enough to motivate that change.

We will surely use the note you provided. 

However, since I'm not used to blindly follow suggestions (even if I trust 
your words with no doubts at all) without complete understanding of what I'm 
doing, I will need to understand what KASan is before copy-paste your note.

Thank you very much,

Fabio



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

* Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()
  2021-09-20 13:03     ` Fabio M. De Francesco
@ 2021-09-20 13:10       ` Dan Carpenter
  2021-09-20 16:17         ` Fabio M. De Francesco
  0 siblings, 1 reply; 39+ messages in thread
From: Dan Carpenter @ 2021-09-20 13:10 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Martin Kaiser

On Mon, Sep 20, 2021 at 03:03:44PM +0200, Fabio M. De Francesco wrote:
> On Monday, September 20, 2021 1:56:47 PM CEST Dan Carpenter wrote:
> > On Mon, Sep 20, 2021 at 01:53:52AM +0200, Fabio M. De Francesco wrote:
> > > Change the type of "data" from __le32 to __le16.
> > > 
> > 
> > You should note in the commit message that:
> > 
> > The last two bytes of "data" are not initialized so the le32_to_cpu(data)
> > technically reads uninitialized data.  This can likely be detected by
> > the KASan checker as reading uninitialized data.  But because the bytes
> > are discarded in the end so this will not affect runtime.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Dear Dan,
> 
> Thanks for your suggestion about this specific topic. 
> 
> We thought that, since "data" is in bitwise AND with 0xffff before being 
> passed to the callee, it was enough to have reviewers know why we're doing 
> that change of type with no further explanations. Actually it seems to be not 
> enough to motivate that change.
> 
> We will surely use the note you provided. 
> 
> However, since I'm not used to blindly follow suggestions (even if I trust 
> your words with no doubts at all) without complete understanding of what I'm 
> doing, I will need to understand what KASan is before copy-paste your note.

Google is your friend!

Either way reading uninitialized data is generally bad.  The trickier
thing is showing that your changes don't affect runtime.  For both of
these le32 to le16 changes.

regards,
dan carpenter


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

* Re: [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains
  2021-09-20 11:55 ` [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Greg Kroah-Hartman
@ 2021-09-20 13:44   ` Fabio M. De Francesco
  2021-09-20 14:06     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-20 13:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Monday, September 20, 2021 1:55:42 PM CEST Greg Kroah-Hartman wrote:
> On Mon, Sep 20, 2021 at 01:53:37AM +0200, Fabio M. De Francesco wrote:
> > --- Preface ---
> > 
> > This is v8 of "shorten and simplify calls chain". The first 14 patches
> > have already been applied to staging-testing, so we have been requested
> > to reset the numbering of the remaining patches to 01/19, while 
discarding
> > from this new submission the above-mentioned 14 patches (otherwise we 
would 
> > have submitted a series containing 33 patches).
> > 
> > The following commit message is provided as it was in v7, both for the 
> > purpose of presenting the whole picture to Maintainers, Reviewers, and to 
> > anybody else who may be interested in knowing the entire design and the
> > evolution since v1 to the current v8.
> > 
> > --- Commit message ---
> > 
> > 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 ---
> > 
> > v7->v8 (old numbering):
> > 	- 1-14:
> > 		Patches applied to staging-testing, so they are 
dropped
> > 		from the current v8;
> > 
> > 	- 15-19:
> > 		Split into 19 patches. Numbering reset to 01. After 
this
> > 		reset, 15-19/19 become 01-19/19 (so we have a total of 
33
> > 		patches in this series. 
> 
> Better, still needs a bit more work.  I took 2 of these to shorten your
> load a bit :)
>
Thanks, Greg. I've already seen that you took those other 2 from the series. 
We are about half way to destination: 14 + 2 already taken. :)

I've not replied to the other messages of yours because they are easy to 
understand, fix or change commit messages, and resend as v9.

Instead I'm replying to this particular message because I'm not able to see 
where "a bit more work" is needed.

Maybe that you don't like that in paragraph "v7->v8 (old numbering" I only  
said that the old 15-19/19 (4 patches) are now split into 19 without further 
information about each logical group within those 19?

Please notice that, immediately after the above-mentioned section there is 
also the "v7->v8 (new numbering for the old 15-19/19):" new section where 
everything is detailed to a level that we considered sufficient.

For your convenience I copy-paste the lines that immediately follow the "old 
numbering" section:

v7->v8 (new numbering for the old 15-19/19):
        - 1-15:
                Old 15/19 and 16/19 are split into 15 patches (as it is 
                required by Greg Kroah-Hartman) in order to make them
                more easily reviewable and for checking that the logic
                is not broken; no significant changes to the resulting
                code;
        - 16-17:
                Old 17/19 and 18/19 become 16/19 and 17/19: There are 
                no significant changes to the code, with the sole
                exception of moving the acquiring of a mutex before
                the test for "bSurpriseRemoved" being 'true';
        - 18-19:
                Old 19/19 is split into new 18/19 and 19/19. The changes
                are split into a first patch that remove a shared buffer
                and a second that remove the mutex protecting the receiving 
                (in usb_read()) and sending (in usb_write()) of the
                USB control messages. 

If it is not what you require, can you please provide some more hints about 
what you would consider the "perfect" approach to rework this changelog?

It would help a lot, because at the moment I cannot see what I'm missing and 
I've been unable to reach Pavel and ask him if he saw something wrong that I 
can't see.

Thanks,

Fabio
> 
> thanks,
> 
> greg k-h
> 





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

* Re: [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains
  2021-09-20 13:44   ` Fabio M. De Francesco
@ 2021-09-20 14:06     ` Greg Kroah-Hartman
  0 siblings, 0 replies; 39+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-20 14:06 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Pavel Skripkin, linux-staging,
	linux-kernel, David Laight, Dan Carpenter, Martin Kaiser

On Mon, Sep 20, 2021 at 03:44:53PM +0200, Fabio M. De Francesco wrote:
> On Monday, September 20, 2021 1:55:42 PM CEST Greg Kroah-Hartman wrote:
> > On Mon, Sep 20, 2021 at 01:53:37AM +0200, Fabio M. De Francesco wrote:
> > > --- Preface ---
> > > 
> > > This is v8 of "shorten and simplify calls chain". The first 14 patches
> > > have already been applied to staging-testing, so we have been requested
> > > to reset the numbering of the remaining patches to 01/19, while 
> discarding
> > > from this new submission the above-mentioned 14 patches (otherwise we 
> would 
> > > have submitted a series containing 33 patches).
> > > 
> > > The following commit message is provided as it was in v7, both for the 
> > > purpose of presenting the whole picture to Maintainers, Reviewers, and to 
> > > anybody else who may be interested in knowing the entire design and the
> > > evolution since v1 to the current v8.
> > > 
> > > --- Commit message ---
> > > 
> > > 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 ---
> > > 
> > > v7->v8 (old numbering):
> > > 	- 1-14:
> > > 		Patches applied to staging-testing, so they are 
> dropped
> > > 		from the current v8;
> > > 
> > > 	- 15-19:
> > > 		Split into 19 patches. Numbering reset to 01. After 
> this
> > > 		reset, 15-19/19 become 01-19/19 (so we have a total of 
> 33
> > > 		patches in this series. 
> > 
> > Better, still needs a bit more work.  I took 2 of these to shorten your
> > load a bit :)
> >
> Thanks, Greg. I've already seen that you took those other 2 from the series. 
> We are about half way to destination: 14 + 2 already taken. :)
> 
> I've not replied to the other messages of yours because they are easy to 
> understand, fix or change commit messages, and resend as v9.
> 
> Instead I'm replying to this particular message because I'm not able to see 
> where "a bit more work" is needed.

The "bit more work" was my review comments on the patches.

thanks,

greg k-h

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

* Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()
  2021-09-20 13:10       ` Dan Carpenter
@ 2021-09-20 16:17         ` Fabio M. De Francesco
  2021-09-20 19:01           ` Dan Carpenter
  0 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-20 16:17 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Martin Kaiser

On Monday, September 20, 2021 3:10:36 PM CEST Dan Carpenter wrote:
> On Mon, Sep 20, 2021 at 03:03:44PM +0200, Fabio M. De Francesco wrote:
> > On Monday, September 20, 2021 1:56:47 PM CEST Dan Carpenter wrote:
> > > On Mon, Sep 20, 2021 at 01:53:52AM +0200, Fabio M. De Francesco wrote:
> > > > Change the type of "data" from __le32 to __le16.
> > > > 
> > > 
> > > You should note in the commit message that:
> > > 
> > > The last two bytes of "data" are not initialized so the 
le32_to_cpu(data)
> > > technically reads uninitialized data.  This can likely be detected by
> > > the KASan checker as reading uninitialized data.  But because the bytes
> > > are discarded in the end so this will not affect runtime.
> > > 
> > > regards,
> > > dan carpenter
> > > 
> > 
> > Dear Dan,
> > 
> > Thanks for your suggestion about this specific topic. 
> > 
> > We thought that, since "data" is in bitwise AND with 0xffff before being 
> > passed to the callee, it was enough to have reviewers know why we're 
doing 
> > that change of type with no further explanations. Actually it seems to be 
not 
> > enough to motivate that change.
> > 
> > We will surely use the note you provided. 
> > 
> > However, since I'm not used to blindly follow suggestions (even if I 
trust 
> > your words with no doubts at all) without complete understanding of what 
I'm 
> > doing, I will need to understand what KASan is before copy-paste your 
note.
> 
> Google is your friend!

Yes, it is :)

I think you were referring to the KernelMemorySanitizer (KMSan), a detector 
of uses of uninitialized memory (but it seems to not be upstream):
https://github.com/google/kmsan

Instead you wrote about the The Kernel Address Sanitizer (KASan) that seems 
to be a dynamic memory error detector designed to find out-of-bound and use-
after-free bugs (this is upstream):
https://www.kernel.org/doc/html/v5.0/dev-tools/kasan.html

Can you please confirm?

Back to the code... uninitialised data is not a problem in the old code, it's 
just bad design. The new code cannot affect runtime, it's just better design. 

There's no change in runtime behaviour because of different protection nets:
Aside from the bitwise AND that truncate that variable two the size of two 
bytes and set the higher bytes to 0, memcpy() inside usbctrl_vendorreq(), the 
new usb_read() and usb_write uses memcpy() with count = size (and size is 
checked also in rtw_writeN()). 

I can't see any bugs. Just bad design, that we fix and possible sanitizer's 
warning, that disappear with our fixes. Am I right?

Thanks,

Fabio


> 
> Either way reading uninitialized data is generally bad.  The trickier
> thing is showing that your changes don't affect runtime.  For both of
> these le32 to le16 changes.
> 
> regards,
> dan carpenter
> 
> 





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

* Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()
  2021-09-20 16:17         ` Fabio M. De Francesco
@ 2021-09-20 19:01           ` Dan Carpenter
  2021-09-20 19:54             ` Fabio M. De Francesco
  0 siblings, 1 reply; 39+ messages in thread
From: Dan Carpenter @ 2021-09-20 19:01 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Martin Kaiser

On Mon, Sep 20, 2021 at 06:17:36PM +0200, Fabio M. De Francesco wrote:
> On Monday, September 20, 2021 3:10:36 PM CEST Dan Carpenter wrote:
> > On Mon, Sep 20, 2021 at 03:03:44PM +0200, Fabio M. De Francesco wrote:
> > > On Monday, September 20, 2021 1:56:47 PM CEST Dan Carpenter wrote:
> > > > On Mon, Sep 20, 2021 at 01:53:52AM +0200, Fabio M. De Francesco wrote:
> > > > > Change the type of "data" from __le32 to __le16.
> > > > > 
> > > > 
> > > > You should note in the commit message that:
> > > > 
> > > > The last two bytes of "data" are not initialized so the 
> le32_to_cpu(data)
> > > > technically reads uninitialized data.  This can likely be detected by
> > > > the KASan checker as reading uninitialized data.  But because the bytes
> > > > are discarded in the end so this will not affect runtime.
> > > > 
> > > > regards,
> > > > dan carpenter
> > > > 
> > > 
> > > Dear Dan,
> > > 
> > > Thanks for your suggestion about this specific topic. 
> > > 
> > > We thought that, since "data" is in bitwise AND with 0xffff before being 
> > > passed to the callee, it was enough to have reviewers know why we're 
> doing 
> > > that change of type with no further explanations. Actually it seems to be 
> not 
> > > enough to motivate that change.
> > > 
> > > We will surely use the note you provided. 
> > > 
> > > However, since I'm not used to blindly follow suggestions (even if I 
> trust 
> > > your words with no doubts at all) without complete understanding of what 
> I'm 
> > > doing, I will need to understand what KASan is before copy-paste your 
> note.
> > 
> > Google is your friend!
> 
> Yes, it is :)
> 
> I think you were referring to the KernelMemorySanitizer (KMSan), a detector 
> of uses of uninitialized memory (but it seems to not be upstream):
> https://github.com/google/kmsan 
> 
> Instead you wrote about the The Kernel Address Sanitizer (KASan) that seems 
> to be a dynamic memory error detector designed to find out-of-bound and use-
> after-free bugs (this is upstream):
> https://www.kernel.org/doc/html/v5.0/dev-tools/kasan.html 
> 
> Can you please confirm?

That sounds probably right.

> 
> Back to the code... uninitialised data is not a problem in the old code, it's 
> just bad design. The new code cannot affect runtime, it's just better design. 

It would be a problem for KMSan and the kbuild-bot will email you about
it.  From your commit message "Change the type of "data" from __le32 to
__le16." it's not clear you understand why the kbuild-bot will email you
and why it's correct to do so.

regards,
dan carpenter



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

* Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()
  2021-09-20 19:01           ` Dan Carpenter
@ 2021-09-20 19:54             ` Fabio M. De Francesco
  2021-09-21  5:35               ` Dan Carpenter
  0 siblings, 1 reply; 39+ messages in thread
From: Fabio M. De Francesco @ 2021-09-20 19:54 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Martin Kaiser

On Monday, September 20, 2021 9:01:33 PM CEST Dan Carpenter wrote:
> On Mon, Sep 20, 2021 at 06:17:36PM +0200, Fabio M. De Francesco wrote:
> > On Monday, September 20, 2021 3:10:36 PM CEST Dan Carpenter wrote:

> > Back to the code... uninitialised data is not a problem in the old code, 
it's 
> > just bad design. The new code cannot affect runtime, it's just better 
design. 
> 
> It would be a problem for KMSan and the kbuild-bot will email you about
> it.  From your commit message "Change the type of "data" from __le32 to
> __le16." it's not clear you understand why the kbuild-bot will email you
> and why it's correct to do so.

I guess that for sure you are right in saying that it is not entirely clear 
why the kbuild-bot will email me. However, since the change is correct, but 
the commit message is not, I confirm that we'll use your note with only a 
slight change (only if you agree): replace KASan with KMSan in your text.

Thanks,

Fabio  
 
> regards,
> dan carpenter




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

* Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()
  2021-09-20 19:54             ` Fabio M. De Francesco
@ 2021-09-21  5:35               ` Dan Carpenter
  0 siblings, 0 replies; 39+ messages in thread
From: Dan Carpenter @ 2021-09-21  5:35 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Pavel Skripkin,
	linux-staging, linux-kernel, David Laight, Martin Kaiser

On Mon, Sep 20, 2021 at 09:54:41PM +0200, Fabio M. De Francesco wrote:
> On Monday, September 20, 2021 9:01:33 PM CEST Dan Carpenter wrote:
> > On Mon, Sep 20, 2021 at 06:17:36PM +0200, Fabio M. De Francesco wrote:
> > > On Monday, September 20, 2021 3:10:36 PM CEST Dan Carpenter wrote:
> 
> > > Back to the code... uninitialised data is not a problem in the old code, 
> it's 
> > > just bad design. The new code cannot affect runtime, it's just better 
> design. 
> > 
> > It would be a problem for KMSan and the kbuild-bot will email you about
> > it.  From your commit message "Change the type of "data" from __le32 to
> > __le16." it's not clear you understand why the kbuild-bot will email you
> > and why it's correct to do so.
> 
> I guess that for sure you are right in saying that it is not entirely clear 
> why the kbuild-bot will email me. However, since the change is correct, but 
> the commit message is not, I confirm that we'll use your note with only a 
> slight change (only if you agree): replace KASan with KMSan in your text.
> 

Perfect.  Thanks.

regards,
dan carpenter


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

end of thread, other threads:[~2021-09-21  5:36 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 01/19] staging: r8188eu: clean up symbols usbctrl_vendorreq() Fabio M. De Francesco
2021-09-20 11:46   ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 02/19] staging: r8188eu: reorder declarations in usbctrl_vendorreq() Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 03/19] staging: r8188eu: remove unnecessary test " Fabio M. De Francesco
2021-09-20 11:47   ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 04/19] staging: r8188eu: reorder comments " Fabio M. De Francesco
2021-09-20 11:48   ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 05/19] staging: r8188eu: remove unnedeed parentheses " Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 06/19] staging: r8188eu: remove unnecessary space " Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 07/19] staging: r8188eu: remove unnecessary comment " Fabio M. De Francesco
2021-09-20 11:48   ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 08/19] staging: r8188eu: fix grammar mistake " Fabio M. De Francesco
2021-09-20 11:49   ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 09/19] staging: r8188eu: remove unnecessary braces " Fabio M. De Francesco
2021-09-20 11:49   ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 10/19] staging: r8188eu: rename symbols in rtw_read*() and rtw_write*() Fabio M. De Francesco
2021-09-20 11:50   ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 11/19] staging: r8188eu: remove unnecessary casts from rtw_{read,write}*() Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 12/19] staging: r8188eu: change the type of a variable in rtw_write16() Fabio M. De Francesco
2021-09-20 11:50   ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 13/19] staging: r8188eu: remove an unneeded buffer from rtw_writeN() Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 14/19] staging: r8188eu: remove an unnecessary bit AND " Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16() Fabio M. De Francesco
2021-09-20 11:51   ` Greg Kroah-Hartman
2021-09-20 11:56   ` Dan Carpenter
2021-09-20 13:03     ` Fabio M. De Francesco
2021-09-20 13:10       ` Dan Carpenter
2021-09-20 16:17         ` Fabio M. De Francesco
2021-09-20 19:01           ` Dan Carpenter
2021-09-20 19:54             ` Fabio M. De Francesco
2021-09-21  5:35               ` Dan Carpenter
2021-09-19 23:53 ` [PATCH v8 16/19] staging: r8188eu: call the new usb_read() from rtw_read{8,16,32}() Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 17/19] staging: r8188eu: call the new usb_write() from rtw_write{8,16,32,N}() Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 18/19] staging: r8188eu: remove shared buffer for USB requests Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 19/19] staging: r8188eu: remove usb_vendor_req_mutex Fabio M. De Francesco
2021-09-20 11:55 ` [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Greg Kroah-Hartman
2021-09-20 13:44   ` Fabio M. De Francesco
2021-09-20 14:06     ` Greg Kroah-Hartman

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.