[PATCH 1/4] wifi: rtw88: Init RX burst length for 8822cu/8822bu/8821cu

[PATCH 1/4] wifi: rtw88: Init RX burst length for 8822cu/8822bu/8821cu

[Bitterblue Smith]

Init RX burst length according to the USB speed.

This is needed in order to make USB RX aggregation work.

Tested with RTL8811CU.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
I would mention in the commit message what BIT_DMA_BURST_CNT,
BIT_DMA_MODE, and BIT_DROP_DATA_EN are doing, but I don't know.
---
 drivers/net/wireless/realtek/rtw88/reg.h      |  8 +++++++
 drivers/net/wireless/realtek/rtw88/rtw8821c.c | 22 +++++++++++++++++++
 drivers/net/wireless/realtek/rtw88/rtw8822b.c | 22 +++++++++++++++++++
 drivers/net/wireless/realtek/rtw88/rtw8822c.c | 22 +++++++++++++++++++
 4 files changed, 74 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/reg.h b/drivers/net/wireless/realtek/rtw88/reg.h
index e7b24465f549..788e450d30d2 100644
--- a/drivers/net/wireless/realtek/rtw88/reg.h
+++ b/drivers/net/wireless/realtek/rtw88/reg.h
@@ -322,6 +322,12 @@
 #define REG_RXDMA_DPR		0x028C
 #define REG_RXDMA_MODE		0x0290
 #define BIT_DMA_MODE		BIT(1)
+#define BIT_DMA_BURST_CNT	GENMASK(3, 2)
+#define BIT_DMA_BURST_SIZE	GENMASK(5, 4)
+#define BIT_DMA_BURST_SIZE_64	2
+#define BIT_DMA_BURST_SIZE_512	1
+#define BIT_DMA_BURST_SIZE_1024	0
+
 #define REG_RXPKTNUM		0x02B0
 
 #define REG_INT_MIG		0x0304
@@ -703,6 +709,8 @@
 
 #define REG_IGN_GNTBT4	0x4160
 
+#define REG_USB_USBSTAT	0xFE11
+
 #define RF_MODE		0x00
 #define RF_MODOPT	0x01
 #define RF_WLINT	0x01
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
index 526e8de77b3e..55b6fe874710 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
@@ -204,6 +204,25 @@ static void rtw8821c_phy_set_param(struct rtw_dev *rtwdev)
 	rtw8821c_phy_bf_init(rtwdev);
 }
 
+static void rtw8821cu_init_burst_pkt_len(struct rtw_dev *rtwdev)
+{
+	u8 rxdma, burst_size;
+
+	rxdma = BIT_DMA_BURST_CNT | BIT_DMA_MODE;
+
+	if (rtw_read8(rtwdev, REG_SYS_CFG2 + 3) == 0x20)
+		burst_size = BIT_DMA_BURST_SIZE_1024;
+	else if ((rtw_read8(rtwdev, REG_USB_USBSTAT) & 0x3) == 0x1)
+		burst_size = BIT_DMA_BURST_SIZE_512;
+	else
+		burst_size = BIT_DMA_BURST_SIZE_64;
+
+	u8p_replace_bits(&rxdma, burst_size, BIT_DMA_BURST_SIZE);
+
+	rtw_write8(rtwdev, REG_RXDMA_MODE, rxdma);
+	rtw_write16_set(rtwdev, REG_TXDMA_OFFSET_CHK, BIT_DROP_DATA_EN);
+}
+
 static int rtw8821c_mac_init(struct rtw_dev *rtwdev)
 {
 	u32 value32;
@@ -261,6 +280,9 @@ static int rtw8821c_mac_init(struct rtw_dev *rtwdev)
 	rtw_write32(rtwdev, REG_WMAC_OPTION_FUNCTION + 8, WLAN_MAC_OPT_FUNC2);
 	rtw_write8(rtwdev, REG_WMAC_OPTION_FUNCTION + 4, WLAN_MAC_OPT_NORM_FUNC1);
 
+	if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB)
+		rtw8821cu_init_burst_pkt_len(rtwdev);
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822b.c b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
index 6edb17aea90e..0949eaa2b6c1 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822b.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
@@ -237,6 +237,25 @@ static void rtw8822b_phy_set_param(struct rtw_dev *rtwdev)
 #define WLAN_NAV_CFG		(WLAN_RDG_NAV | (WLAN_TXOP_NAV << 16))
 #define WLAN_RX_TSF_CFG		(WLAN_CCK_RX_TSF | (WLAN_OFDM_RX_TSF) << 8)
 
+static void rtw8822bu_init_burst_pkt_len(struct rtw_dev *rtwdev)
+{
+	u8 rxdma, burst_size;
+
+	rxdma = BIT_DMA_BURST_CNT | BIT_DMA_MODE;
+
+	if (rtw_read8(rtwdev, REG_SYS_CFG2 + 3) == 0x20)
+		burst_size = BIT_DMA_BURST_SIZE_1024;
+	else if ((rtw_read8(rtwdev, REG_USB_USBSTAT) & 0x3) == 0x1)
+		burst_size = BIT_DMA_BURST_SIZE_512;
+	else
+		burst_size = BIT_DMA_BURST_SIZE_64;
+
+	u8p_replace_bits(&rxdma, burst_size, BIT_DMA_BURST_SIZE);
+
+	rtw_write8(rtwdev, REG_RXDMA_MODE, rxdma);
+	rtw_write16_set(rtwdev, REG_TXDMA_OFFSET_CHK, BIT_DROP_DATA_EN);
+}
+
 static int rtw8822b_mac_init(struct rtw_dev *rtwdev)
 {
 	u32 value32;
@@ -284,6 +303,9 @@ static int rtw8822b_mac_init(struct rtw_dev *rtwdev)
 	rtw_write8_set(rtwdev, REG_SND_PTCL_CTRL,
 		       BIT_DIS_CHK_VHTSIGB_CRC);
 
+	if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB)
+		rtw8822bu_init_burst_pkt_len(rtwdev);
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.c b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
index e265a35184ab..2a90a879196b 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
@@ -2001,6 +2001,25 @@ static void rtw8822c_phy_set_param(struct rtw_dev *rtwdev)
 #define MAC_CLK_SPEED	80 /* 80M */
 #define EFUSE_PCB_INFO_OFFSET	0xCA
 
+static void rtw8822cu_init_burst_pkt_len(struct rtw_dev *rtwdev)
+{
+	u8 rxdma, burst_size;
+
+	rxdma = BIT_DMA_BURST_CNT | BIT_DMA_MODE;
+
+	if (rtw_read8(rtwdev, REG_SYS_CFG2 + 3) == 0x20)
+		burst_size = BIT_DMA_BURST_SIZE_1024;
+	else if ((rtw_read8(rtwdev, REG_USB_USBSTAT) & 0x3) == 0x1)
+		burst_size = BIT_DMA_BURST_SIZE_512;
+	else
+		burst_size = BIT_DMA_BURST_SIZE_64;
+
+	u8p_replace_bits(&rxdma, burst_size, BIT_DMA_BURST_SIZE);
+
+	rtw_write8(rtwdev, REG_RXDMA_MODE, rxdma);
+	rtw_write16_set(rtwdev, REG_TXDMA_OFFSET_CHK, BIT_DROP_DATA_EN);
+}
+
 static int rtw8822c_mac_init(struct rtw_dev *rtwdev)
 {
 	u8 value8;
@@ -2127,6 +2146,9 @@ static int rtw8822c_mac_init(struct rtw_dev *rtwdev)
 	/* Interrupt migration configuration */
 	rtw_write32(rtwdev, REG_INT_MIG, WLAN_MAC_INT_MIG_CFG);
 
+	if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB)
+		rtw8822cu_init_burst_pkt_len(rtwdev);
+
 	return 0;
 }
 
