staging: r8188eu: Use new usb_control_msg_recv/send()

PATCH In review Fabio M. De Francesco 3 participants 11 messages Aug 25, 2021 – Aug 26, 2021 Staging

Share & tools

Fabio M. De Francesco

author

2021-08-25 03:53 UTC #0 lore raw

v3 cover staging: r8188eu: Use new usb_control_msg_recv/send()

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

Signature · 1 line

2.32.0

Fabio M. De Francesco

author

2021-08-25 03:53 UTC #1 lore raw

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

Signed-off-by Fabio M. De Francesco <[email protected]>

---

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 <[email protected]>, remove 'pipe' and pass an explicit 0 to the new API. According to suggestions by Pavel Skripkin <[email protected]>, 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 <[email protected]> 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(-)

drivers/staging/r8188eu/hal/usb_ops_linux.c +17 ~~-28~~

index a93d5cfe4635..21581ca5f214 100644
@@ -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:

Signature · 1 line

2.32.0

Pavel Skripkin

Reviewed-by on v3 1/2

2021-08-25 09:01 UTC #2 lore raw

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

Quoted 8 lines · 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 <[email protected]>
Signed-off-by Fabio M. De Francesco <[email protected]>

Reviewed-by Pavel Skripkin <[email protected]>

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

With regards,
Pavel Skripkin

Greg Kroah-Hartman

maintainer

2021-08-26 10:48 UTC #3 lore raw

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

In reply to Fabio M. De Francesco

Quoted 21 lines · 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 <[email protected]>
Signed-off-by Fabio M. De Francesco <[email protected]>
---
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 <[email protected]>, remove 'pipe' and pass an explicit 0 to the new API. According to suggestions by Pavel Skripkin <[email protected]>, 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 <[email protected]> who kindly offered his time for the purpose of testing v1.

Quoted 2 lines

 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

Fabio M. De Francesco

author

2021-08-26 14:24 UTC #4 lore raw

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

Quoted 22 lines · CEST Greg Kroah-Hartman

Quoted 21 lines · 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 <[email protected]>
Signed-off-by Fabio M. De Francesco <[email protected]>
---
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 <[email protected]>, remove 'pipe' and pass an explicit 0 to the new API. According to suggestions by Pavel Skripkin <[email protected]>, 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 <[email protected]> who kindly offered his time for the purpose of testing v1.

Quoted 4 lines

Quoted 2 lines
 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.

Quoted 6 lines

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?

Quoted 6 lines

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

Quoted 4 lines

thanks,
greg k-h

Greg Kroah-Hartman

maintainer

2021-08-26 14:45 UTC #5 lore raw

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

Quoted 23 lines · Fabio M. De Francesco

Quoted 22 lines · CEST Greg Kroah-Hartman
Quoted 21 lines · 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 <[email protected]>
Signed-off-by Fabio M. De Francesco <[email protected]>
---
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 <[email protected]>, remove 'pipe' and pass an explicit 0 to the new API. According to suggestions by Pavel Skripkin <[email protected]>, 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 <[email protected]> who kindly offered his time for the purpose of testing v1.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
 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?

Quoted 7 lines

Quoted 6 lines
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.

Quoted 7 lines

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

Fabio M. De Francesco

author

2021-08-26 15:43 UTC #6 lore raw

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

Quoted 27 lines · CEST Greg Kroah-Hartman

Quoted 26 lines · Fabio M. De Francesco
Quoted 25 lines · CEST Greg Kroah-Hartman
Quoted 23 lines · 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 <[email protected]>
Signed-off-by Fabio M. De Francesco <[email protected]>
---
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 <[email protected]>, remove 'pipe' and pass an explicit 0 to the new API. According to suggestions by Pavel Skripkin <[email protected]>, 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 <[email protected]> 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(-)

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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).

Quoted 24 lines

Quoted 7 lines
Quoted 6 lines
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.
Quoted 7 lines
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.

Quoted 8 lines

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.

Quoted 1 line

good luck!

Thanks, I'll need it :-)

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

Fabio

Quoted 1 line

greg k-h

Pavel Skripkin

Tested-by on v3 1/2

2021-08-26 18:18 UTC #7 lore raw

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

In reply to Fabio M. De Francesco

Quoted 9 lines · 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 <[email protected]>
Signed-off-by Fabio M. De Francesco <[email protected]>
---

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

Tested-by Pavel Skripkin <[email protected]>

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

With regards,
Pavel Skripkin

Fabio M. De Francesco

author

2021-08-26 18:56 UTC #8 lore raw

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

Quoted 14 lines · CEST Pavel Skripkin

Quoted 9 lines · 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 <[email protected]>
Signed-off-by Fabio M. De Francesco <[email protected]>
---
FYI, I've tested this patch with TP-Link TL-WN722N v3 + qemu :)

Quoted 8 lines

Tested-by Pavel Skripkin <[email protected]>
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/[email protected]/

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

Pavel Skripkin

2021-08-26 19:12 UTC #9 lore raw

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

Quoted 44 lines · Fabio M. De Francesco

Quoted 14 lines · CEST Pavel Skripkin
Quoted 9 lines · 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 <[email protected]>
Signed-off-by Fabio M. De Francesco <[email protected]>
---
FYI, I've tested this patch with TP-Link TL-WN722N v3 + qemu :)
Quoted 8 lines
Tested-by Pavel Skripkin <[email protected]>
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:

Quoted 7 lines

https://lore.kernel.org/lkml/[email protected]/
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

Fabio M. De Francesco

author

2021-08-25 03:53 UTC #10 lore raw

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

In reply to Fabio M. De Francesco

Signed-off-by Fabio M. De Francesco <[email protected]>

---

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

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

drivers/staging/r8188eu/hal/usb_ops_linux.c +11 ~~-11~~

index 21581ca5f214..85da4fb5bf12 100644
@@ -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;

Signature · 1 line

2.32.0