Skip to contentSkip to conversation

wifi: rtlwifi: rtl8192cu: Fix TX aggregation

PATCH Superseded Bitterblue Smith 2 participants 4 messages Feb 8, 2024 – Feb 15, 2024 Realtek wireless driver (rtlwifi family)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 wifi: rtlwifi: rtl8192cu: Fix TX aggregation

rtl8192cu is checking rtl_mac.tids when deciding if it should enable aggregation. This is wrong because rtl_mac.tids is not initialised anywhere. Check rtl_sta_info.tids instead, which is initialised.

Also, when enabling aggregation also enable RTS. The vendor driver does this, my router does this. It seems like the thing to do.

Also also, it seems right to set the AMPDU density only when enabling aggregation.

Also also also, delete the unused member rtl_mac.tids and the unused macros RTL_AGG_ON and RTL_AGG_OFF.

Naturally, with working AMPDU the download/upload speeds are better.

Before: 59/32 Mbps.
After:  68/46 Mbps.
---
 .../wireless/realtek/rtlwifi/rtl8192cu/trx.c  | 27 ++++++++++---------
 .../wireless/realtek/rtlwifi/rtl8192cu/trx.h  |  2 --
 drivers/net/wireless/realtek/rtlwifi/wifi.h   |  3 ---
 3 files changed, 15 insertions(+), 17 deletions(-)
drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c +15 -12
index e5c81c1c63c0..cbbd1dab8af0 100644
@@ -475,8 +475,9 @@ void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw,
 	struct rtl_priv *rtlpriv = rtl_priv(hw);
 	struct rtl_mac *mac = rtl_mac(rtl_priv(hw));
 	struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
-	u8 *qc = ieee80211_get_qos_ctl(hdr);
-	u8 tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
+	struct rtl_sta_info *sta_entry;
+	u8 agg_state = RTL_AGG_STOP;
+	u8 ampdu_density = 0;
 	u16 seq_number;
 	__le16 fc = hdr->frame_control;
 	u8 rate_flag = info->control.rates[0].flags;
@@ -498,10 +499,20 @@ void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw,
 	set_tx_desc_tx_rate(txdesc, tcb_desc->hw_rate);
 	if (tcb_desc->use_shortgi || tcb_desc->use_shortpreamble)
 		set_tx_desc_data_shortgi(txdesc, 1);
-	if (mac->tids[tid].agg.agg_state == RTL_AGG_ON &&
-		    info->flags & IEEE80211_TX_CTL_AMPDU) {
+
+	if (sta) {
+		sta_entry = (struct rtl_sta_info *)sta->drv_priv;
+		agg_state = sta_entry->tids[ieee80211_get_tid(hdr)].agg.agg_state;
+		ampdu_density = sta->deflink.ht_cap.ampdu_density;
+	}
+
+	if (agg_state == RTL_AGG_OPERATIONAL &&
+	    info->flags & IEEE80211_TX_CTL_AMPDU) {
 		set_tx_desc_agg_enable(txdesc, 1);
 		set_tx_desc_max_agg_num(txdesc, 0x14);
+		set_tx_desc_ampdu_density(txdesc, ampdu_density);
+		tcb_desc->rts_enable = 1;
+		tcb_desc->rts_rate = DESC_RATE24M;
 	} else {
 		set_tx_desc_agg_break(txdesc, 1);
 	}
@@ -536,14 +547,6 @@ void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw,
 		set_tx_desc_data_bw(txdesc, 0);
 		set_tx_desc_data_sc(txdesc, 0);
 	}
