[PATCH v3 0/2] staging: r8188eu: Use new usb_control_msg_recv/send()

[PATCH v3 0/2] staging: r8188eu: Use new usb_control_msg_recv/send()

[Fabio M. De Francesco]

Replace usb_control_msg() with the new usb_control_msg_recv() and
usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
Remove no more needed variables. Move out of an if-else block
some code that it is no more dependent on status < 0. Remove
redundant code depending on status > 0 or status == len.

After replacing usb_control_msg() with the new usb_control_msg_recv() and
usb_control_msg_send() API of USB Core, remove camelcase from the pIo_buf
variable that is passed as argument to the new API and remove the initial
'p' (that probably stands for "pointer") from the same pIo_buf and from
the pintfhdl and pdata arguments of usbctrl_vendorreq().


Fabio M. De Francesco (2):
  staging: r8188eu: Use usb_control_msg_recv/send() in
    usbctrl_vendorreq()
  staging: r8188eu: Make some clean-ups in usbctrl_vendorreq()

 drivers/staging/r8188eu/hal/usb_ops_linux.c | 65 +++++++++------------
 1 file changed, 27 insertions(+), 38 deletions(-)

-- 
2.32.0

[PATCH v3 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Fabio M. De Francesco]

Replace usb_control_msg() with the new usb_control_msg_recv() and
usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
Remove no more needed variables. Move out of an if-else block
some code that it is no more dependent on status < 0. Remove
redundant code depending on status > 0 or status == len.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---

v2->v3: Restore the test for success of usb_control_message_recv/send
that was inadvertently removed. Issue reported by Pavel Skripkin.

v1->v2: According to suggestions by Christophe JAILLET 
<christophe.jaillet@wanadoo.fr>, remove 'pipe' and pass an explicit 0
to the new API. According to suggestions by Pavel Skripkin 
<paskripkin@gmail.com>, remove an extra if-else that is no more needed, 
since status can be 0 and < 0 and there is no 3rd state, like it was before.
Many thanks to them and also to Phillip Potter <phil@philpotter.co.uk>
who kindly offered his time for the purpose of testing v1.

 drivers/staging/r8188eu/hal/usb_ops_linux.c | 45 ++++++++-------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index a93d5cfe4635..21581ca5f214 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -15,9 +15,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 	struct adapter	*adapt = pintfhdl->padapter;
 	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
 	struct usb_device *udev = dvobjpriv->pusbdev;
-	unsigned int pipe;
 	int status = 0;
-	u8 reqtype;
 	u8 *pIo_buf;
 	int vendorreq_times = 0;
 
@@ -44,22 +42,22 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 	}
 
 	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
-		memset(pIo_buf, 0, len);
-
 		if (requesttype == 0x01) {
-			pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
-			reqtype =  REALTEK_USB_VENQT_READ;
+			status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
+						      REALTEK_USB_VENQT_READ, value,
+						      REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
+						      len, RTW_USB_CONTROL_MSG_TIMEOUT,
+						      GFP_KERNEL);
 		} else {
-			pipe = usb_sndctrlpipe(udev, 0);/* write_out */
-			reqtype =  REALTEK_USB_VENQT_WRITE;
 			memcpy(pIo_buf, pdata, len);
+			status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
+						      REALTEK_USB_VENQT_WRITE, value,
+						      REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
+						      len, RTW_USB_CONTROL_MSG_TIMEOUT,
+						      GFP_KERNEL);
 		}
 
-		status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
-					 reqtype, value, REALTEK_USB_VENQT_CMD_IDX,
-					 pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT);
-
-		if (status == len) {   /*  Success this control transfer. */
+		if (!status) {   /*  Success this control transfer. */
 			rtw_reset_continual_urb_error(dvobjpriv);
 			if (requesttype == 0x01)
 				memcpy(pdata, pIo_buf,  len);
@@ -68,20 +66,11 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 				value, (requesttype == 0x01) ? "read" : "write",
 				len, status, *(u32 *)pdata, vendorreq_times);
 
-			if (status < 0) {
-				if (status == (-ESHUTDOWN) || status == -ENODEV) {
-					adapt->bSurpriseRemoved = true;
-				} else {
-					struct hal_data_8188e	*haldata = GET_HAL_DATA(adapt);
-					haldata->srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL;
-				}
-			} else { /*  status != len && status >= 0 */
-				if (status > 0) {
-					if (requesttype == 0x01) {
-						/*  For Control read transfer, we have to copy the read data from pIo_buf to pdata. */
-						memcpy(pdata, pIo_buf,  len);
-					}
-				}
+			if (status == (-ESHUTDOWN) || status == -ENODEV) {
+				adapt->bSurpriseRemoved = true;
+			} else {
+				struct hal_data_8188e	*haldata = GET_HAL_DATA(adapt);
+				haldata->srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL;
 			}
 
 			if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
@@ -92,7 +81,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 		}
 
 		/*  firmware download is checksumed, don't retry */
-		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || status == len)
+		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || !status)
 			break;
 	}
 release_mutex:
