LinuxLists.cc - [PATCH 01/10] staging: r8188eu: remove unnecessary cast

[permalink] [raw]

Subject: [PATCH 05/10] staging: r8188eu: remove an unused enum

The VENDOR_READ and VENDOR_WRITE defines are not used.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/include/usb_ops.h | 4 ----
1 file changed, 4 deletions(-)

diff --git a/drivers/staging/r8188eu/include/usb_ops.h b/drivers/staging/r8188eu/include/usb_ops.h
index b6a1cd536adf..c53cc54b6b87 100644
--- a/drivers/staging/r8188eu/include/usb_ops.h
+++ b/drivers/staging/r8188eu/include/usb_ops.h
@@ -13,10 +13,6 @@
#define REALTEK_USB_VENQT_CMD_REQ 0x05
#define REALTEK_USB_VENQT_CMD_IDX 0x00

-enum {
- VENDOR_WRITE = 0x00,
- VENDOR_READ = 0x01,
-};
#define ALIGNMENT_UNIT 16
#define MAX_VENDOR_REQ_CMD_SIZE 254 /* 8188cu SIE Support */
#define MAX_USB_IO_CTL_SIZE (MAX_VENDOR_REQ_CMD_SIZE + ALIGNMENT_UNIT)
--
2.20.1

2021-08-21 16:51:54

[permalink] [raw]

Subject: [PATCH 03/10] staging: rtl8188eu: use actual request type as parameter

At the moment, usbctrl_vendorreq's requesttype parameter must be set to
1 for reading and 0 for writing. It's then converted to the actual
bmRequestType for the USB control request. We can simplify the code and
avoid this conversion if the caller passes the actual bmRequestType.

This patch is an adaptation of commit 788fde031027 ("staging: rtl8188eu:
use actual request type as parameter") for the new r8188eu driver.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/hal/usb_ops_linux.c | 46 ++++++---------------
1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index a11a0597e515..dccb9fd34777 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -15,7 +15,6 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
struct usb_device *udev = dvobjpriv->pusbdev;
unsigned int pipe;
int status = 0;
- u8 reqtype;
u8 *pIo_buf;
int vendorreq_times = 0;

@@ -44,26 +43,24 @@ 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) {
+ if (requesttype == REALTEK_USB_VENQT_READ) {
pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
- reqtype = REALTEK_USB_VENQT_READ;
} else {
pipe = usb_sndctrlpipe(udev, 0);/* write_out */
- reqtype = REALTEK_USB_VENQT_WRITE;
memcpy(pIo_buf, pdata, len);
}

status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
- reqtype, value, REALTEK_USB_VENQT_CMD_IDX,
+ requesttype, value, REALTEK_USB_VENQT_CMD_IDX,
pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT);

if (status == len) { /* Success this control transfer. */
rtw_reset_continual_urb_error(dvobjpriv);
- if (requesttype == 0x01)
+ if (requesttype == REALTEK_USB_VENQT_READ)
memcpy(pdata, pIo_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",
+ value, (requesttype == REALTEK_USB_VENQT_READ) ? "read" : "write",
len, status, *(u32 *)pdata, vendorreq_times);

if (status < 0) {
@@ -75,7 +72,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
}
} else { /* status != len && status >= 0 */
if (status > 0) {
- if (requesttype == 0x01) {
+ if (requesttype == REALTEK_USB_VENQT_READ) {
/* For Control read transfer, we have to copy the read data from pIo_buf to pdata. */
memcpy(pdata, pIo_buf, len);
}
@@ -101,19 +98,16 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,

static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)
{
- u8 requesttype;
u16 wvalue;
u16 len;
u8 data = 0;

- requesttype = 0x01;/* read_in */
-
wvalue = (u16)(addr & 0x0000ffff);
len = 1;

- usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
+ usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);

@@ -123,57 +117,49 @@ static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)

static u16 usb_read16(struct intf_hdl *pintfhdl, u32 addr)
{
- u8 requesttype;
u16 wvalue;
u16 len;
__le32 data;

- requesttype = 0x01;/* read_in */
wvalue = (u16)(addr & 0x0000ffff);
len = 2;
- usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
+ usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);

return (u16)(le32_to_cpu(data) & 0xffff);
}

static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
{
- u8 requesttype;
u16 wvalue;
u16 len;
__le32 data;

- requesttype = 0x01;/* read_in */
-
wvalue = (u16)(addr & 0x0000ffff);
len = 4;

- usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
+ usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);

return le32_to_cpu(data);
}

static int usb_write8(struct intf_hdl *pintfhdl, u32 addr, u8 val)
{
- u8 requesttype;
u16 wvalue;
u16 len;
u8 data;
int ret;

- requesttype = 0x00;/* write_out */
wvalue = (u16)(addr & 0x0000ffff);
len = 1;
data = val;
- ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
+ ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);

return ret;
}

static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
{
- u8 requesttype;
u16 wvalue;
u16 len;
__le32 data;
@@ -181,14 +167,12 @@ static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)

- requesttype = 0x00;/* write_out */
-
wvalue = (u16)(addr & 0x0000ffff);
len = 2;

data = cpu_to_le32(val & 0x0000ffff);

- ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
+ ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);

@@ -197,7 +181,6 @@ static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)

static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
{
- u8 requesttype;
u16 wvalue;
u16 len;
__le32 data;
@@ -205,13 +188,11 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)

- requesttype = 0x00;/* write_out */
-
wvalue = (u16)(addr & 0x0000ffff);
len = 4;
data = cpu_to_le32(val);

- ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
+ ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);

@@ -220,7 +201,6 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)

static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
{
- u8 requesttype;
u16 wvalue;
u16 len;
u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
@@ -228,13 +208,11 @@ static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata

- requesttype = 0x00;/* write_out */
-
wvalue = (u16)(addr & 0x0000ffff);
len = length;
memcpy(buf, pdata, len);

- ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, len, requesttype);
+ ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, len, REALTEK_USB_VENQT_WRITE);

