[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

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

[Phillip Potter]

On Tue, 24 Aug 2021 at 17:18, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Aug 24, 2021 at 07:04:31PM +0300, Pavel Skripkin wrote:
> > On 8/24/21 6:59 PM, Fabio M. De Francesco wrote:
> > > On Tuesday, August 24, 2021 5:43:26 PM CEST Pavel Skripkin wrote:
> > > > On 8/24/21 6:39 PM, Fabio M. De Francesco wrote:
> > > > > Oh, I know where it comes from... :)
> > > > > > It's a patch of mine that is in the queue, waiting to be
> > > > reviewed and applied.
> > > > > Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/
> > > > > oh.... there are _a lot_ of pending changes :)
> > > >
> > > > I guess, we need smth like public-mirror with already reviewed and
> > > > working changes
> > >
> > > It's becoming a serious problem. A lot of times I see people who is asked to
> > > rebase and resend, not because they forget to fetch the current tree, instead
> > > because the tree changes as soon as Greg start to apply the first patches in the
> > > queue and the other patches at the end of the queue cannot be applied.
> > >
> > > Anyway,I understand that Greg cannot apply a patch at a time soon after
> > > submission but in the while the queue grows larger and larger.
> > >
> >
> >
> > It can be easily fixed. We need public fork somewhere (github,
> > git.kernel.org ...) and we should ask Greg to add remote-branch to his tree.
>
> No, not going to happen, sorry.  I will catch up with patches when I get
> the chance and then all will be fine.  This is highly unusual that there
> are loads of people all working on the same staging driver.  No idea why
> everyone jumped on this single one...
>
> relax, there is no rush here...
>
> greg k-h

Yeah I'm with Greg on this one - we don't need github forks etc, my
strategy has thus far been to just wait for staging-testing to
coalesce into a more up-to-date state and then work on top of that as
needed. Extra forks just introduce more complexity and more to watch +
keep track of in my opinion, as the e-mails still keep flowing in
anyway :-)

Regards,
Phil

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

[Pavel Skripkin]

On 8/24/21 7:18 PM, Greg Kroah-Hartman wrote:
>> It can be easily fixed. We need public fork somewhere (github,
>> git.kernel.org ...) and we should ask Greg to add remote-branch to his tree.
> 
> No, not going to happen, sorry.  I will catch up with patches when I get
> the chance and then all will be fine.  This is highly unusual that there

Ok, sorry for this idea, I am not familiar with maintaining process :(

> are loads of people all working on the same staging driver.  No idea why
> everyone jumped on this single one...
> 

My guess is
	1. It's new
	2. There is a lot work to do

> relax, there is no rush here...
> 

We don't try to rush, just discussing. Sorry to bother you :)



With regards,
Pavel Skripkin

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

[Greg Kroah-Hartman]

On Tue, Aug 24, 2021 at 07:04:31PM +0300, Pavel Skripkin wrote:
> On 8/24/21 6:59 PM, Fabio M. De Francesco wrote:
> > On Tuesday, August 24, 2021 5:43:26 PM CEST Pavel Skripkin wrote:
> > > On 8/24/21 6:39 PM, Fabio M. De Francesco wrote:
> > > > Oh, I know where it comes from... :)
> > > > > It's a patch of mine that is in the queue, waiting to be
> > > reviewed and applied.
> > > > Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/
> > > > oh.... there are _a lot_ of pending changes :)
> > > 
> > > I guess, we need smth like public-mirror with already reviewed and
> > > working changes
> > 
> > It's becoming a serious problem. A lot of times I see people who is asked to
> > rebase and resend, not because they forget to fetch the current tree, instead
> > because the tree changes as soon as Greg start to apply the first patches in the
> > queue and the other patches at the end of the queue cannot be applied.
> > 
> > Anyway,I understand that Greg cannot apply a patch at a time soon after
> > submission but in the while the queue grows larger and larger.
> > 
> 
> 
> It can be easily fixed. We need public fork somewhere (github,
> git.kernel.org ...) and we should ask Greg to add remote-branch to his tree.

No, not going to happen, sorry.  I will catch up with patches when I get
the chance and then all will be fine.  This is highly unusual that there
are loads of people all working on the same staging driver.  No idea why
everyone jumped on this single one...