-- 
2.32.0

[PATCH v3 2/2] staging: r8188eu: Make clean-ups in usbctrl_vendorreq()

[Fabio M. De Francesco]

After replacing usb_control_msg() with the new usb_control_msg_recv() and
usb_control_msg_send() API of USB Core, remove camelcase from the pIo_buf
variable that is passed as argument to the new API and remove the initial
'p' (that probably stands for "pointer") from the same pIo_buf and from
the pintfhdl and pdata arguments of usbctrl_vendorreq().

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---

v2->v3: No changes.
v1->v2: No changes.

 drivers/staging/r8188eu/hal/usb_ops_linux.c | 22 ++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 21581ca5f214..85da4fb5bf12 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -10,13 +10,13 @@
 #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 adapter	*adapt = intfhdl->padapter;
 	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
 	struct usb_device *udev = dvobjpriv->pusbdev;
 	int status = 0;
-	u8 *pIo_buf;
+	u8 *io_buf;
 	int vendorreq_times = 0;
 
 	if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) {
@@ -33,10 +33,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;
 	}
@@ -45,14 +45,14 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 		if (requesttype == 0x01) {
 			status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
 						      REALTEK_USB_VENQT_READ, value,
-						      REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
+						      REALTEK_USB_VENQT_CMD_IDX, io_buf,
 						      len, RTW_USB_CONTROL_MSG_TIMEOUT,
 						      GFP_KERNEL);
 		} else {
-			memcpy(pIo_buf, pdata, len);
+			memcpy(io_buf, data, len);
 			status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
 						      REALTEK_USB_VENQT_WRITE, value,
-						      REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
+						      REALTEK_USB_VENQT_CMD_IDX, io_buf,
 						      len, RTW_USB_CONTROL_MSG_TIMEOUT,
 						      GFP_KERNEL);
 		}
@@ -60,11 +60,11 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 		if (!status) {   /*  Success this control transfer. */
 			rtw_reset_continual_urb_error(dvobjpriv);
 			if (requesttype == 0x01)
-				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 == 0x01) ? "read" : "write",
-				len, status, *(u32 *)pdata, vendorreq_times);
+				len, status, *(u32 *)data, vendorreq_times);
 
 			if (status == (-ESHUTDOWN) || status == -ENODEV) {
 				adapt->bSurpriseRemoved = true;
-- 
2.32.0

Re: [PATCH v3 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Pavel Skripkin]

On 8/25/21 6:53 AM, Fabio M. De Francesco wrote:
> Replace usb_control_msg() with the new usb_control_msg_recv() and
> usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> Remove no more needed variables. Move out of an if-else block
> some code that it is no more dependent on status < 0. Remove
> redundant code depending on status > 0 or status == len.
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Reviewed-by: Pavel Skripkin <paskripkin@gmail.com>


I cannot review the second one, since it's not applicable on top of 
staging-next :)




With regards,
Pavel Skripkin

Re: [PATCH v3 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Greg Kroah-Hartman]

On Wed, Aug 25, 2021 at 05:53:10AM +0200, Fabio M. De Francesco wrote:
> Replace usb_control_msg() with the new usb_control_msg_recv() and
> usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> Remove no more needed variables. Move out of an if-else block
> some code that it is no more dependent on status < 0. Remove
> redundant code depending on status > 0 or status == len.
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> 
> v2->v3: Restore the test for success of usb_control_message_recv/send
> that was inadvertently removed. Issue reported by Pavel Skripkin.
> 
> v1->v2: According to suggestions by Christophe JAILLET 
> <christophe.jaillet@wanadoo.fr>, remove 'pipe' and pass an explicit 0
> to the new API. According to suggestions by Pavel Skripkin 
> <paskripkin@gmail.com>, remove an extra if-else that is no more needed, 
> since status can be 0 and < 0 and there is no 3rd state, like it was before.
> Many thanks to them and also to Phillip Potter <phil@philpotter.co.uk>
> who kindly offered his time for the purpose of testing v1.
> 
>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 45 ++++++++-------------
>  1 file changed, 17 insertions(+), 28 deletions(-)