return ret;
}
--
2.20.1

2021-08-21 16:53:02

[permalink] [raw]

Subject: [PATCH 07/10] staging: r8188eu: clean up the usb_writeXY functions

Remove unnecessary variables, summarize declarations and assignments.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/hal/usb_ops_linux.c | 50 ++++-----------------
1 file changed, 8 insertions(+), 42 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index cb969a200681..e01f1ac19596 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -128,59 +128,25 @@ static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)

static int usb_write8(struct intf_hdl *pintfhdl, u32 addr, u8 val)
{
- u16 wvalue;
- u16 len;
- u8 data;
- int ret;
-
-
- wvalue = (u16)(addr & 0x0000ffff);
- len = 1;
- data = val;
- ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
+ u16 wvalue = (u16)(addr & 0x0000ffff);

- return ret;
+ return usbctrl_vendorreq(pintfhdl, wvalue, &val, 1, REALTEK_USB_VENQT_WRITE);
}

static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
{
- u16 wvalue;
- u16 len;
- __le32 data;
- int ret;
-
-
-
- wvalue = (u16)(addr & 0x0000ffff);
- len = 2;
-
- data = cpu_to_le32(val & 0x0000ffff);
-
- ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
-
-
+ u16 wvalue = (u16)(addr & 0x0000ffff);
+ __le32 data = cpu_to_le32(val & 0x0000ffff);

- return ret;
+ return usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_WRITE);
}

static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
{
- u16 wvalue;
- u16 len;
- __le32 data;
- int ret;
-
-
-
- wvalue = (u16)(addr & 0x0000ffff);
- len = 4;
- data = cpu_to_le32(val);
-
- ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
-
-
+ u16 wvalue = (u16)(addr & 0x0000ffff);
+ __le32 data = cpu_to_le32(val);

- return ret;
+ return usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_WRITE);
}

static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
--
2.20.1

2021-08-21 16:53:08

[permalink] [raw]

Subject: [PATCH 06/10] staging: r8188eu: clean up the usb_readXY functions

Remove unnecessary variables, summarize declarations and assignments.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/hal/usb_ops_linux.c | 30 +++++----------------
1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index dccb9fd34777..cb969a200681 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -98,46 +98,30 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,

static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)
{
- u16 wvalue;
- u16 len;
- u8 data = 0;
-
-
-
- wvalue = (u16)(addr & 0x0000ffff);
- len = 1;
-
- usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
-
+ u16 wvalue = (u16)(addr & 0x0000ffff);
+ u8 data;

+ usbctrl_vendorreq(pintfhdl, wvalue, &data, 1, REALTEK_USB_VENQT_READ);

return data;
-
}

static u16 usb_read16(struct intf_hdl *pintfhdl, u32 addr)
{
- u16 wvalue;
- u16 len;
+ u16 wvalue = (u16)(addr & 0x0000ffff);
__le32 data;

- wvalue = (u16)(addr & 0x0000ffff);
- len = 2;
- usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
+ usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_READ);

return (u16)(le32_to_cpu(data) & 0xffff);
}

static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
{
- u16 wvalue;
- u16 len;
+ u16 wvalue = (u16)(addr & 0x0000ffff);
__le32 data;

- wvalue = (u16)(addr & 0x0000ffff);
- len = 4;
-
- usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
+ usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_READ);

return le32_to_cpu(data);
}
--
2.20.1

2021-08-21 16:53:41

[permalink] [raw]

Subject: [PATCH 09/10] staging: r8188eu: remove unused members of struct _io_ops

Remove function pointers which are not used by the r8188eu driver.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/include/rtw_io.h | 3 ---
1 file changed, 3 deletions(-)

diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
index f1b3074fa075..4b41c7b03972 100644
--- a/drivers/staging/r8188eu/include/rtw_io.h
+++ b/drivers/staging/r8188eu/include/rtw_io.h
@@ -100,13 +100,10 @@ struct _io_ops {
u8 *pmem);
void (*_write_mem)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
u8 *pmem);
- void (*_sync_irp_protocol_rw)(struct io_queue *pio_q);
- u32 (*_read_interrupt)(struct intf_hdl *pintfhdl, u32 addr);
u32 (*_read_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
u8 *pmem);
u32 (*_write_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
u8 *pmem);
- u32 (*_write_scsi)(struct intf_hdl *pintfhdl,u32 cnt, u8 *pmem);
void (*_read_port_cancel)(struct intf_hdl *pintfhdl);
void (*_write_port_cancel)(struct intf_hdl *pintfhdl);
};
--
2.20.1

2021-08-21 16:55:34

[permalink] [raw]

Subject: [PATCH 08/10] staging: r8188eu: clean up the usb_writeN

Remove unnecessary variables, check the length.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/hal/usb_ops_linux.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index e01f1ac19596..5408383ccec3 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -151,20 +151,15 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)

static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
{
- u16 wvalue;
- u16 len;
+ u16 wvalue = (u16)(addr & 0x0000ffff);
u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
- int ret;
-

+ if (length > VENDOR_CMD_MAX_DATA_LEN)
+ return -EINVAL;

- wvalue = (u16)(addr & 0x0000ffff);
- len = length;
- memcpy(buf, pdata, len);
+ memcpy(buf, pdata, length);

- ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, len, REALTEK_USB_VENQT_WRITE);
-
- return ret;
+ return usbctrl_vendorreq(pintfhdl, wvalue, buf, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
}

static void interrupt_handler_8188eu(struct adapter *adapt, u16 pkt_len, u8 *pbuf)
--
2.20.1

2021-08-21 16:55:40

[permalink] [raw]