-- 
2.45.2

[PATCH 2/4] wifi: rtw88: usb: Update the RX stats after every frame

[Bitterblue Smith]

Update the number of received unicast data frames and bytes every time
a frame is received. This is what the PCI and SDIO drivers do.

This has an influence on the power saving, bluetooth coexistence, and
(in a future patch) the use of RX aggregation.

Tested with RTL8811CU and RTL8723DU.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
 drivers/net/wireless/realtek/rtw88/usb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index 251a5726f3ee..73948078068f 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -579,6 +579,7 @@ static void rtw_usb_rx_handler(struct work_struct *work)
 
 		skb_put(skb, pkt_stat.pkt_len);
 		skb_reserve(skb, pkt_offset);
+		rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
 		memcpy(skb->cb, &rx_status, sizeof(rx_status));
 		ieee80211_rx_irqsafe(rtwdev->hw, skb);
 	}
-- 
2.45.2

RE: [PATCH 2/4] wifi: rtw88: usb: Update the RX stats after every frame

[Ping-Ke Shih]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> Update the number of received unicast data frames and bytes every time
> a frame is received. This is what the PCI and SDIO drivers do.
> 
> This has an influence on the power saving, bluetooth coexistence, and
> (in a future patch) the use of RX aggregation.
> 
> Tested with RTL8811CU and RTL8723DU.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

[PATCH 3/4] wifi: rtw88: usb: Support RX aggregation

[Bitterblue Smith]

The chips can be configured to aggregate several frames into a single
USB transfer. Modify rtw_usb_rx_handler() to support this case.

RX aggregation improves the RX speed on certain ARM systems, like the
NanoPi NEO Core2.

Currently none of the chips are configured to aggregate frames.

Tested with RTL8811CU and RTL8723DU.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
 drivers/net/wireless/realtek/rtw88/usb.c | 57 +++++++++++++++---------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index 73948078068f..d61be1029a7b 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -546,11 +546,12 @@ static void rtw_usb_rx_handler(struct work_struct *work)
 	struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_work);
 	struct rtw_dev *rtwdev = rtwusb->rtwdev;
 	const struct rtw_chip_info *chip = rtwdev->chip;
-	struct rtw_rx_pkt_stat pkt_stat;
+	u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
 	struct ieee80211_rx_status rx_status;
+	u32 pkt_offset, next_pkt, urb_len;
+	struct rtw_rx_pkt_stat pkt_stat;
+	struct sk_buff *next_skb = NULL;
 	struct sk_buff *skb;
-	u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
-	u32 pkt_offset;
 	u8 *rx_desc;
 	int limit;
 
@@ -559,29 +560,44 @@ static void rtw_usb_rx_handler(struct work_struct *work)
 		if (!skb)
 			break;
 
-		rx_desc = skb->data;
-		chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
-					 &rx_status);
-		pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
-			     pkt_stat.shift;
-
-		if (pkt_stat.is_c2h) {
-			skb_put(skb, pkt_stat.pkt_len + pkt_offset);
-			rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
-			continue;
-		}
-
 		if (skb_queue_len(&rtwusb->rx_queue) >= RTW_USB_MAX_RXQ_LEN) {
 			dev_dbg_ratelimited(rtwdev->dev, "failed to get rx_queue, overflow\n");
 			dev_kfree_skb_any(skb);
 			continue;
 		}
 
-		skb_put(skb, pkt_stat.pkt_len);
-		skb_reserve(skb, pkt_offset);
-		rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
-		memcpy(skb->cb, &rx_status, sizeof(rx_status));
-		ieee80211_rx_irqsafe(rtwdev->hw, skb);
+		urb_len = skb->len;
+
+		do {
+			rx_desc = skb->data;
+			chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
+						 &rx_status);
+			pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
+				     pkt_stat.shift;
+
+			next_pkt = round_up(pkt_stat.pkt_len + pkt_offset, 8);
+
+			if (urb_len >= next_pkt + pkt_desc_sz)
+				next_skb = skb_clone(skb, GFP_KERNEL);
+
+			if (pkt_stat.is_c2h) {
+				skb_trim(skb, pkt_stat.pkt_len + pkt_offset);
+				rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
+			} else {
+				skb_pull(skb, pkt_offset);
+				skb_trim(skb, pkt_stat.pkt_len);
+				rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
+				memcpy(skb->cb, &rx_status, sizeof(rx_status));
+				ieee80211_rx_irqsafe(rtwdev->hw, skb);
+			}
+
+			skb = next_skb;
+			if (skb)
+				skb_pull(next_skb, next_pkt);
+
+			urb_len -= next_pkt;
+			next_skb = NULL;
+		} while (skb && urb_len >= pkt_desc_sz);
 	}
 }
 
@@ -625,6 +641,7 @@ static void rtw_usb_read_port_complete(struct urb *urb)
 			if (skb)
 				dev_kfree_skb_any(skb);
 		} else {
+			skb_put(skb, urb->actual_length);
 			skb_queue_tail(&rtwusb->rx_queue, skb);
 			queue_work(rtwusb->rxwq, &rtwusb->rx_work);
 		}
-- 
2.45.2

RE: [PATCH 3/4] wifi: rtw88: usb: Support RX aggregation

[Ping-Ke Shih]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> 
> The chips can be configured to aggregate several frames into a single
> USB transfer. Modify rtw_usb_rx_handler() to support this case.
> 
> RX aggregation improves the RX speed on certain ARM systems, like the
> NanoPi NEO Core2.
> 
> Currently none of the chips are configured to aggregate frames.
> 
> Tested with RTL8811CU and RTL8723DU.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtw88/usb.c | 57 +++++++++++++++---------
>  1 file changed, 37 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index 73948078068f..d61be1029a7b 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -546,11 +546,12 @@ static void rtw_usb_rx_handler(struct work_struct *work)
>         struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_work);
>         struct rtw_dev *rtwdev = rtwusb->rtwdev;
>         const struct rtw_chip_info *chip = rtwdev->chip;
> -       struct rtw_rx_pkt_stat pkt_stat;
> +       u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
>         struct ieee80211_rx_status rx_status;
> +       u32 pkt_offset, next_pkt, urb_len;
> +       struct rtw_rx_pkt_stat pkt_stat;
> +       struct sk_buff *next_skb = NULL;
>         struct sk_buff *skb;
> -       u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> -       u32 pkt_offset;
>         u8 *rx_desc;
>         int limit;
> 
> @@ -559,29 +560,44 @@ static void rtw_usb_rx_handler(struct work_struct *work)
>                 if (!skb)
>                         break;
> 
> -               rx_desc = skb->data;
> -               chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
> -                                        &rx_status);
> -               pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> -                            pkt_stat.shift;
> -
> -               if (pkt_stat.is_c2h) {
> -                       skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> -                       rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
> -                       continue;
> -               }
> -
>                 if (skb_queue_len(&rtwusb->rx_queue) >= RTW_USB_MAX_RXQ_LEN) {
>                         dev_dbg_ratelimited(rtwdev->dev, "failed to get rx_queue, overflow\n");
>                         dev_kfree_skb_any(skb);
>                         continue;
>                 }
> 
> -               skb_put(skb, pkt_stat.pkt_len);
> -               skb_reserve(skb, pkt_offset);
> -               rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> -               memcpy(skb->cb, &rx_status, sizeof(rx_status));
> -               ieee80211_rx_irqsafe(rtwdev->hw, skb);
> +               urb_len = skb->len;
> +
> +               do {
> +                       rx_desc = skb->data;
> +                       chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
> +                                                &rx_status);
> +                       pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> +                                    pkt_stat.shift;
> +
> +                       next_pkt = round_up(pkt_stat.pkt_len + pkt_offset, 8);
> +
> +                       if (urb_len >= next_pkt + pkt_desc_sz)
> +                               next_skb = skb_clone(skb, GFP_KERNEL);
> +
> +                       if (pkt_stat.is_c2h) {
> +                               skb_trim(skb, pkt_stat.pkt_len + pkt_offset);
> +                               rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
> +                       } else {
> +                               skb_pull(skb, pkt_offset);
> +                               skb_trim(skb, pkt_stat.pkt_len);
> +                               rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> +                               memcpy(skb->cb, &rx_status, sizeof(rx_status));
> +                               ieee80211_rx_irqsafe(rtwdev->hw, skb);
> +                       }
> +
> +                       skb = next_skb;
> +                       if (skb)
> +                               skb_pull(next_skb, next_pkt);
> +
> +                       urb_len -= next_pkt;

