From: Daniel Drake <drake@endlessm.com>
To: Chris Chiu <chiu@endlessm.com>
Cc: jes.sorensen@gmail.com, Kalle Valo <kvalo@codeaurora.org>,
David Miller <davem@davemloft.net>,
linux-wireless <linux-wireless@vger.kernel.org>,
netdev <netdev@vger.kernel.org>,
Linux Kernel <linux-kernel@vger.kernel.org>,
Linux Upstreaming Team <linux@endlessm.com>,
Larry Finger <Larry.Finger@lwfinger.net>
Subject: Re: [RFC PATCH 2/2] rtl8xxxu: Add watchdog to update rate mask by signal strength
Date: Thu, 9 May 2019 16:11:17 +0800 [thread overview]
Message-ID: <CAD8Lp47_-6d2wCAs5QbuR6Mw2w91TyJ9W3kFiJHH4F_6dXqnHg@mail.gmail.com> (raw)
In-Reply-To: <20190503072146.49999-3-chiu@endlessm.com>
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
>
next prev parent reply other threads:[~2019-05-09 8:11 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-03 7:21 [RFC PATCH 0/2] Improve TX performance of RTL8723BU on rtl8xxxu driver Chris Chiu
2019-05-03 7:21 ` [RFC PATCH 1/2] rtl8xxxu: Add rate adaptive related data Chris Chiu
2019-05-03 7:49 ` Kalle Valo
2019-05-09 8:11 ` Daniel Drake
2019-05-09 9:29 ` Joe Perches
2019-05-03 7:21 ` [RFC PATCH 2/2] rtl8xxxu: Add watchdog to update rate mask by signal strength Chris Chiu
2019-05-09 8:11 ` Daniel Drake [this message]
2019-05-09 9:17 ` Chris Chiu
2019-05-09 11:24 ` Daniel Drake
2019-05-10 8:36 ` Chris Chiu
2019-05-21 18:38 ` Daniel Drake
2019-05-27 6:38 ` Chris Chiu
2019-05-27 16:40 ` Daniel Drake
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAD8Lp47_-6d2wCAs5QbuR6Mw2w91TyJ9W3kFiJHH4F_6dXqnHg@mail.gmail.com \
--to=drake@endlessm.com \
--cc=Larry.Finger@lwfinger.net \
--cc=chiu@endlessm.com \
--cc=davem@davemloft.net \
--cc=jes.sorensen@gmail.com \
--cc=kvalo@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=linux@endlessm.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).