Subject: [PATCH 10/10] staging: r8188eu: set pipe only once

Set the pipe for reading or writing in usbctrl_vendorreq only once.
There's no need to set it again for every retry.

This patch is an adaptation of commit 889ed8b5e374 ("staging: rtl8188eu:
set pipe only once") for the new r8188eu driver.

Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/hal/usb_ops_linux.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 5408383ccec3..5a55ee38d7b8 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -40,15 +40,16 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
goto release_mutex;
}

- while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
- memset(pIo_buf, 0, len);
+ if (requesttype == REALTEK_USB_VENQT_READ)
+ pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
+ else
+ pipe = usb_sndctrlpipe(udev, 0);/* write_out */

- if (requesttype == REALTEK_USB_VENQT_READ) {
- pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
- } else {
- pipe = usb_sndctrlpipe(udev, 0);/* write_out */
+ while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
+ if (requesttype == REALTEK_USB_VENQT_READ)
+ memset(pIo_buf, 0, len);
+ else
memcpy(pIo_buf, pdata, len);
- }

status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
requesttype, value, REALTEK_USB_VENQT_CMD_IDX,
--
2.20.1

2021-08-21 17:29:05

[permalink] [raw]

Subject: Re: [PATCH 01/10] staging: r8188eu: remove unnecessary cast

On Sat, 21 Aug 2021 at 17:49, Martin Kaiser <[email protected]> wrote:
>
> name is a const char * by default. This type should be ok for r8188eu.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/os_dep/usb_intf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> index e002070f7fba..72556ac10d7d 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> @@ -61,7 +61,7 @@ struct rtw_usb_drv {
> };
>
> static struct rtw_usb_drv rtl8188e_usb_drv = {
> - .usbdrv.name = (char *)"r8188eu",
> + .usbdrv.name = "r8188eu",
> .usbdrv.probe = rtw_drv_init,
> .usbdrv.disconnect = rtw_dev_remove,
> .usbdrv.id_table = rtw_usb_id_tbl,
> --
> 2.20.1
>

Looks ok to me, thanks. I would consider using a cover letter style
[PATCH 00/10] style approach as an addition in future though, just my
personal opinion.

Acked-by: Phillip Potter <[email protected]>

Regards,
Phil

2021-08-21 17:30:43

[permalink] [raw]

Subject: Re: [PATCH 05/10] staging: r8188eu: remove an unused enum

On Sat, 21 Aug 2021 at 17:49, Martin Kaiser <[email protected]> wrote:
>
> The VENDOR_READ and VENDOR_WRITE defines are not used.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/include/usb_ops.h | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/include/usb_ops.h b/drivers/staging/r8188eu/include/usb_ops.h
> index b6a1cd536adf..c53cc54b6b87 100644
> --- a/drivers/staging/r8188eu/include/usb_ops.h
> +++ b/drivers/staging/r8188eu/include/usb_ops.h
> @@ -13,10 +13,6 @@
> #define REALTEK_USB_VENQT_CMD_REQ 0x05
> #define REALTEK_USB_VENQT_CMD_IDX 0x00
>
> -enum {
> - VENDOR_WRITE = 0x00,
> - VENDOR_READ = 0x01,
> -};
> #define ALIGNMENT_UNIT 16
> #define MAX_VENDOR_REQ_CMD_SIZE 254 /* 8188cu SIE Support */
> #define MAX_USB_IO_CTL_SIZE (MAX_VENDOR_REQ_CMD_SIZE + ALIGNMENT_UNIT)
> --
> 2.20.1
>

Acked-by: Phillip Potter <[email protected]>

Regards,
Phil

2021-08-21 17:32:01

[permalink] [raw]

Subject: Re: [PATCH 09/10] staging: r8188eu: remove unused members of struct _io_ops

On Sat, 21 Aug 2021 at 17:50, Martin Kaiser <[email protected]> wrote:
>
> Remove function pointers which are not used by the r8188eu driver.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/include/rtw_io.h | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
> index f1b3074fa075..4b41c7b03972 100644
> --- a/drivers/staging/r8188eu/include/rtw_io.h
> +++ b/drivers/staging/r8188eu/include/rtw_io.h
> @@ -100,13 +100,10 @@ struct _io_ops {
> u8 *pmem);
> void (*_write_mem)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
> u8 *pmem);
> - void (*_sync_irp_protocol_rw)(struct io_queue *pio_q);
> - u32 (*_read_interrupt)(struct intf_hdl *pintfhdl, u32 addr);
> u32 (*_read_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
> u8 *pmem);
> u32 (*_write_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
> u8 *pmem);
> - u32 (*_write_scsi)(struct intf_hdl *pintfhdl,u32 cnt, u8 *pmem);
> void (*_read_port_cancel)(struct intf_hdl *pintfhdl);
> void (*_write_port_cancel)(struct intf_hdl *pintfhdl);
> };
> --
> 2.20.1
>

Acked-by: Phillip Potter <[email protected]>

Regards,
Phil

2021-08-21 17:47:36

[permalink] [raw]

Subject: Re: [PATCH 03/10] staging: rtl8188eu: use actual request type as parameter