I would like a checking to prevent underflow caused by unexpected hardware data.

> +                       next_skb = NULL;
> +               } while (skb && urb_len >= pkt_desc_sz);
>         }
>  }
> 
> @@ -625,6 +641,7 @@ static void rtw_usb_read_port_complete(struct urb *urb)
>                         if (skb)
>                                 dev_kfree_skb_any(skb);
>                 } else {
> +                       skb_put(skb, urb->actual_length);
>                         skb_queue_tail(&rtwusb->rx_queue, skb);
>                         queue_work(rtwusb->rxwq, &rtwusb->rx_work);
>                 }
> --
> 2.45.2

Re: [PATCH 3/4] wifi: rtw88: usb: Support RX aggregation

[Bitterblue Smith]

On 30/07/2024 07:33, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>>
>> The chips can be configured to aggregate several frames into a single
>> USB transfer. Modify rtw_usb_rx_handler() to support this case.
>>
>> RX aggregation improves the RX speed on certain ARM systems, like the
>> NanoPi NEO Core2.
>>
>> Currently none of the chips are configured to aggregate frames.
>>
>> Tested with RTL8811CU and RTL8723DU.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>>  drivers/net/wireless/realtek/rtw88/usb.c | 57 +++++++++++++++---------
>>  1 file changed, 37 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
>> index 73948078068f..d61be1029a7b 100644
>> --- a/drivers/net/wireless/realtek/rtw88/usb.c
>> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
>> @@ -546,11 +546,12 @@ static void rtw_usb_rx_handler(struct work_struct *work)
>>         struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_work);
>>         struct rtw_dev *rtwdev = rtwusb->rtwdev;
>>         const struct rtw_chip_info *chip = rtwdev->chip;
>> -       struct rtw_rx_pkt_stat pkt_stat;
>> +       u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
>>         struct ieee80211_rx_status rx_status;
>> +       u32 pkt_offset, next_pkt, urb_len;
>> +       struct rtw_rx_pkt_stat pkt_stat;
>> +       struct sk_buff *next_skb = NULL;
>>         struct sk_buff *skb;
>> -       u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
>> -       u32 pkt_offset;
>>         u8 *rx_desc;
>>         int limit;
>>
>> @@ -559,29 +560,44 @@ static void rtw_usb_rx_handler(struct work_struct *work)
>>                 if (!skb)
>>                         break;
>>
>> -               rx_desc = skb->data;
>> -               chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
>> -                                        &rx_status);
>> -               pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
>> -                            pkt_stat.shift;
>> -
>> -               if (pkt_stat.is_c2h) {
>> -                       skb_put(skb, pkt_stat.pkt_len + pkt_offset);
>> -                       rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
>> -                       continue;
>> -               }
>> -
>>                 if (skb_queue_len(&rtwusb->rx_queue) >= RTW_USB_MAX_RXQ_LEN) {
>>                         dev_dbg_ratelimited(rtwdev->dev, "failed to get rx_queue, overflow\n");
>>                         dev_kfree_skb_any(skb);
>>                         continue;
>>                 }
>>
>> -               skb_put(skb, pkt_stat.pkt_len);
>> -               skb_reserve(skb, pkt_offset);
>> -               rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
>> -               memcpy(skb->cb, &rx_status, sizeof(rx_status));
>> -               ieee80211_rx_irqsafe(rtwdev->hw, skb);
>> +               urb_len = skb->len;
>> +
>> +               do {
>> +                       rx_desc = skb->data;
>> +                       chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
>> +                                                &rx_status);
>> +                       pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
>> +                                    pkt_stat.shift;
>> +
>> +                       next_pkt = round_up(pkt_stat.pkt_len + pkt_offset, 8);
>> +
>> +                       if (urb_len >= next_pkt + pkt_desc_sz)
>> +                               next_skb = skb_clone(skb, GFP_KERNEL);
>> +
>> +                       if (pkt_stat.is_c2h) {
>> +                               skb_trim(skb, pkt_stat.pkt_len + pkt_offset);
>> +                               rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
>> +                       } else {
>> +                               skb_pull(skb, pkt_offset);
>> +                               skb_trim(skb, pkt_stat.pkt_len);
>> +                               rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
>> +                               memcpy(skb->cb, &rx_status, sizeof(rx_status));
>> +                               ieee80211_rx_irqsafe(rtwdev->hw, skb);
>> +                       }
>> +
>> +                       skb = next_skb;
>> +                       if (skb)
>> +                               skb_pull(next_skb, next_pkt);
>> +
>> +                       urb_len -= next_pkt;
> 
> I would like a checking to prevent underflow caused by unexpected hardware data.
> 

There is a check above: if (urb_len >= next_pkt + pkt_desc_sz)
If this check fails, skb becomes NULL and the loop stops.

>> +                       next_skb = NULL;
>> +               } while (skb && urb_len >= pkt_desc_sz);>>         }
>>  }
>>
>> @@ -625,6 +641,7 @@ static void rtw_usb_read_port_complete(struct urb *urb)
>>                         if (skb)
>>                                 dev_kfree_skb_any(skb);
>>                 } else {
>> +                       skb_put(skb, urb->actual_length);
>>                         skb_queue_tail(&rtwusb->rx_queue, skb);
>>                         queue_work(rtwusb->rxwq, &rtwusb->rx_work);
>>                 }
>> --
>> 2.45.2
> 

Re: [PATCH 3/4] wifi: rtw88: usb: Support RX aggregation

[Sascha Hauer]

