| Message ID | 20190503072146.49999-3-chiu@endlessm.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | Improve TX performance of RTL8723BU on rtl8xxxu driver | expand |
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 >
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 > >
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
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
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
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
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
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");