On Sat, 21 Aug 2021 at 17:49, Martin Kaiser <[email protected]> wrote:
>
> At the moment, usbctrl_vendorreq's requesttype parameter must be set to
> 1 for reading and 0 for writing. It's then converted to the actual
> bmRequestType for the USB control request. We can simplify the code and
> avoid this conversion if the caller passes the actual bmRequestType.
>
> This patch is an adaptation of commit 788fde031027 ("staging: rtl8188eu:
> use actual request type as parameter") for the new r8188eu driver.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 46 ++++++---------------
> 1 file changed, 12 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index a11a0597e515..dccb9fd34777 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -15,7 +15,6 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> struct usb_device *udev = dvobjpriv->pusbdev;
> unsigned int pipe;
> int status = 0;
> - u8 reqtype;
> u8 *pIo_buf;
> int vendorreq_times = 0;
>
> @@ -44,26 +43,24 @@ 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) {
> + if (requesttype == REALTEK_USB_VENQT_READ) {
> pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
> - reqtype = REALTEK_USB_VENQT_READ;
> } else {
> pipe = usb_sndctrlpipe(udev, 0);/* write_out */
> - reqtype = REALTEK_USB_VENQT_WRITE;
> memcpy(pIo_buf, pdata, len);
> }
>
> status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
> - reqtype, value, REALTEK_USB_VENQT_CMD_IDX,
> + requesttype, value, REALTEK_USB_VENQT_CMD_IDX,
> pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT);
>
> if (status == len) { /* Success this control transfer. */
> rtw_reset_continual_urb_error(dvobjpriv);
> - if (requesttype == 0x01)
> + if (requesttype == REALTEK_USB_VENQT_READ)
> memcpy(pdata, pIo_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",
> + value, (requesttype == REALTEK_USB_VENQT_READ) ? "read" : "write",
> len, status, *(u32 *)pdata, vendorreq_times);
>
> if (status < 0) {
> @@ -75,7 +72,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> }
> } else { /* status != len && status >= 0 */
> if (status > 0) {
> - if (requesttype == 0x01) {
> + if (requesttype == REALTEK_USB_VENQT_READ) {
> /* For Control read transfer, we have to copy the read data from pIo_buf to pdata. */
> memcpy(pdata, pIo_buf, len);
> }
> @@ -101,19 +98,16 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>
> static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> u8 data = 0;
>
>
>
> - requesttype = 0x01;/* read_in */
> -
> wvalue = (u16)(addr & 0x0000ffff);
> len = 1;
>
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
>
>
>
> @@ -123,57 +117,49 @@ static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)
>
> static u16 usb_read16(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> __le32 data;
>
> - requesttype = 0x01;/* read_in */
> wvalue = (u16)(addr & 0x0000ffff);
> len = 2;
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
>
> return (u16)(le32_to_cpu(data) & 0xffff);
> }
>
> static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> __le32 data;
>
> - requesttype = 0x01;/* read_in */
> -
> wvalue = (u16)(addr & 0x0000ffff);
> len = 4;
>
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
>
> return le32_to_cpu(data);
> }
>
> static int usb_write8(struct intf_hdl *pintfhdl, u32 addr, u8 val)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> u8 data;
> int ret;
>
>
> - requesttype = 0x00;/* write_out */
> wvalue = (u16)(addr & 0x0000ffff);
> len = 1;
> data = val;
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
>
> return ret;
> }
>
> static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> __le32 data;
> @@ -181,14 +167,12 @@ static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
>
>
>
> - requesttype = 0x00;/* write_out */
> -
> wvalue = (u16)(addr & 0x0000ffff);
> len = 2;
>
> data = cpu_to_le32(val & 0x0000ffff);
>
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
>
>
>
> @@ -197,7 +181,6 @@ static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
>
> static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> __le32 data;
> @@ -205,13 +188,11 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
>
>
>
> - requesttype = 0x00;/* write_out */
> -
> wvalue = (u16)(addr & 0x0000ffff);
> len = 4;
> data = cpu_to_le32(val);
>
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
>
>
>
> @@ -220,7 +201,6 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
>
> static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
> @@ -228,13 +208,11 @@ static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata
>
>
>
> - requesttype = 0x00;/* write_out */
> -
> wvalue = (u16)(addr & 0x0000ffff);
> len = length;
> memcpy(buf, pdata, len);
>
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, len, requesttype);
> + ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, len, REALTEK_USB_VENQT_WRITE);
>
> return ret;
> }
> --
> 2.20.1
>

Acked-by: Phillip Potter <[email protected]>

Regards,
Phil

2021-08-21 17:49:00

[permalink] [raw]

Subject: Re: [PATCH 06/10] staging: r8188eu: clean up the usb_readXY functions

On Sat, 21 Aug 2021 at 17:49, Martin Kaiser <[email protected]> wrote:
>
> Remove unnecessary variables, summarize declarations and assignments.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 30 +++++----------------
> 1 file changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index dccb9fd34777..cb969a200681 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -98,46 +98,30 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>
> static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u16 wvalue;
> - u16 len;
> - u8 data = 0;
> -
> -
> -
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 1;
> -
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
> -
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> + u8 data;
>
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, 1, REALTEK_USB_VENQT_READ);
>
> return data;
> -
> }
>
> static u16 usb_read16(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u16 wvalue;
> - u16 len;
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> __le32 data;
>
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 2;
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_READ);
>
> return (u16)(le32_to_cpu(data) & 0xffff);
> }
>
> static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u16 wvalue;
> - u16 len;
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> __le32 data;
>
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 4;
> -
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_READ);
>
> return le32_to_cpu(data);
> }
> --
> 2.20.1
>

Acked-by: Phillip Potter <[email protected]>

Regards,
Phil

2021-08-21 17:49:41

[permalink] [raw]

Subject: Re: [PATCH 07/10] staging: r8188eu: clean up the usb_writeXY functions

