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

On Tue, 24 Aug 2021 at 16:24, Fabio M. De Francesco
[off-list ref] wrote:

On Tuesday, August 24, 2021 4:35:38 PM CEST Pavel Skripkin wrote:

quoted

On 8/24/21 5:28 PM, Fabio M. De Francesco wrote:

quoted

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

v1->v2: According to suggestions by Christophe JAILLET
[off-list ref], remove 'pipe' and pass an explicit 0
to the new API. According to suggestions by Pavel Skripkin
[off-list ref], 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 [off-list ref]
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!

quoted

Other changes look good to me, thanks

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

Regards,

Fabio

quoted

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