On Thu, May 9, 2019 at 4:11 PM Daniel Drake [off-list ref] wrote:

Hi Chris,

Great work on finding this!

On Fri, May 3, 2019 at 3:22 PM Chris Chiu [off-list ref] wrote:

quoted

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.

quoted

---
 .../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.

quoted

 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().

quoted

+               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.

quoted

+                       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