On Sat, 21 Aug 2021 at 17:49, Martin Kaiser <[email protected]> wrote:
>
> Remove unnecessary variables, summarize declarations and assignments.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 50 ++++-----------------
> 1 file changed, 8 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index cb969a200681..e01f1ac19596 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -128,59 +128,25 @@ static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
>
> static int usb_write8(struct intf_hdl *pintfhdl, u32 addr, u8 val)
> {
> - u16 wvalue;
> - u16 len;
> - u8 data;
> - int ret;
> -
> -
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 1;
> - data = val;
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
> + u16 wvalue = (u16)(addr & 0x0000ffff);
>
> - return ret;
> + return usbctrl_vendorreq(pintfhdl, wvalue, &val, 1, REALTEK_USB_VENQT_WRITE);
> }
>
> static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
> {
> - u16 wvalue;
> - u16 len;
> - __le32 data;
> - int ret;
> -
> -
> -
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 2;
> -
> - data = cpu_to_le32(val & 0x0000ffff);
> -
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
> -
> -
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> + __le32 data = cpu_to_le32(val & 0x0000ffff);
>
> - return ret;
> + return usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_WRITE);
> }
>
> static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
> {
> - u16 wvalue;
> - u16 len;
> - __le32 data;
> - int ret;
> -
> -
> -
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 4;
> - data = cpu_to_le32(val);
> -
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
> -
> -
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> + __le32 data = cpu_to_le32(val);
>
> - return ret;
> + return usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_WRITE);
> }
>
> static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
> --
> 2.20.1
>
Acked-by: Phillip Potter <[email protected]>

Regards,
Phil

2021-08-21 17:50:04

[permalink] [raw]

Subject: Re: [PATCH 10/10] staging: r8188eu: set pipe only once

On Sat, 21 Aug 2021 at 17:50, Martin Kaiser <[email protected]> wrote:
>
> Set the pipe for reading or writing in usbctrl_vendorreq only once.
> There's no need to set it again for every retry.
>
> This patch is an adaptation of commit 889ed8b5e374 ("staging: rtl8188eu:
> set pipe only once") for the new r8188eu driver.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index 5408383ccec3..5a55ee38d7b8 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -40,15 +40,16 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> goto release_mutex;
> }
>
> - while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> - memset(pIo_buf, 0, len);
> + if (requesttype == REALTEK_USB_VENQT_READ)
> + pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
> + else
> + pipe = usb_sndctrlpipe(udev, 0);/* write_out */
>
> - if (requesttype == REALTEK_USB_VENQT_READ) {
> - pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
> - } else {
> - pipe = usb_sndctrlpipe(udev, 0);/* write_out */
> + while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> + if (requesttype == REALTEK_USB_VENQT_READ)
> + memset(pIo_buf, 0, len);
> + else
> memcpy(pIo_buf, pdata, len);
> - }
>
> status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
> requesttype, value, REALTEK_USB_VENQT_CMD_IDX,
> --
> 2.20.1
>

Acked-by: Phillip Potter <[email protected]>

Regards,
Phil

2021-08-21 17:50:10

[permalink] [raw]

Subject: Re: [PATCH 08/10] staging: r8188eu: clean up the usb_writeN

On Sat, 21 Aug 2021 at 17:50, Martin Kaiser <[email protected]> wrote:
>
> Remove unnecessary variables, check the length.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index e01f1ac19596..5408383ccec3 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -151,20 +151,15 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
>
> static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
> {
> - u16 wvalue;
> - u16 len;
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
> - int ret;
> -
>
> + if (length > VENDOR_CMD_MAX_DATA_LEN)
> + return -EINVAL;
>
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = length;
> - memcpy(buf, pdata, len);
> + memcpy(buf, pdata, length);
>
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, len, REALTEK_USB_VENQT_WRITE);
> -
> - return ret;
> + return usbctrl_vendorreq(pintfhdl, wvalue, buf, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
> }
>
> static void interrupt_handler_8188eu(struct adapter *adapt, u16 pkt_len, u8 *pbuf)
> --
> 2.20.1
>

Acked-by: Phillip Potter <[email protected]>

Regards,
Phil

2021-08-22 11:57:23

[permalink] [raw]

Subject: Re: [PATCH 01/10] staging: r8188eu: remove unnecessary cast