-	rcu_read_lock();
-	sta = ieee80211_find_sta(mac->vif, mac->bssid);
-	if (sta) {
-		u8 ampdu_density = sta->deflink.ht_cap.ampdu_density;
-
-		set_tx_desc_ampdu_density(txdesc, ampdu_density);
-	}
-	rcu_read_unlock();
 	if (info->control.hw_key) {
 		struct ieee80211_key_conf *keyconf = info->control.hw_key;
 
drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.h +0 -2
index 5f81cab205cc..caeebe4480ff 100644
@@ -11,8 +11,6 @@
 #define RTL92C_SIZE_MAX_RX_BUFFER		15360   /* 8192 */
 #define RX_DRV_INFO_SIZE_UNIT			8
 
-#define RTL_AGG_ON				1
-
 enum usb_rx_agg_mode {
 	USB_RX_AGG_DISABLE,
 	USB_RX_AGG_DMA,
drivers/net/wireless/realtek/rtlwifi/wifi.h +0 -3
index 3821f6e31447..b65d54115eac 100644
@@ -1397,8 +1397,6 @@ struct rtl_phy {
 #define RTL_AGG_PROGRESS			1
 #define RTL_AGG_START				2
 #define RTL_AGG_OPERATIONAL			3
-#define RTL_AGG_OFF				0
-#define RTL_AGG_ON				1
 #define RTL_RX_AGG_START			1
 #define RTL_RX_AGG_STOP				0
 #define RTL_AGG_EMPTYING_HW_QUEUE_ADDBA		2
@@ -1473,7 +1471,6 @@ struct rtl_mac {
 	enum nl80211_iftype opmode;
 
 	/*Probe Beacon management */
-	struct rtl_tid_data tids[MAX_TID_COUNT];
 	enum rtl_link_state link_state;
 
 	int n_channels;
Signature · 1 line
2.43.0
maintainer
#1 lore raw
Quoted 43 lines · Bitterblue Smith

rtl8192cu is checking rtl_mac.tids when deciding if it should enable aggregation. This is wrong because rtl_mac.tids is not initialised anywhere. Check rtl_sta_info.tids instead, which is initialised.

Also, when enabling aggregation also enable RTS. The vendor driver does this, my router does this. It seems like the thing to do.

Also also, it seems right to set the AMPDU density only when enabling aggregation.

Also also also, delete the unused member rtl_mac.tids and the unused macros RTL_AGG_ON and RTL_AGG_OFF.

Naturally, with working AMPDU the download/upload speeds are better.

Before: 59/32 Mbps.
After:  68/46 Mbps.
---
 .../wireless/realtek/rtlwifi/rtl8192cu/trx.c  | 27 ++++++++++---------
 .../wireless/realtek/rtlwifi/rtl8192cu/trx.h  |  2 --
 drivers/net/wireless/realtek/rtlwifi/wifi.h   |  3 ---
 3 files changed, 15 insertions(+), 17 deletions(-)
drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c +3 -2
index e5c81c1c63c0..cbbd1dab8af0 100644
@@ -475,8 +475,9 @@ void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw,
        struct rtl_priv *rtlpriv = rtl_priv(hw);
        struct rtl_mac *mac = rtl_mac(rtl_priv(hw));
        struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
-       u8 *qc = ieee80211_get_qos_ctl(hdr);
-       u8 tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
+       struct rtl_sta_info *sta_entry;
+       u8 agg_state = RTL_AGG_STOP;
+       u8 ampdu_density = 0;
        u16 seq_number;
        __le16 fc = hdr->frame_control;
        u8 rate_flag = info->control.rates[0].flags;

drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw, · line 504

Quoted 8 lines
drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c +3 -2
@@ -498,10 +499,20 @@ void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw,
        set_tx_desc_tx_rate(txdesc, tcb_desc->hw_rate);
        if (tcb_desc->use_shortgi || tcb_desc->use_shortpreamble)
                set_tx_desc_data_shortgi(txdesc, 1);
-       if (mac->tids[tid].agg.agg_state == RTL_AGG_ON &&
-                   info->flags & IEEE80211_TX_CTL_AMPDU) {
+
+       if (sta) {
+               sta_entry = (struct rtl_sta_info *)sta->drv_priv;

nit: It would be better to be separated statement of ieee80211_get_tid(hdr)

tid = ieee80211_get_tid(hdr);
agg_state = sta_entry->tids[tid].agg.agg_state;

drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw, · line 512

Quoted 8 lines
drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c +6 -0
+               agg_state = sta_entry->tids[ieee80211_get_tid(hdr)].agg.agg_state;
+               ampdu_density = sta->deflink.ht_cap.ampdu_density;
+       }
+
+       if (agg_state == RTL_AGG_OPERATIONAL &&
+           info->flags & IEEE80211_TX_CTL_AMPDU) {
                set_tx_desc_agg_enable(txdesc, 1);
                set_tx_desc_max_agg_num(txdesc, 0x14);

Have you tried larger number of this?
As IEEE80211_MAX_AMPDU_BUF_HT is 0x40, set 0x3f here to get higher TX throughput if USB speed can afford.

You can reference an equation of rtw89:

	ampdu_num = (u8)((rtwsta->ampdu_params[tid].agg_num ?
			  rtwsta->ampdu_params[tid].agg_num :
			  4 << sta->deflink.ht_cap.ampdu_factor) - 1);

This is new for rtlwifi, so we can have this later.

[...]

#2 lore raw
Quoted 55 lines · Ping-Ke Shih
Quoted 43 lines · Bitterblue Smith

rtl8192cu is checking rtl_mac.tids when deciding if it should enable aggregation. This is wrong because rtl_mac.tids is not initialised anywhere. Check rtl_sta_info.tids instead, which is initialised.

Also, when enabling aggregation also enable RTS. The vendor driver does this, my router does this. It seems like the thing to do.

Also also, it seems right to set the AMPDU density only when enabling aggregation.

Also also also, delete the unused member rtl_mac.tids and the unused macros RTL_AGG_ON and RTL_AGG_OFF.

Naturally, with working AMPDU the download/upload speeds are better.

Before: 59/32 Mbps.
After:  68/46 Mbps.
---
 .../wireless/realtek/rtlwifi/rtl8192cu/trx.c  | 27 ++++++++++---------
 .../wireless/realtek/rtlwifi/rtl8192cu/trx.h  |  2 --
 drivers/net/wireless/realtek/rtlwifi/wifi.h   |  3 ---
 3 files changed, 15 insertions(+), 17 deletions(-)
drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c +3 -2
index e5c81c1c63c0..cbbd1dab8af0 100644
@@ -475,8 +475,9 @@ void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw,
        struct rtl_priv *rtlpriv = rtl_priv(hw);
        struct rtl_mac *mac = rtl_mac(rtl_priv(hw));
        struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
-       u8 *qc = ieee80211_get_qos_ctl(hdr);
-       u8 tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
+       struct rtl_sta_info *sta_entry;
+       u8 agg_state = RTL_AGG_STOP;
+       u8 ampdu_density = 0;
        u16 seq_number;
        __le16 fc = hdr->frame_control;
        u8 rate_flag = info->control.rates[0].flags;

drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw, · line 504

Quoted 8 lines
drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c +3 -2
@@ -498,10 +499,20 @@ void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw,
        set_tx_desc_tx_rate(txdesc, tcb_desc->hw_rate);
        if (tcb_desc->use_shortgi || tcb_desc->use_shortpreamble)
                set_tx_desc_data_shortgi(txdesc, 1);
-       if (mac->tids[tid].agg.agg_state == RTL_AGG_ON &&
-                   info->flags & IEEE80211_TX_CTL_AMPDU) {
+
+       if (sta) {
+               sta_entry = (struct rtl_sta_info *)sta->drv_priv;

nit: It would be better to be separated statement of ieee80211_get_tid(hdr)

Quoted 3 lines

tid = ieee80211_get_tid(hdr);
agg_state = sta_entry->tids[tid].agg.agg_state;

All right.

Quoted 9 lines

drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw, · line 512

Quoted 8 lines
drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c +5 -0
 > +               agg_state = sta_entry->tids[ieee80211_get_tid(hdr)].agg.agg_state;
+               ampdu_density = sta->deflink.ht_cap.ampdu_density;
+       }
+
+       if (agg_state == RTL_AGG_OPERATIONAL &&
+           info->flags & IEEE80211_TX_CTL_AMPDU) {
                set_tx_desc_agg_enable(txdesc, 1);
                set_tx_desc_max_agg_num(txdesc, 0x14);
Quoted 4 lines

Have you tried larger number of this?
As IEEE80211_MAX_AMPDU_BUF_HT is 0x40, set 0x3f here to get higher TX throughput if USB speed can afford.

With this patch, the upload speed is 46 Mbps. But that's just because my device is detected as 1T2R and it's not allowed to use rates above MCS7. When I fix that problem the upload speed is 82 Mbps in the same conditions, about 3 meters from the router.

If I then move the device just 60 cm away, the upload speed is 92 Mbps.

I tried 0x1f now (this field is only 5 bits). The upload speed doesn't get better. Maybe it gets a bit worse.

Quoted 13 lines

You can reference an equation of rtw89:

	ampdu_num = (u8)((rtwsta->ampdu_params[tid].agg_num ?
			  rtwsta->ampdu_params[tid].agg_num :
			  4 << sta->deflink.ht_cap.ampdu_factor) - 1);

This is new for rtlwifi, so we can have this later.

[...]

maintainer
#3 lore raw
Quoted 23 lines

-----Original Message-----
From: Bitterblue Smith <[email protected]>
Sent: Tuesday, February 13, 2024 10:33 PM
To: Ping-Ke Shih <[email protected]>; [email protected]
Cc: [email protected]
Subject: Re: [PATCH] wifi: rtlwifi: rtl8192cu: Fix TX aggregation

Quoted 7 lines · Ping-Ke Shih
Quoted 1 line · Bitterblue Smith
                set_tx_desc_max_agg_num(txdesc, 0x14);

Have you tried larger number of this?
As IEEE80211_MAX_AMPDU_BUF_HT is 0x40, set 0x3f here to get higher TX throughput if USB speed can afford.

With this patch, the upload speed is 46 Mbps. But that's just because my device is detected as 1T2R and it's not allowed to use rates above MCS7. When I fix that problem the upload speed is 82 Mbps in the same conditions, about 3 meters from the router.

Quoted 5 lines

If I then move the device just 60 cm away, the upload speed is 92 Mbps.

I tried 0x1f now (this field is only 5 bits). The upload speed doesn't get better. Maybe it gets a bit worse.

It could have more interference with larger aggregation number. Let's keep it as it was. Thanks for the try.

Ping-Ke