[RFC PATCH 0/2] Improve TX performance of RTL8723BU on rtl8xxxu driver

[RFC PATCH 0/2] Improve TX performance of RTL8723BU on rtl8xxxu driver

[Chris Chiu]

We have 3 laptops which connect the wifi by the same RTL8723BU.
The PCI VID/PID of the wifi chip is 10EC:B720 which is supported.
They have the same problem with the in-kernel rtl8xxxu driver, the
iperf (as a client to an ethernet-connected server) gets ~1Mbps.
Nevertheless, the signal strength is reported as around -40dBm,
which is quite good. From the wireshark capture, the tx rate for each
data and qos data packet is only 1Mbps. Compare to the driver from
https://github.com/lwfinger/rtl8723bu, the same iperf test gets ~12
Mbps or more. The signal strength is reported similarly around
-40dBm. That's why we want to find out the cause and improve.

After reading the source code of the rtl8xxxu driver and Larry's, the
major difference is that Larry's driver has a watchdog which will keep
monitoring the signal quality and updating the rate mask just like the
rtl8xxxu_gen2_update_rate_mask() does if signal quality changes.
And this kind of watchdog also exists in rtlwifi driver of some specific
chips, ex rtl8192ee, rtl8188ee, rtl8723ae, rtl8821ae...etc. They have
the same member function named dm_watchdog and will invoke the
corresponding dm_refresh_rate_adaptive_mask to adjust the tx rate
mask.

Thus I created 2 commits and try to do the same thing on rtl8xxxu.
After applying these commits, the tx rate of each data and qos data
packet will be 39Mbps (MCS4) with the 0xf00000 as its tx rate mask.
The 20th bit ~ 23th bit means MCS4 to MCS7. It means that the firmware
still picks the lowest rate from the rate mask and explains why the
tx rate of data and qos data is always lowest 1Mbps because the default
rate mask passed is almost 0xFFFFFFF ranges from the basic CCK rate, 
OFDM rate, and MCS rate. However, with Larry's driver, the tx rate 
observed from wireshark under the same condition is almost 65Mbps or
72Mbps.

I believe the firmware of RTL8723BU may need fix. And I think we
can still bring in the dm_watchdog as rtlwifi to improve from the
driver side. Please leave precious comments for my commits and
suggest what I can do better. Or suggest if there's any better idea
to fix this. Thanks.

Chris Chiu (2):
  rtl8xxxu: Add rate adaptive related data
  rtl8xxxu: Add watchdog to update rate mask by signal strength

 .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  53 ++++++
 .../realtek/rtl8xxxu/rtl8xxxu_8723b.c         | 151 ++++++++++++++++++
 .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c |  83 +++++++++-
 3 files changed, 286 insertions(+), 1 deletion(-)

-- 
2.21.0

[RFC PATCH 1/2] rtl8xxxu: Add rate adaptive related data

[Chris Chiu]

Add wireless mode, signal strength level, and rate table index
to tell the firmware that we need to adjust the tx rate bitmap
accordingly.
---
 .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  | 45 +++++++++++++++++++
 .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 45 ++++++++++++++++++-
 2 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
