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

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

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

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

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

-- 
2.32.0

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

[Fabio M. De Francesco]

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

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

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

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

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

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

[Pavel Skripkin]

On 8/24/21 5:28 PM, Fabio M. De Francesco wrote:
> Replace usb_control_msg() with the new usb_control_msg_recv() and
> usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> Remove no more needed variables. Move out of an if-else block
> some code that it is no more dependent on status < 0. Remove
> redundant code depending on status > 0 or status == len.
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> 
> v1->v2: According to suggestions by Christophe JAILLET
> <christophe.jaillet@wanadoo.fr>, remove 'pipe' and pass an explicit 0
> to the new API. According to suggestions by Pavel Skripkin
> <paskripkin@gmail.com>, remove an extra if-else that is no more needed,
> since status can be 0 and < 0 and there is no 3rd state, like it was before.
> Many thanks to both them and to Phillip Potter <phil@philpotter.co.uk>
> who kindly offered his time for the purpose of testing v1.
>   
> 
>   drivers/staging/r8188eu/hal/usb_ops_linux.c | 45 ++++++++-------------
>   1 file changed, 17 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index a93d5cfe4635..13e925d21e00 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -15,9 +15,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>   	struct adapter	*adapt = pintfhdl->padapter;
>   	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
>   	struct usb_device *udev = dvobjpriv->pusbdev;
> -	unsigned int pipe;
>   	int status = 0;
> -	u8 reqtype;
>   	u8 *pIo_buf;
>   	int vendorreq_times = 0;
>   
> @@ -44,22 +42,22 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>   	}
>   
>   	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> -		memset(pIo_buf, 0, len);
> -
>   		if (requesttype == 0x01) {
> -			pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
> -			reqtype =  REALTEK_USB_VENQT_READ;
> +			status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> +						      REALTEK_USB_VENQT_READ, value,
> +						      REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
> +						      len, RTW_USB_CONTROL_MSG_TIMEOUT,
> +						      GFP_KERNEL);
>   		} else {
> -			pipe = usb_sndctrlpipe(udev, 0);/* write_out */
> -			reqtype =  REALTEK_USB_VENQT_WRITE;
>   			memcpy(pIo_buf, pdata, len);
> +			status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> +						      REALTEK_USB_VENQT_WRITE, value,
> +						      REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
> +						      len, RTW_USB_CONTROL_MSG_TIMEOUT,
> +						      GFP_KERNEL);
>   		}
>   
> -		status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
> -					 reqtype, value, REALTEK_USB_VENQT_CMD_IDX,
> -					 pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT);
> -
> -		if (status == len) {   /*  Success this control transfer. */
> +		if (!status) {   /*  Success this control transfer. */
>   			rtw_reset_continual_urb_error(dvobjpriv);
>   			if (requesttype == 0x01)
>   				memcpy(pdata, pIo_buf,  len);
> @@ -68,20 +66,11 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>   				value, (requesttype == 0x01) ? "read" : "write",
>   				len, status, *(u32 *)pdata, vendorreq_times);
>   
> -			if (status < 0) {
> -				if (status == (-ESHUTDOWN) || status == -ENODEV) {
> -					adapt->bSurpriseRemoved = true;
> -				} else {
> -					struct hal_data_8188e	*haldata = GET_HAL_DATA(adapt);
> -					haldata->srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL;
> -				}
> -			} else { /*  status != len && status >= 0 */
> -				if (status > 0) {
> -					if (requesttype == 0x01) {
> -						/*  For Control read transfer, we have to copy the read data from pIo_buf to pdata. */
> -						memcpy(pdata, pIo_buf,  len);
> -					}
> -				}
> +			if (status == (-ESHUTDOWN) || status == -ENODEV) {
> +				adapt->bSurpriseRemoved = true;
> +			} else {
> +				struct hal_data_8188e	*haldata = GET_HAL_DATA(adapt);
> +				haldata->srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL;
>   			}
>   
>   			if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
> @@ -92,7 +81,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>   		}
>   
>   		/*  firmware download is checksumed, don't retry */
> -		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || status == len)
> +		if (value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS)
>   			break;


Shouldn't we break the loop when we have received all data? I think,
status == len should be replaced with !status. Correct me if I am wrong.


Other changes look good to me, thanks



With regards,
Pavel Skripkin

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

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 v2 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Fabio M. De Francesco]