On Sun, Jul 28, 2024 at 10:42:32PM +0300, Bitterblue Smith wrote:
> The chips can be configured to aggregate several frames into a single
> USB transfer. Modify rtw_usb_rx_handler() to support this case.
> 
> RX aggregation improves the RX speed on certain ARM systems, like the
> NanoPi NEO Core2.
> 
> Currently none of the chips are configured to aggregate frames.
> 
> Tested with RTL8811CU and RTL8723DU.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtw88/usb.c | 57 +++++++++++++++---------
>  1 file changed, 37 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index 73948078068f..d61be1029a7b 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -546,11 +546,12 @@ static void rtw_usb_rx_handler(struct work_struct *work)
>  	struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_work);
>  	struct rtw_dev *rtwdev = rtwusb->rtwdev;
>  	const struct rtw_chip_info *chip = rtwdev->chip;
> -	struct rtw_rx_pkt_stat pkt_stat;
> +	u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
>  	struct ieee80211_rx_status rx_status;
> +	u32 pkt_offset, next_pkt, urb_len;
> +	struct rtw_rx_pkt_stat pkt_stat;
> +	struct sk_buff *next_skb = NULL;
>  	struct sk_buff *skb;
> -	u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> -	u32 pkt_offset;
>  	u8 *rx_desc;
>  	int limit;
>  
> @@ -559,29 +560,44 @@ static void rtw_usb_rx_handler(struct work_struct *work)
>  		if (!skb)
>  			break;
>  
> -		rx_desc = skb->data;
> -		chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
> -					 &rx_status);
> -		pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> -			     pkt_stat.shift;
> -
> -		if (pkt_stat.is_c2h) {
> -			skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> -			rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
> -			continue;
> -		}
> -
>  		if (skb_queue_len(&rtwusb->rx_queue) >= RTW_USB_MAX_RXQ_LEN) {
>  			dev_dbg_ratelimited(rtwdev->dev, "failed to get rx_queue, overflow\n");
>  			dev_kfree_skb_any(skb);
>  			continue;
>  		}
>  
> -		skb_put(skb, pkt_stat.pkt_len);
> -		skb_reserve(skb, pkt_offset);
> -		rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> -		memcpy(skb->cb, &rx_status, sizeof(rx_status));
> -		ieee80211_rx_irqsafe(rtwdev->hw, skb);
> +		urb_len = skb->len;
> +
> +		do {
> +			rx_desc = skb->data;
> +			chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
> +						 &rx_status);
> +			pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> +				     pkt_stat.shift;
> +
> +			next_pkt = round_up(pkt_stat.pkt_len + pkt_offset, 8);
> +
> +			if (urb_len >= next_pkt + pkt_desc_sz)
> +				next_skb = skb_clone(skb, GFP_KERNEL);

You could add a:
			else
				next_skb = NULL;

here and drop the next_skb = NULL from the end of the loop. No
functional change, but easier to read.

> +
> +			if (pkt_stat.is_c2h) {
> +				skb_trim(skb, pkt_stat.pkt_len + pkt_offset);
> +				rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
> +			} else {
> +				skb_pull(skb, pkt_offset);
> +				skb_trim(skb, pkt_stat.pkt_len);
> +				rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> +				memcpy(skb->cb, &rx_status, sizeof(rx_status));
> +				ieee80211_rx_irqsafe(rtwdev->hw, skb);
> +			}
> +
> +			skb = next_skb;
> +			if (skb)
> +				skb_pull(next_skb, next_pkt);

You could use skb instead of next_skb here. Both are the same, so no
functional change, just makes it a bit easier to read when you use the
same variable that you just tested for validity.

> +
> +			urb_len -= next_pkt;
> +			next_skb = NULL;
> +		} while (skb && urb_len >= pkt_desc_sz);

You can drop the urb_len >= pkt_desc_sz check. It will be exactly true
when skb is non NULL as well.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] wifi: rtw88: usb: Support RX aggregation

[Bitterblue Smith]

On 30/07/2024 09:39, Sascha Hauer wrote:
> On Sun, Jul 28, 2024 at 10:42:32PM +0300, Bitterblue Smith wrote:
>> The chips can be configured to aggregate several frames into a single
>> USB transfer. Modify rtw_usb_rx_handler() to support this case.
>>
>> RX aggregation improves the RX speed on certain ARM systems, like the
>> NanoPi NEO Core2.
>>
>> Currently none of the chips are configured to aggregate frames.
>>
>> Tested with RTL8811CU and RTL8723DU.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>>  drivers/net/wireless/realtek/rtw88/usb.c | 57 +++++++++++++++---------
>>  1 file changed, 37 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
>> index 73948078068f..d61be1029a7b 100644
>> --- a/drivers/net/wireless/realtek/rtw88/usb.c
>> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
>> @@ -546,11 +546,12 @@ static void rtw_usb_rx_handler(struct work_struct *work)
>>  	struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_work);
>>  	struct rtw_dev *rtwdev = rtwusb->rtwdev;
>>  	const struct rtw_chip_info *chip = rtwdev->chip;
>> -	struct rtw_rx_pkt_stat pkt_stat;
>> +	u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
>>  	struct ieee80211_rx_status rx_status;
>> +	u32 pkt_offset, next_pkt, urb_len;
>> +	struct rtw_rx_pkt_stat pkt_stat;
>> +	struct sk_buff *next_skb = NULL;
>>  	struct sk_buff *skb;
>> -	u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
>> -	u32 pkt_offset;
>>  	u8 *rx_desc;
>>  	int limit;
>>  
>> @@ -559,29 +560,44 @@ static void rtw_usb_rx_handler(struct work_struct *work)
>>  		if (!skb)
>>  			break;
>>  
>> -		rx_desc = skb->data;
>> -		chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
>> -					 &rx_status);
>> -		pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
>> -			     pkt_stat.shift;
>> -
>> -		if (pkt_stat.is_c2h) {
>> -			skb_put(skb, pkt_stat.pkt_len + pkt_offset);
>> -			rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
>> -			continue;
>> -		}
>> -
>>  		if (skb_queue_len(&rtwusb->rx_queue) >= RTW_USB_MAX_RXQ_LEN) {
>>  			dev_dbg_ratelimited(rtwdev->dev, "failed to get rx_queue, overflow\n");
>>  			dev_kfree_skb_any(skb);
>>  			continue;
>>  		}
>>  
>> -		skb_put(skb, pkt_stat.pkt_len);
>> -		skb_reserve(skb, pkt_offset);
>> -		rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
>> -		memcpy(skb->cb, &rx_status, sizeof(rx_status));
>> -		ieee80211_rx_irqsafe(rtwdev->hw, skb);
>> +		urb_len = skb->len;
>> +
>> +		do {
>> +			rx_desc = skb->data;
>> +			chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
>> +						 &rx_status);
>> +			pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
>> +				     pkt_stat.shift;
>> +
>> +			next_pkt = round_up(pkt_stat.pkt_len + pkt_offset, 8);
>> +
>> +			if (urb_len >= next_pkt + pkt_desc_sz)
>> +				next_skb = skb_clone(skb, GFP_KERNEL);
> 
> You could add a:
> 			else
> 				next_skb = NULL;
> 
> here and drop the next_skb = NULL from the end of the loop. No
> functional change, but easier to read.
> 
>> +
>> +			if (pkt_stat.is_c2h) {
>> +				skb_trim(skb, pkt_stat.pkt_len + pkt_offset);
>> +				rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
>> +			} else {
>> +				skb_pull(skb, pkt_offset);
>> +				skb_trim(skb, pkt_stat.pkt_len);
>> +				rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
>> +				memcpy(skb->cb, &rx_status, sizeof(rx_status));
>> +				ieee80211_rx_irqsafe(rtwdev->hw, skb);
>> +			}
>> +
>> +			skb = next_skb;
>> +			if (skb)
>> +				skb_pull(next_skb, next_pkt);
> 
> You could use skb instead of next_skb here. Both are the same, so no
> functional change, just makes it a bit easier to read when you use the
> same variable that you just tested for validity.
> 
>> +
>> +			urb_len -= next_pkt;
>> +			next_skb = NULL;
>> +		} while (skb && urb_len >= pkt_desc_sz);
> 
> You can drop the urb_len >= pkt_desc_sz check. It will be exactly true
> when skb is non NULL as well.
> 
> Sascha
> 

That all sounds correct, thank you.

[PATCH 4/4] wifi: rtw88: Enable USB RX aggregation for 8822c/8822b/8821c

[Bitterblue Smith]

Enable USB RX aggregation when there is at least 1 Mbps RX or TX
traffic, otherwise disable it.