This doesn't apply to my tree at all.  Please rebase and resend.

But first, are you sure you want to use these new functions here?  This
is a "common" function that is called from different places for
different things.  How about unwinding the callers of this function
first, to see if they really need all of the complexity in this function
at all, and if not, then call the real USB function in those locations
instead.

It's only used in this single file, so it shouldn't be that hard to
unwind (after seeing where those calls are made from, and if they even
need to be present at all.  Hint, look at the mess of where _write16 and
friends are set to realize that structure is not needed at all, right?
It's a long chain, the more you pull on it, the messier you realize it
is...)

thanks,

greg k-h

Re: [PATCH v3 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Fabio M. De Francesco]

On Thursday, August 26, 2021 12:48:37 PM CEST Greg Kroah-Hartman wrote:
> On Wed, Aug 25, 2021 at 05:53:10AM +0200, Fabio M. De Francesco wrote:
> > Replace usb_control_msg() with the new usb_control_msg_recv() and
> > usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> > Remove no more needed variables. Move out of an if-else block
> > some code that it is no more dependent on status < 0. Remove
> > redundant code depending on status > 0 or status == len.
> > 
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> > v2->v3: Restore the test for success of usb_control_message_recv/send
> > that was inadvertently removed. Issue reported by Pavel Skripkin.
> > 
> > v1->v2: According to suggestions by Christophe JAILLET 
> > <christophe.jaillet@wanadoo.fr>, remove 'pipe' and pass an explicit 0
> > to the new API. According to suggestions by Pavel Skripkin 
> > <paskripkin@gmail.com>, remove an extra if-else that is no more needed, 
> > since status can be 0 and < 0 and there is no 3rd state, like it was before.
> > Many thanks to them and also to Phillip Potter <phil@philpotter.co.uk>
> > who kindly offered his time for the purpose of testing v1.
> > 
> >  drivers/staging/r8188eu/hal/usb_ops_linux.c | 45 ++++++++-------------
> >  1 file changed, 17 insertions(+), 28 deletions(-)
> 
> This doesn't apply to my tree at all.  Please rebase and resend.

This series cannot apply to your tree until another one of mine is applied 
("staging: r8188eu: Remove _enter/_exit_critical_mutex()"). This series builds
on the previous patch.

> But first, are you sure you want to use these new functions here?  This
> is a "common" function that is called from different places for
> different things.  How about unwinding the callers of this function
> first, to see if they really need all of the complexity in this function
> at all, and if not, then call the real USB function in those locations
> instead.

I think it could be fine to simply refactor usbctrl_vendorreq() to use the newer
API with no necessity to directly use them at least in six different places in
hal/usb_ops_linux.c. The only users of this helper are usb_read8/16/32() and
usb_write8/16/32(). Why do you prefer using usb_control_msg_recv/send() 
directly in the callers? I guess it would lead to redundant code, more or less
the same code repeated again and again within the above-mentioned six callers.
What do we improve by doing as you suggest? What am I missing?
 
> It's only used in this single file, so it shouldn't be that hard to
> unwind (after seeing where those calls are made from, and if they even
> need to be present at all.  Hint, look at the mess of where _write16 and
> friends are set to realize that structure is not needed at all, right?
> It's a long chain, the more you pull on it, the messier you realize it
> is...)

I've already exposed my POV above. However, I know that Pavel is working on
usb_read*() and usb_write*() and I wouldn't avoid to change those functions
while he is changing them. Shouldn't I better avoid further changes until 
my "Remove _enter/_exit_critical_mutexes()" get accepted (or definitely rejected)
and also wait for Pavel's series to be merged? Since usb_control_msg_recv/send()
don't return the length of the messages, my patch would break his checks of
ret == len and lead to serious bugs. I'd wait for his patches and then remove 
the ret == len check when we get rid of usb_control_msg() and use the new API.

What about my idea?

Thanks,

Fabio

> thanks,
> 
> greg k-h
> 

Re: [PATCH v3 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Greg Kroah-Hartman]