On Tuesday, August 24, 2021 4:35:38 PM CEST Pavel Skripkin wrote:
> On 8/24/21 5:28 PM, Fabio M. De Francesco wrote:
> > Replace usb_control_msg() with the new usb_control_msg_recv() and
> > usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> > Remove no more needed variables. Move out of an if-else block
> > some code that it is no more dependent on status < 0. Remove
> > redundant code depending on status > 0 or status == len.
> > 
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> > v1->v2: According to suggestions by Christophe JAILLET
> > <christophe.jaillet@wanadoo.fr>, remove 'pipe' and pass an explicit 0
> > to the new API. According to suggestions by Pavel Skripkin
> > <paskripkin@gmail.com>, remove an extra if-else that is no more needed,
> > since status can be 0 and < 0 and there is no 3rd state, like it was before.
> > Many thanks to both them and to Phillip Potter <phil@philpotter.co.uk>
> > who kindly offered his time for the purpose of testing v1.
> >   
> > 
> >   drivers/staging/r8188eu/hal/usb_ops_linux.c | 45 ++++++++-------------
> >   1 file changed, 17 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > index a93d5cfe4635..13e925d21e00 100644
> > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > @@ -15,9 +15,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> >   	struct adapter	*adapt = pintfhdl->padapter;
> >   	struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
> >   	struct usb_device *udev = dvobjpriv->pusbdev;
> > -	unsigned int pipe;
> >   	int status = 0;
> > -	u8 reqtype;
> >   	u8 *pIo_buf;
> >   	int vendorreq_times = 0;
> >   
> > @@ -44,22 +42,22 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> >   	}
> >   
> >   	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> > -		memset(pIo_buf, 0, len);
> > -
> >   		if (requesttype == 0x01) {
> > -			pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
> > -			reqtype =  REALTEK_USB_VENQT_READ;
> > +			status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> > +						      REALTEK_USB_VENQT_READ, value,
> > +						      REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
> > +						      len, RTW_USB_CONTROL_MSG_TIMEOUT,
> > +						      GFP_KERNEL);
> >   		} else {
> > -			pipe = usb_sndctrlpipe(udev, 0);/* write_out */
> > -			reqtype =  REALTEK_USB_VENQT_WRITE;
> >   			memcpy(pIo_buf, pdata, len);
> > +			status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> > +						      REALTEK_USB_VENQT_WRITE, value,
> > +						      REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
> > +						      len, RTW_USB_CONTROL_MSG_TIMEOUT,
> > +						      GFP_KERNEL);
> >   		}
> >   
> > -		status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
> > -					 reqtype, value, REALTEK_USB_VENQT_CMD_IDX,
> > -					 pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT);
> > -
> > -		if (status == len) {   /*  Success this control transfer. */
> > +		if (!status) {   /*  Success this control transfer. */
> >   			rtw_reset_continual_urb_error(dvobjpriv);
> >   			if (requesttype == 0x01)
> >   				memcpy(pdata, pIo_buf,  len);
> > @@ -68,20 +66,11 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> >   				value, (requesttype == 0x01) ? "read" : "write",
> >   				len, status, *(u32 *)pdata, vendorreq_times);
> >   
> > -			if (status < 0) {
> > -				if (status == (-ESHUTDOWN) || status == -ENODEV) {
> > -					adapt->bSurpriseRemoved = true;
> > -				} else {
> > -					struct hal_data_8188e	*haldata = GET_HAL_DATA(adapt);
> > -					haldata->srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL;
> > -				}
> > -			} else { /*  status != len && status >= 0 */
> > -				if (status > 0) {
> > -					if (requesttype == 0x01) {
> > -						/*  For Control read transfer, we have to copy the read data from pIo_buf to pdata. */
> > -						memcpy(pdata, pIo_buf,  len);
> > -					}
> > -				}
> > +			if (status == (-ESHUTDOWN) || status == -ENODEV) {
> > +				adapt->bSurpriseRemoved = true;
> > +			} else {
> > +				struct hal_data_8188e	*haldata = GET_HAL_DATA(adapt);
> > +				haldata->srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL;
> >   			}
> >   
> >   			if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
> > @@ -92,7 +81,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> >   		}
> >   
> >   		/*  firmware download is checksumed, don't retry */
> > -		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || status == len)
> > +		if (value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS)
> >   			break;
> 
> 
> Shouldn't we break the loop when we have received all data? I think,
> status == len should be replaced with !status. Correct me if I am wrong.