USB RX aggregation improves the RX speed on certain ARM systems, like
the NanoPi NEO Core2. With RTL8811CU, before: 28 Mbps, after: 231 Mbps.

The official drivers for these chips use the same logic for SDIO, but
for some reason rtw88_sdio always enables RX aggregation, so this patch
only toggles aggregation for USB devices.

RTL8703B is likely not found in USB devices, and RTL8723DU doesn't like
aggregation.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
 drivers/net/wireless/realtek/rtw88/main.c     | 18 +++++++++++----
 drivers/net/wireless/realtek/rtw88/main.h     |  1 +
 drivers/net/wireless/realtek/rtw88/rtw8821c.c | 23 +++++++++++++++++++
 drivers/net/wireless/realtek/rtw88/rtw8822b.c | 23 +++++++++++++++++++
 drivers/net/wireless/realtek/rtw88/rtw8822c.c | 23 +++++++++++++++++++
 5 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 9d9d33a4a503..b3a089b4f707 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -210,8 +210,10 @@ static void rtw_watch_dog_work(struct work_struct *work)
 	struct rtw_dev *rtwdev = container_of(work, struct rtw_dev,
 					      watch_dog_work.work);
 	struct rtw_traffic_stats *stats = &rtwdev->stats;
+	const struct rtw_chip_info *chip = rtwdev->chip;
 	struct rtw_watch_dog_iter_data data = {};
 	bool busy_traffic = test_bit(RTW_FLAG_BUSY_TRAFFIC, rtwdev->flags);
+	u32 tx_unicast_shift, rx_unicast_shift;
 	bool ps_active;
 
 	mutex_lock(&rtwdev->mutex);
@@ -236,13 +238,21 @@ static void rtw_watch_dog_work(struct work_struct *work)
 	else
 		ps_active = false;
 
-	ewma_tp_add(&stats->tx_ewma_tp,
-		    (u32)(stats->tx_unicast >> RTW_TP_SHIFT));
-	ewma_tp_add(&stats->rx_ewma_tp,
-		    (u32)(stats->rx_unicast >> RTW_TP_SHIFT));
+	tx_unicast_shift = stats->tx_unicast >> RTW_TP_SHIFT;
+	rx_unicast_shift = stats->rx_unicast >> RTW_TP_SHIFT;
+
+	ewma_tp_add(&stats->tx_ewma_tp, tx_unicast_shift);
+	ewma_tp_add(&stats->rx_ewma_tp, rx_unicast_shift);
 	stats->tx_throughput = ewma_tp_read(&stats->tx_ewma_tp);
 	stats->rx_throughput = ewma_tp_read(&stats->rx_ewma_tp);
 
