Re: [RFC PATCH 1/2] rtl8xxxu: Add rate adaptive related data
From: Daniel Drake <hidden>
Date: 2019-05-09 08:11:40
Also in:
linux-wireless, lkml
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 [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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!