On 8/21/21 6:48 PM, Martin Kaiser wrote:
> name is a const char * by default. This type should be ok for r8188eu.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/os_dep/usb_intf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> index e002070f7fba..72556ac10d7d 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> @@ -61,7 +61,7 @@ struct rtw_usb_drv {
> };
>
> static struct rtw_usb_drv rtl8188e_usb_drv = {
> - .usbdrv.name = (char *)"r8188eu",
> + .usbdrv.name = "r8188eu",
> .usbdrv.probe = rtw_drv_init,
> .usbdrv.disconnect = rtw_dev_remove,
> .usbdrv.id_table = rtw_usb_id_tbl,
>

Looks good to me.

Acked-by: Michael Straube <[email protected]>

Thanks,
Michael

2021-08-22 12:11:21

[permalink] [raw]

Subject: Re: [PATCH 03/10] staging: rtl8188eu: use actual request type as parameter

On 8/21/21 6:48 PM, Martin Kaiser wrote:
> At the moment, usbctrl_vendorreq's requesttype parameter must be set to
> 1 for reading and 0 for writing. It's then converted to the actual
> bmRequestType for the USB control request. We can simplify the code and
> avoid this conversion if the caller passes the actual bmRequestType.
>
> This patch is an adaptation of commit 788fde031027 ("staging: rtl8188eu:
> use actual request type as parameter") for the new r8188eu driver.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 46 ++++++---------------
> 1 file changed, 12 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index a11a0597e515..dccb9fd34777 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -15,7 +15,6 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> struct usb_device *udev = dvobjpriv->pusbdev;
> unsigned int pipe;
> int status = 0;
> - u8 reqtype;
> u8 *pIo_buf;
> int vendorreq_times = 0;
>
> @@ -44,26 +43,24 @@ 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) {
> + if (requesttype == REALTEK_USB_VENQT_READ) {
> pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
> - reqtype = REALTEK_USB_VENQT_READ;
> } else {
> pipe = usb_sndctrlpipe(udev, 0);/* write_out */
> - reqtype = REALTEK_USB_VENQT_WRITE;
> memcpy(pIo_buf, pdata, len);
> }
>
> status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
> - reqtype, value, REALTEK_USB_VENQT_CMD_IDX,
> + requesttype, value, REALTEK_USB_VENQT_CMD_IDX,
> pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT);
>
> if (status == len) { /* Success this control transfer. */
> rtw_reset_continual_urb_error(dvobjpriv);
> - if (requesttype == 0x01)
> + if (requesttype == REALTEK_USB_VENQT_READ)
> memcpy(pdata, pIo_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",
> + value, (requesttype == REALTEK_USB_VENQT_READ) ? "read" : "write",
> len, status, *(u32 *)pdata, vendorreq_times);
>
> if (status < 0) {
> @@ -75,7 +72,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> }
> } else { /* status != len && status >= 0 */
> if (status > 0) {
> - if (requesttype == 0x01) {
> + if (requesttype == REALTEK_USB_VENQT_READ) {
> /* For Control read transfer, we have to copy the read data from pIo_buf to pdata. */
> memcpy(pdata, pIo_buf, len);
> }
> @@ -101,19 +98,16 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>
> static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> u8 data = 0;
>
>
>
> - requesttype = 0x01;/* read_in */
> -
> wvalue = (u16)(addr & 0x0000ffff);
> len = 1;
>
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
>
>
>
> @@ -123,57 +117,49 @@ static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)
>
> static u16 usb_read16(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> __le32 data;
>
> - requesttype = 0x01;/* read_in */
> wvalue = (u16)(addr & 0x0000ffff);
> len = 2;
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
>
> return (u16)(le32_to_cpu(data) & 0xffff);
> }
>
> static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> __le32 data;
>
> - requesttype = 0x01;/* read_in */
> -
> wvalue = (u16)(addr & 0x0000ffff);
> len = 4;
>
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
>
> return le32_to_cpu(data);
> }
>
> static int usb_write8(struct intf_hdl *pintfhdl, u32 addr, u8 val)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> u8 data;
> int ret;
>
>
> - requesttype = 0x00;/* write_out */
> wvalue = (u16)(addr & 0x0000ffff);
> len = 1;
> data = val;
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
>
> return ret;
> }
>
> static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> __le32 data;
> @@ -181,14 +167,12 @@ static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
>
>
>
> - requesttype = 0x00;/* write_out */
> -
> wvalue = (u16)(addr & 0x0000ffff);
> len = 2;
>
> data = cpu_to_le32(val & 0x0000ffff);
>
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
>
>
>
> @@ -197,7 +181,6 @@ static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
>
> static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> __le32 data;
> @@ -205,13 +188,11 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
>
>
>
> - requesttype = 0x00;/* write_out */
> -
> wvalue = (u16)(addr & 0x0000ffff);
> len = 4;
> data = cpu_to_le32(val);
>
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> + ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
>
>
>
> @@ -220,7 +201,6 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
>
> static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
> {
> - u8 requesttype;
> u16 wvalue;
> u16 len;
> u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
> @@ -228,13 +208,11 @@ static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata
>
>
>
> - requesttype = 0x00;/* write_out */
> -
> wvalue = (u16)(addr & 0x0000ffff);
> len = length;
> memcpy(buf, pdata, len);
>
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, len, requesttype);
> + ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, len, REALTEK_USB_VENQT_WRITE);
>
> return ret;
> }
>

Looks good to me.

Acked-by: Michael Straube <[email protected]>

Thanks,
Michael

2021-08-22 12:21:04

[permalink] [raw]

Subject: Re: [PATCH 05/10] staging: r8188eu: remove an unused enum

On 8/21/21 6:48 PM, Martin Kaiser wrote:
> The VENDOR_READ and VENDOR_WRITE defines are not used.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/include/usb_ops.h | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/include/usb_ops.h b/drivers/staging/r8188eu/include/usb_ops.h
> index b6a1cd536adf..c53cc54b6b87 100644
> --- a/drivers/staging/r8188eu/include/usb_ops.h
> +++ b/drivers/staging/r8188eu/include/usb_ops.h
> @@ -13,10 +13,6 @@
> #define REALTEK_USB_VENQT_CMD_REQ 0x05
> #define REALTEK_USB_VENQT_CMD_IDX 0x00
>
> -enum {
> - VENDOR_WRITE = 0x00,
> - VENDOR_READ = 0x01,
> -};
> #define ALIGNMENT_UNIT 16
> #define MAX_VENDOR_REQ_CMD_SIZE 254 /* 8188cu SIE Support */
> #define MAX_USB_IO_CTL_SIZE (MAX_VENDOR_REQ_CMD_SIZE + ALIGNMENT_UNIT)
>

Looks good to me.

Acked-by: Michael Straube <[email protected]>

Thanks,
Michael

2021-08-22 12:27:54

[permalink] [raw]

Subject: Re: [PATCH 06/10] staging: r8188eu: clean up the usb_readXY functions

On 8/21/21 6:48 PM, Martin Kaiser wrote:
> Remove unnecessary variables, summarize declarations and assignments.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 30 +++++----------------
> 1 file changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index dccb9fd34777..cb969a200681 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -98,46 +98,30 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
>
> static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u16 wvalue;
> - u16 len;
> - u8 data = 0;
> -
> -
> -
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 1;
> -
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
> -
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> + u8 data;
>
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, 1, REALTEK_USB_VENQT_READ);
>
> return data;
> -
> }
>
> static u16 usb_read16(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u16 wvalue;
> - u16 len;
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> __le32 data;
>
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 2;
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_READ);
>
> return (u16)(le32_to_cpu(data) & 0xffff);
> }
>
> static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
> {
> - u16 wvalue;
> - u16 len;
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> __le32 data;
>
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 4;
> -
> - usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_READ);
> + usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_READ);
>
> return le32_to_cpu(data);
> }
>

Looks good to me.

Acked-by: Michael Straube <[email protected]>