+	if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB && chip->ops->rx_aggregation) {
+		if (tx_unicast_shift < 1 && rx_unicast_shift < 1)
+			chip->ops->rx_aggregation(rtwdev, false);
+		else
+			chip->ops->rx_aggregation(rtwdev, true);
+	}
+
 	/* reset tx/rx statictics */
 	stats->tx_unicast = 0;
 	stats->rx_unicast = 0;
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
index 9d21637cf5d5..65bedd1668cc 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -888,6 +888,7 @@ struct rtw_chip_ops {
 	void (*fill_txdesc_checksum)(struct rtw_dev *rtwdev,
 				     struct rtw_tx_pkt_info *pkt_info,
 				     u8 *txdesc);
+	void (*rx_aggregation)(struct rtw_dev *rtwdev, bool enable);
 
 	/* for coex */
 	void (*coex_set_init)(struct rtw_dev *rtwdev);
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
index 55b6fe874710..3efdb41f22c5 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
@@ -1276,6 +1276,28 @@ static void rtw8821c_fill_txdesc_checksum(struct rtw_dev *rtwdev,
 	fill_txdesc_checksum_common(txdesc, 16);
 }
 
+static void rtw8821c_rx_aggregation(struct rtw_dev *rtwdev, bool enable)
+{
+	u8 size, timeout;
+	u16 val16;
+
+	rtw_write32_set(rtwdev, REG_RXDMA_AGG_PG_TH, BIT_EN_PRE_CALC);
+	rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN);
+	rtw_write8_clr(rtwdev, REG_RXDMA_AGG_PG_TH + 3, BIT(7));
+
+	if (enable) {
+		size = 0x5;
+		timeout = 0x20;
+	} else {
+		size = 0x0;
+		timeout = 0x1;
+	}
+	val16 = u16_encode_bits(size, BIT_RXDMA_AGG_PG_TH) |
+		u16_encode_bits(timeout, BIT_DMA_AGG_TO_V1);
+
+	rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16);
+}
+
 static struct rtw_pwr_seq_cmd trans_carddis_to_cardemu_8821c[] = {
 	{0x0086,
 	 RTW_PWR_CUT_ALL_MSK,
@@ -1724,6 +1746,7 @@ static struct rtw_chip_ops rtw8821c_ops = {
 	.set_gid_table		= rtw_bf_set_gid_table,
 	.cfg_csi_rate		= rtw_bf_cfg_csi_rate,
 	.fill_txdesc_checksum	= rtw8821c_fill_txdesc_checksum,
+	.rx_aggregation		= rtw8821c_rx_aggregation,
 
 	.coex_set_init		= rtw8821c_coex_cfg_init,
 	.coex_set_ant_switch	= rtw8821c_coex_cfg_ant_switch,
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822b.c b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
index 0949eaa2b6c1..52bcdf3cf043 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822b.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
@@ -1638,6 +1638,28 @@ static void rtw8822b_fill_txdesc_checksum(struct rtw_dev *rtwdev,
 	fill_txdesc_checksum_common(txdesc, words);
 }
 
+static void rtw8822b_rx_aggregation(struct rtw_dev *rtwdev, bool enable)
+{
+	u8 size, timeout;
+	u16 val16;
+
+	rtw_write32_set(rtwdev, REG_RXDMA_AGG_PG_TH, BIT_EN_PRE_CALC);
+	rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN);
+	rtw_write8_clr(rtwdev, REG_RXDMA_AGG_PG_TH + 3, BIT(7));
+
+	if (enable) {
+		size = 0x5;
+		timeout = 0x20;
+	} else {
+		size = 0x0;
+		timeout = 0x1;
+	}
+	val16 = u16_encode_bits(size, BIT_RXDMA_AGG_PG_TH) |
+		u16_encode_bits(timeout, BIT_DMA_AGG_TO_V1);
+
+	rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16);
+}
+
 static const struct rtw_pwr_seq_cmd trans_carddis_to_cardemu_8822b[] = {
 	{0x0086,
 	 RTW_PWR_CUT_ALL_MSK,
@@ -2214,6 +2236,7 @@ static struct rtw_chip_ops rtw8822b_ops = {
 	.adaptivity_init	= rtw8822b_adaptivity_init,
 	.adaptivity		= rtw8822b_adaptivity,
 	.fill_txdesc_checksum	= rtw8822b_fill_txdesc_checksum,
+	.rx_aggregation		= rtw8822b_rx_aggregation,
 
 	.coex_set_init		= rtw8822b_coex_cfg_init,
 	.coex_set_ant_switch	= rtw8822b_coex_cfg_ant_switch,
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.c b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
index 2a90a879196b..9d3ed8992133 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
@@ -4612,6 +4612,28 @@ static void rtw8822c_fill_txdesc_checksum(struct rtw_dev *rtwdev,
 	fill_txdesc_checksum_common(txdesc, words);
 }
 
+static void rtw8822c_rx_aggregation(struct rtw_dev *rtwdev, bool enable)
+{
+	u8 size, timeout;
+	u16 val16;
+
+	rtw_write32_set(rtwdev, REG_RXDMA_AGG_PG_TH, BIT_EN_PRE_CALC);
+	rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN);
+	rtw_write8_clr(rtwdev, REG_RXDMA_AGG_PG_TH + 3, BIT(7));
+
+	if (enable) {
+		size = 0x5;
+		timeout = 0x20;
+	} else {
+		size = 0x0;
+		timeout = 0x1;
+	}
+	val16 = u16_encode_bits(size, BIT_RXDMA_AGG_PG_TH) |
+		u16_encode_bits(timeout, BIT_DMA_AGG_TO_V1);
+
+	rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16);
+}
+
 static const struct rtw_pwr_seq_cmd trans_carddis_to_cardemu_8822c[] = {
 	{0x0086,
 	 RTW_PWR_CUT_ALL_MSK,
@@ -5036,6 +5058,7 @@ static struct rtw_chip_ops rtw8822c_ops = {
 	.config_tx_path		= rtw8822c_config_tx_path,
 	.config_txrx_mode	= rtw8822c_config_trx_mode,
 	.fill_txdesc_checksum	= rtw8822c_fill_txdesc_checksum,
+	.rx_aggregation		= rtw8822c_rx_aggregation,
 
 	.coex_set_init		= rtw8822c_coex_cfg_init,
 	.coex_set_ant_switch	= NULL,
-- 
2.45.2

RE: [PATCH 4/4] wifi: rtw88: Enable USB RX aggregation for 8822c/8822b/8821c

[Ping-Ke Shih]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> 
> Enable USB RX aggregation when there is at least 1 Mbps RX or TX
> traffic, otherwise disable it.
> 
> USB RX aggregation improves the RX speed on certain ARM systems, like
> the NanoPi NEO Core2. With RTL8811CU, before: 28 Mbps, after: 231 Mbps.
> 
> The official drivers for these chips use the same logic for SDIO, but
> for some reason rtw88_sdio always enables RX aggregation, so this patch
> only toggles aggregation for USB devices.
> 
> RTL8703B is likely not found in USB devices, and RTL8723DU doesn't like
> aggregation.

Please explicitly set .rx_aggregation = NULL to these two chips, so
we know these two chips don't have this feature.

> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtw88/main.c     | 18 +++++++++++----
>  drivers/net/wireless/realtek/rtw88/main.h     |  1 +
>  drivers/net/wireless/realtek/rtw88/rtw8821c.c | 23 +++++++++++++++++++
>  drivers/net/wireless/realtek/rtw88/rtw8822b.c | 23 +++++++++++++++++++
>  drivers/net/wireless/realtek/rtw88/rtw8822c.c | 23 +++++++++++++++++++
>  5 files changed, 84 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index 9d9d33a4a503..b3a089b4f707 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -210,8 +210,10 @@ static void rtw_watch_dog_work(struct work_struct *work)
>         struct rtw_dev *rtwdev = container_of(work, struct rtw_dev,
>                                               watch_dog_work.work);
>         struct rtw_traffic_stats *stats = &rtwdev->stats;
> +       const struct rtw_chip_info *chip = rtwdev->chip;
>         struct rtw_watch_dog_iter_data data = {};
>         bool busy_traffic = test_bit(RTW_FLAG_BUSY_TRAFFIC, rtwdev->flags);
> +       u32 tx_unicast_shift, rx_unicast_shift;
>         bool ps_active;
> 
>         mutex_lock(&rtwdev->mutex);
> @@ -236,13 +238,21 @@ static void rtw_watch_dog_work(struct work_struct *work)
>         else
>                 ps_active = false;
> 
> -       ewma_tp_add(&stats->tx_ewma_tp,
> -                   (u32)(stats->tx_unicast >> RTW_TP_SHIFT));
> -       ewma_tp_add(&stats->rx_ewma_tp,
> -                   (u32)(stats->rx_unicast >> RTW_TP_SHIFT));
> +       tx_unicast_shift = stats->tx_unicast >> RTW_TP_SHIFT;
> +       rx_unicast_shift = stats->rx_unicast >> RTW_TP_SHIFT;

{tx,rx}_unicast_mbps because 'shift' is to get Mbps.

> +
> +       ewma_tp_add(&stats->tx_ewma_tp, tx_unicast_shift);
> +       ewma_tp_add(&stats->rx_ewma_tp, rx_unicast_shift);
>         stats->tx_throughput = ewma_tp_read(&stats->tx_ewma_tp);
>         stats->rx_throughput = ewma_tp_read(&stats->rx_ewma_tp);
> 
> +       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB && chip->ops->rx_aggregation) {
> +               if (tx_unicast_shift < 1 && rx_unicast_shift < 1)
> +                       chip->ops->rx_aggregation(rtwdev, false);
> +               else
> +                       chip->ops->rx_aggregation(rtwdev, true);
> +       }

Move this chunk to a function with arguments {tx,rx}_unicast_mbps.
The function name might be something like rtw_dynamic_usb_rx_aggregation().

> +
>         /* reset tx/rx statictics */
>         stats->tx_unicast = 0;
>         stats->rx_unicast = 0;
> diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
> index 9d21637cf5d5..65bedd1668cc 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.h
> +++ b/drivers/net/wireless/realtek/rtw88/main.h
> @@ -888,6 +888,7 @@ struct rtw_chip_ops {
>         void (*fill_txdesc_checksum)(struct rtw_dev *rtwdev,
>                                      struct rtw_tx_pkt_info *pkt_info,
>                                      u8 *txdesc);
> +       void (*rx_aggregation)(struct rtw_dev *rtwdev, bool enable);
> 
>         /* for coex */
>         void (*coex_set_init)(struct rtw_dev *rtwdev);
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> index 55b6fe874710..3efdb41f22c5 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> @@ -1276,6 +1276,28 @@ static void rtw8821c_fill_txdesc_checksum(struct rtw_dev *rtwdev,
>         fill_txdesc_checksum_common(txdesc, 16);
>  }
> 
> +static void rtw8821c_rx_aggregation(struct rtw_dev *rtwdev, bool enable)
> +{
> +       u8 size, timeout;
> +       u16 val16;
> +
> +       rtw_write32_set(rtwdev, REG_RXDMA_AGG_PG_TH, BIT_EN_PRE_CALC);
> +       rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN);
> +       rtw_write8_clr(rtwdev, REG_RXDMA_AGG_PG_TH + 3, BIT(7));
> +
> +       if (enable) {
> +               size = 0x5;
> +               timeout = 0x20;
> +       } else {
> +               size = 0x0;
> +               timeout = 0x1;
> +       }
> +       val16 = u16_encode_bits(size, BIT_RXDMA_AGG_PG_TH) |
> +               u16_encode_bits(timeout, BIT_DMA_AGG_TO_V1);
> +
> +       rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16);
> +}
> +

All use the same settings. Move this to rtw_usb_rx_aggregation() called by
rtw_dynamic_usb_rx_aggregation().

Re: [PATCH 4/4] wifi: rtw88: Enable USB RX aggregation for 8822c/8822b/8821c

[Bitterblue Smith]

On 30/07/2024 08:47, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>>
>> Enable USB RX aggregation when there is at least 1 Mbps RX or TX
>> traffic, otherwise disable it.
>>
>> USB RX aggregation improves the RX speed on certain ARM systems, like
>> the NanoPi NEO Core2. With RTL8811CU, before: 28 Mbps, after: 231 Mbps.
>>
>> The official drivers for these chips use the same logic for SDIO, but
>> for some reason rtw88_sdio always enables RX aggregation, so this patch
>> only toggles aggregation for USB devices.
>>
>> RTL8703B is likely not found in USB devices, and RTL8723DU doesn't like
>> aggregation.
> 
> Please explicitly set .rx_aggregation = NULL to these two chips, so
> we know these two chips don't have this feature.
> 
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>>  drivers/net/wireless/realtek/rtw88/main.c     | 18 +++++++++++----
>>  drivers/net/wireless/realtek/rtw88/main.h     |  1 +
>>  drivers/net/wireless/realtek/rtw88/rtw8821c.c | 23 +++++++++++++++++++
>>  drivers/net/wireless/realtek/rtw88/rtw8822b.c | 23 +++++++++++++++++++
>>  drivers/net/wireless/realtek/rtw88/rtw8822c.c | 23 +++++++++++++++++++
>>  5 files changed, 84 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
>> index 9d9d33a4a503..b3a089b4f707 100644
>> --- a/drivers/net/wireless/realtek/rtw88/main.c
>> +++ b/drivers/net/wireless/realtek/rtw88/main.c
>> @@ -210,8 +210,10 @@ static void rtw_watch_dog_work(struct work_struct *work)
>>         struct rtw_dev *rtwdev = container_of(work, struct rtw_dev,
>>                                               watch_dog_work.work);
>>         struct rtw_traffic_stats *stats = &rtwdev->stats;
>> +       const struct rtw_chip_info *chip = rtwdev->chip;
>>         struct rtw_watch_dog_iter_data data = {};
>>         bool busy_traffic = test_bit(RTW_FLAG_BUSY_TRAFFIC, rtwdev->flags);
>> +       u32 tx_unicast_shift, rx_unicast_shift;
>>         bool ps_active;
>>
>>         mutex_lock(&rtwdev->mutex);
>> @@ -236,13 +238,21 @@ static void rtw_watch_dog_work(struct work_struct *work)
>>         else
>>                 ps_active = false;
>>
>> -       ewma_tp_add(&stats->tx_ewma_tp,
>> -                   (u32)(stats->tx_unicast >> RTW_TP_SHIFT));
>> -       ewma_tp_add(&stats->rx_ewma_tp,
>> -                   (u32)(stats->rx_unicast >> RTW_TP_SHIFT));
>> +       tx_unicast_shift = stats->tx_unicast >> RTW_TP_SHIFT;
>> +       rx_unicast_shift = stats->rx_unicast >> RTW_TP_SHIFT;
> 
> {tx,rx}_unicast_mbps because 'shift' is to get Mbps.
> 
>> +
>> +       ewma_tp_add(&stats->tx_ewma_tp, tx_unicast_shift);
>> +       ewma_tp_add(&stats->rx_ewma_tp, rx_unicast_shift);
>>         stats->tx_throughput = ewma_tp_read(&stats->tx_ewma_tp);
>>         stats->rx_throughput = ewma_tp_read(&stats->rx_ewma_tp);
>>
>> +       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB && chip->ops->rx_aggregation) {
>> +               if (tx_unicast_shift < 1 && rx_unicast_shift < 1)
>> +                       chip->ops->rx_aggregation(rtwdev, false);
>> +               else
>> +                       chip->ops->rx_aggregation(rtwdev, true);
>> +       }
> 
> Move this chunk to a function with arguments {tx,rx}_unicast_mbps.
> The function name might be something like rtw_dynamic_usb_rx_aggregation().
> 
>> +
>>         /* reset tx/rx statictics */
>>         stats->tx_unicast = 0;
>>         stats->rx_unicast = 0;
>> diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
>> index 9d21637cf5d5..65bedd1668cc 100644
>> --- a/drivers/net/wireless/realtek/rtw88/main.h
>> +++ b/drivers/net/wireless/realtek/rtw88/main.h
>> @@ -888,6 +888,7 @@ struct rtw_chip_ops {
>>         void (*fill_txdesc_checksum)(struct rtw_dev *rtwdev,
>>                                      struct rtw_tx_pkt_info *pkt_info,
>>                                      u8 *txdesc);
>> +       void (*rx_aggregation)(struct rtw_dev *rtwdev, bool enable);
>>
>>         /* for coex */
>>         void (*coex_set_init)(struct rtw_dev *rtwdev);
>> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
>> b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
>> index 55b6fe874710..3efdb41f22c5 100644
>> --- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
>> +++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
>> @@ -1276,6 +1276,28 @@ static void rtw8821c_fill_txdesc_checksum(struct rtw_dev *rtwdev,
>>         fill_txdesc_checksum_common(txdesc, 16);
>>  }
>>
>> +static void rtw8821c_rx_aggregation(struct rtw_dev *rtwdev, bool enable)
>> +{
>> +       u8 size, timeout;
>> +       u16 val16;
>> +
>> +       rtw_write32_set(rtwdev, REG_RXDMA_AGG_PG_TH, BIT_EN_PRE_CALC);
>> +       rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN);
>> +       rtw_write8_clr(rtwdev, REG_RXDMA_AGG_PG_TH + 3, BIT(7));
>> +
>> +       if (enable) {
>> +               size = 0x5;
>> +               timeout = 0x20;
>> +       } else {
>> +               size = 0x0;
>> +               timeout = 0x1;
>> +       }
>> +       val16 = u16_encode_bits(size, BIT_RXDMA_AGG_PG_TH) |
>> +               u16_encode_bits(timeout, BIT_DMA_AGG_TO_V1);
>> +
>> +       rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16);
>> +}
>> +
> 
> All use the same settings. Move this to rtw_usb_rx_aggregation() called by
> rtw_dynamic_usb_rx_aggregation().
> 

These three chips use the same settings. RTL8821AU/RTL8812AU
will be a bit different:

static void rtw8821au_rx_aggregation(struct rtw_dev *rtwdev, bool enable)
{
	struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
	u8 rxagg_usb_size, rxagg_usb_timeout;
	u16 val16;

	if (rtwusb->udev->speed == USB_SPEED_SUPER) {
		rxagg_usb_size = 0x7;
		rxagg_usb_timeout = 0x1a;
	} else {
		rxagg_usb_size = 0x5;
		rxagg_usb_timeout = 0x20;
	}

	if (!enable) {
		rxagg_usb_size = 0x0;
		rxagg_usb_timeout = 0x1;
	}

	val16 = (rxagg_usb_timeout << 8) | rxagg_usb_size;
	rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16);

	rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN);
}