relax, there is no rush here...

greg k-h

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

[Pavel Skripkin]

On 8/24/21 6:59 PM, Fabio M. De Francesco wrote:
> On Tuesday, August 24, 2021 5:43:26 PM CEST Pavel Skripkin wrote:
>> On 8/24/21 6:39 PM, Fabio M. De Francesco wrote:
>> > Oh, I know where it comes from... :)
>> > 
>> > It's a patch of mine that is in the queue, waiting to be reviewed and applied.
>> > Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/
>> > 
>> oh.... there are _a lot_ of pending changes :)
>> 
>> I guess, we need smth like public-mirror with already reviewed and 
>> working changes
> 
> It's becoming a serious problem. A lot of times I see people who is asked to
> rebase and resend, not because they forget to fetch the current tree, instead
> because the tree changes as soon as Greg start to apply the first patches in the
> queue and the other patches at the end of the queue cannot be applied.
> 
> Anyway,I understand that Greg cannot apply a patch at a time soon after
> submission but in the while the queue grows larger and larger.
> 


It can be easily fixed. We need public fork somewhere (github, 
git.kernel.org ...) and we should ask Greg to add remote-branch to his 
tree.

Then one of the maintainers/reviewers should accept patches to this fork 
+ send pull requests every week (I guess).


I can help with picking up and testing after I receive my device and set 
up qemu environment for testing :)




With regards,
Pavel Skripkin

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

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 5:43:26 PM CEST Pavel Skripkin wrote:
> On 8/24/21 6:39 PM, Fabio M. De Francesco wrote:
> > Oh, I know where it comes from... :)
> > 
> > It's a patch of mine that is in the queue, waiting to be reviewed and applied.
> > Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/
> > 
> oh.... there are _a lot_ of pending changes :)
> 
> I guess, we need smth like public-mirror with already reviewed and 
> working changes

It's becoming a serious problem. A lot of times I see people who is asked to
rebase and resend, not because they forget to fetch the current tree, instead 
because the tree changes as soon as Greg start to apply the first patches in the
queue and the other patches at the end of the queue cannot be applied.

Anyway,I understand that Greg cannot apply a patch at a time soon after 
submission but in the while the queue grows larger and larger.

Regards,

Fabio

> With regards,
> Pavel Skripkin
> 

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

[Pavel Skripkin]

On 8/24/21 6:39 PM, Fabio M. De Francesco wrote:
> On Tuesday, August 24, 2021 5:26:23 PM CEST Pavel Skripkin wrote:
>> I found the problem:
>> 
>> >  	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;
>> 
>> 
>> I don't know from where mutex_lock() comes from. In staging-next I have
>> 
>> _enter_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL);
>> 
>> instead of
>> 
>> mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
>> 
> Oh, I know where it comes from... :)
> 
> It's a patch of mine that is in the queue, waiting to be reviewed and applied.
> Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/
> 


oh.... there are _a lot_ of pending changes :)

I guess, we need smth like public-mirror with already reviewed and 
working changes




With regards,
Pavel Skripkin

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

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 5:26:23 PM CEST Pavel Skripkin wrote:
> I found the problem:
> 
> >  	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;
> 
> 
> I don't know from where mutex_lock() comes from. In staging-next I have
> 
> _enter_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL);
> 
> instead of
> 
> mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
> 
Oh, I know where it comes from... :)

It's a patch of mine that is in the queue, waiting to be reviewed and applied.
Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/

Thanks,

Fabio
> 
> With regards,
> Pavel Skripkin
> 

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

[Pavel Skripkin]

On 8/24/21 6:15 PM, Fabio M. De Francesco wrote:
> On Tuesday, August 24, 2021 4:39:51 PM CEST Pavel Skripkin wrote:
>> On 8/24/21 5:28 PM, Fabio M. De Francesco 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 | 22 ++++++++++-----------
>> >   1 file changed, 11 insertions(+), 11 deletions(-)
>> > 
>> 
>> I cannot apply this one on top of the first one:
>> 
>> error: patch failed: drivers/staging/r8188eu/hal/usb_ops_linux.c:33
>> error: drivers/staging/r8188eu/hal/usb_ops_linux.c: patch does not apply
>> 
>> With regards,
>> Pavel Skripkin
> 