Thanks,
Michael

2021-08-22 12:31:29

[permalink] [raw]

Subject: Re: [PATCH 07/10] staging: r8188eu: clean up the usb_writeXY functions

On 8/21/21 6:48 PM, Martin Kaiser wrote:
> Remove unnecessary variables, summarize declarations and assignments.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 50 ++++-----------------
> 1 file changed, 8 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index cb969a200681..e01f1ac19596 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -128,59 +128,25 @@ static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
>
> static int usb_write8(struct intf_hdl *pintfhdl, u32 addr, u8 val)
> {
> - u16 wvalue;
> - u16 len;
> - u8 data;
> - int ret;
> -
> -
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 1;
> - data = val;
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
> + u16 wvalue = (u16)(addr & 0x0000ffff);
>
> - return ret;
> + return usbctrl_vendorreq(pintfhdl, wvalue, &val, 1, REALTEK_USB_VENQT_WRITE);
> }
>
> static int usb_write16(struct intf_hdl *pintfhdl, u32 addr, u16 val)
> {
> - u16 wvalue;
> - u16 len;
> - __le32 data;
> - int ret;
> -
> -
> -
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 2;
> -
> - data = cpu_to_le32(val & 0x0000ffff);
> -
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
> -
> -
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> + __le32 data = cpu_to_le32(val & 0x0000ffff);
>
> - return ret;
> + return usbctrl_vendorreq(pintfhdl, wvalue, &data, 2, REALTEK_USB_VENQT_WRITE);
> }
>
> static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
> {
> - u16 wvalue;
> - u16 len;
> - __le32 data;
> - int ret;
> -
> -
> -
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = 4;
> - data = cpu_to_le32(val);
> -
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, REALTEK_USB_VENQT_WRITE);
> -
> -
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> + __le32 data = cpu_to_le32(val);
>
> - return ret;
> + return usbctrl_vendorreq(pintfhdl, wvalue, &data, 4, REALTEK_USB_VENQT_WRITE);
> }
>
> static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
>

Looks good to me.

Acked-by: Michael Straube <[email protected]>

Thanks,
Michael

2021-08-22 13:27:40

[permalink] [raw]

Subject: Re: [PATCH 08/10] staging: r8188eu: clean up the usb_writeN

On 8/21/21 6:48 PM, Martin Kaiser wrote:
> Remove unnecessary variables, check the length.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index e01f1ac19596..5408383ccec3 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -151,20 +151,15 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
>
> static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
> {
> - u16 wvalue;
> - u16 len;
> + u16 wvalue = (u16)(addr & 0x0000ffff);
> u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
> - int ret;
> -
>
> + if (length > VENDOR_CMD_MAX_DATA_LEN)
> + return -EINVAL;
>
> - wvalue = (u16)(addr & 0x0000ffff);
> - len = length;
> - memcpy(buf, pdata, len);
> + memcpy(buf, pdata, length);

Hi Martin, shouldn't this be

memcpy(buf, pdata, (length & 0xffff));

>
> - ret = usbctrl_vendorreq(pintfhdl, wvalue, buf, len, REALTEK_USB_VENQT_WRITE);
> -
> - return ret;
> + return usbctrl_vendorreq(pintfhdl, wvalue, buf, (length & 0xffff), REALTEK_USB_VENQT_WRITE);
> }
>
> static void interrupt_handler_8188eu(struct adapter *adapt, u16 pkt_len, u8 *pbuf)
>

Regards,
Michael

2021-08-22 13:31:30

[permalink] [raw]

Subject: Re: [PATCH 09/10] staging: r8188eu: remove unused members of struct _io_ops

On 8/21/21 6:48 PM, Martin Kaiser wrote:
> Remove function pointers which are not used by the r8188eu driver.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/include/rtw_io.h | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/include/rtw_io.h b/drivers/staging/r8188eu/include/rtw_io.h
> index f1b3074fa075..4b41c7b03972 100644
> --- a/drivers/staging/r8188eu/include/rtw_io.h
> +++ b/drivers/staging/r8188eu/include/rtw_io.h
> @@ -100,13 +100,10 @@ struct _io_ops {
> u8 *pmem);
> void (*_write_mem)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
> u8 *pmem);
> - void (*_sync_irp_protocol_rw)(struct io_queue *pio_q);
> - u32 (*_read_interrupt)(struct intf_hdl *pintfhdl, u32 addr);
> u32 (*_read_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
> u8 *pmem);
> u32 (*_write_port)(struct intf_hdl *pintfhdl, u32 addr, u32 cnt,
> u8 *pmem);
> - u32 (*_write_scsi)(struct intf_hdl *pintfhdl,u32 cnt, u8 *pmem);
> void (*_read_port_cancel)(struct intf_hdl *pintfhdl);
> void (*_write_port_cancel)(struct intf_hdl *pintfhdl);
> };
>

Looks good to me.

Acked-by: Michael Straube <[email protected]>

Thanks,
Michael

2021-08-22 14:06:59

[permalink] [raw]

Subject: Re: [PATCH 10/10] staging: r8188eu: set pipe only once

On 8/21/21 6:48 PM, Martin Kaiser wrote:
> Set the pipe for reading or writing in usbctrl_vendorreq only once.
> There's no need to set it again for every retry.
>
> This patch is an adaptation of commit 889ed8b5e374 ("staging: rtl8188eu:
> set pipe only once") for the new r8188eu driver.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/hal/usb_ops_linux.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index 5408383ccec3..5a55ee38d7b8 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -40,15 +40,16 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
> goto release_mutex;
> }
>
> - while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> - memset(pIo_buf, 0, len);
> + if (requesttype == REALTEK_USB_VENQT_READ)
> + pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
> + else
> + pipe = usb_sndctrlpipe(udev, 0);/* write_out */
>
> - if (requesttype == REALTEK_USB_VENQT_READ) {
> - pipe = usb_rcvctrlpipe(udev, 0);/* read_in */
> - } else {
> - pipe = usb_sndctrlpipe(udev, 0);/* write_out */
> + while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
> + if (requesttype == REALTEK_USB_VENQT_READ)
> + memset(pIo_buf, 0, len);
> + else
> memcpy(pIo_buf, pdata, len);
> - }
>
> status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
> requesttype, value, REALTEK_USB_VENQT_CMD_IDX,
>

