2024-01-10 13:46:46

by Bitterblue Smith

[permalink] [raw]
Subject: [PATCH v2] wifi: rtlwifi: rtl_usb: Use sync register writes

Currently rtl_usb performs register writes using the async
usb_submit_urb() function. This appears to work fine for the RTL8192CU,
but the RTL8192DU (soon to be supported by rtlwifi) has a problem:
it transmits everything at the 1M rate in the 2.4 GHz band. (The 5 GHz
band is still untested.)

With this patch, rtl_usb performs the register writes using the
synchronous usb_control_msg() function, and the RTL8192DU works
normally. The RTL8192CU still works.

The vendor drivers use the async writes in only one function,
rtl8192du_trigger_gpio_0 / rtl8192cu_trigger_gpio_0, which probably
doesn't even run in real life. They use sync writes everywhere else.

Also, remove "sync" and "async" from the names of the members of
struct rtl_io to avoid confusion:

write{8,16,32}_async -> write{8,16,32}
read{8,16,32}_sync -> read{8,16,32}

Signed-off-by: Bitterblue Smith <[email protected]>
---
v2:
- Rename the members of struct rtl_io.
---
drivers/net/wireless/realtek/rtlwifi/pci.c | 12 +-
drivers/net/wireless/realtek/rtlwifi/usb.c | 163 ++++++--------------
drivers/net/wireless/realtek/rtlwifi/wifi.h | 30 ++--
3 files changed, 66 insertions(+), 139 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
index 96ce05bcf0b3..d059cfe5a2a9 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -378,13 +378,13 @@ static void _rtl_pci_io_handler_init(struct device *dev,

rtlpriv->io.dev = dev;

- rtlpriv->io.write8_async = pci_write8_async;
- rtlpriv->io.write16_async = pci_write16_async;
- rtlpriv->io.write32_async = pci_write32_async;
+ rtlpriv->io.write8 = pci_write8_async;
+ rtlpriv->io.write16 = pci_write16_async;
+ rtlpriv->io.write32 = pci_write32_async;

- rtlpriv->io.read8_sync = pci_read8_sync;
- rtlpriv->io.read16_sync = pci_read16_sync;
- rtlpriv->io.read32_sync = pci_read32_sync;
+ rtlpriv->io.read8 = pci_read8_sync;
+ rtlpriv->io.read16 = pci_read16_sync;
+ rtlpriv->io.read32 = pci_read32_sync;
}