And RTL8814AU will be more different.

I suppose they can be moved to the same place, like the burst
stuff. Maybe struct rtw_hci_ops should have a new member called
rx_aggregation, in case someone decides to make the SDIO driver
use dynamic RX aggregation as well?

RE: [PATCH 4/4] wifi: rtw88: Enable USB RX aggregation for 8822c/8822b/8821c

[Ping-Ke Shih]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> I suppose they can be moved to the same place, like the burst
> stuff. Maybe struct rtw_hci_ops should have a new member called
> rx_aggregation, in case someone decides to make the SDIO driver
> use dynamic RX aggregation as well?

Not sure. We can just leave NULL or dummy function for SDIO temporarily. 

RE: [PATCH 1/4] wifi: rtw88: Init RX burst length for 8822cu/8822bu/8821cu

[Ping-Ke Shih]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> Init RX burst length according to the USB speed.
> 
> This is needed in order to make USB RX aggregation work.
> 
> Tested with RTL8811CU.

Having a throughput after this change would be better.

> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
> I would mention in the commit message what BIT_DMA_BURST_CNT,
> BIT_DMA_MODE, and BIT_DROP_DATA_EN are doing, but I don't know.

That will be helpful to other developers. Please put them in second paragraph. 

[...]

> +static void rtw8821cu_init_burst_pkt_len(struct rtw_dev *rtwdev)
> +{
> +       u8 rxdma, burst_size;
> +
> +       rxdma = BIT_DMA_BURST_CNT | BIT_DMA_MODE;
> +
> +       if (rtw_read8(rtwdev, REG_SYS_CFG2 + 3) == 0x20)
> +               burst_size = BIT_DMA_BURST_SIZE_1024;
> +       else if ((rtw_read8(rtwdev, REG_USB_USBSTAT) & 0x3) == 0x1)
> +               burst_size = BIT_DMA_BURST_SIZE_512;
> +       else
> +               burst_size = BIT_DMA_BURST_SIZE_64;
> +
> +       u8p_replace_bits(&rxdma, burst_size, BIT_DMA_BURST_SIZE);
> +
> +       rtw_write8(rtwdev, REG_RXDMA_MODE, rxdma);
> +       rtw_write16_set(rtwdev, REG_TXDMA_OFFSET_CHK, BIT_DROP_DATA_EN);
> +}
> +