I found the problem:

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


I don't know from where mutex_lock() comes from. In staging-next I have

_enter_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL);

instead of

mutex_lock(&dvobjpriv->usb_vendor_req_mutex);




With regards,
Pavel Skripkin

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

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 4:39:51 PM CEST Pavel Skripkin wrote:
> On 8/24/21 5:28 PM, Fabio M. De Francesco 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 | 22 ++++++++++-----------
> >   1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> 
> I cannot apply this one on top of the first one:
> 
> error: patch failed: drivers/staging/r8188eu/hal/usb_ops_linux.c:33
> error: drivers/staging/r8188eu/hal/usb_ops_linux.c: patch does not apply
> 
> With regards,
> Pavel Skripkin

This is the same problem that yesterday Philip had. I cannot understand why it can
happen, because I've worked on this soon after 1/2 and in the while Greg didn't 
apply nothing. I've only worked on one function both in 1/2 and in 2/2 and I would expect
that either both of them apply or none of them. What am I missing?

Thanks,

Fabio
 

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

[Pavel Skripkin]

On 8/24/21 5:28 PM, Fabio M. De Francesco 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 | 22 ++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
> 

I cannot apply this one on top of the first one:

error: patch failed: drivers/staging/r8188eu/hal/usb_ops_linux.c:33
error: drivers/staging/r8188eu/hal/usb_ops_linux.c: patch does not apply




With regards,
Pavel Skripkin

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

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 13e925d21e00..a038aaa5a624 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -10,13 +10,13 @@
 #include "../include/recv_osdep.h"
 #include "../include/rtl8188e_hal.h"
 
-static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, u16 len, u8 requesttype)
+static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u16 len, u8 requesttype)
 {
-	struct adapter	*adapt = pintfhdl->padapter;
+	struct adapter	*adapt = intfhdl->padapter;
 	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
 	struct usb_device *udev = dvobjpriv->pusbdev;
 	int status = 0;
-	u8 *pIo_buf;
+	u8 *io_buf;
 	int vendorreq_times = 0;
 
 	if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) {
@@ -33,10 +33,10 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
 
 	/*  Acquire IO memory for vendorreq */
-	pIo_buf = dvobjpriv->usb_vendor_req_buf;
+	io_buf = dvobjpriv->usb_vendor_req_buf;
 
-	if (!pIo_buf) {
-		DBG_88E("[%s] pIo_buf == NULL\n", __func__);
+	if (!io_buf) {
+		DBG_88E("[%s] io_buf == NULL\n", __func__);
 		status = -ENOMEM;
 		goto release_mutex;
 	}
@@ -45,14 +45,14 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 		if (requesttype == 0x01) {
 			status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
 						      REALTEK_USB_VENQT_READ, value,
-						      REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
+						      REALTEK_USB_VENQT_CMD_IDX, io_buf,
 						      len, RTW_USB_CONTROL_MSG_TIMEOUT,
 						      GFP_KERNEL);
 		} else {
-			memcpy(pIo_buf, pdata, len);
+			memcpy(io_buf, data, len);
 			status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
 						      REALTEK_USB_VENQT_WRITE, value,
-						      REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
+						      REALTEK_USB_VENQT_CMD_IDX, io_buf,
 						      len, RTW_USB_CONTROL_MSG_TIMEOUT,
 						      GFP_KERNEL);
 		}
@@ -60,11 +60,11 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 		if (!status) {   /*  Success this control transfer. */
 			rtw_reset_continual_urb_error(dvobjpriv);
 			if (requesttype == 0x01)
-				memcpy(pdata, pIo_buf,  len);
+				memcpy(data, io_buf,  len);
 		} else { /*  error cases */
 			DBG_88E("reg 0x%x, usb %s %u fail, status:%d value=0x%x, vendorreq_times:%d\n",
 				value, (requesttype == 0x01) ? "read" : "write",
-				len, status, *(u32 *)pdata, vendorreq_times);
+				len, status, *(u32 *)data, vendorreq_times);
 
 			if (status == (-ESHUTDOWN) || status == -ENODEV) {
 				adapt->bSurpriseRemoved = true;
-- 
2.32.0