Yes, correct. I was about to replace status == len with !status when I've 
been interrupted by a phone call and then I forgot to do the work :(

I'll send a v3 this night. Unfortunately now I have to go. While at that
I'll also try to understand why 2/2 cannot apply. Sigh!
 
> Other changes look good to me, thanks

Thanks to you, Christophe and Philip for all the help you gave.

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

[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 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 v2 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

[Phillip Potter]

On Tue, 24 Aug 2021 at 16:24, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> On Tuesday, August 24, 2021 4:35:38 PM CEST Pavel Skripkin wrote:
> > On 8/24/21 5:28 PM, Fabio M. De Francesco wrote:
> > > Replace usb_control_msg() with the new usb_control_msg_recv() and
> > > usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> > > Remove no more needed variables. Move out of an if-else block
> > > some code that it is no more dependent on status < 0. Remove
> > > redundant code depending on status > 0 or status == len.
> > >
> > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > >
> > > v1->v2: According to suggestions by Christophe JAILLET
> > > <christophe.jaillet@wanadoo.fr>, remove 'pipe' and pass an explicit 0
> > > to the new API. According to suggestions by Pavel Skripkin
> > > <paskripkin@gmail.com>, remove an extra if-else that is no more needed,
> > > since status can be 0 and < 0 and there is no 3rd state, like it was before.
> > > Many thanks to both them and to Phillip Potter <phil@philpotter.co.uk>
> > > who kindly offered his time for the purpose of testing v1.
> > >
> > >
> > >   drivers/staging/r8188eu/hal/usb_ops_linux.c | 45 ++++++++-------------
> > >   1 file changed, 17 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > > index a93d5cfe4635..13e925d21e00 100644
> > > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > > @@ -15,9 +15,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> > >     struct adapter  *adapt = pintfhdl->padapter;
> > >     struct dvobj_priv  *dvobjpriv = adapter_to_dvobj(adapt);
> > >     struct usb_device *udev = dvobjpriv->pusbdev;
> > > -   unsigned int pipe;
> > >     int status = 0;
> > > -   u8 reqtype;
> > >     u8 *pIo_buf;
> > >     int vendorreq_times = 0;
> > >
> > > @@ -44,22 +42,22 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> > >     }
> > >
> > >     while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> > > -           memset(pIo_buf, 0, len);
> > > -
> > >             if (requesttype == 0x01) {
> > > -                   pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
> > > -                   reqtype =  REALTEK_USB_VENQT_READ;
> > > +                   status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> > > +                                                 REALTEK_USB_VENQT_READ, value,
> > > +                                                 REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
> > > +                                                 len, RTW_USB_CONTROL_MSG_TIMEOUT,
> > > +                                                 GFP_KERNEL);
> > >             } else {
> > > -                   pipe = usb_sndctrlpipe(udev, 0);/* write_out */
> > > -                   reqtype =  REALTEK_USB_VENQT_WRITE;
> > >                     memcpy(pIo_buf, pdata, len);
> > > +                   status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> > > +                                                 REALTEK_USB_VENQT_WRITE, value,
> > > +                                                 REALTEK_USB_VENQT_CMD_IDX, pIo_buf,
> > > +                                                 len, RTW_USB_CONTROL_MSG_TIMEOUT,
> > > +                                                 GFP_KERNEL);
> > >             }
> > >
> > > -           status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
> > > -                                    reqtype, value, REALTEK_USB_VENQT_CMD_IDX,
> > > -                                    pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT);
> > > -
> > > -           if (status == len) {   /*  Success this control transfer. */
> > > +           if (!status) {   /*  Success this control transfer. */
> > >                     rtw_reset_continual_urb_error(dvobjpriv);
> > >                     if (requesttype == 0x01)
> > >                             memcpy(pdata, pIo_buf,  len);
> > > @@ -68,20 +66,11 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> > >                             value, (requesttype == 0x01) ? "read" : "write",
> > >                             len, status, *(u32 *)pdata, vendorreq_times);
> > >
> > > -                   if (status < 0) {
> > > -                           if (status == (-ESHUTDOWN) || status == -ENODEV) {
> > > -                                   adapt->bSurpriseRemoved = true;
> > > -                           } else {
> > > -                                   struct hal_data_8188e   *haldata = GET_HAL_DATA(adapt);
> > > -                                   haldata->srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL;
> > > -                           }
> > > -                   } else { /*  status != len && status >= 0 */
> > > -                           if (status > 0) {
> > > -                                   if (requesttype == 0x01) {
> > > -                                           /*  For Control read transfer, we have to copy the read data from pIo_buf to pdata. */
> > > -                                           memcpy(pdata, pIo_buf,  len);
> > > -                                   }
> > > -                           }
> > > +                   if (status == (-ESHUTDOWN) || status == -ENODEV) {
> > > +                           adapt->bSurpriseRemoved = true;
> > > +                   } else {
> > > +                           struct hal_data_8188e   *haldata = GET_HAL_DATA(adapt);
> > > +                           haldata->srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL;
> > >                     }
> > >
> > >                     if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
> > > @@ -92,7 +81,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> > >             }
> > >
> > >             /*  firmware download is checksumed, don't retry */
> > > -           if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || status == len)
> > > +           if (value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS)
> > >                     break;
> >
> >
> > Shouldn't we break the loop when we have received all data? I think,
> > status == len should be replaced with !status. Correct me if I am wrong.
>
> Yes, correct. I was about to replace status == len with !status when I've
> been interrupted by a phone call and then I forgot to do the work :(
>
> I'll send a v3 this night. Unfortunately now I have to go. While at that
> I'll also try to understand why 2/2 cannot apply. Sigh!
>
> > Other changes look good to me, thanks
>
> Thanks to you, Christophe and Philip for all the help you gave.
>
> Regards,
>
> Fabio
>
> > With regards,
> > Pavel Skripkin
> >
>
>
>
>

Happy to help - might be worth waiting for staging-testing to have
more patches merged in and rebasing after that.

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