index 8828baf26e7b..771f58aa7cae 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
@@ -1195,6 +1195,50 @@ struct rtl8723bu_c2h {
 
 struct rtl8xxxu_fileops;
 
+/*mlme related.*/
+enum wireless_mode {
+	WIRELESS_MODE_UNKNOWN = 0,
+	//Sub-Element
+	WIRELESS_MODE_B = BIT(0), // tx: cck only , rx: cck only, hw: cck
+	WIRELESS_MODE_G = BIT(1), // tx: ofdm only, rx: ofdm & cck, hw: cck & ofdm
+	WIRELESS_MODE_A = BIT(2), // tx: ofdm only, rx: ofdm only, hw: ofdm only
+	WIRELESS_MODE_N_24G = BIT(3), // tx: MCS only, rx: MCS & cck, hw: MCS & cck
+	WIRELESS_MODE_N_5G = BIT(4), // tx: MCS only, rx: MCS & ofdm, hw: ofdm only
+	WIRELESS_AUTO = BIT(5),
+	WIRELESS_MODE_AC = BIT(6),
+	WIRELESS_MODE_MAX = (WIRELESS_MODE_B|WIRELESS_MODE_G|WIRELESS_MODE_A|WIRELESS_MODE_N_24G|WIRELESS_MODE_N_5G|WIRELESS_MODE_AC),
+};
+
+/* from rtlwifi/wifi.h */
+enum ratr_table_mode_new {
+        RATEID_IDX_BGN_40M_2SS = 0,
+        RATEID_IDX_BGN_40M_1SS = 1,
+        RATEID_IDX_BGN_20M_2SS_BN = 2,
+        RATEID_IDX_BGN_20M_1SS_BN = 3,
+        RATEID_IDX_GN_N2SS = 4,
+        RATEID_IDX_GN_N1SS = 5,
+        RATEID_IDX_BG = 6,
+        RATEID_IDX_G = 7,
+        RATEID_IDX_B = 8,
+        RATEID_IDX_VHT_2SS = 9,
+        RATEID_IDX_VHT_1SS = 10,
+        RATEID_IDX_MIX1 = 11,
+        RATEID_IDX_MIX2 = 12,
+        RATEID_IDX_VHT_3SS = 13,
+        RATEID_IDX_BGN_3SS = 14,
+};
+
+#define RTL8XXXU_RATR_STA_INIT 0
+#define RTL8XXXU_RATR_STA_HIGH 1
+#define RTL8XXXU_RATR_STA_MID  2
+#define RTL8XXXU_RATR_STA_LOW  3
+
+struct rtl8xxxu_rate_adaptive {
+	u16 wireless_mode;
+	u8 ratr_index;
+	u8 rssi_level;		/* INIT, HIGH, MIDDLE, LOW */
+} __packed;
+
 struct rtl8xxxu_priv {
 	struct ieee80211_hw *hw;
 	struct usb_device *udev;
@@ -1299,6 +1343,7 @@ struct rtl8xxxu_priv {
 	u8 pi_enabled:1;
 	u8 no_pape:1;
 	u8 int_buf[USB_INTR_CONTENT_LENGTH];
+	struct rtl8xxxu_rate_adaptive ra_info;
 };
 
 struct rtl8xxxu_rx_urb {
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 039e5ca9d2e4..360e9bd837e5 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -4345,7 +4345,7 @@ void rtl8xxxu_gen2_update_rate_mask(struct rtl8xxxu_priv *priv,
 	h2c.b_macid_cfg.ramask3 = (ramask >> 24) & 0xff;
 
 	h2c.ramask.arg = 0x80;
-	h2c.b_macid_cfg.data1 = 0;
+	h2c.b_macid_cfg.data1 = priv->ra_info.ratr_index;
 	if (sgi)
 		h2c.b_macid_cfg.data1 |= BIT(7);
 
@@ -4485,6 +4485,43 @@ static void rtl8xxxu_set_basic_rates(struct rtl8xxxu_priv *priv, u32 rate_cfg)
 	rtl8xxxu_write8(priv, REG_INIRTS_RATE_SEL, rate_idx);
 }
 
+static u16
+rtl8xxxu_wireless_mode(struct ieee80211_hw *hw, struct ieee80211_sta *sta)
+{
+	u16 network_type = WIRELESS_MODE_UNKNOWN;
+	u32 rate_mask;
+
+	rate_mask = (sta->supp_rates[0] & 0xfff) |
+		    (sta->ht_cap.mcs.rx_mask[0] << 12) |
+		    (sta->ht_cap.mcs.rx_mask[0] << 20);
+
+	if (hw->conf.chandef.chan->band == NL80211_BAND_5GHZ)
+	{
+		if (sta->vht_cap.vht_supported)
+			network_type = WIRELESS_MODE_AC;
+		else if (sta->ht_cap.ht_supported)
+			network_type = WIRELESS_MODE_N_5G;
+
+		network_type |= WIRELESS_MODE_A;
+	}
+	else
+	{
+		if (sta->vht_cap.vht_supported)
+			network_type = WIRELESS_MODE_AC;
+		else if (sta->ht_cap.ht_supported)
+			network_type = WIRELESS_MODE_N_24G;
+
+		if (sta->supp_rates[0] <= 0xf)
+			network_type |= WIRELESS_MODE_B;
+		else if (sta->supp_rates[0] & 0xf)
+			network_type |= (WIRELESS_MODE_B | WIRELESS_MODE_G);
+		else
+			network_type |= WIRELESS_MODE_G;
+	}
+
+	return network_type;
+}
+
 static void
 rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 			  struct ieee80211_bss_conf *bss_conf, u32 changed)
@@ -4503,6 +4540,7 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 		if (bss_conf->assoc) {
 			u32 ramask;
 			int sgi = 0;
+			struct rtl8xxxu_rate_adaptive *ra;
 
 			rcu_read_lock();
 			sta = ieee80211_find_sta(vif, bss_conf->bssid);
@@ -4527,6 +4565,11 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 				sgi = 1;
 			rcu_read_unlock();
 
+			ra = &priv->ra_info;
+			ra->wireless_mode = rtl8xxxu_wireless_mode(hw, sta);
+			ra->ratr_index = RATEID_IDX_BGN_40M_2SS;
+			ra->rssi_level = RTL8XXXU_RATR_STA_INIT;
+
 			priv->fops->update_rate_mask(priv, ramask, sgi);
 
 			rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff);
-- 
2.21.0

[RFC PATCH 2/2] rtl8xxxu: Add watchdog to update rate mask by signal strength

[Chris Chiu]

Introduce watchdog to monitor signal then update the rate mask
accordingly. The rate mask update logic comes from the rtlwifi
refresh_rate_adaptive_mask() from different chips.
---
 .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |   8 +
 .../realtek/rtl8xxxu/rtl8xxxu_8723b.c         | 151 ++++++++++++++++++
 .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c |  38 +++++
 3 files changed, 197 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
index 771f58aa7cae..f97271951053 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
@@ -1239,6 +1239,11 @@ struct rtl8xxxu_rate_adaptive {
 	u8 rssi_level;		/* INIT, HIGH, MIDDLE, LOW */
 } __packed;
 
+struct rtl8xxxu_watchdog {
+	struct ieee80211_vif *vif;
+	struct delayed_work ra_wq;
+};
+
 struct rtl8xxxu_priv {
 	struct ieee80211_hw *hw;
 	struct usb_device *udev;
@@ -1344,6 +1349,7 @@ struct rtl8xxxu_priv {
 	u8 no_pape:1;
 	u8 int_buf[USB_INTR_CONTENT_LENGTH];
 	struct rtl8xxxu_rate_adaptive ra_info;
+	struct rtl8xxxu_watchdog watchdog;
 };
 
 struct rtl8xxxu_rx_urb {
@@ -1380,6 +1386,8 @@ struct rtl8xxxu_fileops {
 			      bool ht40);
 	void (*update_rate_mask) (struct rtl8xxxu_priv *priv,
 				  u32 ramask, int sgi);
+	void (*refresh_rate_mask) (struct rtl8xxxu_priv *priv, int signal,
+				   struct ieee80211_sta *sta);
 	void (*report_connect) (struct rtl8xxxu_priv *priv,
 				u8 macid, bool connect);
 	void (*fill_txdesc) (struct ieee80211_hw *hw, struct ieee80211_hdr *hdr,
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
index 26b674aca125..92c35afecae0 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
@@ -1645,6 +1645,156 @@ static void rtl8723bu_init_statistics(struct rtl8xxxu_priv *priv)
 	rtl8xxxu_write32(priv, REG_OFDM0_FA_RSTC, val32);
 }
 
+static u8 rtl8723b_signal_to_rssi(int signal)
+{
+	if (signal < -95)
+		signal = -95;
+	return (u8)(signal + 95);
+}
+
+static void rtl8723b_refresh_rate_mask(struct rtl8xxxu_priv *priv,
+				       int signal, struct ieee80211_sta *sta)
+{
+	struct rtl8xxxu_rate_adaptive *ra;
+	struct ieee80211_hw *hw = priv->hw;
+	u16 wireless_mode;
+	u8 rssi_level, ratr_index;
+	u8 txbw_40mhz;
+	u8 rssi, rssi_thresh_high, rssi_thresh_low;
+
+	ra = &priv->ra_info;
+	wireless_mode = ra->wireless_mode;
+	rssi_level = ra->rssi_level;
+	rssi = rtl8723b_signal_to_rssi(signal);
+	ratr_index = ra->ratr_index;
+	txbw_40mhz = (hw->conf.chandef.width == NL80211_CHAN_WIDTH_40)? 1 : 0;
+
+	switch (rssi_level) {
+	case RTL8XXXU_RATR_STA_HIGH:
+		rssi_thresh_high = 50;
+		rssi_thresh_low = 20;
+		break;
+	case RTL8XXXU_RATR_STA_MID:
+		rssi_thresh_high = 55;
+		rssi_thresh_low = 20;
+		break;
+	case RTL8XXXU_RATR_STA_LOW:
+		rssi_thresh_high = 60;
+		rssi_thresh_low = 25;
+		break;
+	default:
+		rssi_thresh_high = 50;
+		rssi_thresh_low = 20;
+		break;
+	}
+
+	if (rssi > rssi_thresh_high)
+		rssi_level = RTL8XXXU_RATR_STA_HIGH;
+	else if (rssi > rssi_thresh_low)
+		rssi_level = RTL8XXXU_RATR_STA_MID;
+	else
+		rssi_level = RTL8XXXU_RATR_STA_LOW;
+
+	if (rssi_level != ra->rssi_level) {
+		int sgi = 0;
+		u32 rate_bitmap = 0;
+
+		rcu_read_lock();
+		rate_bitmap = (sta->supp_rates[0] & 0xfff) |
+			      sta->ht_cap.mcs.rx_mask[0] << 12 |
+                              sta->ht_cap.mcs.rx_mask[1] << 20;
+		if (sta->ht_cap.cap &
+		    (IEEE80211_HT_CAP_SGI_40 | IEEE80211_HT_CAP_SGI_20))
+			sgi = 1;
+		rcu_read_unlock();
+
+		switch (wireless_mode) {
+		case WIRELESS_MODE_B:
+			ratr_index = RATEID_IDX_B;
+			if (rate_bitmap & 0x0000000c)
+				rate_bitmap &= 0x0000000d;
+			else
+				rate_bitmap &= 0x0000000f;
+			break;
+		case WIRELESS_MODE_A:
+		case WIRELESS_MODE_G:
+			ratr_index = RATEID_IDX_G;
+			if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
+				rate_bitmap &= 0x00000f00;
+			else
+				rate_bitmap &= 0x00000ff0;
+			break;
+		case (WIRELESS_MODE_B|WIRELESS_MODE_G):
+			ratr_index = RATEID_IDX_BG;
+			if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
+				rate_bitmap &= 0x00000f00;
+			else if (rssi_level == RTL8XXXU_RATR_STA_MID)
+				rate_bitmap &= 0x00000ff0;
+			else
+				rate_bitmap &= 0x00000ff5;
+			break;
+		case WIRELESS_MODE_N_24G:
+		case WIRELESS_MODE_N_5G:
+		case (WIRELESS_MODE_G|WIRELESS_MODE_N_24G):
+		case (WIRELESS_MODE_A|WIRELESS_MODE_N_5G):
+			if (priv->tx_paths == 2 && priv->rx_paths == 2)
+				ratr_index = RATEID_IDX_GN_N2SS;
+			else
+				ratr_index = RATEID_IDX_GN_N1SS;
+		case (WIRELESS_MODE_B|WIRELESS_MODE_G|WIRELESS_MODE_N_24G):
+		case (WIRELESS_MODE_B|WIRELESS_MODE_N_24G):
+			if (txbw_40mhz) {
+				if (priv->tx_paths == 2 && priv->rx_paths == 2)
+					ratr_index = RATEID_IDX_BGN_40M_2SS;
+				else
+					ratr_index = RATEID_IDX_BGN_40M_1SS;
+			}
+			else {
+				if (priv->tx_paths == 2 && priv->rx_paths == 2)
+					ratr_index = RATEID_IDX_BGN_20M_2SS_BN;
+				else
+					ratr_index = RATEID_IDX_BGN_20M_1SS_BN;
+			}
+
+			if (priv->tx_paths == 2 && priv->rx_paths == 2) {
+				if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
+					rate_bitmap &= 0x0f8f0000;
+				else if (rssi_level == RTL8XXXU_RATR_STA_MID)
+					rate_bitmap &= 0x0f8ff000;
+				else {
+					if (txbw_40mhz)
+						rate_bitmap &= 0x0f8ff015;
+					else
+						rate_bitmap &= 0x0f8ff005;
+				}
+			}
+			else {
+				if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
+					rate_bitmap &= 0x000f0000;
+				else if (rssi_level == RTL8XXXU_RATR_STA_MID)
+					rate_bitmap &= 0x000ff000;
+				else {
+					if (txbw_40mhz)
+						rate_bitmap &= 0x000ff015;
+					else
+						rate_bitmap &= 0x000ff005;
+				}
+			}
+			break;
+		default:
+			ratr_index = RATEID_IDX_BGN_40M_2SS;
+			rate_bitmap &= 0x0fffffff;
+			break;
+		}
+
+		ra->ratr_index = ratr_index;
+		ra->rssi_level = rssi_level;
+		priv->fops->update_rate_mask(priv, rate_bitmap, sgi);
+	}
+
+	return;
+}
+
 struct rtl8xxxu_fileops rtl8723bu_fops = {
 	.parse_efuse = rtl8723bu_parse_efuse,
 	.load_firmware = rtl8723bu_load_firmware,
@@ -1665,6 +1815,7 @@ struct rtl8xxxu_fileops rtl8723bu_fops = {
 	.usb_quirks = rtl8xxxu_gen2_usb_quirks,
 	.set_tx_power = rtl8723b_set_tx_power,
 	.update_rate_mask = rtl8xxxu_gen2_update_rate_mask,
+	.refresh_rate_mask = rtl8723b_refresh_rate_mask,
 	.report_connect = rtl8xxxu_gen2_report_connect,
 	.fill_txdesc = rtl8xxxu_fill_txdesc_v2,
 	.writeN_block_size = 1024,
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 360e9bd837e5..8db479986e97 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -4565,6 +4565,7 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 				sgi = 1;
 			rcu_read_unlock();
 
+			priv->watchdog.vif = vif;
 			ra = &priv->ra_info;
 			ra->wireless_mode = rtl8xxxu_wireless_mode(hw, sta);
 			ra->ratr_index = RATEID_IDX_BGN_40M_2SS;
@@ -5822,6 +5823,38 @@ rtl8xxxu_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 	return 0;
 }
 
+static void rtl8xxxu_watchdog_callback(struct work_struct *work)
+{
+	struct ieee80211_vif *vif;
+	struct rtl8xxxu_watchdog *wdog;
+	struct rtl8xxxu_priv *priv;
+
+	wdog = container_of(work, struct rtl8xxxu_watchdog, ra_wq.work);
+	priv = container_of(wdog, struct rtl8xxxu_priv, watchdog);
+	vif = wdog->vif;
+
+	if (vif) {
+		int signal;
+		struct ieee80211_sta *sta;
+
+		rcu_read_lock();
+		sta = ieee80211_find_sta(vif, vif->bss_conf.bssid);
+		if (!sta) {
+			struct device *dev = &priv->udev->dev;
+			dev_info(dev, "%s: no sta found\n", __func__);
+			rcu_read_unlock();
+			return;
+		}
+		rcu_read_unlock();
+
+		signal = ieee80211_ave_rssi(vif);
+		if (priv->fops->refresh_rate_mask)
+			priv->fops->refresh_rate_mask(priv, signal, sta);
+	}
+
+	schedule_delayed_work(&priv->watchdog.ra_wq, 2 * HZ);
+}
+
 static int rtl8xxxu_start(struct ieee80211_hw *hw)
 {
 	struct rtl8xxxu_priv *priv = hw->priv;
@@ -5878,6 +5911,8 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
 
 		ret = rtl8xxxu_submit_rx_urb(priv, rx_urb);
 	}
+
+	schedule_delayed_work(&priv->watchdog.ra_wq, 2* HZ);
 exit:
 	/*
 	 * Accept all data and mgmt frames
@@ -6101,6 +6136,7 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
 	INIT_LIST_HEAD(&priv->rx_urb_pending_list);
 	spin_lock_init(&priv->rx_urb_lock);
 	INIT_WORK(&priv->rx_urb_wq, rtl8xxxu_rx_urb_work);
+	INIT_DELAYED_WORK(&priv->watchdog.ra_wq, rtl8xxxu_watchdog_callback);
 
 	usb_set_intfdata(interface, hw);
 
@@ -6226,6 +6262,8 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
 	mutex_destroy(&priv->usb_buf_mutex);
 	mutex_destroy(&priv->h2c_mutex);
 
+	cancel_delayed_work_sync(&priv->watchdog.ra_wq);
+
 	if (priv->udev->state != USB_STATE_NOTATTACHED) {
 		dev_info(&priv->udev->dev,
 			 "Device still attached, trying to reset\n");
-- 
2.21.0

Re: [RFC PATCH 1/2] rtl8xxxu: Add rate adaptive related data

[Kalle Valo]

Chris Chiu <chiu@endlessm.com> writes:

> Add wireless mode, signal strength level, and rate table index
> to tell the firmware that we need to adjust the tx rate bitmap
> accordingly.

Please read Developer's Certificate of Origin and add Signed-off-by to
the commit logs:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#signed-off-by_missing

-- 
Kalle Valo

Re: [RFC PATCH 2/2] rtl8xxxu: Add watchdog to update rate mask by signal strength

[Daniel Drake]

Hi Chris,

Great work on finding this!

On Fri, May 3, 2019 at 3:22 PM Chris Chiu <chiu@endlessm.com> wrote:
> Introduce watchdog to monitor signal then update the rate mask
> accordingly. The rate mask update logic comes from the rtlwifi
> refresh_rate_adaptive_mask() from different chips.

You should expand your commit message here to summarise the key points
in the cover letter. Specifically that matching this aspect of the
vendor driver results in a significant TX performance increase which
was previously stuck at 1mbps.

> ---
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |   8 +
>  .../realtek/rtl8xxxu/rtl8xxxu_8723b.c         | 151 ++++++++++++++++++
>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c |  38 +++++
>  3 files changed, 197 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index 771f58aa7cae..f97271951053 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -1239,6 +1239,11 @@ struct rtl8xxxu_rate_adaptive {
>         u8 rssi_level;          /* INIT, HIGH, MIDDLE, LOW */
>  } __packed;
>
> +struct rtl8xxxu_watchdog {
> +       struct ieee80211_vif *vif;
> +       struct delayed_work ra_wq;
> +};

Having to store the vif address under the device-specific private
structure may be a layering violation, but I'm not fully grasping how
all this fits together. Can anyone from linux-wireless help?

The existing rtl8xxxu_add_interface() code appears to allow multiple
STA interfaces to be added. Does that imply that the hardware should
support connecting to multiple APs on different channels? I'm pretty
sure the hardware doesn't support that; if so we could do something
similar to ar5523.c where it only allows a single vif, and can easily
store that pointer in the device-specific structure.

Or if there's a valid reason to support multiple vifs, then we need to
figure out how to implement this watchdog. As shown below, the
watchdog needs to know the supported rate info of the AP you are
connected to, and the RSSI, and that comes from a specific vif. If
multiple vifs are present, how would we know which one to choose for
this rate adjustment?

>  struct rtl8xxxu_priv {
>         struct ieee80211_hw *hw;
>         struct usb_device *udev;
> @@ -1344,6 +1349,7 @@ struct rtl8xxxu_priv {
>         u8 no_pape:1;
>         u8 int_buf[USB_INTR_CONTENT_LENGTH];
>         struct rtl8xxxu_rate_adaptive ra_info;
> +       struct rtl8xxxu_watchdog watchdog;
>  };
>
>  struct rtl8xxxu_rx_urb {
> @@ -1380,6 +1386,8 @@ struct rtl8xxxu_fileops {
>                               bool ht40);
>         void (*update_rate_mask) (struct rtl8xxxu_priv *priv,
>                                   u32 ramask, int sgi);
> +       void (*refresh_rate_mask) (struct rtl8xxxu_priv *priv, int signal,
> +                                  struct ieee80211_sta *sta);
>         void (*report_connect) (struct rtl8xxxu_priv *priv,
>                                 u8 macid, bool connect);
>         void (*fill_txdesc) (struct ieee80211_hw *hw, struct ieee80211_hdr *hdr,
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> index 26b674aca125..92c35afecae0 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> @@ -1645,6 +1645,156 @@ static void rtl8723bu_init_statistics(struct rtl8xxxu_priv *priv)
>         rtl8xxxu_write32(priv, REG_OFDM0_FA_RSTC, val32);
>  }
>
> +static u8 rtl8723b_signal_to_rssi(int signal)
> +{
> +       if (signal < -95)
> +               signal = -95;
> +       return (u8)(signal + 95);
> +}
> +
> +static void rtl8723b_refresh_rate_mask(struct rtl8xxxu_priv *priv,
> +                                      int signal, struct ieee80211_sta *sta)
> +{
> +       struct rtl8xxxu_rate_adaptive *ra;
> +       struct ieee80211_hw *hw = priv->hw;
> +       u16 wireless_mode;
> +       u8 rssi_level, ratr_index;
> +       u8 txbw_40mhz;
> +       u8 rssi, rssi_thresh_high, rssi_thresh_low;
> +
> +       ra = &priv->ra_info;
> +       wireless_mode = ra->wireless_mode;
> +       rssi_level = ra->rssi_level;
> +       rssi = rtl8723b_signal_to_rssi(signal);
> +       ratr_index = ra->ratr_index;
> +       txbw_40mhz = (hw->conf.chandef.width == NL80211_CHAN_WIDTH_40)? 1 : 0;
> +
> +       switch (rssi_level) {
> +       case RTL8XXXU_RATR_STA_HIGH:
> +               rssi_thresh_high = 50;
> +               rssi_thresh_low = 20;
> +               break;
> +       case RTL8XXXU_RATR_STA_MID:
> +               rssi_thresh_high = 55;
> +               rssi_thresh_low = 20;
> +               break;
> +       case RTL8XXXU_RATR_STA_LOW:
> +               rssi_thresh_high = 60;
> +               rssi_thresh_low = 25;
> +               break;
> +       default:
> +               rssi_thresh_high = 50;
> +               rssi_thresh_low = 20;
> +               break;
> +       }
> +
> +       if (rssi > rssi_thresh_high)
> +               rssi_level = RTL8XXXU_RATR_STA_HIGH;
> +       else if (rssi > rssi_thresh_low)
> +               rssi_level = RTL8XXXU_RATR_STA_MID;
> +       else
> +               rssi_level = RTL8XXXU_RATR_STA_LOW;
> +
> +       if (rssi_level != ra->rssi_level) {
> +               int sgi = 0;
> +               u32 rate_bitmap = 0;
> +
> +               rcu_read_lock();
> +               rate_bitmap = (sta->supp_rates[0] & 0xfff) |
> +                             sta->ht_cap.mcs.rx_mask[0] << 12 |
> +                              sta->ht_cap.mcs.rx_mask[1] << 20;
> +               if (sta->ht_cap.cap &
> +                   (IEEE80211_HT_CAP_SGI_40 | IEEE80211_HT_CAP_SGI_20))
> +                       sgi = 1;
> +               rcu_read_unlock();
> +
> +               switch (wireless_mode) {
> +               case WIRELESS_MODE_B:
> +                       ratr_index = RATEID_IDX_B;
> +                       if (rate_bitmap & 0x0000000c)
> +                               rate_bitmap &= 0x0000000d;
> +                       else
> +                               rate_bitmap &= 0x0000000f;
> +                       break;
> +               case WIRELESS_MODE_A:
> +               case WIRELESS_MODE_G:
> +                       ratr_index = RATEID_IDX_G;
> +                       if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
> +                               rate_bitmap &= 0x00000f00;
> +                       else
> +                               rate_bitmap &= 0x00000ff0;
> +                       break;
> +               case (WIRELESS_MODE_B|WIRELESS_MODE_G):
> +                       ratr_index = RATEID_IDX_BG;
> +                       if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
> +                               rate_bitmap &= 0x00000f00;
> +                       else if (rssi_level == RTL8XXXU_RATR_STA_MID)
> +                               rate_bitmap &= 0x00000ff0;
> +                       else
> +                               rate_bitmap &= 0x00000ff5;
> +                       break;
> +               case WIRELESS_MODE_N_24G:
> +               case WIRELESS_MODE_N_5G:
> +               case (WIRELESS_MODE_G|WIRELESS_MODE_N_24G):
> +               case (WIRELESS_MODE_A|WIRELESS_MODE_N_5G):
> +                       if (priv->tx_paths == 2 && priv->rx_paths == 2)
> +                               ratr_index = RATEID_IDX_GN_N2SS;
> +                       else
> +                               ratr_index = RATEID_IDX_GN_N1SS;
> +               case (WIRELESS_MODE_B|WIRELESS_MODE_G|WIRELESS_MODE_N_24G):
> +               case (WIRELESS_MODE_B|WIRELESS_MODE_N_24G):
> +                       if (txbw_40mhz) {
> +                               if (priv->tx_paths == 2 && priv->rx_paths == 2)
> +                                       ratr_index = RATEID_IDX_BGN_40M_2SS;
> +                               else
> +                                       ratr_index = RATEID_IDX_BGN_40M_1SS;
> +                       }
> +                       else {
> +                               if (priv->tx_paths == 2 && priv->rx_paths == 2)
> +                                       ratr_index = RATEID_IDX_BGN_20M_2SS_BN;
> +                               else
> +                                       ratr_index = RATEID_IDX_BGN_20M_1SS_BN;
> +                       }
> +
> +                       if (priv->tx_paths == 2 && priv->rx_paths == 2) {
> +                               if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
> +                                       rate_bitmap &= 0x0f8f0000;
> +                               else if (rssi_level == RTL8XXXU_RATR_STA_MID)
> +                                       rate_bitmap &= 0x0f8ff000;
> +                               else {
> +                                       if (txbw_40mhz)
> +                                               rate_bitmap &= 0x0f8ff015;
> +                                       else
> +                                               rate_bitmap &= 0x0f8ff005;
> +                               }
> +                       }
> +                       else {
> +                               if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
> +                                       rate_bitmap &= 0x000f0000;
> +                               else if (rssi_level == RTL8XXXU_RATR_STA_MID)
> +                                       rate_bitmap &= 0x000ff000;
> +                               else {
> +                                       if (txbw_40mhz)
> +                                               rate_bitmap &= 0x000ff015;
> +                                       else
> +                                               rate_bitmap &= 0x000ff005;
> +                               }
> +                       }
> +                       break;
> +               default:
> +                       ratr_index = RATEID_IDX_BGN_40M_2SS;
> +                       rate_bitmap &= 0x0fffffff;
> +                       break;
> +               }
> +
> +               ra->ratr_index = ratr_index;
> +               ra->rssi_level = rssi_level;
> +               priv->fops->update_rate_mask(priv, rate_bitmap, sgi);
> +       }
> +
> +       return;
> +}
> +
>  struct rtl8xxxu_fileops rtl8723bu_fops = {
>         .parse_efuse = rtl8723bu_parse_efuse,
>         .load_firmware = rtl8723bu_load_firmware,
> @@ -1665,6 +1815,7 @@ struct rtl8xxxu_fileops rtl8723bu_fops = {
>         .usb_quirks = rtl8xxxu_gen2_usb_quirks,
>         .set_tx_power = rtl8723b_set_tx_power,
>         .update_rate_mask = rtl8xxxu_gen2_update_rate_mask,
> +       .refresh_rate_mask = rtl8723b_refresh_rate_mask,
>         .report_connect = rtl8xxxu_gen2_report_connect,
>         .fill_txdesc = rtl8xxxu_fill_txdesc_v2,
>         .writeN_block_size = 1024,
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 360e9bd837e5..8db479986e97 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -4565,6 +4565,7 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>                                 sgi = 1;
>                         rcu_read_unlock();
>
> +                       priv->watchdog.vif = vif;
>                         ra = &priv->ra_info;
>                         ra->wireless_mode = rtl8xxxu_wireless_mode(hw, sta);
>                         ra->ratr_index = RATEID_IDX_BGN_40M_2SS;
> @@ -5822,6 +5823,38 @@ rtl8xxxu_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>         return 0;
>  }
>
> +static void rtl8xxxu_watchdog_callback(struct work_struct *work)
> +{
> +       struct ieee80211_vif *vif;
> +       struct rtl8xxxu_watchdog *wdog;
> +       struct rtl8xxxu_priv *priv;
> +
> +       wdog = container_of(work, struct rtl8xxxu_watchdog, ra_wq.work);
> +       priv = container_of(wdog, struct rtl8xxxu_priv, watchdog);
> +       vif = wdog->vif;
> +
> +       if (vif) {
> +               int signal;
> +               struct ieee80211_sta *sta;
> +
> +               rcu_read_lock();

Can you explain the lock/unlock here?

> +               sta = ieee80211_find_sta(vif, vif->bss_conf.bssid);
> +               if (!sta) {
> +                       struct device *dev = &priv->udev->dev;
> +                       dev_info(dev, "%s: no sta found\n", __func__);

Does this result in a kernel log message every 2 seconds when the wifi
interface is not associated to an AP?

> +                       rcu_read_unlock();
> +                       return;
> +               }
> +               rcu_read_unlock();
> +
> +               signal = ieee80211_ave_rssi(vif);
> +               if (priv->fops->refresh_rate_mask)
> +                       priv->fops->refresh_rate_mask(priv, signal, sta);
> +       }
> +
> +       schedule_delayed_work(&priv->watchdog.ra_wq, 2 * HZ);
> +}
> +
>  static int rtl8xxxu_start(struct ieee80211_hw *hw)
>  {
>         struct rtl8xxxu_priv *priv = hw->priv;
> @@ -5878,6 +5911,8 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>
>                 ret = rtl8xxxu_submit_rx_urb(priv, rx_urb);
>         }
> +
> +       schedule_delayed_work(&priv->watchdog.ra_wq, 2* HZ);
>  exit:
>         /*
>          * Accept all data and mgmt frames
> @@ -6101,6 +6136,7 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
>         INIT_LIST_HEAD(&priv->rx_urb_pending_list);
>         spin_lock_init(&priv->rx_urb_lock);
>         INIT_WORK(&priv->rx_urb_wq, rtl8xxxu_rx_urb_work);
> +       INIT_DELAYED_WORK(&priv->watchdog.ra_wq, rtl8xxxu_watchdog_callback);
>
>         usb_set_intfdata(interface, hw);
>
> @@ -6226,6 +6262,8 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
>         mutex_destroy(&priv->usb_buf_mutex);
>         mutex_destroy(&priv->h2c_mutex);
>
> +       cancel_delayed_work_sync(&priv->watchdog.ra_wq);
> +
>         if (priv->udev->state != USB_STATE_NOTATTACHED) {
>                 dev_info(&priv->udev->dev,
>                          "Device still attached, trying to reset\n");
> --
> 2.21.0
>

Re: [RFC PATCH 1/2] rtl8xxxu: Add rate adaptive related data

[Daniel Drake]

Hi Chris,

Thanks for this! Some suggestions below, although let me know if any
don't make sense.

On Fri, May 3, 2019 at 3:22 PM Chris Chiu <chiu@endlessm.com> wrote:
>
> Add wireless mode, signal strength level, and rate table index
> to tell the firmware that we need to adjust the tx rate bitmap
> accordingly.
> ---
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  | 45 +++++++++++++++++++
>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 45 ++++++++++++++++++-
>  2 files changed, 89 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index 8828baf26e7b..771f58aa7cae 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -1195,6 +1195,50 @@ struct rtl8723bu_c2h {
>
>  struct rtl8xxxu_fileops;
>
> +/*mlme related.*/
> +enum wireless_mode {
> +       WIRELESS_MODE_UNKNOWN = 0,
> +       //Sub-Element

Run these patches through checkpatch.pl, it'll have some suggestions
to bring the coding style in line, for example not using // style
comments.

> +       WIRELESS_MODE_B = BIT(0), // tx: cck only , rx: cck only, hw: cck
> +       WIRELESS_MODE_G = BIT(1), // tx: ofdm only, rx: ofdm & cck, hw: cck & ofdm
> +       WIRELESS_MODE_A = BIT(2), // tx: ofdm only, rx: ofdm only, hw: ofdm only
> +       WIRELESS_MODE_N_24G = BIT(3), // tx: MCS only, rx: MCS & cck, hw: MCS & cck
> +       WIRELESS_MODE_N_5G = BIT(4), // tx: MCS only, rx: MCS & ofdm, hw: ofdm only
> +       WIRELESS_AUTO = BIT(5),
> +       WIRELESS_MODE_AC = BIT(6),
> +       WIRELESS_MODE_MAX = (WIRELESS_MODE_B|WIRELESS_MODE_G|WIRELESS_MODE_A|WIRELESS_MODE_N_24G|WIRELESS_MODE_N_5G|WIRELESS_MODE_AC),
> +};
> +
> +/* from rtlwifi/wifi.h */
> +enum ratr_table_mode_new {
> +        RATEID_IDX_BGN_40M_2SS = 0,
> +        RATEID_IDX_BGN_40M_1SS = 1,
> +        RATEID_IDX_BGN_20M_2SS_BN = 2,
> +        RATEID_IDX_BGN_20M_1SS_BN = 3,
> +        RATEID_IDX_GN_N2SS = 4,
> +        RATEID_IDX_GN_N1SS = 5,
> +        RATEID_IDX_BG = 6,
> +        RATEID_IDX_G = 7,
> +        RATEID_IDX_B = 8,
> +        RATEID_IDX_VHT_2SS = 9,
> +        RATEID_IDX_VHT_1SS = 10,
> +        RATEID_IDX_MIX1 = 11,
> +        RATEID_IDX_MIX2 = 12,
> +        RATEID_IDX_VHT_3SS = 13,
> +        RATEID_IDX_BGN_3SS = 14,
> +};
> +
> +#define RTL8XXXU_RATR_STA_INIT 0
> +#define RTL8XXXU_RATR_STA_HIGH 1
> +#define RTL8XXXU_RATR_STA_MID  2
> +#define RTL8XXXU_RATR_STA_LOW  3
> +
> +struct rtl8xxxu_rate_adaptive {
> +       u16 wireless_mode;
> +       u8 ratr_index;
> +       u8 rssi_level;          /* INIT, HIGH, MIDDLE, LOW */
> +} __packed;

It would be better/cleaner to avoid storing data in per-device
structures if at all possible.

For wireless_mode, I think you should just call
rtl8xxxu_wireless_mode() every time from rtl8723b_refresh_rate_mask().
The work done there is simple (i.e. it's not expensive to call) and
then we avoid having to store data (which might become stale etc).

For ratr_index, I believe you can just make it a parameter passed to
rtl8xxxu_gen2_update_rate_mask which is the only consumer of this
variable. The two callsites (rtl8xxxu_bss_info_changed and
rtl8723b_refresh_rate_mask) already know which value they want to be
used.

rssi_level is the one that you probably do want to store, since I see
the logic in patch 2 - if the rssi level hasn't changed then you don't
need to set the rate mask again, and that's a good idea since it
reduces bus traffic. You could move this into rtl8xxxu_priv rather
than having its own separate structure.

After making those changes it might even make sense to collapse this
all into a single patch; you can judge!

Re: [RFC PATCH 2/2] rtl8xxxu: Add watchdog to update rate mask by signal strength

[Chris Chiu]

On Thu, May 9, 2019 at 4:11 PM Daniel Drake <drake@endlessm.com> wrote:
>
> Hi Chris,
>
> Great work on finding this!
>
> On Fri, May 3, 2019 at 3:22 PM Chris Chiu <chiu@endlessm.com> wrote:
> > Introduce watchdog to monitor signal then update the rate mask
> > accordingly. The rate mask update logic comes from the rtlwifi
> > refresh_rate_adaptive_mask() from different chips.
>
> You should expand your commit message here to summarise the key points
> in the cover letter. Specifically that matching this aspect of the
> vendor driver results in a significant TX performance increase which
> was previously stuck at 1mbps.
>
> > ---
> >  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |   8 +
> >  .../realtek/rtl8xxxu/rtl8xxxu_8723b.c         | 151 ++++++++++++++++++
> >  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c |  38 +++++
> >  3 files changed, 197 insertions(+)
> >
> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> > index 771f58aa7cae..f97271951053 100644
> > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> > @@ -1239,6 +1239,11 @@ struct rtl8xxxu_rate_adaptive {
> >         u8 rssi_level;          /* INIT, HIGH, MIDDLE, LOW */
> >  } __packed;
> >
> > +struct rtl8xxxu_watchdog {
> > +       struct ieee80211_vif *vif;
> > +       struct delayed_work ra_wq;
> > +};
>
> Having to store the vif address under the device-specific private
> structure may be a layering violation, but I'm not fully grasping how
> all this fits together. Can anyone from linux-wireless help?
>
> The existing rtl8xxxu_add_interface() code appears to allow multiple
> STA interfaces to be added. Does that imply that the hardware should
> support connecting to multiple APs on different channels? I'm pretty
> sure the hardware doesn't support that; if so we could do something
> similar to ar5523.c where it only allows a single vif, and can easily
> store that pointer in the device-specific structure.
>
> Or if there's a valid reason to support multiple vifs, then we need to
> figure out how to implement this watchdog. As shown below, the
> watchdog needs to know the supported rate info of the AP you are
> connected to, and the RSSI, and that comes from a specific vif. If
> multiple vifs are present, how would we know which one to choose for
> this rate adjustment?
>

I need the vif because there's seems no easy way to get RSSI. Please
suggest if there's any better idea for this. I believe multiple vifs is for AP
mode (with more than 1 virtual AP/SSIDs) and the Station+AP coexist
mode. But the rtl8xxxu driver basically supports only Station mode. So
the vif helps get the RSSI. Or I will need to record the RSSI in
rtl8xxxu_rx_parse_phystats() and store in rtl8xxxu_priv. That's a little
nasty. Maybe someone can point out how to retrieve the RSSI from
specific register or which part of code in rtlwifi I can use to get the
undecorated signal strength.

> >  struct rtl8xxxu_priv {
> >         struct ieee80211_hw *hw;
> >         struct usb_device *udev;
> > @@ -1344,6 +1349,7 @@ struct rtl8xxxu_priv {
> >         u8 no_pape:1;
> >         u8 int_buf[USB_INTR_CONTENT_LENGTH];
> >         struct rtl8xxxu_rate_adaptive ra_info;
> > +       struct rtl8xxxu_watchdog watchdog;
> >  };
> >
> >  struct rtl8xxxu_rx_urb {
> > @@ -1380,6 +1386,8 @@ struct rtl8xxxu_fileops {
> >                               bool ht40);
> >         void (*update_rate_mask) (struct rtl8xxxu_priv *priv,
> >                                   u32 ramask, int sgi);
> > +       void (*refresh_rate_mask) (struct rtl8xxxu_priv *priv, int signal,
> > +                                  struct ieee80211_sta *sta);
> >         void (*report_connect) (struct rtl8xxxu_priv *priv,
> >                                 u8 macid, bool connect);
> >         void (*fill_txdesc) (struct ieee80211_hw *hw, struct ieee80211_hdr *hdr,
> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> > index 26b674aca125..92c35afecae0 100644
> > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> > @@ -1645,6 +1645,156 @@ static void rtl8723bu_init_statistics(struct rtl8xxxu_priv *priv)
> >         rtl8xxxu_write32(priv, REG_OFDM0_FA_RSTC, val32);
> >  }
> >
> > +static u8 rtl8723b_signal_to_rssi(int signal)
> > +{
> > +       if (signal < -95)
> > +               signal = -95;
> > +       return (u8)(signal + 95);
> > +}
> > +
> > +static void rtl8723b_refresh_rate_mask(struct rtl8xxxu_priv *priv,
> > +                                      int signal, struct ieee80211_sta *sta)
> > +{
> > +       struct rtl8xxxu_rate_adaptive *ra;
> > +       struct ieee80211_hw *hw = priv->hw;
> > +       u16 wireless_mode;
> > +       u8 rssi_level, ratr_index;
> > +       u8 txbw_40mhz;
> > +       u8 rssi, rssi_thresh_high, rssi_thresh_low;
> > +
> > +       ra = &priv->ra_info;
> > +       wireless_mode = ra->wireless_mode;
> > +       rssi_level = ra->rssi_level;
> > +       rssi = rtl8723b_signal_to_rssi(signal);
> > +       ratr_index = ra->ratr_index;
> > +       txbw_40mhz = (hw->conf.chandef.width == NL80211_CHAN_WIDTH_40)? 1 : 0;
> > +
> > +       switch (rssi_level) {
> > +       case RTL8XXXU_RATR_STA_HIGH:
> > +               rssi_thresh_high = 50;
> > +               rssi_thresh_low = 20;
> > +               break;
> > +       case RTL8XXXU_RATR_STA_MID:
> > +               rssi_thresh_high = 55;
> > +               rssi_thresh_low = 20;
> > +               break;
> > +       case RTL8XXXU_RATR_STA_LOW:
> > +               rssi_thresh_high = 60;
> > +               rssi_thresh_low = 25;
> > +               break;
> > +       default:
> > +               rssi_thresh_high = 50;
> > +               rssi_thresh_low = 20;
> > +               break;
> > +       }
> > +
> > +       if (rssi > rssi_thresh_high)
> > +               rssi_level = RTL8XXXU_RATR_STA_HIGH;
> > +       else if (rssi > rssi_thresh_low)
> > +               rssi_level = RTL8XXXU_RATR_STA_MID;
> > +       else
> > +               rssi_level = RTL8XXXU_RATR_STA_LOW;
> > +
> > +       if (rssi_level != ra->rssi_level) {
> > +               int sgi = 0;
> > +               u32 rate_bitmap = 0;
> > +
> > +               rcu_read_lock();
> > +               rate_bitmap = (sta->supp_rates[0] & 0xfff) |
> > +                             sta->ht_cap.mcs.rx_mask[0] << 12 |
> > +                              sta->ht_cap.mcs.rx_mask[1] << 20;
> > +               if (sta->ht_cap.cap &
> > +                   (IEEE80211_HT_CAP_SGI_40 | IEEE80211_HT_CAP_SGI_20))
> > +                       sgi = 1;
> > +               rcu_read_unlock();
> > +
> > +               switch (wireless_mode) {
> > +               case WIRELESS_MODE_B:
> > +                       ratr_index = RATEID_IDX_B;
> > +                       if (rate_bitmap & 0x0000000c)
> > +                               rate_bitmap &= 0x0000000d;
> > +                       else
> > +                               rate_bitmap &= 0x0000000f;
> > +                       break;
> > +               case WIRELESS_MODE_A:
> > +               case WIRELESS_MODE_G:
> > +                       ratr_index = RATEID_IDX_G;
> > +                       if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
> > +                               rate_bitmap &= 0x00000f00;
> > +                       else
> > +                               rate_bitmap &= 0x00000ff0;
> > +                       break;
> > +               case (WIRELESS_MODE_B|WIRELESS_MODE_G):
> > +                       ratr_index = RATEID_IDX_BG;
> > +                       if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
> > +                               rate_bitmap &= 0x00000f00;
> > +                       else if (rssi_level == RTL8XXXU_RATR_STA_MID)
> > +                               rate_bitmap &= 0x00000ff0;
> > +                       else
> > +                               rate_bitmap &= 0x00000ff5;
> > +                       break;
> > +               case WIRELESS_MODE_N_24G:
> > +               case WIRELESS_MODE_N_5G:
> > +               case (WIRELESS_MODE_G|WIRELESS_MODE_N_24G):
> > +               case (WIRELESS_MODE_A|WIRELESS_MODE_N_5G):
> > +                       if (priv->tx_paths == 2 && priv->rx_paths == 2)
> > +                               ratr_index = RATEID_IDX_GN_N2SS;
> > +                       else
> > +                               ratr_index = RATEID_IDX_GN_N1SS;
> > +               case (WIRELESS_MODE_B|WIRELESS_MODE_G|WIRELESS_MODE_N_24G):
> > +               case (WIRELESS_MODE_B|WIRELESS_MODE_N_24G):
> > +                       if (txbw_40mhz) {
> > +                               if (priv->tx_paths == 2 && priv->rx_paths == 2)
> > +                                       ratr_index = RATEID_IDX_BGN_40M_2SS;
> > +                               else
> > +                                       ratr_index = RATEID_IDX_BGN_40M_1SS;
> > +                       }
> > +                       else {
> > +                               if (priv->tx_paths == 2 && priv->rx_paths == 2)
> > +                                       ratr_index = RATEID_IDX_BGN_20M_2SS_BN;
> > +                               else
> > +                                       ratr_index = RATEID_IDX_BGN_20M_1SS_BN;
> > +                       }
> > +
> > +                       if (priv->tx_paths == 2 && priv->rx_paths == 2) {
> > +                               if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
> > +                                       rate_bitmap &= 0x0f8f0000;
> > +                               else if (rssi_level == RTL8XXXU_RATR_STA_MID)
> > +                                       rate_bitmap &= 0x0f8ff000;
> > +                               else {
> > +                                       if (txbw_40mhz)
> > +                                               rate_bitmap &= 0x0f8ff015;
> > +                                       else
> > +                                               rate_bitmap &= 0x0f8ff005;
> > +                               }
> > +                       }
> > +                       else {
> > +                               if (rssi_level == RTL8XXXU_RATR_STA_HIGH)
> > +                                       rate_bitmap &= 0x000f0000;
> > +                               else if (rssi_level == RTL8XXXU_RATR_STA_MID)
> > +                                       rate_bitmap &= 0x000ff000;
> > +                               else {
> > +                                       if (txbw_40mhz)
> > +                                               rate_bitmap &= 0x000ff015;
> > +                                       else
> > +                                               rate_bitmap &= 0x000ff005;
> > +                               }
> > +                       }
> > +                       break;
> > +               default:
> > +                       ratr_index = RATEID_IDX_BGN_40M_2SS;
> > +                       rate_bitmap &= 0x0fffffff;
> > +                       break;
> > +               }
> > +
> > +               ra->ratr_index = ratr_index;
> > +               ra->rssi_level = rssi_level;
> > +               priv->fops->update_rate_mask(priv, rate_bitmap, sgi);
> > +       }
> > +
> > +       return;
> > +}
> > +
> >  struct rtl8xxxu_fileops rtl8723bu_fops = {
> >         .parse_efuse = rtl8723bu_parse_efuse,
> >         .load_firmware = rtl8723bu_load_firmware,
> > @@ -1665,6 +1815,7 @@ struct rtl8xxxu_fileops rtl8723bu_fops = {
> >         .usb_quirks = rtl8xxxu_gen2_usb_quirks,
> >         .set_tx_power = rtl8723b_set_tx_power,
> >         .update_rate_mask = rtl8xxxu_gen2_update_rate_mask,
> > +       .refresh_rate_mask = rtl8723b_refresh_rate_mask,
> >         .report_connect = rtl8xxxu_gen2_report_connect,
> >         .fill_txdesc = rtl8xxxu_fill_txdesc_v2,
> >         .writeN_block_size = 1024,
> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> > index 360e9bd837e5..8db479986e97 100644
> > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> > @@ -4565,6 +4565,7 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> >                                 sgi = 1;
> >                         rcu_read_unlock();
> >
> > +                       priv->watchdog.vif = vif;
> >                         ra = &priv->ra_info;
> >                         ra->wireless_mode = rtl8xxxu_wireless_mode(hw, sta);
> >                         ra->ratr_index = RATEID_IDX_BGN_40M_2SS;
> > @@ -5822,6 +5823,38 @@ rtl8xxxu_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> >         return 0;
> >  }
> >
> > +static void rtl8xxxu_watchdog_callback(struct work_struct *work)
> > +{
> > +       struct ieee80211_vif *vif;
> > +       struct rtl8xxxu_watchdog *wdog;
> > +       struct rtl8xxxu_priv *priv;
> > +
> > +       wdog = container_of(work, struct rtl8xxxu_watchdog, ra_wq.work);
> > +       priv = container_of(wdog, struct rtl8xxxu_priv, watchdog);
> > +       vif = wdog->vif;
> > +
> > +       if (vif) {
> > +               int signal;
> > +               struct ieee80211_sta *sta;
> > +
> > +               rcu_read_lock();
>
> Can you explain the lock/unlock here?
>

Actually it may not be mandatory because the sta_info_get_bss()
will do this inside ieee80211_find_sta().

> > +               sta = ieee80211_find_sta(vif, vif->bss_conf.bssid);
> > +               if (!sta) {
> > +                       struct device *dev = &priv->udev->dev;
> > +                       dev_info(dev, "%s: no sta found\n", __func__);
>
> Does this result in a kernel log message every 2 seconds when the wifi
> interface is not associated to an AP?

Yes. It prints every 2 seconds if not associated.

>
> > +                       rcu_read_unlock();
> > +                       return;
> > +               }
> > +               rcu_read_unlock();
> > +
> > +               signal = ieee80211_ave_rssi(vif);
> > +               if (priv->fops->refresh_rate_mask)
> > +                       priv->fops->refresh_rate_mask(priv, signal, sta);
> > +       }
> > +
> > +       schedule_delayed_work(&priv->watchdog.ra_wq, 2 * HZ);
> > +}
> > +
> >  static int rtl8xxxu_start(struct ieee80211_hw *hw)
> >  {
> >         struct rtl8xxxu_priv *priv = hw->priv;
> > @@ -5878,6 +5911,8 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
> >
> >                 ret = rtl8xxxu_submit_rx_urb(priv, rx_urb);
> >         }
> > +
> > +       schedule_delayed_work(&priv->watchdog.ra_wq, 2* HZ);
> >  exit:
> >         /*
> >          * Accept all data and mgmt frames
> > @@ -6101,6 +6136,7 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
> >         INIT_LIST_HEAD(&priv->rx_urb_pending_list);
> >         spin_lock_init(&priv->rx_urb_lock);
> >         INIT_WORK(&priv->rx_urb_wq, rtl8xxxu_rx_urb_work);
> > +       INIT_DELAYED_WORK(&priv->watchdog.ra_wq, rtl8xxxu_watchdog_callback);
> >
> >         usb_set_intfdata(interface, hw);
> >
> > @@ -6226,6 +6262,8 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
> >         mutex_destroy(&priv->usb_buf_mutex);
> >         mutex_destroy(&priv->h2c_mutex);
> >
> > +       cancel_delayed_work_sync(&priv->watchdog.ra_wq);
> > +
> >         if (priv->udev->state != USB_STATE_NOTATTACHED) {
> >                 dev_info(&priv->udev->dev,
> >                          "Device still attached, trying to reset\n");
> > --
> > 2.21.0
> >

Re: [RFC PATCH 1/2] rtl8xxxu: Add rate adaptive related data

[Joe Perches]

On Thu, 2019-05-09 at 16:11 +0800, Daniel Drake wrote:
> On Fri, May 3, 2019 at 3:22 PM Chris Chiu <chiu@endlessm.com> wrote:
> > Add wireless mode, signal strength level, and rate table index
> > to tell the firmware that we need to adjust the tx rate bitmap
> > accordingly.
[]
> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
[]
> > +/*mlme related.*/
> > +enum wireless_mode {
> > +       WIRELESS_MODE_UNKNOWN = 0,
> > +       //Sub-Element
> 
> Run these patches through checkpatch.pl, it'll have some suggestions
> to bring the coding style in line, for example not using // style
> comments.

just fyi:

checkpatch ignores // comments since 2016
(new in 2019: unless you add --ignore=c99_comment_tolerance)

These are the relevant checkpatch commits:

In 2016, commit dadf680de3c2 ("checkpatch: allow c99 style // comments")
In 2019, commit 98005e8c743f ("checkpatch: allow reporting C99 style comments")

Re: [RFC PATCH 2/2] rtl8xxxu: Add watchdog to update rate mask by signal strength

[Daniel Drake]

On Thu, May 9, 2019 at 5:17 PM Chris Chiu <chiu@endlessm.com> wrote:
> I need the vif because there's seems no easy way to get RSSI. Please
> suggest if there's any better idea for this. I believe multiple vifs is for AP
> mode (with more than 1 virtual AP/SSIDs) and the Station+AP coexist
> mode. But the rtl8xxxu driver basically supports only Station mode.

Yes, the driver only lets you create station interfaces, but it lets
you create several of them.
I'm not sure if that is intentional (and meaningful), or if its a bug.
Maybe you can experiment with multiple station interfaces and see if
it works in a meaningful way?

Daniel

Re: [RFC PATCH 2/2] rtl8xxxu: Add watchdog to update rate mask by signal strength

[Chris Chiu]

On Thu, May 9, 2019 at 7:24 PM Daniel Drake <drake@endlessm.com> wrote:
>
> On Thu, May 9, 2019 at 5:17 PM Chris Chiu <chiu@endlessm.com> wrote:
> > I need the vif because there's seems no easy way to get RSSI. Please
> > suggest if there's any better idea for this. I believe multiple vifs is for AP
> > mode (with more than 1 virtual AP/SSIDs) and the Station+AP coexist
> > mode. But the rtl8xxxu driver basically supports only Station mode.
>
> Yes, the driver only lets you create station interfaces, but it lets
> you create several of them.
> I'm not sure if that is intentional (and meaningful), or if its a bug.
> Maybe you can experiment with multiple station interfaces and see if
> it works in a meaningful way?
>
> Daniel

I've verified that multiple virtual interface can not work simultaneously in
STA mode. I assigned different mac address for different vifs, I can only
bring only one interface up. If I want to bring the second vif up, it always
complains "SIOCSIFFLAGS: Device or resource busy". But I'm sure that
STA vif can coexist with monitor mode vif. So I may need to add the code
login to make the watchdog only work with the vif in STA mode. Then I
can also store vif in the rtl8xxxu_priv structure if we presume it only works
for station mode. Anyone can suggest whether if this assumption is correct
or not?

Chris

Re: [RFC PATCH 2/2] rtl8xxxu: Add watchdog to update rate mask by signal strength

[Daniel Drake]

On Fri, May 10, 2019 at 2:37 AM Chris Chiu <chiu@endlessm.com> wrote:
> I've verified that multiple virtual interface can not work simultaneously in
> STA mode. I assigned different mac address for different vifs, I can only
> bring only one interface up. If I want to bring the second vif up, it always
> complains "SIOCSIFFLAGS: Device or resource busy".

Interesting. Can you go deeper into that so that we can be more
confident of this limitation?

ieee80211_open() is the starting point.
ieee80211_check_concurrent_iface() is one candidate to generate -EBUSY
but from inspection, I don't think that's happening in this case,
perhaps you can keep following through in order to figure out which
part of the code is not allowing the 2nd STA interface to come up.

Daniel

Re: [RFC PATCH 2/2] rtl8xxxu: Add watchdog to update rate mask by signal strength

[Chris Chiu]

On Wed, May 22, 2019 at 2:38 AM Daniel Drake <drake@endlessm.com> wrote:
>
> On Fri, May 10, 2019 at 2:37 AM Chris Chiu <chiu@endlessm.com> wrote:
> > I've verified that multiple virtual interface can not work simultaneously in
> > STA mode. I assigned different mac address for different vifs, I can only
> > bring only one interface up. If I want to bring the second vif up, it always
> > complains "SIOCSIFFLAGS: Device or resource busy".
>
> Interesting. Can you go deeper into that so that we can be more
> confident of this limitation?
>
> ieee80211_open() is the starting point.
> ieee80211_check_concurrent_iface() is one candidate to generate -EBUSY
> but from inspection, I don't think that's happening in this case,
> perhaps you can keep following through in order to figure out which
> part of the code is not allowing the 2nd STA interface to come up.
>
> Daniel

The -EBUSY is returned by the ieee80211_check_combinations() in the
ieee80211_check_concurrent_iface() function which is invoked each time
doing ieee80211_open().
The ieee80211_check_combinations() returns the -EBUSY because of
cfg80211_check_combinations() will iterate all interfaces of different types
then checks the combination is valid or not, which in this case the number
of interface combination accumulated by cfg80211_iter_sum_ifcombos is 0
when I'm trying to bring up the second station interface.

Chris

Re: [RFC PATCH 2/2] rtl8xxxu: Add watchdog to update rate mask by signal strength

[Daniel Drake]

On Mon, May 27, 2019 at 12:38 AM Chris Chiu <chiu@endlessm.com> wrote:
> The -EBUSY is returned by the ieee80211_check_combinations() in the
> ieee80211_check_concurrent_iface() function which is invoked each time
> doing ieee80211_open().
> The ieee80211_check_combinations() returns the -EBUSY because of
> cfg80211_check_combinations() will iterate all interfaces of different types
> then checks the combination is valid or not, which in this case the number
> of interface combination accumulated by cfg80211_iter_sum_ifcombos is 0
> when I'm trying to bring up the second station interface.

Thanks for clarifying. I see, rtl8xxxu does not provide any
iface_combinations so the default is to reject parallel interfaces.

Given that we can now confidently say that multiple interfaces are not
supported, I think that inside rtl8xxxu_add_interface() you could
store a pointer to the vif inside the rtl8xxxu_priv structure (and
clear it in rtl8xxxu_remove_interface). Plus for clarity, add a
comment pointing out that only a single interface is supported, and
make rtl8xxxu_add_interface() acually fail if we already had a
non-NULL pointer stored in the priv structure.

Then you can simplify the patch to remove the ugly storing of vif
inside rtl8xxxu_watchdog. You can store the delayed_work in the main
priv struct too, dropping rtl8xxxu_watchdog altogether.

Daniel