All use the same setup. 
Can we move it to usb.c? Maybe rtw_usb_interface_cfg() is a good place?
(still exclude untested chips.)

Re: [PATCH 1/4] wifi: rtw88: Init RX burst length for 8822cu/8822bu/8821cu

[Bitterblue Smith]

On 30/07/2024 06:57, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>> Init RX burst length according to the USB speed.
>>
>> This is needed in order to make USB RX aggregation work.
>>
>> Tested with RTL8811CU.
> 
> Having a throughput after this change would be better.
> 
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>> I would mention in the commit message what BIT_DMA_BURST_CNT,
>> BIT_DMA_MODE, and BIT_DROP_DATA_EN are doing, but I don't know.
> 
> That will be helpful to other developers. Please put them in second paragraph. 
> 
> [...]
> 
>> +static void rtw8821cu_init_burst_pkt_len(struct rtw_dev *rtwdev)
>> +{
>> +       u8 rxdma, burst_size;
>> +
>> +       rxdma = BIT_DMA_BURST_CNT | BIT_DMA_MODE;
>> +
>> +       if (rtw_read8(rtwdev, REG_SYS_CFG2 + 3) == 0x20)
>> +               burst_size = BIT_DMA_BURST_SIZE_1024;
>> +       else if ((rtw_read8(rtwdev, REG_USB_USBSTAT) & 0x3) == 0x1)
>> +               burst_size = BIT_DMA_BURST_SIZE_512;
>> +       else
>> +               burst_size = BIT_DMA_BURST_SIZE_64;
>> +
>> +       u8p_replace_bits(&rxdma, burst_size, BIT_DMA_BURST_SIZE);
>> +
>> +       rtw_write8(rtwdev, REG_RXDMA_MODE, rxdma);
>> +       rtw_write16_set(rtwdev, REG_TXDMA_OFFSET_CHK, BIT_DROP_DATA_EN);
>> +}
>> +
> 
> All use the same setup. 
> Can we move it to usb.c? Maybe rtw_usb_interface_cfg() is a good place?
> (still exclude untested chips.)
> 

rtw_usb_interface_cfg() is a good place. I will move it there.
The other chips will complicate it a bit, but that's okay.

RTL8723DU doesn't check for USB 3, of course:

	if (rtwusb->udev->speed == USB_SPEED_HIGH)
		burst_size = BIT_DMA_BURST_SIZE_512;
	else
		burst_size = BIT_DMA_BURST_SIZE_64;

RTL8821AU/RTL8812AU:

	if (chip->id == RTW_CHIP_TYPE_8821A)
		speedvalue = BIT(7);
	else
		speedvalue = rtw_read8(rtwdev, REG_SYS_CFG2 + 3);

	if (speedvalue & BIT(7)) { /* USB 2/1.1 Mode */
		temp = rtw_read8(rtwdev, 0xfe17);
		if (((temp >> 4) & 0x03) == 0)
			burst_size = BIT_DMA_BURST_SIZE_512;
		else
			burst_size = BIT_DMA_BURST_SIZE_64;
	} else { /* USB3 Mode */
		burst_size = BIT_DMA_BURST_SIZE_1024;
	}

RTL8814AU:

	speedvalue = rtw_read8(rtwdev, REG_SYS_CFG2 + 3);

	if (speedvalue & BIT(7)) { /* USB 2/1.1 Mode */
		if (rtwusb->udev->speed == USB_SPEED_HIGH)
			burst_size = BIT_DMA_BURST_SIZE_512;
		else
			burst_size = BIT_DMA_BURST_SIZE_64;
	} else { /* USB3 Mode */
		burst_size = BIT_DMA_BURST_SIZE_1024;
	}

I don't understand why we can't just check rtwusb->udev->speed
instead of reading various registers. Then they could all use
the same code.

(By the way, RTL8821AU/RTL8812AU is ready now. I will send
the patches after this patch set is sorted out. There are about
16 smaller patches to prepare things, and then the new driver.)

RE: [PATCH 1/4] wifi: rtw88: Init RX burst length for 8822cu/8822bu/8821cu

[Ping-Ke Shih]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> On 30/07/2024 06:57, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >> Init RX burst length according to the USB speed.
> >>
> >> This is needed in order to make USB RX aggregation work.
> >>
> >> Tested with RTL8811CU.
> >
> > Having a throughput after this change would be better.
> >
> >>
> >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> >> ---
> >> I would mention in the commit message what BIT_DMA_BURST_CNT,
> >> BIT_DMA_MODE, and BIT_DROP_DATA_EN are doing, but I don't know.
> >
> > That will be helpful to other developers. Please put them in second paragraph.
> >
> > [...]
> >
> >> +static void rtw8821cu_init_burst_pkt_len(struct rtw_dev *rtwdev)
> >> +{
> >> +       u8 rxdma, burst_size;
> >> +
> >> +       rxdma = BIT_DMA_BURST_CNT | BIT_DMA_MODE;
> >> +
> >> +       if (rtw_read8(rtwdev, REG_SYS_CFG2 + 3) == 0x20)
> >> +               burst_size = BIT_DMA_BURST_SIZE_1024;
> >> +       else if ((rtw_read8(rtwdev, REG_USB_USBSTAT) & 0x3) == 0x1)
> >> +               burst_size = BIT_DMA_BURST_SIZE_512;
> >> +       else
> >> +               burst_size = BIT_DMA_BURST_SIZE_64;
> >> +
> >> +       u8p_replace_bits(&rxdma, burst_size, BIT_DMA_BURST_SIZE);
> >> +
> >> +       rtw_write8(rtwdev, REG_RXDMA_MODE, rxdma);
> >> +       rtw_write16_set(rtwdev, REG_TXDMA_OFFSET_CHK, BIT_DROP_DATA_EN);
> >> +}
> >> +
> >
> > All use the same setup.
> > Can we move it to usb.c? Maybe rtw_usb_interface_cfg() is a good place?
> > (still exclude untested chips.)
> >
> 
> rtw_usb_interface_cfg() is a good place. I will move it there.
> The other chips will complicate it a bit, but that's okay.

Maybe we can have init_burst_pkt_len_{v1, v2, v3, ...}, but...

> 
> I don't understand why we can't just check rtwusb->udev->speed
> instead of reading various registers. Then they could all use
> the same code.

It seems to be better to just use rtwusb->udev->speed. 
I ask USB team but no clear answer, but they prefer rtwusb->udev->speed as well.

> 
> (By the way, RTL8821AU/RTL8812AU is ready now. I will send
> the patches after this patch set is sorted out. There are about
> 16 smaller patches to prepare things, and then the new driver.)

Nice work!