static bool _rtl_update_earlymode_info(struct ieee80211_hw *hw,
diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
index 30bf2775a335..7b91d882f969 100644
--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -23,86 +23,23 @@ MODULE_DESCRIPTION("USB basic driver for rtlwifi");

#define MAX_USBCTRL_VENDORREQ_TIMES 10

-static void usbctrl_async_callback(struct urb *urb)
-{
- if (urb) {
- /* free dr */
- kfree(urb->setup_packet);
- /* free databuf */
- kfree(urb->transfer_buffer);
- }
-}
-
-static int _usbctrl_vendorreq_async_write(struct usb_device *udev, u8 request,
- u16 value, u16 index, void *pdata,
- u16 len)
-{
- int rc;
- unsigned int pipe;
- u8 reqtype;
- struct usb_ctrlrequest *dr;
- struct urb *urb;
- const u16 databuf_maxlen = REALTEK_USB_VENQT_MAX_BUF_SIZE;
- u8 *databuf;
-
- if (WARN_ON_ONCE(len > databuf_maxlen))
- len = databuf_maxlen;
-
- pipe = usb_sndctrlpipe(udev, 0); /* write_out */
- reqtype = REALTEK_USB_VENQT_WRITE;
-
- dr = kzalloc(sizeof(*dr), GFP_ATOMIC);
- if (!dr)
- return -ENOMEM;
-
- databuf = kzalloc(databuf_maxlen, GFP_ATOMIC);
- if (!databuf) {
- kfree(dr);
- return -ENOMEM;
- }
-
- urb = usb_alloc_urb(0, GFP_ATOMIC);
- if (!urb) {
- kfree(databuf);
- kfree(dr);
- return -ENOMEM;
- }
-
- dr->bRequestType = reqtype;
- dr->bRequest = request;
- dr->wValue = cpu_to_le16(value);
- dr->wIndex = cpu_to_le16(index);
- dr->wLength = cpu_to_le16(len);
- /* data are already in little-endian order */
- memcpy(databuf, pdata, len);
- usb_fill_control_urb(urb, udev, pipe,
- (unsigned char *)dr, databuf, len,
- usbctrl_async_callback, NULL);
- rc = usb_submit_urb(urb, GFP_ATOMIC);
- if (rc < 0) {
- kfree(databuf);
- kfree(dr);
- }
- usb_free_urb(urb);
- return rc;
-}
-
-static int _usbctrl_vendorreq_sync_read(struct usb_device *udev, u8 request,
- u16 value, u16 index, void *pdata,
- u16 len)
+static void _usbctrl_vendorreq_sync(struct usb_device *udev, u8 reqtype,
+ u16 value, void *pdata, u16 len)
{
unsigned int pipe;
int status;
- u8 reqtype;
int vendorreq_times = 0;
static int count;

- pipe = usb_rcvctrlpipe(udev, 0); /* read_in */
- reqtype = REALTEK_USB_VENQT_READ;
+ if (reqtype == REALTEK_USB_VENQT_READ)
+ pipe = usb_rcvctrlpipe(udev, 0); /* read_in */
+ else
+ pipe = usb_sndctrlpipe(udev, 0); /* write_out */

do {
- status = usb_control_msg(udev, pipe, request, reqtype, value,
- index, pdata, len, 1000);
+ status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,
+ reqtype, value, REALTEK_USB_VENQT_CMD_IDX,
+ pdata, len, 1000);
if (status < 0) {
/* firmware download is checksumed, don't retry */
if ((value >= FW_8192C_START_ADDRESS &&
@@ -114,18 +51,15 @@ static int _usbctrl_vendorreq_sync_read(struct usb_device *udev, u8 request,
} while (++vendorreq_times < MAX_USBCTRL_VENDORREQ_TIMES);

if (status < 0 && count++ < 4)
- pr_err("reg 0x%x, usbctrl_vendorreq TimeOut! status:0x%x value=0x%x\n",
- value, status, *(u32 *)pdata);
- return status;
+ pr_err("reg 0x%x, usbctrl_vendorreq TimeOut! status:0x%x value=0x%x reqtype=0x%x\n",
+ value, status, *(u32 *)pdata, reqtype);
}

static u32 _usb_read_sync(struct rtl_priv *rtlpriv, u32 addr, u16 len)
{
struct device *dev = rtlpriv->io.dev;
struct usb_device *udev = to_usb_device(dev);
- u8 request;
u16 wvalue;
- u16 index;
__le32 *data;
unsigned long flags;

@@ -134,14 +68,33 @@ static u32 _usb_read_sync(struct rtl_priv *rtlpriv, u32 addr, u16 len)
rtlpriv->usb_data_index = 0;
data = &rtlpriv->usb_data[rtlpriv->usb_data_index];
spin_unlock_irqrestore(&rtlpriv->locks.usb_lock, flags);
- request = REALTEK_USB_VENQT_CMD_REQ;
- index = REALTEK_USB_VENQT_CMD_IDX; /* n/a */

wvalue = (u16)addr;
- _usbctrl_vendorreq_sync_read(udev, request, wvalue, index, data, len);
+ _usbctrl_vendorreq_sync(udev, REALTEK_USB_VENQT_READ, wvalue, data, len);
return le32_to_cpu(*data);
}

+
+static void _usb_write_sync(struct rtl_priv *rtlpriv, u32 addr, u32 val, u16 len)
+{
+ struct device *dev = rtlpriv->io.dev;
+ struct usb_device *udev = to_usb_device(dev);
+ unsigned long flags;
+ __le32 *data;
+ u16 wvalue;
+
+ spin_lock_irqsave(&rtlpriv->locks.usb_lock, flags);
+ if (++rtlpriv->usb_data_index >= RTL_USB_MAX_RX_COUNT)
+ rtlpriv->usb_data_index = 0;
+ data = &rtlpriv->usb_data[rtlpriv->usb_data_index];
+ spin_unlock_irqrestore(&rtlpriv->locks.usb_lock, flags);
+
+ wvalue = (u16)(addr & 0x0000ffff);
+ *data = cpu_to_le32(val);
+
+ _usbctrl_vendorreq_sync(udev, REALTEK_USB_VENQT_WRITE, wvalue, data, len);
+}
+
static u8 _usb_read8_sync(struct rtl_priv *rtlpriv, u32 addr)
{
return (u8)_usb_read_sync(rtlpriv, addr, 1);
@@ -157,45 +110,19 @@ static u32 _usb_read32_sync(struct rtl_priv *rtlpriv, u32 addr)
return _usb_read_sync(rtlpriv, addr, 4);
}

-static void _usb_write_async(struct usb_device *udev, u32 addr, u32 val,
- u16 len)
-{
- u8 request;
- u16 wvalue;
- u16 index;
- __le32 data;
- int ret;
-
- request = REALTEK_USB_VENQT_CMD_REQ;
- index = REALTEK_USB_VENQT_CMD_IDX; /* n/a */
- wvalue = (u16)(addr&0x0000ffff);
- data = cpu_to_le32(val);
-
- ret = _usbctrl_vendorreq_async_write(udev, request, wvalue,
- index, &data, len);
- if (ret < 0)
- dev_err(&udev->dev, "error %d writing at 0x%x\n", ret, addr);
-}
-
-static void _usb_write8_async(struct rtl_priv *rtlpriv, u32 addr, u8 val)
+static void _usb_write8_sync(struct rtl_priv *rtlpriv, u32 addr, u8 val)
{
- struct device *dev = rtlpriv->io.dev;
-
- _usb_write_async(to_usb_device(dev), addr, val, 1);
+ _usb_write_sync(rtlpriv, addr, val, 1);
}

-static void _usb_write16_async(struct rtl_priv *rtlpriv, u32 addr, u16 val)
+static void _usb_write16_sync(struct rtl_priv *rtlpriv, u32 addr, u16 val)
{
- struct device *dev = rtlpriv->io.dev;
-
- _usb_write_async(to_usb_device(dev), addr, val, 2);
+ _usb_write_sync(rtlpriv, addr, val, 2);
}

-static void _usb_write32_async(struct rtl_priv *rtlpriv, u32 addr, u32 val)
+static void _usb_write32_sync(struct rtl_priv *rtlpriv, u32 addr, u32 val)
{
- struct device *dev = rtlpriv->io.dev;
-
- _usb_write_async(to_usb_device(dev), addr, val, 4);
+ _usb_write_sync(rtlpriv, addr, val, 4);
}

static void _rtl_usb_io_handler_init(struct device *dev,
@@ -205,12 +132,12 @@ static void _rtl_usb_io_handler_init(struct device *dev,

rtlpriv->io.dev = dev;
mutex_init(&rtlpriv->io.bb_mutex);
- rtlpriv->io.write8_async = _usb_write8_async;
- rtlpriv->io.write16_async = _usb_write16_async;
- rtlpriv->io.write32_async = _usb_write32_async;
- rtlpriv->io.read8_sync = _usb_read8_sync;
- rtlpriv->io.read16_sync = _usb_read16_sync;
- rtlpriv->io.read32_sync = _usb_read32_sync;
+ rtlpriv->io.write8 = _usb_write8_sync;
+ rtlpriv->io.write16 = _usb_write16_sync;
+ rtlpriv->io.write32 = _usb_write32_sync;
+ rtlpriv->io.read8 = _usb_read8_sync;
+ rtlpriv->io.read16 = _usb_read16_sync;
+ rtlpriv->io.read32 = _usb_read32_sync;
}

static void _rtl_usb_io_handler_release(struct ieee80211_hw *hw)
diff --git a/drivers/net/wireless/realtek/rtlwifi/wifi.h b/drivers/net/wireless/realtek/rtlwifi/wifi.h
index d87cd2252eac..53af324f3807 100644
--- a/drivers/net/wireless/realtek/rtlwifi/wifi.h
+++ b/drivers/net/wireless/realtek/rtlwifi/wifi.h
@@ -1447,13 +1447,13 @@ struct rtl_io {
/*PCI IO map */
unsigned long pci_base_addr; /*device I/O address */

- void (*write8_async)(struct rtl_priv *rtlpriv, u32 addr, u8 val);
- void (*write16_async)(struct rtl_priv *rtlpriv, u32 addr, u16 val);
- void (*write32_async)(struct rtl_priv *rtlpriv, u32 addr, u32 val);
+ void (*write8)(struct rtl_priv *rtlpriv, u32 addr, u8 val);
+ void (*write16)(struct rtl_priv *rtlpriv, u32 addr, u16 val);
+ void (*write32)(struct rtl_priv *rtlpriv, u32 addr, u32 val);

- u8 (*read8_sync)(struct rtl_priv *rtlpriv, u32 addr);
- u16 (*read16_sync)(struct rtl_priv *rtlpriv, u32 addr);
- u32 (*read32_sync)(struct rtl_priv *rtlpriv, u32 addr);
+ u8 (*read8)(struct rtl_priv *rtlpriv, u32 addr);
+ u16 (*read16)(struct rtl_priv *rtlpriv, u32 addr);
+ u32 (*read32)(struct rtl_priv *rtlpriv, u32 addr);

};

@@ -2916,25 +2916,25 @@ extern u8 channel5g_80m[CHANNEL_MAX_NUMBER_5G_80M];

static inline u8 rtl_read_byte(struct rtl_priv *rtlpriv, u32 addr)
{
- return rtlpriv->io.read8_sync(rtlpriv, addr);
+ return rtlpriv->io.read8(rtlpriv, addr);
}

static inline u16 rtl_read_word(struct rtl_priv *rtlpriv, u32 addr)
{
- return rtlpriv->io.read16_sync(rtlpriv, addr);
+ return rtlpriv->io.read16(rtlpriv, addr);
}

static inline u32 rtl_read_dword(struct rtl_priv *rtlpriv, u32 addr)
{
- return rtlpriv->io.read32_sync(rtlpriv, addr);
+ return rtlpriv->io.read32(rtlpriv, addr);
}

static inline void rtl_write_byte(struct rtl_priv *rtlpriv, u32 addr, u8 val8)
{
- rtlpriv->io.write8_async(rtlpriv, addr, val8);
+ rtlpriv->io.write8(rtlpriv, addr, val8);

if (rtlpriv->cfg->write_readback)
- rtlpriv->io.read8_sync(rtlpriv, addr);
+ rtlpriv->io.read8(rtlpriv, addr);
}

static inline void rtl_write_byte_with_val32(struct ieee80211_hw *hw,
@@ -2947,19 +2947,19 @@ static inline void rtl_write_byte_with_val32(struct ieee80211_hw *hw,

static inline void rtl_write_word(struct rtl_priv *rtlpriv, u32 addr, u16 val16)
{
- rtlpriv->io.write16_async(rtlpriv, addr, val16);
+ rtlpriv->io.write16(rtlpriv, addr, val16);

if (rtlpriv->cfg->write_readback)
- rtlpriv->io.read16_sync(rtlpriv, addr);
+ rtlpriv->io.read16(rtlpriv, addr);
}

static inline void rtl_write_dword(struct rtl_priv *rtlpriv,
u32 addr, u32 val32)
{
- rtlpriv->io.write32_async(rtlpriv, addr, val32);
+ rtlpriv->io.write32(rtlpriv, addr, val32);

if (rtlpriv->cfg->write_readback)
- rtlpriv->io.read32_sync(rtlpriv, addr);
+ rtlpriv->io.read32(rtlpriv, addr);
}

static inline u32 rtl_get_bbreg(struct ieee80211_hw *hw,
--
2.43.0


2024-01-11 01:59:48

by Ping-Ke Shih

[permalink] [raw]
Subject: RE: [PATCH v2] wifi: rtlwifi: rtl_usb: Use sync register writes



> -----Original Message-----
> From: Bitterblue Smith <[email protected]>
> Sent: Wednesday, January 10, 2024 9:47 PM
> To: [email protected]
> Cc: Ping-Ke Shih <[email protected]>; Larry Finger <[email protected]>
> Subject: [PATCH v2] wifi: rtlwifi: rtl_usb: Use sync register writes
>
> Currently rtl_usb performs register writes using the async
> usb_submit_urb() function. This appears to work fine for the RTL8192CU,
> but the RTL8192DU (soon to be supported by rtlwifi) has a problem:
> it transmits everything at the 1M rate in the 2.4 GHz band. (The 5 GHz
> band is still untested.)
>
> With this patch, rtl_usb performs the register writes using the
> synchronous usb_control_msg() function, and the RTL8192DU works
> normally. The RTL8192CU still works.
>
> The vendor drivers use the async writes in only one function,
> rtl8192du_trigger_gpio_0 / rtl8192cu_trigger_gpio_0, which probably
> doesn't even run in real life. They use sync writes everywhere else.
>
> Also, remove "sync" and "async" from the names of the members of
> struct rtl_io to avoid confusion:
>
> write{8,16,32}_async -> write{8,16,32}
> read{8,16,32}_sync -> read{8,16,32}
>
> Signed-off-by: Bitterblue Smith <[email protected]>

Acked-by: Ping-Ke Shih <[email protected]>

> ---
> v2:
> - Rename the members of struct rtl_io.
> ---
> drivers/net/wireless/realtek/rtlwifi/pci.c | 12 +-
> drivers/net/wireless/realtek/rtlwifi/usb.c | 163 ++++++--------------
> drivers/net/wireless/realtek/rtlwifi/wifi.h | 30 ++--
> 3 files changed, 66 insertions(+), 139 deletions(-)
>

[...]

> @@ -114,18 +51,15 @@ static int _usbctrl_vendorreq_sync_read(struct usb_device *udev, u8 request,
> } while (++vendorreq_times < MAX_USBCTRL_VENDORREQ_TIMES);
>
> if (status < 0 && count++ < 4)
> - pr_err("reg 0x%x, usbctrl_vendorreq TimeOut! status:0x%x value=0x%x\n",
> - value, status, *(u32 *)pdata);
> - return status;
> + pr_err("reg 0x%x, usbctrl_vendorreq TimeOut! status:0x%x value=0x%x reqtype=0x%x\n",

nit: Since you touch this, would you like to use dev_err() instead?

> + value, status, *(u32 *)pdata, reqtype);
> }
>

[...]

2024-01-11 14:55:02

by Bitterblue Smith

[permalink] [raw]
Subject: Re: [PATCH v2] wifi: rtlwifi: rtl_usb: Use sync register writes

On 11/01/2024 03:59, Ping-Ke Shih wrote:
>
>
>> -----Original Message-----
>> From: Bitterblue Smith <[email protected]>
>> Sent: Wednesday, January 10, 2024 9:47 PM
>> To: [email protected]
>> Cc: Ping-Ke Shih <[email protected]>; Larry Finger <[email protected]>
>> Subject: [PATCH v2] wifi: rtlwifi: rtl_usb: Use sync register writes
>>
>> Currently rtl_usb performs register writes using the async
>> usb_submit_urb() function. This appears to work fine for the RTL8192CU,
>> but the RTL8192DU (soon to be supported by rtlwifi) has a problem:
>> it transmits everything at the 1M rate in the 2.4 GHz band. (The 5 GHz
>> band is still untested.)
>>
>> With this patch, rtl_usb performs the register writes using the
>> synchronous usb_control_msg() function, and the RTL8192DU works
>> normally. The RTL8192CU still works.
>>
>> The vendor drivers use the async writes in only one function,
>> rtl8192du_trigger_gpio_0 / rtl8192cu_trigger_gpio_0, which probably
>> doesn't even run in real life. They use sync writes everywhere else.
>>
>> Also, remove "sync" and "async" from the names of the members of
>> struct rtl_io to avoid confusion:
>>
>> write{8,16,32}_async -> write{8,16,32}
>> read{8,16,32}_sync -> read{8,16,32}
>>
>> Signed-off-by: Bitterblue Smith <[email protected]>
>
> Acked-by: Ping-Ke Shih <[email protected]>
>
>> ---
>> v2:
>> - Rename the members of struct rtl_io.
>> ---
>> drivers/net/wireless/realtek/rtlwifi/pci.c | 12 +-
>> drivers/net/wireless/realtek/rtlwifi/usb.c | 163 ++++++--------------
>> drivers/net/wireless/realtek/rtlwifi/wifi.h | 30 ++--
>> 3 files changed, 66 insertions(+), 139 deletions(-)
>>
>
> [...]
>
>> @@ -114,18 +51,15 @@ static int _usbctrl_vendorreq_sync_read(struct usb_device *udev, u8 request,
>> } while (++vendorreq_times < MAX_USBCTRL_VENDORREQ_TIMES);
>>
>> if (status < 0 && count++ < 4)
>> - pr_err("reg 0x%x, usbctrl_vendorreq TimeOut! status:0x%x value=0x%x\n",
>> - value, status, *(u32 *)pdata);
>> - return status;
>> + pr_err("reg 0x%x, usbctrl_vendorreq TimeOut! status:0x%x value=0x%x reqtype=0x%x\n",
>
> nit: Since you touch this, would you like to use dev_err() instead?
>
Sure, I can do that.

>> + value, status, *(u32 *)pdata, reqtype);
>> }
>>
>
> [...]
>