staging: r8188eu: Use new usb_control_msg_recv/send()
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 camelcase and the unneded initial 'p' (for pointer?) from the pIo_buf variable 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 | 47 ++++++++++----------- 1 file changed, 23 insertions(+), 24 deletions(-)
Signature · 1 line
2.32.0
PATCH 1/2 staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Replace usb_control_msg() with the new usb_control_msg_recv() and usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
---
Thanks to Pavel Skripkin <[email protected]> for his review of the RFC patch.
drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++-----------
1 file changed, 12 insertions(+), 13 deletions(-)
drivers/staging/r8188eu/hal/usb_ops_linux.c +12 -13
@@ -15,9 +15,8 @@ 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; +u8 pipe;int status =0 ; -u8 reqtype;u8 *pIo_buf;int vendorreq_times =0 ;
@@ -44,22 +43,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, pipe,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, pipe,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);
Signature · 1 line
2.32.0
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Quoted 64 lines · Fabio M. De Francesco
Replace
usb_control_msg()with the newusb_control_msg_recv()andusb_control_msg_send()API of USB Core inusbctrl_vendorreq().Suggested-by Greg Kroah-Hartman <[email protected]>Signed-off-by Fabio M. De Francesco <[email protected]>---
Thanks to Pavel Skripkin <[email protected]> for his review of the RFC patch.
drivers/staging/r8188eu/hal/usb_ops_linux.c| 25 ++++++++++-----------1 file changed, 12 insertions(+), 13 deletions(-)drivers/staging/r8188eu/hal/usb_ops_linux.c +12
-14@@ -15,9 +15,8 @@ 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; +u8 pipe;int status =0 ; -u8 reqtype;u8 *pIo_buf;int vendorreq_times =0 ;@@ -44,22 +43,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, pipe,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, pipe,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); --
Quoted 2 lines
2.32.0
Dear Fabio,
Thanks for the patch. Sorry, but for some reason with my N10-Nano I can't get a connection at all with this patch applied - it just won't associate with my network. Interface shows up and no OOPS in log, but just disassociates/no IP address/interface down etc. so perhaps semantics differ slightly here somehow? Tried two separate rollbacks/builds/runs just to make sure I wasn't losing my mind :-)
Regards,
Phil
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Quoted 78 lines · CEST Phillip Potter
Quoted 64 lines · Fabio M. De Francesco
Replace
usb_control_msg()with the newusb_control_msg_recv()andusb_control_msg_send()API of USB Core inusbctrl_vendorreq().Suggested-by Greg Kroah-Hartman <[email protected]>Signed-off-by Fabio M. De Francesco <[email protected]>---
Thanks to Pavel Skripkin <[email protected]> for his review of the RFC patch.
drivers/staging/r8188eu/hal/usb_ops_linux.c| 25 ++++++++++-----------1 file changed, 12 insertions(+), 13 deletions(-)drivers/staging/r8188eu/hal/usb_ops_linux.c +12
-14@@ -15,9 +15,8 @@ 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; +u8 pipe;int status =0 ; -u8 reqtype;u8 *pIo_buf;int vendorreq_times =0 ;@@ -44,22 +43,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, pipe,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, pipe,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); --Quoted 2 lines
2.32.0
Dear Fabio,
Thanks for the patch. Sorry, but for some reason with my N10-Nano I can't get a connection at all with this patch applied - it just won't associate with my network. Interface shows up and no OOPS in log, but just disassociates/no IP address/interface down etc. so perhaps semantics differ slightly here somehow? Tried two separate rollbacks/builds/runs just to make sure I wasn't losing my mind :-)
Quoted 3 lines
Regards,
Phil
Dear Philip,
Thanks for testing. As I wrote in my RFC, I strongly suspected that I was not able to correctly understand the semantics of the new API. I'll try to read the code anew and try to understand what is wrong here.
However, I also think that I won't be able to figure it out. Maybe that I have to wait for Greg to give me some hint about what are the errors in using usb_control_msg_send/recv() the way I did.
Anyway, thanks a lot for the time you spent testing.
Regards,
Fabio
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Quoted 92 lines · CEST Fabio M. De Francesco
Quoted 78 lines · CEST Phillip Potter
Quoted 64 lines · Fabio M. De Francesco
Replace
usb_control_msg()with the newusb_control_msg_recv()andusb_control_msg_send()API of USB Core inusbctrl_vendorreq().Suggested-by Greg Kroah-Hartman <[email protected]>Signed-off-by Fabio M. De Francesco <[email protected]>---
Thanks to Pavel Skripkin <[email protected]> for his review of the RFC patch.
drivers/staging/r8188eu/hal/usb_ops_linux.c| 25 ++++++++++-----------1 file changed, 12 insertions(+), 13 deletions(-)drivers/staging/r8188eu/hal/usb_ops_linux.c +12
-14@@ -15,9 +15,8 @@ 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; +u8 pipe;int status =0 ; -u8 reqtype;u8 *pIo_buf;int vendorreq_times =0 ;@@ -44,22 +43,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, pipe,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, pipe,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); --Quoted 2 lines
2.32.0
Dear Fabio,
Thanks for the patch. Sorry, but for some reason with my N10-Nano I can't get a connection at all with this patch applied - it just won't associate with my network. Interface shows up and no OOPS in log, but just disassociates/no IP address/interface down etc. so perhaps semantics differ slightly here somehow? Tried two separate rollbacks/builds/runs just to make sure I wasn't losing my mind :-)
Quoted 3 lines
Regards,
PhilDear Philip,
Thanks for testing. As I wrote in my RFC, I strongly suspected that I was not able to correctly understand the semantics of the new API. I'll try to read the code anew and try to understand what is wrong here.
However, I also think that I won't be able to figure it out. Maybe that I have to wait for Greg to give me some hint about what are the errors in using usb_control_msg_send/recv() the way I did.
Quoted 6 lines
Anyway, thanks a lot for the time you spent testing.
Regards,
Fabio
Dear Philip,
I think that I've inadvertently switched the order by which usb_control_msg_send() and memcpy() are called. I'm very sorry for not doing my tests, but (as I had said before) at the moment I don't have my device with me.
I'm about to send a v2 series.
Thanks very much for testing on my behalf.
Regards,
Fabio
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Quoted 3 lines · CEST Fabio M. De Francesco
I think that I've inadvertently switched the order by which
usb_control_msg_send()andmemcpy()are called. I'm very sorry for not doing my tests, but (as I had said before) at the moment I don't have my device with me.
No, I did not switch them. There must be something else... Sorry for the noise.
Fabio
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Le 24/08/2021 à 04:01, Fabio M. De Francesco a écrit :
Quoted 10 lines · CEST Fabio M. De Francesco
Quoted 3 lines
I think that I've inadvertently switched the order by which
usb_control_msg_send()andmemcpy()are called. I'm very sorry for not doing my tests, but (as I had said before) at the moment I don't have my device with me.No, I did not switch them. There must be something else... Sorry for the noise.
Fabio
Hi,
'usb_control_msg_recv()' looks like:
int usb_control_msg_recv(struct usb_device *dev, __u8 endpoint, ...)
{
unsigned int pipe = usb_rcvctrlpipe(dev, endpoint);
...
ret = usb_control_msg(dev, pipe, ...);'usb_control_msg()' looks like:
int usb_control_msg(struct usb_device *dev, unsigned int pipe, ...) {
The difference is that one expect an 'endpoint' (and compute the pipe from it), and the other expect a 'pipe'.
Also, in your code, 'pipe' looks un-initialized.
So, my guess is that you should rename 'pipe' into 'endpoint' (to keep the semantic), have "endpoint = 0;" somewhere and pass it to usb_control_msg_{recv|send}. Or just remove 'pipe' and pass an explicit 0 directly.
Not sure it is enough, but it looks like a difference between before and after your patch.
just my 2c,
CJ
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Quoted 23 lines · CEST Christophe JAILLET
Quoted 10 lines · Fabio M. De Francesco
Quoted 3 lines · CEST Fabio M. De Francesco
I think that I've inadvertently switched the order by which
usb_control_msg_send()andmemcpy()are called. I'm very sorry for not doing my tests, but (as I had said before) at the moment I don't have my device with me.No, I did not switch them. There must be something else... Sorry for the noise.
Fabio
Hi,
'
usb_control_msg_recv()' looks like:int
usb_control_msg_recv(struct usb_device *dev, __u8 endpoint, ...){ unsigned int pipe = usb_rcvctrlpipe(dev, endpoint); ... ret = usb_control_msg(dev, pipe, ...);
Quoted 6 lines
'
usb_control_msg()' looks like:
intusb_control_msg(struct usb_device *dev, unsigned int pipe, ...){The difference is that one expect an 'endpoint' (and compute the pipe from it), and the other expect a 'pipe'.
Hi Christophe,
Yes, correct. That's why I changed the type of 'pipe' from "unsigned int" to "u8". I also saw that usb_control_msg_recv/send take care of calling usb_rcvctrpipe() and usb_sndctrlpipe(); so, in my patch I deleted those calls.
Not related to my patch... why Linux has u8 and __u8? What are the different use cases they are meant for?
Quoted 1 line
Also, in your code, 'pipe' looks un-initialized.
Oh yes, good catch. Thanks!
Quoted 5 lines
So, my guess is that you should rename 'pipe' into 'endpoint' (to keep the semantic),
have "endpoint = 0;" somewhere and pass it to usb_control_msg_{recv|send}.
Or just remove 'pipe' and pass an explicit 0 directly.
I've just seen that in other drivers the code passes an explicit 0. So, also according to your suggestion, I'll remove "pipe/endpoint".
Quoted 2 lines
Not sure it is enough, but it looks like a difference between before and after your patch.
Since I cannot see other issues, I'm about to fix the code as said above and then submit a v2 series.
Your 2c are worth much more than how much you think :)
Thanks very much,
Fabio
Quoted 3 lines
just my 2c,
CJ
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Le 24/08/2021 à 12:38, Fabio M. De Francesco a écrit :
Quoted 2 lines
Not related to my patch... why Linux has u8 and
__u8? What are the different use cases they are meant for?
Maybe:
https://elixir.bootlin.com/linux/v5.14-rc6/source/include/uapi/asm-generic/int-l64.h#L16
helps ?
Quoted 1 line
Your 2c are worth much more than how much you think :)
If you insist, I could send you my Paypal address, but knowing that it may help someone should already be enough for me :).
Let me know if it was the root cause of the issue.
CJ
Quoted 13 lines
Thanks very much,
Fabio
Quoted 3 lines
just my 2c,
CJ
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
In reply to Fabio M. De FrancescoQuoted 7 lines · Fabio M. De Francesco
Dear Philip,
I think that I've inadvertently switched the order by which
usb_control_msg_send()andmemcpy()are called. I'm very sorry for not doing my tests, but (as I had said before) at the moment I don't have my device with me.
Quoted 10 lines
I'm about to send a v2 series.
Thanks very much for testing on my behalf.
Regards,
Fabio
Dear Fabio,
Just going through my e-mails now after the work day, so sorry for not replying sooner. Don't feel you have to apologise for things like this honestly, we are all human and there is a huge amount of churn on this driver at the moment. I know there are subsequent e-mails indicating this was not the problem anyway, but in any case, I am happy to test when I'm able to, and don't worry if it doesn't always work - my code is guilty of the same thing constantly :-)
Regards,
Phil
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
In reply to Fabio M. De FrancescoQuoted 53 lines · Fabio M. De Francesco
Replace
usb_control_msg()with the newusb_control_msg_recv()andusb_control_msg_send()API of USB Core inusbctrl_vendorreq().Suggested-by Greg Kroah-Hartman <[email protected]>Signed-off-by Fabio M. De Francesco <[email protected]>---
Thanks to Pavel Skripkin <[email protected]> for his review of the RFC patch.
drivers/staging/r8188eu/hal/usb_ops_linux.c| 25 ++++++++++-----------1 file changed, 12 insertions(+), 13 deletions(-)drivers/staging/r8188eu/hal/usb_ops_linux.c +11
-8@@ -15,9 +15,8 @@ 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; +u8 pipe;int status =0 ; -u8 reqtype;u8 *pIo_buf;int vendorreq_times =0 ;@@ -44,22 +43,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, pipe,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, pipe,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 ); }
drivers/staging/r8188eu/hal/usb_ops_linux.c static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, · line 64
Quoted 10 lines
drivers/staging/r8188eu/hal/usb_ops_linux.c +1
-5- 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);
Hi, Fabio!
Christophe is right about semantic part. Also,
if (!status) {
} else {
if (status < 0) { <-
|
} else { |
|
} <-
} Extra if-else is not needed, since status can be 0 and < 0, there is no 3rd state, like it was before.
With regards,
Pavel Skripkin
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Quoted 11 lines · CEST Pavel Skripkin
Quoted 10 lines · Fabio M. De Francesco
Replace
usb_control_msg()with the newusb_control_msg_recv()andusb_control_msg_send()API of USB Core inusbctrl_vendorreq().Suggested-by Greg Kroah-Hartman <[email protected]>Signed-off-by Fabio M. De Francesco <[email protected]>---
Thanks to Pavel Skripkin <[email protected]> for his review of the RFC patch.
Quoted 8 lines
Quoted 5 lines
drivers/staging/r8188eu/hal/usb_ops_linux.c| 25 ++++++++++-----------1 file changed, 12 insertions(+), 13 deletions(-)[...]
Hi, Fabio!
Christophe is right about semantic part.
Hi Pavel,
I haven't yet read Christophe's message (but I'm going to do it ASAP). I hope he found out what is wrong with the code, what made Phil's tests fail.
Quoted 12 lines
Also,
if (!status) {
} else { if (status < 0) { <- | } else { | | } <- }
Quoted 2 lines
Extra if-else is not needed, since status can be 0 and < 0, there is no 3rd state, like it was before.
Correct, thanks!
Now I read the following from the documentation of the new API...
"Return: If successful, 0 is returned, Otherwise, a negative error number."
I'll remove that status < 0 check and whatever else is no more necessary. Thanks, again :)
Regards,
Fabio
Quoted 3 lines
With regards,
Pavel Skripkin
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Quoted 43 lines · Fabio M. De Francesco
Quoted 11 lines · CEST Pavel Skripkin
Quoted 10 lines · Fabio M. De Francesco
Replace
usb_control_msg()with the newusb_control_msg_recv()andusb_control_msg_send()API of USB Core inusbctrl_vendorreq().Suggested-by Greg Kroah-Hartman <[email protected]>Signed-off-by Fabio M. De Francesco <[email protected]>---
Thanks to Pavel Skripkin <[email protected]> for his review of the RFC patch.
Quoted 8 lines
Quoted 5 lines
drivers/staging/r8188eu/hal/usb_ops_linux.c| 25 ++++++++++-----------1 file changed, 12 insertions(+), 13 deletions(-)[...]
Hi, Fabio!
Christophe is right about semantic part.
Hi Pavel,
I haven't yet read Christophe's message (but I'm going to do it ASAP). I hope he found out what is wrong with the code, what made Phil's tests fail.
Quoted 12 lines
Also,
if (!status) {
} else { if (status < 0) { <- | } else { | | } <- }Quoted 1 line
Extra if-else is not needed, since status can be 0 and < 0, there is no 3rd state, like it was before.
Correct, thanks!
Quoted 9 lines
Now I read the following from the documentation of the new API...
"Return: If successful, 0 is returned, Otherwise, a negative error number."
I'll remove that status < 0 check and whatever else is no more necessary. Thanks, again :)
Regards,
Btw, not related to your patch, but I start think, that this check:
if (!pIo_buf) {
DBG_88E("[%s] pIo_buf == NULL\n", __func__);
status = -ENOMEM;
goto release_mutex;
}Should be wrapped as
if (WARN_ON(unlikely(!pIo_buf)) {
...
}Since usb_vendor_req_buf is initialized in ->probe() and I can't see possible calltrace, which can cause zeroing this pointer.
Something _completely_ wrong is going on if usb_vendor_req_buf is NULL, and we should complain loud about it. What do you think?
With regards,
Pavel Skripkin
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Quoted 10 lines · CEST Pavel Skripkin
Btw, not related to your patch, but I start think, that this check:
if (!pIo_buf) { DBG_88E("[%s] pIo_buf == NULL\n", __func__); status = -ENOMEM; goto release_mutex; }
Quoted 8 lines
Should be wrapped as
if (WARN_ON(unlikely(!pIo_buf)) { ... }Since
usb_vendor_req_bufis initialized in ->probe() and I can't see possible calltrace, which can cause zeroing this pointer.
I see that usb_vendor_req_buf is initialized in rtw_init_intf_priv(). It depends on a kzalloc() success on allocating memory. Obviously it could fail to allocate. If it fails, rtw_init_intf_priv() returns _FAIL to its caller(s) (whichever they are - I didn't go too deep in understanding the possible calls chains).
What does really matter is that dvobjpriv->usb_vendor_req_buf _could_ _indeed_ _be_ in an _un-initialized_ _status_ when it is assigned to pIo_buf.
Quoted 2 lines
Something _completely_ wrong is going on if
usb_vendor_req_bufis NULL, and we should complain loud about it. What do you think?
That "if (!pIo_buf)" in usbctrl_vendorreq() manages a possible but remote (I guess) un-initialization by releasing a mutex and returning -ENOMEM.
I think that technically speaking it would suffice if callers read and manage properly the -ENOMEM returned by usbctrl_vendorreq().
Said that, I have no sufficient experience to say if exiting and returning -ENOMEM would suffice to not make happen "_something _completely_ wrong_" or if a WARN_ON is required. I'm sorry for not being able to go beyond the confirmation that indeed dvobjpriv->usb_vendor_req could be un-initialized.
Regards,
Fabio
Quoted 3 lines
With regards,
Pavel Skripkin
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Quoted 17 lines · Fabio M. De Francesco
Quoted 16 lines · CEST Pavel Skripkin
Btw, not related to your patch, but I start think, that this check:
if (!pIo_buf) { DBG_88E("[%s] pIo_buf == NULL\n", __func__); status = -ENOMEM; goto release_mutex; }Should be wrapped as
if (WARN_ON(unlikely(!pIo_buf)) { ... }
Quoted 7 lines
Quoted 1 line
Since
usb_vendor_req_bufis initialized in ->probe() and I can't see possible calltrace, which can cause zeroing this pointer.I see that
usb_vendor_req_bufis initialized inrtw_init_intf_priv(). It depends on akzalloc()success on allocating memory. Obviously it could fail to allocate. If it fails,rtw_init_intf_priv()returns_FAILto its caller(s) (whichever they are - I didn't go too deep in understanding the possible calls chains).
Call chain is the most interesting part here :)
rtw_drv_init() <-- probe()
usb_dvobj_init()
rtw_init_intf_priv()If kzalloc fails, then whole ->probe() routine fails, i.e device will be disconnected. There is no read() calls before rtw_init_intf_priv(), so if kzalloc() call was successful, there is no way how usb_vendor_req_buf can be NULL, since read() can happen only in case of successfully connected device.
Anyway, it can be NULL in case of out-of-bound write or smth else, but there is no explicit usb_alloc_vendor_req_buf = NULL in this driver. We should complain about completely wrong driver behavior, IMO :)
Does it make sense?
With regards,
Pavel Skripkin
Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()
Quoted 27 lines · CEST Pavel Skripkin
Quoted 17 lines · Fabio M. De Francesco
Quoted 16 lines · CEST Pavel Skripkin
Btw, not related to your patch, but I start think, that this check:
if (!pIo_buf) { DBG_88E("[%s] pIo_buf == NULL\n", __func__); status = -ENOMEM; goto release_mutex; }Should be wrapped as
if (WARN_ON(unlikely(!pIo_buf)) { ... }Quoted 8 lines
Quoted 2 lines
Since
usb_vendor_req_bufis initialized in ->probe() and I can't see possible calltrace, which can cause zeroing this pointer.I see that
usb_vendor_req_bufis initialized inrtw_init_intf_priv(). It depends on akzalloc()success on allocating memory. Obviously it could fail to allocate. If it fails,rtw_init_intf_priv()returns_FAILto its caller(s) (whichever they are - I didn't go too deep in understanding the possible calls chains).
Quoted 8 lines
Call chain is the most interesting part here :)
rtw_drv_init() <-- probe() usb_dvobj_init() rtw_init_intf_priv()If kzalloc fails, then whole ->probe() routine fails, i.e device will be disconnected.
I guess that if probe fails and then the device get disconnected it's not a big problem, in the sense that nothing of very bad could happen.
Quoted 4 lines
There is no
read()calls beforertw_init_intf_priv(), so
ifkzalloc()call was successful, there is no way howusb_vendor_req_bufcan be NULL, sinceread()can happen only in case of successfully connected device.
Yes, though I have very little knowledge of how drivers work, it make sense to me too that read(s) can happen only in case of successful connection.
Quoted 1 line
Anyway, it can be NULL in case of out-of-bound write or smth else,
This is really something I don't know.
Quoted 5 lines
but
there is no explicitusb_alloc_vendor_req_buf= NULL in this driver. We should complain about completely wrong driver behavior, IMO :)Does it make sense?
I'm not sure, whether or not we now have an answer to your question about the necessity to use WARN_ON... I think it's up to your judgement, because I cannot help on this topic :(
Regards,
Fabio
Quoted 3 lines
With regards,
Pavel Skripkin
PATCH 2/2 staging: r8188eu: Make some clean-ups in usbctrl_vendorreq()
In reply to Fabio M. De FrancescoAfter 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().
--- drivers/staging/r8188eu/hal/usb_ops_linux.c | 26 ++++++++++----------- 1 file changed, 13 insertions(+), 13 deletions(-)
drivers/staging/r8188eu/hal/usb_ops_linux.c +13 -13
@@ -10,14 +10,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 adapter *adapt = intfhdl->padapter;struct dvobj_priv *dvobjpriv =adapter_to_dvobj (adapt);struct usb_device *udev = dvobjpriv->pusbdev;u8 pipe;int status =0 ; -u8 *pIo_buf; +u8 *io_buf;int vendorreq_times =0 ;if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) {
@@ -34,10 +34,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,25 +47,25 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, status =usb_control_msg_recv (udev, pipe,REALTEK_USB_VENQT_CMD_REQ ,REALTEK_USB_VENQT_READ , value,REALTEK_USB_VENQT_CMD_IDX , - pIo_buf, len,RTW_USB_CONTROL_MSG_TIMEOUT , + 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, pipe,REALTEK_USB_VENQT_CMD_REQ ,REALTEK_USB_VENQT_WRITE , value,REALTEK_USB_VENQT_CMD_IDX , - pIo_buf, len,RTW_USB_CONTROL_MSG_TIMEOUT , + io_buf, len,RTW_USB_CONTROL_MSG_TIMEOUT ,GFP_KERNEL ); }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 <0 ) {if (status == (-ESHUTDOWN ) || status == -ENODEV ) {
@@ -77,8 +77,8 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, }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); +/* For Control read transfer, we have to copy the read data from io_buf to data. */ +memcpy (data, io_buf, len); } } }
Signature · 1 line
2.32.0
Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq()
Quoted 91 lines · Fabio M. De Francesco
After replacing
usb_control_msg()with the newusb_control_msg_recv()andusb_control_msg_send()API of USB Core, remove camelcase from thepIo_bufvariable that is passed as argument to the new API and remove the initial 'p' (that probably stands for "pointer") from the samepIo_bufand from the pintfhdl and pdata arguments ofusbctrl_vendorreq().Signed-off-by Fabio M. De Francesco <[email protected]>--- drivers/staging/r8188eu/hal/usb_ops_linux.c | 26 ++++++++++----------- 1 file changed, 13 insertions(+), 13 deletions(-)drivers/staging/r8188eu/hal/usb_ops_linux.c +13
-14@@ -10,14 +10,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 adapter *adapt = intfhdl->padapter;struct dvobj_priv *dvobjpriv =adapter_to_dvobj (adapt);struct usb_device *udev = dvobjpriv->pusbdev;u8 pipe;int status =0 ; -u8 *pIo_buf; +u8 *io_buf;int vendorreq_times =0 ;if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) {@@ -34,10 +34,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,25 +47,25 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, status =usb_control_msg_recv (udev, pipe,REALTEK_USB_VENQT_CMD_REQ ,REALTEK_USB_VENQT_READ , value,REALTEK_USB_VENQT_CMD_IDX , - pIo_buf, len,RTW_USB_CONTROL_MSG_TIMEOUT , + 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, pipe,REALTEK_USB_VENQT_CMD_REQ ,REALTEK_USB_VENQT_WRITE , value,REALTEK_USB_VENQT_CMD_IDX , - pIo_buf, len,RTW_USB_CONTROL_MSG_TIMEOUT , + io_buf, len,RTW_USB_CONTROL_MSG_TIMEOUT ,GFP_KERNEL ); }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 <0 ) {if (status == (-ESHUTDOWN ) || status == -ENODEV ) {@@ -77,8 +77,8 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, }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); +/* For Control read transfer, we have to copy the read data from io_buf to data. */ +memcpy (data, io_buf, len); } } } --
Quoted 2 lines
2.32.0
Dear Fabio,
This one doesn't apply at all for me - it may just be because of the churn in my local tree though, I don't have all previous patches applied as I've been testing select series etc. Many thanks.
Regards,
Phil