On Thu, Aug 26, 2021 at 04:24:35PM +0200, Fabio M. De Francesco wrote:
> On Thursday, August 26, 2021 12:48:37 PM CEST Greg Kroah-Hartman wrote:
> > On Wed, Aug 25, 2021 at 05:53:10AM +0200, Fabio M. De Francesco wrote:
> > > Replace usb_control_msg() with the new usb_control_msg_recv() and
> > > usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> > > Remove no more needed variables. Move out of an if-else block
> > > some code that it is no more dependent on status < 0. Remove
> > > redundant code depending on status > 0 or status == len.
> > > 
> > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > > 
> > > v2->v3: Restore the test for success of usb_control_message_recv/send
> > > that was inadvertently removed. Issue reported by Pavel Skripkin.
> > > 
> > > v1->v2: According to suggestions by Christophe JAILLET 
> > > <christophe.jaillet@wanadoo.fr>, remove 'pipe' and pass an explicit 0
> > > to the new API. According to suggestions by Pavel Skripkin 
> > > <paskripkin@gmail.com>, remove an extra if-else that is no more needed, 
> > > since status can be 0 and < 0 and there is no 3rd state, like it was before.
> > > Many thanks to them and also to Phillip Potter <phil@philpotter.co.uk>
> > > who kindly offered his time for the purpose of testing v1.
> > > 
> > >  drivers/staging/r8188eu/hal/usb_ops_linux.c | 45 ++++++++-------------
> > >  1 file changed, 17 insertions(+), 28 deletions(-)
> > 
> > This doesn't apply to my tree at all.  Please rebase and resend.
> 
> This series cannot apply to your tree until another one of mine is applied 
> ("staging: r8188eu: Remove _enter/_exit_critical_mutex()"). This series builds
> on the previous patch.

When you do that, please let me know somehow that this is the case,
otherwise how am I supposed to guess that?

> > But first, are you sure you want to use these new functions here?  This
> > is a "common" function that is called from different places for
> > different things.  How about unwinding the callers of this function
> > first, to see if they really need all of the complexity in this function
> > at all, and if not, then call the real USB function in those locations
> > instead.
> 
> I think it could be fine to simply refactor usbctrl_vendorreq() to use the newer
> API with no necessity to directly use them at least in six different places in
> hal/usb_ops_linux.c. The only users of this helper are usb_read8/16/32() and
> usb_write8/16/32(). Why do you prefer using usb_control_msg_recv/send() 
> directly in the callers? I guess it would lead to redundant code, more or less
> the same code repeated again and again within the above-mentioned six callers.
> What do we improve by doing as you suggest? What am I missing?

If you unwind the mess, you will find that the code will be much easier
to understand.

As an example, look at usb_write8().  Where is it ever called?  Why do
we have it at all?  It's only used in 1 place, and then that function
unwinds into rtw_write8(), which is used in a lot of places, and never
checked at all, making the majority of the logic in this function
totally unneeded and useless.

Same for rtw_write16() and rtw_write32().  After unwinding the mess you
see that the logic you are working to try to clean up in this patch
series is pretty much not used / needed at all, right?  So why do it?

Unwind the mess into a useful set of functions the driver can actually
call that is not 2-4 function pointers deep and then we can talk about
unifying things, if they are really needed.  But right now, it's
impossible to tell.

good luck!

greg k-h

Re: [PATCH v3 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Fabio M. De Francesco]

On Thursday, August 26, 2021 4:45:33 PM CEST Greg Kroah-Hartman wrote:
> On Thu, Aug 26, 2021 at 04:24:35PM +0200, Fabio M. De Francesco wrote:
> > On Thursday, August 26, 2021 12:48:37 PM CEST Greg Kroah-Hartman wrote:
> > > On Wed, Aug 25, 2021 at 05:53:10AM +0200, Fabio M. De Francesco wrote:
> > > > Replace usb_control_msg() with the new usb_control_msg_recv() and
> > > > usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> > > > Remove no more needed variables. Move out of an if-else block
> > > > some code that it is no more dependent on status < 0. Remove
> > > > redundant code depending on status > 0 or status == len.
> > > > 
> > > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > ---
> > > > 
> > > > v2->v3: Restore the test for success of usb_control_message_recv/send
> > > > that was inadvertently removed. Issue reported by Pavel Skripkin.
> > > > 
> > > > v1->v2: According to suggestions by Christophe JAILLET 
> > > > <christophe.jaillet@wanadoo.fr>, remove 'pipe' and pass an explicit 0
> > > > to the new API. According to suggestions by Pavel Skripkin 
> > > > <paskripkin@gmail.com>, remove an extra if-else that is no more needed, 
> > > > since status can be 0 and < 0 and there is no 3rd state, like it was before.
> > > > Many thanks to them and also to Phillip Potter <phil@philpotter.co.uk>
> > > > who kindly offered his time for the purpose of testing v1.
> > > > 
> > > >  drivers/staging/r8188eu/hal/usb_ops_linux.c | 45 ++++++++-------------
> > > >  1 file changed, 17 insertions(+), 28 deletions(-)
> > > 
> > > This doesn't apply to my tree at all.  Please rebase and resend.
> > 
> > This series cannot apply to your tree until another one of mine is applied 
> > ("staging: r8188eu: Remove _enter/_exit_critical_mutex()"). This series builds
> > on the previous patch.
> 
> When you do that, please let me know somehow that this is the case,
> otherwise how am I supposed to guess that?

