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

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

-- 
2.32.0

[PATCH 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().

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

Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the
RFC patch.
 
drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++-----------
1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index a93d5cfe4635..6f51660b967a 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -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);
-- 
2.32.0

[PATCH 2/2] staging: r8188eu: Make some 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>
---
 drivers/staging/r8188eu/hal/usb_ops_linux.c | 26 ++++++++++-----------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 6f51660b967a..5ce31ae18ed8 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -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);
 					}
 				}
 			}
-- 
2.32.0

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

[Phillip Potter]

On Mon, 23 Aug 2021 at 23:38, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> Replace usb_control_msg() with the new usb_control_msg_recv() and
> usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>
> Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the
> RFC patch.
>
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++-----------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index a93d5cfe4635..6f51660b967a 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -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);
> --
> 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 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq()

[Phillip Potter]

On Mon, 23 Aug 2021 at 23:38, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> 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>
> ---
>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 26 ++++++++++-----------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index 6f51660b967a..5ce31ae18ed8 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -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);
>                                         }
>                                 }
>                         }
> --
> 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

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

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 2:08:49 AM CEST Phillip Potter wrote:
> On Mon, 23 Aug 2021 at 23:38, Fabio M. De Francesco
> <fmdefrancesco@gmail.com> wrote:
> >
> > Replace usb_control_msg() with the new usb_control_msg_recv() and
> > usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> >
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >
> > Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the
> > RFC patch.
> >
> > drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++-----------
> > 1 file changed, 12 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > index a93d5cfe4635..6f51660b967a 100644
> > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > @@ -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);
> > --
> > 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
> 
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()

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 2:31:11 AM CEST Fabio M. De Francesco wrote:
> On Tuesday, August 24, 2021 2:08:49 AM CEST Phillip Potter wrote:
> > On Mon, 23 Aug 2021 at 23:38, Fabio M. De Francesco
> > <fmdefrancesco@gmail.com> wrote:
> > >
> > > Replace usb_control_msg() with the new usb_control_msg_recv() and
> > > usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> > >
> > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > >
> > > Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the
> > > RFC patch.
> > >
> > > drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++-----------
> > > 1 file changed, 12 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > > index a93d5cfe4635..6f51660b967a 100644
> > > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > > @@ -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);
> > > --
> > > 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
> > 
> 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
> 
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()

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 3:38:03 AM CEST Fabio M. De Francesco wrote:
> 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.

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

[Christophe JAILLET]

Le 24/08/2021 à 04:01, Fabio M. De Francesco a écrit :
> On Tuesday, August 24, 2021 3:38:03 AM CEST Fabio M. De Francesco wrote:
>> 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.
> 
> 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()

[Pavel Skripkin]

On 8/24/21 1:37 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().
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> 
> Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the
> RFC patch.
>   
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++-----------
> 1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index a93d5cfe4635..6f51660b967a 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -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);
> 

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

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 10:13:46 AM CEST Pavel Skripkin wrote:
> On 8/24/21 1:37 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().
> > 
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> > Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the
> > RFC patch.
> >   
> > 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.

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

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

> With regards,
> Pavel Skripkin
> 

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

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 7:44:40 AM CEST Christophe JAILLET wrote:
> Le 24/08/2021 à 04:01, Fabio M. De Francesco a écrit :
> > On Tuesday, August 24, 2021 3:38:03 AM CEST Fabio M. De Francesco wrote:
> >> 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.
> > 
> > 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'.

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? 

> Also, in your code, 'pipe' looks un-initialized.

Oh yes, good catch.  Thanks!

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

> 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

> just my 2c,
> CJ
> 

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

[Pavel Skripkin]

On 8/24/21 11:53 AM, Fabio M. De Francesco wrote:
> On Tuesday, August 24, 2021 10:13:46 AM CEST Pavel Skripkin wrote:
>> On 8/24/21 1:37 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().
>> > 
>> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> > ---
>> > 
>> > Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the
>> > RFC patch.
>> >   
>> > 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.
> 
>> 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.
> 
> 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,
> 

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

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 1:07:46 PM CEST Pavel Skripkin wrote:
> 
> 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.

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.  

> Something _completely_ wrong is going on if usb_vendor_req_buf is 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

> With regards,
> Pavel Skripkin
> 

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

[Pavel Skripkin]

On 8/24/21 3:01 PM, Fabio M. De Francesco wrote:
> On Tuesday, August 24, 2021 1:07:46 PM CEST Pavel Skripkin wrote:
>> 
>> 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.
> 
> 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).
> 

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

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 2:09:10 PM CEST Pavel Skripkin wrote:
> On 8/24/21 3:01 PM, Fabio M. De Francesco wrote:
> > On Tuesday, August 24, 2021 1:07:46 PM CEST Pavel Skripkin wrote:
> >> 
> >> 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.
> > 
> > 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).
> > 
> 
> 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.

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

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.

> Anyway, it can be NULL in case of out-of-bound write or smth else,

This is really something I don't know.

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

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

> With regards,
> Pavel Skripkin
> 

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

[Christophe JAILLET]

Le 24/08/2021 à 12:38, Fabio M. De Francesco a écrit :
> 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 ?

> 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

> 
> Thanks very much,
> 
> Fabio
> 
>> just my 2c,
>> CJ
>>
> 
> 
> 
> 
> 

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

[Phillip Potter]

On Tue, 24 Aug 2021 at 02:38, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> 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
>
>
>

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