Looks good to me.

Acked-by: Michael Straube <[email protected]>

Thanks,
Michael

2021-08-22 17:00:29

[permalink] [raw]

Subject: Re: [PATCH 08/10] staging: r8188eu: clean up the usb_writeN

Hi Michael,

Thus wrote Michael Straube ([email protected]):

> On 8/21/21 6:48 PM, Martin Kaiser wrote:
> > Remove unnecessary variables, check the length.

> > Signed-off-by: Martin Kaiser <[email protected]>
> > ---
> > drivers/staging/r8188eu/hal/usb_ops_linux.c | 15 +++++----------
> > 1 file changed, 5 insertions(+), 10 deletions(-)

> > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > index e01f1ac19596..5408383ccec3 100644
> > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> > @@ -151,20 +151,15 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
> > static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
> > {
> > - u16 wvalue;
> > - u16 len;
> > + u16 wvalue = (u16)(addr & 0x0000ffff);
> > u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
> > - int ret;
> > -
> > + if (length > VENDOR_CMD_MAX_DATA_LEN)
> > + return -EINVAL;
> > - wvalue = (u16)(addr & 0x0000ffff);
> > - len = length;
> > - memcpy(buf, pdata, len);
> > + memcpy(buf, pdata, length);

> Hi Martin, shouldn't this be

> memcpy(buf, pdata, (length & 0xffff));

I don't think this makes any difference. I've already checked that
length <= VENDOR_CMD_MAX_DATA_LEN, which is 254. memcpy takes a size_t
parameter for the number of bytes to copy. length will not overflow
this.

Best regards,
Martin

2021-08-22 17:07:47

[permalink] [raw]

Subject: Re: [PATCH 01/10] staging: r8188eu: remove unnecessary cast

Thus wrote Phillip Potter ([email protected]):

> On Sat, 21 Aug 2021 at 17:49, Martin Kaiser <[email protected]> wrote:

> > name is a const char * by default. This type should be ok for r8188eu.

> > Signed-off-by: Martin Kaiser <[email protected]>
> > ---
> > drivers/staging/r8188eu/os_dep/usb_intf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)

> > diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> > index e002070f7fba..72556ac10d7d 100644
> > --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> > +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> > @@ -61,7 +61,7 @@ struct rtw_usb_drv {
> > };

> > static struct rtw_usb_drv rtl8188e_usb_drv = {
> > - .usbdrv.name = (char *)"r8188eu",
> > + .usbdrv.name = "r8188eu",
> > .usbdrv.probe = rtw_drv_init,
> > .usbdrv.disconnect = rtw_dev_remove,
> > .usbdrv.id_table = rtw_usb_id_tbl,
> > --
> > 2.20.1

Hi Phil,

> Looks ok to me, thanks. I would consider using a cover letter style
> [PATCH 00/10] style approach as an addition in future though, just my
> personal opinion.

> Acked-by: Phillip Potter <[email protected]>

Thanks.

This series is a mixed bag of things I found while poking around in the
code. So I didn't think there was anything useful to say in a cover
letter. Still, I see your point, it makes sense for a patch series to
have a cover letter, I'll add one for future patch series.

Best regards,
Martin

2021-08-22 17:19:39

[permalink] [raw]

Subject: Re: [PATCH 08/10] staging: r8188eu: clean up the usb_writeN

On 8/22/21 6:58 PM, Martin Kaiser wrote:
> Hi Michael,
>
> Thus wrote Michael Straube ([email protected]):
>
>> On 8/21/21 6:48 PM, Martin Kaiser wrote:
>>> Remove unnecessary variables, check the length.
>
>>> Signed-off-by: Martin Kaiser <[email protected]>
>>> ---
>>> drivers/staging/r8188eu/hal/usb_ops_linux.c | 15 +++++----------
>>> 1 file changed, 5 insertions(+), 10 deletions(-)
>
>>> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>>> index e01f1ac19596..5408383ccec3 100644
>>> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
>>> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>>> @@ -151,20 +151,15 @@ static int usb_write32(struct intf_hdl *pintfhdl, u32 addr, u32 val)
>>> static int usb_writeN(struct intf_hdl *pintfhdl, u32 addr, u32 length, u8 *pdata)
>>> {
>>> - u16 wvalue;
>>> - u16 len;
>>> + u16 wvalue = (u16)(addr & 0x0000ffff);
>>> u8 buf[VENDOR_CMD_MAX_DATA_LEN] = {0};
>>> - int ret;
>>> -
>>> + if (length > VENDOR_CMD_MAX_DATA_LEN)
>>> + return -EINVAL;
>>> - wvalue = (u16)(addr & 0x0000ffff);
>>> - len = length;
>>> - memcpy(buf, pdata, len);
>>> + memcpy(buf, pdata, length);
>
>> Hi Martin, shouldn't this be
>
>> memcpy(buf, pdata, (length & 0xffff));
>
> I don't think this makes any difference. I've already checked that
> length <= VENDOR_CMD_MAX_DATA_LEN, which is 254. memcpy takes a size_t
> parameter for the number of bytes to copy. length will not overflow
> this.
>
> Best regards,
> Martin
>

Hi Martin,

ah, I see now. You are right.

Acked-by: Michael Straube <[email protected]>

Thanks,
Michael

2021-08-22 23:51:00