Correct, my fault :( 

To my defense I can only say that I really had forgot that there were the 
above-mentioned previous patch still in your queue. So I didn't immediately 
realized that I had to inform you somehow of this kind of dependence.
I knew that only yesterday, when Pavel wanted to apply this patch to 
his local copy of your then current tree and he couldn't. After some thoughts
I understood that the latter depended on the former, but I guess it was too late 
to inform you. Furthermore, yesterday I thought that you would have applied 
in a FIFO order and that you wouldn't notice any conflict.

Actually I was wrong, because you didn't apply the former and instead asked
me to test it (we talked about that patch some minutes ago in another message).
 
> > > But first, are you sure you want to use these new functions here?  This
> > > is a "common" function that is called from different places for
> > > different things.  How about unwinding the callers of this function
> > > first, to see if they really need all of the complexity in this function
> > > at all, and if not, then call the real USB function in those locations
> > > instead.
> > 
> > I think it could be fine to simply refactor usbctrl_vendorreq() to use the newer
> > API with no necessity to directly use them at least in six different places in
> > hal/usb_ops_linux.c. The only users of this helper are usb_read8/16/32() and
> > usb_write8/16/32(). Why do you prefer using usb_control_msg_recv/send() 
> > directly in the callers? I guess it would lead to redundant code, more or less
> > the same code repeated again and again within the above-mentioned six callers.
> > What do we improve by doing as you suggest? What am I missing?
> 
> If you unwind the mess, you will find that the code will be much easier
> to understand.
> 
> As an example, look at usb_write8().  Where is it ever called?  Why do
> we have it at all?  It's only used in 1 place, and then that function
> unwinds into rtw_write8(), which is used in a lot of places, and never
> checked at all, making the majority of the logic in this function
> totally unneeded and useless.
> 
> Same for rtw_write16() and rtw_write32().  After unwinding the mess you
> see that the logic you are working to try to clean up in this patch
> series is pretty much not used / needed at all, right?  So why do it?
> 
> Unwind the mess into a useful set of functions the driver can actually
> call that is not 2-4 function pointers deep and then we can talk about
> unifying things, if they are really needed.  But right now, it's
> impossible to tell.

Yeah, I know how is the call chain from top (rtw_read/write8/16/32()) to bottom
(usbctrl_vendorreq()) and then to the new USB core API. 

Pavel and I have been talking about this topic while he was working on his 
series ("r8188eu: avoid uninit value bugs").

Aside from this, I re-thought about what you write above and I too find that
having 2-4 function pointers deep is a bad design. Anyway I'm stuck in 
waiting to see what Pavel will submit with his reworking, because I don't 
desire to make patches that conflict with his.

As you often say to all us: there is no hurry! So, I'll wait to see Pavel's final
work before changing whatever could conflict with him.

> good luck!

Thanks, I'll need it :-)

And thanks for the time you spent to clarify your thoughts about these topics.

Fabio
 
> greg k-h

Re: [PATCH v3 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Pavel Skripkin]

On 8/25/21 6:53 AM, Fabio M. De Francesco wrote:
> Replace usb_control_msg() with the new usb_control_msg_recv() and
> usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> Remove no more needed variables. Move out of an if-else block
> some code that it is no more dependent on status < 0. Remove
> redundant code depending on status > 0 or status == len.
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---

FYI, I've tested this patch with TP-Link TL-WN722N v3 + qemu :)


Tested-by: Pavel Skripkin <paskripkin@gmail.com>


NOTE: I am still not able to apply 2/2, so tested tag is only for 1/2


With regards,
Pavel Skripkin

Re: [PATCH v3 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Fabio M. De Francesco]

On Thursday, August 26, 2021 8:18:23 PM CEST Pavel Skripkin wrote:
> On 8/25/21 6:53 AM, Fabio M. De Francesco wrote:
> > Replace usb_control_msg() with the new usb_control_msg_recv() and
> > usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> > Remove no more needed variables. Move out of an if-else block
> > some code that it is no more dependent on status < 0. Remove
> > redundant code depending on status > 0 or status == len.
> > 
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> 
> FYI, I've tested this patch with TP-Link TL-WN722N v3 + qemu :)
> 
> 
> Tested-by: Pavel Skripkin <paskripkin@gmail.com>
> 
> 
> NOTE: I am still not able to apply 2/2, so tested tag is only for 1/2
> 
> 
> With regards,
> Pavel Skripkin

Dear Pavel,

Thanks for testing. It was very kind from you.I'll add this to the Reviewed-tag
that you had already given to my patch.

However, I intend to rebase and resend this 1/2 and the 2/2 of this series, because
(as we already found) they logically follow another patch of mine that is still in the
queue ("staging: r8188eu: Remove _enter/_exit_critical_mutex()").

The patch above has already been reviewed by Greg and he found that it looks 
good, but he cannot apply it because it is not tested (for the reasons I've already
explained with a couple of messages)

Unfortunately, until "Remove _enter/_exit_critical_mutex()" is not tested, Greg 
won't apply it and the 2/2 of this series cannot be applied too.

Please, if you have time, do you mind to test also that? It would allow me to resend 
it with your "Tested-by" tag and Greg will surely apply it. No worries if you have no
time for doing this test, otherwise you may find it at:

https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/

Please, if you are interested, read the whole thread. You'll see that Greg would apply
it, only if tested.

Thanks very much for your help and kindness,

Fabio

Re: [PATCH v3 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Pavel Skripkin]

On 8/26/21 9:56 PM, Fabio M. De Francesco wrote:
> On Thursday, August 26, 2021 8:18:23 PM CEST Pavel Skripkin wrote:
>> On 8/25/21 6:53 AM, Fabio M. De Francesco wrote:
>> > Replace usb_control_msg() with the new usb_control_msg_recv() and
>> > usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
>> > Remove no more needed variables. Move out of an if-else block
>> > some code that it is no more dependent on status < 0. Remove
>> > redundant code depending on status > 0 or status == len.
>> > 
>> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> > ---
>> 
>> FYI, I've tested this patch with TP-Link TL-WN722N v3 + qemu :)
>> 
>> 
>> Tested-by: Pavel Skripkin <paskripkin@gmail.com>
>> 
>> 
>> NOTE: I am still not able to apply 2/2, so tested tag is only for 1/2
>> 
>> 
>> With regards,
>> Pavel Skripkin
> 
> Dear Pavel,
> 
> Thanks for testing. It was very kind from you.I'll add this to the Reviewed-tag
> that you had already given to my patch.
> 
> However, I intend to rebase and resend this 1/2 and the 2/2 of this series, because
> (as we already found) they logically follow another patch of mine that is still in the
> queue ("staging: r8188eu: Remove _enter/_exit_critical_mutex()").
> 
> The patch above has already been reviewed by Greg and he found that it looks
> good, but he cannot apply it because it is not tested (for the reasons I've already
> explained with a couple of messages)
> 
> Unfortunately, until "Remove _enter/_exit_critical_mutex()" is not tested, Greg
> won't apply it and the 2/2 of this series cannot be applied too.
> 
> Please, if you have time, do you mind to test also that? It would allow me to resend
> it with your "Tested-by" tag and Greg will surely apply it. No worries if you have no
> time for doing this test, otherwise you may find it at:
> 
> https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/
> 
> Please, if you are interested, read the whole thread. You'll see that Greg would apply
> it, only if tested.
> 
> Thanks very much for your help and kindness,
> 


Hi, Fabio!


Ok, I will test it until yesterday evening. I want to enable some debug 
options and sanitizers and recompile the kernel :)

Today I will try to automate testing by writing scripts for connect, 
disconnect and other scenarios. I guess, it will help to test various 
corner cases


Simple test actually passed: I've connected to wifi + pinged google, but 
mutexes can cause deadlock and other not cool bugs, so I will try to 
enable as many sanitizers as possible and retest it :)




With regards,
Pavel Skripkin