* [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-26 9:28 ` s.gottschall
0 siblings, 0 replies; 22+ messages in thread
From: s.gottschall @ 2018-04-26 9:28 UTC (permalink / raw)
To: ath10k, linux-wireless; +Cc: kvalo, Sebastian Gottschall
From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original code
initialized the parameter with wrong masked values.
This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
to the QCA sourcecodes.
Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
v2: remove debug messages
---
drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
3 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 5be6386ede8f..df79af89ee71 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
arg->peer_flags |= ar->wmi.peer_flags->bw80;
- if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
+ if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
arg->peer_flags |= ar->wmi.peer_flags->bw160;
+ }
/* Calculate peer NSS capability from VHT capabilities if STA
* supports VHT.
@@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
__le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
- ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
- sta->addr, arg->peer_max_mpdu, arg->peer_flags);
+ if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
+ arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
+ }
- if (arg->peer_vht_rates.rx_max_rate &&
- (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
- switch (arg->peer_vht_rates.rx_max_rate) {
- case 1560:
- /* Must be 2x2 at 160Mhz is all it can do. */
- arg->peer_bw_rxnss_override = 2;
- break;
- case 780:
- /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
- arg->peer_bw_rxnss_override = 1;
- break;
- }
+ if (arg->peer_num_spatial_streams && arg->peer_phymode == MODE_11AC_VHT80_80) {
+ arg->peer_bw_rxnss_override |= BW_NSS_FWCONF_80_80(arg->peer_num_spatial_streams);
}
+
+ /* In very exceptional conditions it is observed that
+ * firmware was receiving bw_rxnss_override as 0 for peer from host, and resulting in Target Assert.
+ * Changing the rxnss_override to minimum nss. This is a temporary WAR. Needs to be fixed
+ * properly.
+ */
+ if (!arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT80_80 || arg->peer_phymode == MODE_11AC_VHT160)) {
+ arg->peer_bw_rxnss_override = BW_NSS_FWCONF_MAP_ENABLE;
+ }
+
+ ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
+ sta->addr, arg->peer_max_mpdu, arg->peer_flags);
}
static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
@@ -2696,9 +2700,9 @@ static int ath10k_peer_assoc_prepare(struct ath10k *ar,
ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
+ ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
- ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
return 0;
}
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 2c36256a441d..7d72fdc703c8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -7212,9 +7212,7 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k *ar, void *buf,
ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
if (arg->peer_bw_rxnss_override)
- cmd->peer_bw_rxnss_override =
- __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
- BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
+ cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
else
cmd->peer_bw_rxnss_override = 0;
}
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 46ae19bb2c92..1fe0aa5523a6 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -6380,7 +6380,19 @@ struct wmi_10_2_peer_assoc_complete_cmd {
__le32 info0; /* WMI_PEER_ASSOC_INFO0_ */
} __packed;
-#define PEER_BW_RXNSS_OVERRIDE_OFFSET 31
+#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
+#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
+#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
+#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
+#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
+#define BW_NSS_FWCONF_MAP_M (0x0000003F)
+
+#define GET_BW_NSS_FWCONF_160(x) ((((x) & BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
+#define GET_BW_NSS_FWCONF_80_80(x) ((((x) & BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
+
+/* Values defined to set 160 MHz Bandwidth NSS Mapping into FW*/
+#define BW_NSS_FWCONF_160(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
+#define BW_NSS_FWCONF_80_80(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) & BW_NSS_FWCONF_MAP_80_80MHZ_M))
struct wmi_10_4_peer_assoc_complete_cmd {
struct wmi_10_2_peer_assoc_complete_cmd cmd;
--
2.14.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-26 9:28 ` s.gottschall
0 siblings, 0 replies; 22+ messages in thread
From: s.gottschall @ 2018-04-26 9:28 UTC (permalink / raw)
To: ath10k, linux-wireless; +Cc: Sebastian Gottschall, kvalo
From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original code
initialized the parameter with wrong masked values.
This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
to the QCA sourcecodes.
Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
v2: remove debug messages
---
drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
3 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 5be6386ede8f..df79af89ee71 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
arg->peer_flags |= ar->wmi.peer_flags->bw80;
- if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
+ if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
arg->peer_flags |= ar->wmi.peer_flags->bw160;
+ }
/* Calculate peer NSS capability from VHT capabilities if STA
* supports VHT.
@@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
__le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
- ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
- sta->addr, arg->peer_max_mpdu, arg->peer_flags);
+ if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
+ arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
+ }
- if (arg->peer_vht_rates.rx_max_rate &&
- (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
- switch (arg->peer_vht_rates.rx_max_rate) {
- case 1560:
- /* Must be 2x2 at 160Mhz is all it can do. */
- arg->peer_bw_rxnss_override = 2;
- break;
- case 780:
- /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
- arg->peer_bw_rxnss_override = 1;
- break;
- }
+ if (arg->peer_num_spatial_streams && arg->peer_phymode == MODE_11AC_VHT80_80) {
+ arg->peer_bw_rxnss_override |= BW_NSS_FWCONF_80_80(arg->peer_num_spatial_streams);
}
+
+ /* In very exceptional conditions it is observed that
+ * firmware was receiving bw_rxnss_override as 0 for peer from host, and resulting in Target Assert.
+ * Changing the rxnss_override to minimum nss. This is a temporary WAR. Needs to be fixed
+ * properly.
+ */
+ if (!arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT80_80 || arg->peer_phymode == MODE_11AC_VHT160)) {
+ arg->peer_bw_rxnss_override = BW_NSS_FWCONF_MAP_ENABLE;
+ }
+
+ ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
+ sta->addr, arg->peer_max_mpdu, arg->peer_flags);
}
static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
@@ -2696,9 +2700,9 @@ static int ath10k_peer_assoc_prepare(struct ath10k *ar,
ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
+ ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
- ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
return 0;
}
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 2c36256a441d..7d72fdc703c8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -7212,9 +7212,7 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k *ar, void *buf,
ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
if (arg->peer_bw_rxnss_override)
- cmd->peer_bw_rxnss_override =
- __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
- BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
+ cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
else
cmd->peer_bw_rxnss_override = 0;
}
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 46ae19bb2c92..1fe0aa5523a6 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -6380,7 +6380,19 @@ struct wmi_10_2_peer_assoc_complete_cmd {
__le32 info0; /* WMI_PEER_ASSOC_INFO0_ */
} __packed;
-#define PEER_BW_RXNSS_OVERRIDE_OFFSET 31
+#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
+#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
+#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
+#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
+#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
+#define BW_NSS_FWCONF_MAP_M (0x0000003F)
+
+#define GET_BW_NSS_FWCONF_160(x) ((((x) & BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
+#define GET_BW_NSS_FWCONF_80_80(x) ((((x) & BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
+
+/* Values defined to set 160 MHz Bandwidth NSS Mapping into FW*/
+#define BW_NSS_FWCONF_160(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
+#define BW_NSS_FWCONF_80_80(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) & BW_NSS_FWCONF_MAP_80_80MHZ_M))
struct wmi_10_4_peer_assoc_complete_cmd {
struct wmi_10_2_peer_assoc_complete_cmd cmd;
--
2.14.1
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
2018-04-26 9:28 ` s.gottschall
@ 2018-04-26 15:30 ` Ben Greear
-1 siblings, 0 replies; 22+ messages in thread
From: Ben Greear @ 2018-04-26 15:30 UTC (permalink / raw)
To: s.gottschall, ath10k, linux-wireless; +Cc: kvalo
On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>
> current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
> to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original code
> initialized the parameter with wrong masked values.
> This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
> if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
> to the QCA sourcecodes.
>
> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>
> v2: remove debug messages
> ---
> drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
> 3 files changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 5be6386ede8f..df79af89ee71 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>
> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
> arg->peer_flags |= ar->wmi.peer_flags->bw160;
> + }
>
> /* Calculate peer NSS capability from VHT capabilities if STA
> * supports VHT.
> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>
> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
> + if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
> + }
So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From what I can tell,
the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if peer is VHT-160,
then of course it can only talk at 2x2.
So, I don't think you can just look at the peer_num_spatial_streams here.
> - if (arg->peer_vht_rates.rx_max_rate &&
> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
> - switch (arg->peer_vht_rates.rx_max_rate) {
> - case 1560:
> - /* Must be 2x2 at 160Mhz is all it can do. */
> - arg->peer_bw_rxnss_override = 2;
> - break;
> - case 780:
> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
> - arg->peer_bw_rxnss_override = 1;
> - break;
> - }
This old code does look wrong, the firmware is using zero-based, so override-0 == nss-1, override-1 == nss-2.
This is confusing enough that it deserves a comment in the code I think....
> + if (arg->peer_num_spatial_streams && arg->peer_phymode == MODE_11AC_VHT80_80) {
> + arg->peer_bw_rxnss_override |= BW_NSS_FWCONF_80_80(arg->peer_num_spatial_streams);
> }
> +
> + /* In very exceptional conditions it is observed that
> + * firmware was receiving bw_rxnss_override as 0 for peer from host, and resulting in Target Assert.
> + * Changing the rxnss_override to minimum nss. This is a temporary WAR. Needs to be fixed
> + * properly.
> + */
> + if (!arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT80_80 || arg->peer_phymode == MODE_11AC_VHT160)) {
> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_MAP_ENABLE;
> + }
> +
> + ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
> + sta->addr, arg->peer_max_mpdu, arg->peer_flags);
> }
>
> static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
> @@ -2696,9 +2700,9 @@ static int ath10k_peer_assoc_prepare(struct ath10k *ar,
> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>
> return 0;
> }
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 2c36256a441d..7d72fdc703c8 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -7212,9 +7212,7 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k *ar, void *buf,
>
> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
> if (arg->peer_bw_rxnss_override)
> - cmd->peer_bw_rxnss_override =
> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
> + cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
> else
> cmd->peer_bw_rxnss_override = 0;
> }
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
> index 46ae19bb2c92..1fe0aa5523a6 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -6380,7 +6380,19 @@ struct wmi_10_2_peer_assoc_complete_cmd {
> __le32 info0; /* WMI_PEER_ASSOC_INFO0_ */
> } __packed;
>
> -#define PEER_BW_RXNSS_OVERRIDE_OFFSET 31
> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
Older FW does not pay attention to the 80_80 bits. It uses masks so it is
backwards-compat, but maybe worth a comment.
> +#define BW_NSS_FWCONF_MAP_M (0x0000003F)
> +
> +#define GET_BW_NSS_FWCONF_160(x) ((((x) & BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
> +#define GET_BW_NSS_FWCONF_80_80(x) ((((x) & BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
> +
> +/* Values defined to set 160 MHz Bandwidth NSS Mapping into FW*/
> +#define BW_NSS_FWCONF_160(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
> +#define BW_NSS_FWCONF_80_80(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) & BW_NSS_FWCONF_MAP_80_80MHZ_M))
>
> struct wmi_10_4_peer_assoc_complete_cmd {
> struct wmi_10_2_peer_assoc_complete_cmd cmd;
>
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-26 15:30 ` Ben Greear
0 siblings, 0 replies; 22+ messages in thread
From: Ben Greear @ 2018-04-26 15:30 UTC (permalink / raw)
To: s.gottschall, ath10k, linux-wireless; +Cc: kvalo
On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>
> current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
> to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original code
> initialized the parameter with wrong masked values.
> This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
> if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
> to the QCA sourcecodes.
>
> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>
> v2: remove debug messages
> ---
> drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
> 3 files changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 5be6386ede8f..df79af89ee71 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>
> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
> arg->peer_flags |= ar->wmi.peer_flags->bw160;
> + }
>
> /* Calculate peer NSS capability from VHT capabilities if STA
> * supports VHT.
> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>
> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
> + if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
> + }
So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From what I can tell,
the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if peer is VHT-160,
then of course it can only talk at 2x2.
So, I don't think you can just look at the peer_num_spatial_streams here.
> - if (arg->peer_vht_rates.rx_max_rate &&
> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
> - switch (arg->peer_vht_rates.rx_max_rate) {
> - case 1560:
> - /* Must be 2x2 at 160Mhz is all it can do. */
> - arg->peer_bw_rxnss_override = 2;
> - break;
> - case 780:
> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
> - arg->peer_bw_rxnss_override = 1;
> - break;
> - }
This old code does look wrong, the firmware is using zero-based, so override-0 == nss-1, override-1 == nss-2.
This is confusing enough that it deserves a comment in the code I think....
> + if (arg->peer_num_spatial_streams && arg->peer_phymode == MODE_11AC_VHT80_80) {
> + arg->peer_bw_rxnss_override |= BW_NSS_FWCONF_80_80(arg->peer_num_spatial_streams);
> }
> +
> + /* In very exceptional conditions it is observed that
> + * firmware was receiving bw_rxnss_override as 0 for peer from host, and resulting in Target Assert.
> + * Changing the rxnss_override to minimum nss. This is a temporary WAR. Needs to be fixed
> + * properly.
> + */
> + if (!arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT80_80 || arg->peer_phymode == MODE_11AC_VHT160)) {
> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_MAP_ENABLE;
> + }
> +
> + ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
> + sta->addr, arg->peer_max_mpdu, arg->peer_flags);
> }
>
> static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
> @@ -2696,9 +2700,9 @@ static int ath10k_peer_assoc_prepare(struct ath10k *ar,
> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>
> return 0;
> }
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 2c36256a441d..7d72fdc703c8 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -7212,9 +7212,7 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k *ar, void *buf,
>
> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
> if (arg->peer_bw_rxnss_override)
> - cmd->peer_bw_rxnss_override =
> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
> + cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
> else
> cmd->peer_bw_rxnss_override = 0;
> }
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
> index 46ae19bb2c92..1fe0aa5523a6 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -6380,7 +6380,19 @@ struct wmi_10_2_peer_assoc_complete_cmd {
> __le32 info0; /* WMI_PEER_ASSOC_INFO0_ */
> } __packed;
>
> -#define PEER_BW_RXNSS_OVERRIDE_OFFSET 31
> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
Older FW does not pay attention to the 80_80 bits. It uses masks so it is
backwards-compat, but maybe worth a comment.
> +#define BW_NSS_FWCONF_MAP_M (0x0000003F)
> +
> +#define GET_BW_NSS_FWCONF_160(x) ((((x) & BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
> +#define GET_BW_NSS_FWCONF_80_80(x) ((((x) & BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
> +
> +/* Values defined to set 160 MHz Bandwidth NSS Mapping into FW*/
> +#define BW_NSS_FWCONF_160(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
> +#define BW_NSS_FWCONF_80_80(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) & BW_NSS_FWCONF_MAP_80_80MHZ_M))
>
> struct wmi_10_4_peer_assoc_complete_cmd {
> struct wmi_10_2_peer_assoc_complete_cmd cmd;
>
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
2018-04-26 15:30 ` Ben Greear
@ 2018-04-26 20:21 ` Sebastian Gottschall
-1 siblings, 0 replies; 22+ messages in thread
From: Sebastian Gottschall @ 2018-04-26 20:21 UTC (permalink / raw)
To: Ben Greear, ath10k, linux-wireless; +Cc: kvalo
Am 26.04.2018 um 17:30 schrieb Ben Greear:
> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>
>> current handling of peer_bw_rxnss_override parameter is based on
>> guessing the VHT160/8080 capability by rx rate. this is wrong and may
>> lead
>> to a non initialized peer_bw_rxnss_override parameter which is
>> required since VHT160 operation mode only supports 2x2 chainmasks in
>> addition the original code
>> initialized the parameter with wrong masked values.
>> This patch uses the peer phymode and peer nss information for correct
>> initialisation of the peer_bw_rxnss_override parameter.
>> if this peer information is not available, we initialize the
>> parameter by minimum nss which is suggested by QCA as temporary
>> workaround according
>> to the QCA sourcecodes.
>>
>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>
>> v2: remove debug messages
>> ---
>> drivers/net/wireless/ath/ath10k/mac.c | 36
>> +++++++++++++++++++----------------
>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>> b/drivers/net/wireless/ath/ath10k/mac.c
>> index 5be6386ede8f..df79af89ee71 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct
>> ath10k *ar,
>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>
>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>> + }
>>
>> /* Calculate peer NSS capability from VHT capabilities if STA
>> * supports VHT.
>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct
>> ath10k *ar,
>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>
>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d
>> flags 0x%x\n",
>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode ==
>> MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>> + arg->peer_bw_rxnss_override =
>> BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>> + }
>
> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From
> what I can tell,
> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if
> peer is VHT-160,
> then of course it can only talk at 2x2.
>
> So, I don't think you can just look at the peer_num_spatial_streams here.
no? rxnss_override is only taked if peer phymode is vht160 or vht80_80.
vht80 is not considered in that code PEER phy_mode, not HOST phy_mode
this parameter is a assoc per peer parameter as far as i can say here.
consider that the firmware will accept anything except 0 for
peer_bw_rxnss_override in vht160 operation mode
if a peer is 80 mhz, the code will be skipped here.
>
>
>> - if (arg->peer_vht_rates.rx_max_rate &&
>> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
>> - switch (arg->peer_vht_rates.rx_max_rate) {
>> - case 1560:
>> - /* Must be 2x2 at 160Mhz is all it can do. */
>> - arg->peer_bw_rxnss_override = 2;
>> - break;
>> - case 780:
>> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
>> - arg->peer_bw_rxnss_override = 1;
>> - break;
>> - }
>
> This old code does look wrong, the firmware is using zero-based, so
> override-0 == nss-1, override-1 == nss-2.
0 = crash
and 1 and 2 is wrong.
+#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
+#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
+#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
+#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
+#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>
> This is confusing enough that it deserves a comment in the code I
> think....
the removal doesnt deserve a comment. i dont know how to explain that
its simply wrong. it uses the wrong
bit masks and this has been written in the initial patch description
>
>> + if (arg->peer_num_spatial_streams && arg->peer_phymode ==
>> MODE_11AC_VHT80_80) {
>> + arg->peer_bw_rxnss_override |=
>> BW_NSS_FWCONF_80_80(arg->peer_num_spatial_streams);
>> }
>> +
>> + /* In very exceptional conditions it is observed that
>> + * firmware was receiving bw_rxnss_override as 0 for peer from
>> host, and resulting in Target Assert.
>> + * Changing the rxnss_override to minimum nss. This is a
>> temporary WAR. Needs to be fixed
>> + * properly.
>> + */
>> + if (!arg->peer_num_spatial_streams && (arg->peer_phymode ==
>> MODE_11AC_VHT80_80 || arg->peer_phymode == MODE_11AC_VHT160)) {
>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_MAP_ENABLE;
>> + }
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d
>> flags 0x%x\n",
>> + sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>> }
>>
>> static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
>> @@ -2696,9 +2700,9 @@ static int ath10k_peer_assoc_prepare(struct
>> ath10k *ar,
>> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
>> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
>> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>
>> return 0;
>> }
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
>> b/drivers/net/wireless/ath/ath10k/wmi.c
>> index 2c36256a441d..7d72fdc703c8 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>> @@ -7212,9 +7212,7 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k
>> *ar, void *buf,
>>
>> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
>> if (arg->peer_bw_rxnss_override)
>> - cmd->peer_bw_rxnss_override =
>> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
>> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
>> + cmd->peer_bw_rxnss_override =
>> __cpu_to_le32(arg->peer_bw_rxnss_override);
>> else
>> cmd->peer_bw_rxnss_override = 0;
>> }
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h
>> b/drivers/net/wireless/ath/ath10k/wmi.h
>> index 46ae19bb2c92..1fe0aa5523a6 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.h
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
>> @@ -6380,7 +6380,19 @@ struct wmi_10_2_peer_assoc_complete_cmd {
>> __le32 info0; /* WMI_PEER_ASSOC_INFO0_ */
>> } __packed;
>>
>> -#define PEER_BW_RXNSS_OVERRIDE_OFFSET 31
>> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
>> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
>> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>
> Older FW does not pay attention to the 80_80 bits. It uses masks so
> it is
> backwards-compat, but maybe worth a comment.
i took that masks from recent 3.5.3 based qca code. its correct and if
its backward compatible, it's still correct
i mean the old code did not coment anything. so dont know how to write
anything more about it now. see patch description
>
>> +#define BW_NSS_FWCONF_MAP_M (0x0000003F)
>> +
>> +#define GET_BW_NSS_FWCONF_160(x) ((((x) &
>> BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
>> +#define GET_BW_NSS_FWCONF_80_80(x) ((((x) &
>> BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
>> +
>> +/* Values defined to set 160 MHz Bandwidth NSS Mapping into FW*/
>> +#define BW_NSS_FWCONF_160(x) (BW_NSS_FWCONF_MAP_ENABLE |
>> (((x - 1) << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
>> +#define BW_NSS_FWCONF_80_80(x) (BW_NSS_FWCONF_MAP_ENABLE |
>> (((x - 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) &
>> BW_NSS_FWCONF_MAP_80_80MHZ_M))
>>
>> struct wmi_10_4_peer_assoc_complete_cmd {
>> struct wmi_10_2_peer_assoc_complete_cmd cmd;
>>
>
> Thanks,
> Ben
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-26 20:21 ` Sebastian Gottschall
0 siblings, 0 replies; 22+ messages in thread
From: Sebastian Gottschall @ 2018-04-26 20:21 UTC (permalink / raw)
To: Ben Greear, ath10k, linux-wireless; +Cc: kvalo
Am 26.04.2018 um 17:30 schrieb Ben Greear:
> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>
>> current handling of peer_bw_rxnss_override parameter is based on
>> guessing the VHT160/8080 capability by rx rate. this is wrong and may
>> lead
>> to a non initialized peer_bw_rxnss_override parameter which is
>> required since VHT160 operation mode only supports 2x2 chainmasks in
>> addition the original code
>> initialized the parameter with wrong masked values.
>> This patch uses the peer phymode and peer nss information for correct
>> initialisation of the peer_bw_rxnss_override parameter.
>> if this peer information is not available, we initialize the
>> parameter by minimum nss which is suggested by QCA as temporary
>> workaround according
>> to the QCA sourcecodes.
>>
>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>
>> v2: remove debug messages
>> ---
>> drivers/net/wireless/ath/ath10k/mac.c | 36
>> +++++++++++++++++++----------------
>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>> b/drivers/net/wireless/ath/ath10k/mac.c
>> index 5be6386ede8f..df79af89ee71 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct
>> ath10k *ar,
>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>
>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>> + }
>>
>> /* Calculate peer NSS capability from VHT capabilities if STA
>> * supports VHT.
>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct
>> ath10k *ar,
>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>
>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d
>> flags 0x%x\n",
>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode ==
>> MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>> + arg->peer_bw_rxnss_override =
>> BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>> + }
>
> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From
> what I can tell,
> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if
> peer is VHT-160,
> then of course it can only talk at 2x2.
>
> So, I don't think you can just look at the peer_num_spatial_streams here.
no? rxnss_override is only taked if peer phymode is vht160 or vht80_80.
vht80 is not considered in that code PEER phy_mode, not HOST phy_mode
this parameter is a assoc per peer parameter as far as i can say here.
consider that the firmware will accept anything except 0 for
peer_bw_rxnss_override in vht160 operation mode
if a peer is 80 mhz, the code will be skipped here.
>
>
>> - if (arg->peer_vht_rates.rx_max_rate &&
>> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
>> - switch (arg->peer_vht_rates.rx_max_rate) {
>> - case 1560:
>> - /* Must be 2x2 at 160Mhz is all it can do. */
>> - arg->peer_bw_rxnss_override = 2;
>> - break;
>> - case 780:
>> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
>> - arg->peer_bw_rxnss_override = 1;
>> - break;
>> - }
>
> This old code does look wrong, the firmware is using zero-based, so
> override-0 == nss-1, override-1 == nss-2.
0 = crash
and 1 and 2 is wrong.
+#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
+#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
+#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
+#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
+#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>
> This is confusing enough that it deserves a comment in the code I
> think....
the removal doesnt deserve a comment. i dont know how to explain that
its simply wrong. it uses the wrong
bit masks and this has been written in the initial patch description
>
>> + if (arg->peer_num_spatial_streams && arg->peer_phymode ==
>> MODE_11AC_VHT80_80) {
>> + arg->peer_bw_rxnss_override |=
>> BW_NSS_FWCONF_80_80(arg->peer_num_spatial_streams);
>> }
>> +
>> + /* In very exceptional conditions it is observed that
>> + * firmware was receiving bw_rxnss_override as 0 for peer from
>> host, and resulting in Target Assert.
>> + * Changing the rxnss_override to minimum nss. This is a
>> temporary WAR. Needs to be fixed
>> + * properly.
>> + */
>> + if (!arg->peer_num_spatial_streams && (arg->peer_phymode ==
>> MODE_11AC_VHT80_80 || arg->peer_phymode == MODE_11AC_VHT160)) {
>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_MAP_ENABLE;
>> + }
>> +
>> + ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d
>> flags 0x%x\n",
>> + sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>> }
>>
>> static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
>> @@ -2696,9 +2700,9 @@ static int ath10k_peer_assoc_prepare(struct
>> ath10k *ar,
>> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
>> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
>> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>
>> return 0;
>> }
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
>> b/drivers/net/wireless/ath/ath10k/wmi.c
>> index 2c36256a441d..7d72fdc703c8 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>> @@ -7212,9 +7212,7 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k
>> *ar, void *buf,
>>
>> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
>> if (arg->peer_bw_rxnss_override)
>> - cmd->peer_bw_rxnss_override =
>> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
>> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
>> + cmd->peer_bw_rxnss_override =
>> __cpu_to_le32(arg->peer_bw_rxnss_override);
>> else
>> cmd->peer_bw_rxnss_override = 0;
>> }
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h
>> b/drivers/net/wireless/ath/ath10k/wmi.h
>> index 46ae19bb2c92..1fe0aa5523a6 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.h
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
>> @@ -6380,7 +6380,19 @@ struct wmi_10_2_peer_assoc_complete_cmd {
>> __le32 info0; /* WMI_PEER_ASSOC_INFO0_ */
>> } __packed;
>>
>> -#define PEER_BW_RXNSS_OVERRIDE_OFFSET 31
>> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
>> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
>> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>
> Older FW does not pay attention to the 80_80 bits. It uses masks so
> it is
> backwards-compat, but maybe worth a comment.
i took that masks from recent 3.5.3 based qca code. its correct and if
its backward compatible, it's still correct
i mean the old code did not coment anything. so dont know how to write
anything more about it now. see patch description
>
>> +#define BW_NSS_FWCONF_MAP_M (0x0000003F)
>> +
>> +#define GET_BW_NSS_FWCONF_160(x) ((((x) &
>> BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
>> +#define GET_BW_NSS_FWCONF_80_80(x) ((((x) &
>> BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
>> +
>> +/* Values defined to set 160 MHz Bandwidth NSS Mapping into FW*/
>> +#define BW_NSS_FWCONF_160(x) (BW_NSS_FWCONF_MAP_ENABLE |
>> (((x - 1) << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
>> +#define BW_NSS_FWCONF_80_80(x) (BW_NSS_FWCONF_MAP_ENABLE |
>> (((x - 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) &
>> BW_NSS_FWCONF_MAP_80_80MHZ_M))
>>
>> struct wmi_10_4_peer_assoc_complete_cmd {
>> struct wmi_10_2_peer_assoc_complete_cmd cmd;
>>
>
> Thanks,
> Ben
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
2018-04-26 20:21 ` Sebastian Gottschall
@ 2018-04-26 20:35 ` Ben Greear
-1 siblings, 0 replies; 22+ messages in thread
From: Ben Greear @ 2018-04-26 20:35 UTC (permalink / raw)
To: Sebastian Gottschall, ath10k, linux-wireless; +Cc: kvalo
On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>
>>> current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
>>> to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original code
>>> initialized the parameter with wrong masked values.
>>> This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
>>> if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
>>> to the QCA sourcecodes.
>>>
>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>
>>> v2: remove debug messages
>>> ---
>>> drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>>> index 5be6386ede8f..df79af89ee71 100644
>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>
>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>> + }
>>>
>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>> * supports VHT.
>>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>
>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>> + }
>>
>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From what I can tell,
>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if peer is VHT-160,
>> then of course it can only talk at 2x2.
>>
>> So, I don't think you can just look at the peer_num_spatial_streams here.
> no? rxnss_override is only taked if peer phymode is vht160 or vht80_80. vht80 is not considered in that code PEER phy_mode, not HOST phy_mode
> this parameter is a assoc per peer parameter as far as i can say here.
> consider that the firmware will accept anything except 0 for peer_bw_rxnss_override in vht160 operation mode
> if a peer is 80 mhz, the code will be skipped here.
From what I can tell, this feature is supposed to configure the rate-ctrl in the firmware to know that
it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can send at higher NSS if it sends
at 80Mhz or lower.
If a 2x2 peer connects to the AP, will it have peer_num_spatial_streams set to 2? If so,
then your code will configure the peer_bw_rxnss_override to indicate it can send at 160Mhz
2x2 as well, right? And if so, then that is incorrect.
Probably if you connected something like a IPQ4019 station to a 9984 AP configured for 160Mhz,
the peer_bw_rxnss_override would be set to 2x2 instead of 1x1?
>>> - if (arg->peer_vht_rates.rx_max_rate &&
>>> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
>>> - switch (arg->peer_vht_rates.rx_max_rate) {
>>> - case 1560:
>>> - /* Must be 2x2 at 160Mhz is all it can do. */
>>> - arg->peer_bw_rxnss_override = 2;
>>> - break;
>>> - case 780:
>>> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
>>> - arg->peer_bw_rxnss_override = 1;
>>> - break;
>>> - }
>>
>> This old code does look wrong, the firmware is using zero-based, so override-0 == nss-1, override-1 == nss-2.
> 0 = crash
>
> and 1 and 2 is wrong.
Yes, it should be 0 and 1. The old code | in the (1<<31) later.
>
> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>>
>> This is confusing enough that it deserves a comment in the code I think....
> the removal doesnt deserve a comment. i dont know how to explain that its simply wrong. it uses the wrong
> bit masks and this has been written in the initial patch description
I think a comment like this would be helpful:
/* Note that the rxnss_override is 0 based, so 0 means nss-1 and 7 means nss-8. */
>>
>>> + if (arg->peer_num_spatial_streams && arg->peer_phymode == MODE_11AC_VHT80_80) {
>>> + arg->peer_bw_rxnss_override |= BW_NSS_FWCONF_80_80(arg->peer_num_spatial_streams);
>>> }
>>> +
>>> + /* In very exceptional conditions it is observed that
>>> + * firmware was receiving bw_rxnss_override as 0 for peer from host, and resulting in Target Assert.
>>> + * Changing the rxnss_override to minimum nss. This is a temporary WAR. Needs to be fixed
>>> + * properly.
>>> + */
>>> + if (!arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT80_80 || arg->peer_phymode == MODE_11AC_VHT160)) {
>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_MAP_ENABLE;
>>> + }
>>> +
>>> + ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
>>> + sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>> }
>>>
>>> static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
>>> @@ -2696,9 +2700,9 @@ static int ath10k_peer_assoc_prepare(struct ath10k *ar,
>>> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
>>> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
>>> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
>>> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
>>> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
>>> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>>
>>> return 0;
>>> }
>>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
>>> index 2c36256a441d..7d72fdc703c8 100644
>>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>>> @@ -7212,9 +7212,7 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k *ar, void *buf,
>>>
>>> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
>>> if (arg->peer_bw_rxnss_override)
>>> - cmd->peer_bw_rxnss_override =
>>> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
>>> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
>>> + cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
>>> else
>>> cmd->peer_bw_rxnss_override = 0;
>>> }
>>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
>>> index 46ae19bb2c92..1fe0aa5523a6 100644
>>> --- a/drivers/net/wireless/ath/ath10k/wmi.h
>>> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
>>> @@ -6380,7 +6380,19 @@ struct wmi_10_2_peer_assoc_complete_cmd {
>>> __le32 info0; /* WMI_PEER_ASSOC_INFO0_ */
>>> } __packed;
>>>
>>> -#define PEER_BW_RXNSS_OVERRIDE_OFFSET 31
>>> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
>>> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
>>> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
>>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
>>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>>
>> Older FW does not pay attention to the 80_80 bits. It uses masks so it is
>> backwards-compat, but maybe worth a comment.
> i took that masks from recent 3.5.3 based qca code. its correct and if its backward compatible, it's still correct
> i mean the old code did not coment anything. so dont know how to write anything more about it now. see patch description
>
>>
>>> +#define BW_NSS_FWCONF_MAP_M (0x0000003F)
>>> +
>>> +#define GET_BW_NSS_FWCONF_160(x) ((((x) & BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
>>> +#define GET_BW_NSS_FWCONF_80_80(x) ((((x) & BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
>>> +
>>> +/* Values defined to set 160 MHz Bandwidth NSS Mapping into FW*/
>>> +#define BW_NSS_FWCONF_160(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
>>> +#define BW_NSS_FWCONF_80_80(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) & BW_NSS_FWCONF_MAP_80_80MHZ_M))
>>>
>>> struct wmi_10_4_peer_assoc_complete_cmd {
>>> struct wmi_10_2_peer_assoc_complete_cmd cmd;
>>>
>>
>> Thanks,
>> Ben
>>
>
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-26 20:35 ` Ben Greear
0 siblings, 0 replies; 22+ messages in thread
From: Ben Greear @ 2018-04-26 20:35 UTC (permalink / raw)
To: Sebastian Gottschall, ath10k, linux-wireless; +Cc: kvalo
On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>
>>> current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
>>> to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original code
>>> initialized the parameter with wrong masked values.
>>> This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
>>> if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
>>> to the QCA sourcecodes.
>>>
>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>
>>> v2: remove debug messages
>>> ---
>>> drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>>> index 5be6386ede8f..df79af89ee71 100644
>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>
>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>> + }
>>>
>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>> * supports VHT.
>>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>
>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>> + }
>>
>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From what I can tell,
>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if peer is VHT-160,
>> then of course it can only talk at 2x2.
>>
>> So, I don't think you can just look at the peer_num_spatial_streams here.
> no? rxnss_override is only taked if peer phymode is vht160 or vht80_80. vht80 is not considered in that code PEER phy_mode, not HOST phy_mode
> this parameter is a assoc per peer parameter as far as i can say here.
> consider that the firmware will accept anything except 0 for peer_bw_rxnss_override in vht160 operation mode
> if a peer is 80 mhz, the code will be skipped here.
From what I can tell, this feature is supposed to configure the rate-ctrl in the firmware to know that
it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can send at higher NSS if it sends
at 80Mhz or lower.
If a 2x2 peer connects to the AP, will it have peer_num_spatial_streams set to 2? If so,
then your code will configure the peer_bw_rxnss_override to indicate it can send at 160Mhz
2x2 as well, right? And if so, then that is incorrect.
Probably if you connected something like a IPQ4019 station to a 9984 AP configured for 160Mhz,
the peer_bw_rxnss_override would be set to 2x2 instead of 1x1?
>>> - if (arg->peer_vht_rates.rx_max_rate &&
>>> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
>>> - switch (arg->peer_vht_rates.rx_max_rate) {
>>> - case 1560:
>>> - /* Must be 2x2 at 160Mhz is all it can do. */
>>> - arg->peer_bw_rxnss_override = 2;
>>> - break;
>>> - case 780:
>>> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
>>> - arg->peer_bw_rxnss_override = 1;
>>> - break;
>>> - }
>>
>> This old code does look wrong, the firmware is using zero-based, so override-0 == nss-1, override-1 == nss-2.
> 0 = crash
>
> and 1 and 2 is wrong.
Yes, it should be 0 and 1. The old code | in the (1<<31) later.
>
> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>>
>> This is confusing enough that it deserves a comment in the code I think....
> the removal doesnt deserve a comment. i dont know how to explain that its simply wrong. it uses the wrong
> bit masks and this has been written in the initial patch description
I think a comment like this would be helpful:
/* Note that the rxnss_override is 0 based, so 0 means nss-1 and 7 means nss-8. */
>>
>>> + if (arg->peer_num_spatial_streams && arg->peer_phymode == MODE_11AC_VHT80_80) {
>>> + arg->peer_bw_rxnss_override |= BW_NSS_FWCONF_80_80(arg->peer_num_spatial_streams);
>>> }
>>> +
>>> + /* In very exceptional conditions it is observed that
>>> + * firmware was receiving bw_rxnss_override as 0 for peer from host, and resulting in Target Assert.
>>> + * Changing the rxnss_override to minimum nss. This is a temporary WAR. Needs to be fixed
>>> + * properly.
>>> + */
>>> + if (!arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT80_80 || arg->peer_phymode == MODE_11AC_VHT160)) {
>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_MAP_ENABLE;
>>> + }
>>> +
>>> + ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
>>> + sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>> }
>>>
>>> static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
>>> @@ -2696,9 +2700,9 @@ static int ath10k_peer_assoc_prepare(struct ath10k *ar,
>>> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
>>> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
>>> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
>>> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
>>> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
>>> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>>
>>> return 0;
>>> }
>>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
>>> index 2c36256a441d..7d72fdc703c8 100644
>>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>>> @@ -7212,9 +7212,7 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k *ar, void *buf,
>>>
>>> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
>>> if (arg->peer_bw_rxnss_override)
>>> - cmd->peer_bw_rxnss_override =
>>> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
>>> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
>>> + cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
>>> else
>>> cmd->peer_bw_rxnss_override = 0;
>>> }
>>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
>>> index 46ae19bb2c92..1fe0aa5523a6 100644
>>> --- a/drivers/net/wireless/ath/ath10k/wmi.h
>>> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
>>> @@ -6380,7 +6380,19 @@ struct wmi_10_2_peer_assoc_complete_cmd {
>>> __le32 info0; /* WMI_PEER_ASSOC_INFO0_ */
>>> } __packed;
>>>
>>> -#define PEER_BW_RXNSS_OVERRIDE_OFFSET 31
>>> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
>>> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
>>> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
>>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
>>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>>
>> Older FW does not pay attention to the 80_80 bits. It uses masks so it is
>> backwards-compat, but maybe worth a comment.
> i took that masks from recent 3.5.3 based qca code. its correct and if its backward compatible, it's still correct
> i mean the old code did not coment anything. so dont know how to write anything more about it now. see patch description
>
>>
>>> +#define BW_NSS_FWCONF_MAP_M (0x0000003F)
>>> +
>>> +#define GET_BW_NSS_FWCONF_160(x) ((((x) & BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
>>> +#define GET_BW_NSS_FWCONF_80_80(x) ((((x) & BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
>>> +
>>> +/* Values defined to set 160 MHz Bandwidth NSS Mapping into FW*/
>>> +#define BW_NSS_FWCONF_160(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
>>> +#define BW_NSS_FWCONF_80_80(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) & BW_NSS_FWCONF_MAP_80_80MHZ_M))
>>>
>>> struct wmi_10_4_peer_assoc_complete_cmd {
>>> struct wmi_10_2_peer_assoc_complete_cmd cmd;
>>>
>>
>> Thanks,
>> Ben
>>
>
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
2018-04-26 20:35 ` Ben Greear
@ 2018-04-27 4:40 ` Sebastian Gottschall
-1 siblings, 0 replies; 22+ messages in thread
From: Sebastian Gottschall @ 2018-04-27 4:40 UTC (permalink / raw)
To: Ben Greear, ath10k, linux-wireless; +Cc: kvalo
Am 26.04.2018 um 22:35 schrieb Ben Greear:
> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>
>>>> current handling of peer_bw_rxnss_override parameter is based on
>>>> guessing the VHT160/8080 capability by rx rate. this is wrong and
>>>> may lead
>>>> to a non initialized peer_bw_rxnss_override parameter which is
>>>> required since VHT160 operation mode only supports 2x2 chainmasks
>>>> in addition the original code
>>>> initialized the parameter with wrong masked values.
>>>> This patch uses the peer phymode and peer nss information for
>>>> correct initialisation of the peer_bw_rxnss_override parameter.
>>>> if this peer information is not available, we initialize the
>>>> parameter by minimum nss which is suggested by QCA as temporary
>>>> workaround according
>>>> to the QCA sourcecodes.
>>>>
>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>
>>>> v2: remove debug messages
>>>> ---
>>>> drivers/net/wireless/ath/ath10k/mac.c | 36
>>>> +++++++++++++++++++----------------
>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>>>> b/drivers/net/wireless/ath/ath10k/mac.c
>>>> index 5be6386ede8f..df79af89ee71 100644
>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct
>>>> ath10k *ar,
>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>
>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>> + }
>>>>
>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>> * supports VHT.
>>>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct
>>>> ath10k *ar,
>>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>
>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d
>>>> flags 0x%x\n",
>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode ==
>>>> MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>> + arg->peer_bw_rxnss_override =
>>>> BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>> + }
>>>
>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP.
>>> From what I can tell,
>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if
>>> peer is VHT-160,
>>> then of course it can only talk at 2x2.
>>>
>>> So, I don't think you can just look at the peer_num_spatial_streams
>>> here.
>> no? rxnss_override is only taked if peer phymode is vht160 or
>> vht80_80. vht80 is not considered in that code PEER phy_mode, not
>> HOST phy_mode
>> this parameter is a assoc per peer parameter as far as i can say here.
>> consider that the firmware will accept anything except 0 for
>> peer_bw_rxnss_override in vht160 operation mode
>> if a peer is 80 mhz, the code will be skipped here.
>
> From what I can tell, this feature is supposed to configure the
> rate-ctrl in the firmware to know that
> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can
> send at higher NSS if it sends
> at 80Mhz or lower.
right. but thats exactly what it should does in case that a peer is
connecting with vht160 / 80_80
and the peer itself does also send his own nss capabilities which is
used if available. if not ,it uses the fallback.
>
> If a 2x2 peer connects to the AP, will it have
> peer_num_spatial_streams set to 2?
yes. i had some debug code in my initial early versions. the peer does
transmit his own nss capabilities.
> If so,
> then your code will configure the peer_bw_rxnss_override to indicate
> it can send at 160Mhz
> 2x2 as well, right? And if so, then that is incorrect.
no. since nss_override is only set if peer phymode is 160 mhz as well
>
> Probably if you connected something like a IPQ4019 station to a 9984
> AP configured for 160Mhz,
> the peer_bw_rxnss_override would be set to 2x2 instead of 1x1?
depends how the ipq4019 chipset is configured. if it itself is
configured to 160 mhz and configured to 2x2, yes
but the peer itself can also send with 1x1, then the ap will also send
just 1x1
but as i said. this all is just used if the peer is configured to vht160
and if num_spatial_stream is set by peer.
of not this is all ignored and only 1<<31 is masked. this way has been
taken from qca's propertiery code
or lets say not taken but the behaviour has been translated
>
>
>>>> - if (arg->peer_vht_rates.rx_max_rate &&
>>>> - (sta->vht_cap.cap &
>>>> IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
>>>> - switch (arg->peer_vht_rates.rx_max_rate) {
>>>> - case 1560:
>>>> - /* Must be 2x2 at 160Mhz is all it can do. */
>>>> - arg->peer_bw_rxnss_override = 2;
>>>> - break;
>>>> - case 780:
>>>> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
>>>> - arg->peer_bw_rxnss_override = 1;
>>>> - break;
>>>> - }
>>>
>>> This old code does look wrong, the firmware is using zero-based, so
>>> override-0 == nss-1, override-1 == nss-2.
>> 0 = crash
>>
>> and 1 and 2 is wrong.
>
> Yes, it should be 0 and 1. The old code | in the (1<<31) later.
no just just 0 and 1
consider the 80+80 case
which may me | 1<<3 in addition
80+80 has a independend mapping. 1<<31 must be set in any way if vht160
or vht80+80 is configured
in 80+80, both vht160 and 80+80 must be configured to 1x1 or 2x2
in vht160 only the vht160 part must be masked.
>
>>
>> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
>> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
>> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>>>
>>> This is confusing enough that it deserves a comment in the code I
>>> think....
>> the removal doesnt deserve a comment. i dont know how to explain that
>> its simply wrong. it uses the wrong
>> bit masks and this has been written in the initial patch description
>
> I think a comment like this would be helpful:
>
> /* Note that the rxnss_override is 0 based, so 0 means nss-1 and 7
> means nss-8. */
>
>>>
>>>> + if (arg->peer_num_spatial_streams && arg->peer_phymode ==
>>>> MODE_11AC_VHT80_80) {
>>>> + arg->peer_bw_rxnss_override |=
>>>> BW_NSS_FWCONF_80_80(arg->peer_num_spatial_streams);
>>>> }
>>>> +
>>>> + /* In very exceptional conditions it is observed that
>>>> + * firmware was receiving bw_rxnss_override as 0 for peer from
>>>> host, and resulting in Target Assert.
>>>> + * Changing the rxnss_override to minimum nss. This is a
>>>> temporary WAR. Needs to be fixed
>>>> + * properly.
>>>> + */
>>>> + if (!arg->peer_num_spatial_streams && (arg->peer_phymode ==
>>>> MODE_11AC_VHT80_80 || arg->peer_phymode == MODE_11AC_VHT160)) {
>>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_MAP_ENABLE;
>>>> + }
>>>> +
>>>> + ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d
>>>> flags 0x%x\n",
>>>> + sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>> }
>>>>
>>>> static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
>>>> @@ -2696,9 +2700,9 @@ static int ath10k_peer_assoc_prepare(struct
>>>> ath10k *ar,
>>>> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
>>>> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
>>>> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
>>>> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>>> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
>>>> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
>>>> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>>>
>>>> return 0;
>>>> }
>>>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
>>>> b/drivers/net/wireless/ath/ath10k/wmi.c
>>>> index 2c36256a441d..7d72fdc703c8 100644
>>>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>>>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>>>> @@ -7212,9 +7212,7 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k
>>>> *ar, void *buf,
>>>>
>>>> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
>>>> if (arg->peer_bw_rxnss_override)
>>>> - cmd->peer_bw_rxnss_override =
>>>> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
>>>> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
>>>> + cmd->peer_bw_rxnss_override =
>>>> __cpu_to_le32(arg->peer_bw_rxnss_override);
>>>> else
>>>> cmd->peer_bw_rxnss_override = 0;
>>>> }
>>>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h
>>>> b/drivers/net/wireless/ath/ath10k/wmi.h
>>>> index 46ae19bb2c92..1fe0aa5523a6 100644
>>>> --- a/drivers/net/wireless/ath/ath10k/wmi.h
>>>> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
>>>> @@ -6380,7 +6380,19 @@ struct wmi_10_2_peer_assoc_complete_cmd {
>>>> __le32 info0; /* WMI_PEER_ASSOC_INFO0_ */
>>>> } __packed;
>>>>
>>>> -#define PEER_BW_RXNSS_OVERRIDE_OFFSET 31
>>>> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
>>>> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
>>>> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
>>>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
>>>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>>>
>>> Older FW does not pay attention to the 80_80 bits. It uses masks so
>>> it is
>>> backwards-compat, but maybe worth a comment.
>> i took that masks from recent 3.5.3 based qca code. its correct and
>> if its backward compatible, it's still correct
>> i mean the old code did not coment anything. so dont know how to
>> write anything more about it now. see patch description
>>
>>>
>>>> +#define BW_NSS_FWCONF_MAP_M (0x0000003F)
>>>> +
>>>> +#define GET_BW_NSS_FWCONF_160(x) ((((x) &
>>>> BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
>>>> +#define GET_BW_NSS_FWCONF_80_80(x) ((((x) &
>>>> BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
>>>> +
>>>> +/* Values defined to set 160 MHz Bandwidth NSS Mapping into FW*/
>>>> +#define BW_NSS_FWCONF_160(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1)
>>>> << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
>>>> +#define BW_NSS_FWCONF_80_80(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x -
>>>> 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) & BW_NSS_FWCONF_MAP_80_80MHZ_M))
>>>>
>>>> struct wmi_10_4_peer_assoc_complete_cmd {
>>>> struct wmi_10_2_peer_assoc_complete_cmd cmd;
>>>>
>>>
>>> Thanks,
>>> Ben
>>>
>>
>
> Thanks,
> Ben
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-27 4:40 ` Sebastian Gottschall
0 siblings, 0 replies; 22+ messages in thread
From: Sebastian Gottschall @ 2018-04-27 4:40 UTC (permalink / raw)
To: Ben Greear, ath10k, linux-wireless; +Cc: kvalo
Am 26.04.2018 um 22:35 schrieb Ben Greear:
> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>
>>>> current handling of peer_bw_rxnss_override parameter is based on
>>>> guessing the VHT160/8080 capability by rx rate. this is wrong and
>>>> may lead
>>>> to a non initialized peer_bw_rxnss_override parameter which is
>>>> required since VHT160 operation mode only supports 2x2 chainmasks
>>>> in addition the original code
>>>> initialized the parameter with wrong masked values.
>>>> This patch uses the peer phymode and peer nss information for
>>>> correct initialisation of the peer_bw_rxnss_override parameter.
>>>> if this peer information is not available, we initialize the
>>>> parameter by minimum nss which is suggested by QCA as temporary
>>>> workaround according
>>>> to the QCA sourcecodes.
>>>>
>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>
>>>> v2: remove debug messages
>>>> ---
>>>> drivers/net/wireless/ath/ath10k/mac.c | 36
>>>> +++++++++++++++++++----------------
>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>>>> b/drivers/net/wireless/ath/ath10k/mac.c
>>>> index 5be6386ede8f..df79af89ee71 100644
>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct
>>>> ath10k *ar,
>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>
>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>> + }
>>>>
>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>> * supports VHT.
>>>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct
>>>> ath10k *ar,
>>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>
>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d
>>>> flags 0x%x\n",
>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode ==
>>>> MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>> + arg->peer_bw_rxnss_override =
>>>> BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>> + }
>>>
>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP.
>>> From what I can tell,
>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if
>>> peer is VHT-160,
>>> then of course it can only talk at 2x2.
>>>
>>> So, I don't think you can just look at the peer_num_spatial_streams
>>> here.
>> no? rxnss_override is only taked if peer phymode is vht160 or
>> vht80_80. vht80 is not considered in that code PEER phy_mode, not
>> HOST phy_mode
>> this parameter is a assoc per peer parameter as far as i can say here.
>> consider that the firmware will accept anything except 0 for
>> peer_bw_rxnss_override in vht160 operation mode
>> if a peer is 80 mhz, the code will be skipped here.
>
> From what I can tell, this feature is supposed to configure the
> rate-ctrl in the firmware to know that
> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can
> send at higher NSS if it sends
> at 80Mhz or lower.
right. but thats exactly what it should does in case that a peer is
connecting with vht160 / 80_80
and the peer itself does also send his own nss capabilities which is
used if available. if not ,it uses the fallback.
>
> If a 2x2 peer connects to the AP, will it have
> peer_num_spatial_streams set to 2?
yes. i had some debug code in my initial early versions. the peer does
transmit his own nss capabilities.
> If so,
> then your code will configure the peer_bw_rxnss_override to indicate
> it can send at 160Mhz
> 2x2 as well, right? And if so, then that is incorrect.
no. since nss_override is only set if peer phymode is 160 mhz as well
>
> Probably if you connected something like a IPQ4019 station to a 9984
> AP configured for 160Mhz,
> the peer_bw_rxnss_override would be set to 2x2 instead of 1x1?
depends how the ipq4019 chipset is configured. if it itself is
configured to 160 mhz and configured to 2x2, yes
but the peer itself can also send with 1x1, then the ap will also send
just 1x1
but as i said. this all is just used if the peer is configured to vht160
and if num_spatial_stream is set by peer.
of not this is all ignored and only 1<<31 is masked. this way has been
taken from qca's propertiery code
or lets say not taken but the behaviour has been translated
>
>
>>>> - if (arg->peer_vht_rates.rx_max_rate &&
>>>> - (sta->vht_cap.cap &
>>>> IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
>>>> - switch (arg->peer_vht_rates.rx_max_rate) {
>>>> - case 1560:
>>>> - /* Must be 2x2 at 160Mhz is all it can do. */
>>>> - arg->peer_bw_rxnss_override = 2;
>>>> - break;
>>>> - case 780:
>>>> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
>>>> - arg->peer_bw_rxnss_override = 1;
>>>> - break;
>>>> - }
>>>
>>> This old code does look wrong, the firmware is using zero-based, so
>>> override-0 == nss-1, override-1 == nss-2.
>> 0 = crash
>>
>> and 1 and 2 is wrong.
>
> Yes, it should be 0 and 1. The old code | in the (1<<31) later.
no just just 0 and 1
consider the 80+80 case
which may me | 1<<3 in addition
80+80 has a independend mapping. 1<<31 must be set in any way if vht160
or vht80+80 is configured
in 80+80, both vht160 and 80+80 must be configured to 1x1 or 2x2
in vht160 only the vht160 part must be masked.
>
>>
>> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
>> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
>> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>>>
>>> This is confusing enough that it deserves a comment in the code I
>>> think....
>> the removal doesnt deserve a comment. i dont know how to explain that
>> its simply wrong. it uses the wrong
>> bit masks and this has been written in the initial patch description
>
> I think a comment like this would be helpful:
>
> /* Note that the rxnss_override is 0 based, so 0 means nss-1 and 7
> means nss-8. */
>
>>>
>>>> + if (arg->peer_num_spatial_streams && arg->peer_phymode ==
>>>> MODE_11AC_VHT80_80) {
>>>> + arg->peer_bw_rxnss_override |=
>>>> BW_NSS_FWCONF_80_80(arg->peer_num_spatial_streams);
>>>> }
>>>> +
>>>> + /* In very exceptional conditions it is observed that
>>>> + * firmware was receiving bw_rxnss_override as 0 for peer from
>>>> host, and resulting in Target Assert.
>>>> + * Changing the rxnss_override to minimum nss. This is a
>>>> temporary WAR. Needs to be fixed
>>>> + * properly.
>>>> + */
>>>> + if (!arg->peer_num_spatial_streams && (arg->peer_phymode ==
>>>> MODE_11AC_VHT80_80 || arg->peer_phymode == MODE_11AC_VHT160)) {
>>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_MAP_ENABLE;
>>>> + }
>>>> +
>>>> + ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d
>>>> flags 0x%x\n",
>>>> + sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>> }
>>>>
>>>> static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
>>>> @@ -2696,9 +2700,9 @@ static int ath10k_peer_assoc_prepare(struct
>>>> ath10k *ar,
>>>> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
>>>> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
>>>> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
>>>> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>>> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
>>>> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
>>>> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>>>
>>>> return 0;
>>>> }
>>>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
>>>> b/drivers/net/wireless/ath/ath10k/wmi.c
>>>> index 2c36256a441d..7d72fdc703c8 100644
>>>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>>>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>>>> @@ -7212,9 +7212,7 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k
>>>> *ar, void *buf,
>>>>
>>>> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
>>>> if (arg->peer_bw_rxnss_override)
>>>> - cmd->peer_bw_rxnss_override =
>>>> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
>>>> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
>>>> + cmd->peer_bw_rxnss_override =
>>>> __cpu_to_le32(arg->peer_bw_rxnss_override);
>>>> else
>>>> cmd->peer_bw_rxnss_override = 0;
>>>> }
>>>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h
>>>> b/drivers/net/wireless/ath/ath10k/wmi.h
>>>> index 46ae19bb2c92..1fe0aa5523a6 100644
>>>> --- a/drivers/net/wireless/ath/ath10k/wmi.h
>>>> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
>>>> @@ -6380,7 +6380,19 @@ struct wmi_10_2_peer_assoc_complete_cmd {
>>>> __le32 info0; /* WMI_PEER_ASSOC_INFO0_ */
>>>> } __packed;
>>>>
>>>> -#define PEER_BW_RXNSS_OVERRIDE_OFFSET 31
>>>> +#define BW_NSS_FWCONF_MAP_ENABLE (1 << 31)
>>>> +#define BW_NSS_FWCONF_MAP_160MHZ_S (0)
>>>> +#define BW_NSS_FWCONF_MAP_160MHZ_M (0x00000007)
>>>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_S (3)
>>>> +#define BW_NSS_FWCONF_MAP_80_80MHZ_M (0x00000038)
>>>
>>> Older FW does not pay attention to the 80_80 bits. It uses masks so
>>> it is
>>> backwards-compat, but maybe worth a comment.
>> i took that masks from recent 3.5.3 based qca code. its correct and
>> if its backward compatible, it's still correct
>> i mean the old code did not coment anything. so dont know how to
>> write anything more about it now. see patch description
>>
>>>
>>>> +#define BW_NSS_FWCONF_MAP_M (0x0000003F)
>>>> +
>>>> +#define GET_BW_NSS_FWCONF_160(x) ((((x) &
>>>> BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
>>>> +#define GET_BW_NSS_FWCONF_80_80(x) ((((x) &
>>>> BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
>>>> +
>>>> +/* Values defined to set 160 MHz Bandwidth NSS Mapping into FW*/
>>>> +#define BW_NSS_FWCONF_160(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1)
>>>> << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
>>>> +#define BW_NSS_FWCONF_80_80(x) (BW_NSS_FWCONF_MAP_ENABLE | (((x -
>>>> 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) & BW_NSS_FWCONF_MAP_80_80MHZ_M))
>>>>
>>>> struct wmi_10_4_peer_assoc_complete_cmd {
>>>> struct wmi_10_2_peer_assoc_complete_cmd cmd;
>>>>
>>>
>>> Thanks,
>>> Ben
>>>
>>
>
> Thanks,
> Ben
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
2018-04-27 4:40 ` Sebastian Gottschall
@ 2018-04-27 16:07 ` Ben Greear
-1 siblings, 0 replies; 22+ messages in thread
From: Ben Greear @ 2018-04-27 16:07 UTC (permalink / raw)
To: Sebastian Gottschall, ath10k, linux-wireless; +Cc: kvalo
On 04/26/2018 09:40 PM, Sebastian Gottschall wrote:
> Am 26.04.2018 um 22:35 schrieb Ben Greear:
>> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>
>>>>> current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
>>>>> to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original code
>>>>> initialized the parameter with wrong masked values.
>>>>> This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
>>>>> if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
>>>>> to the QCA sourcecodes.
>>>>>
>>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>
>>>>> v2: remove debug messages
>>>>> ---
>>>>> drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
>>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>>>>> index 5be6386ede8f..df79af89ee71 100644
>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>>
>>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>>> + }
>>>>>
>>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>>> * supports VHT.
>>>>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>>
>>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
>>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>>> + }
>>>>
>>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From what I can tell,
>>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if peer is VHT-160,
>>>> then of course it can only talk at 2x2.
>>>>
>>>> So, I don't think you can just look at the peer_num_spatial_streams here.
>>> no? rxnss_override is only taked if peer phymode is vht160 or vht80_80. vht80 is not considered in that code PEER phy_mode, not HOST phy_mode
>>> this parameter is a assoc per peer parameter as far as i can say here.
>>> consider that the firmware will accept anything except 0 for peer_bw_rxnss_override in vht160 operation mode
>>> if a peer is 80 mhz, the code will be skipped here.
>>
>> From what I can tell, this feature is supposed to configure the rate-ctrl in the firmware to know that
>> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can send at higher NSS if it sends
>> at 80Mhz or lower.
> right. but thats exactly what it should does in case that a peer is connecting with vht160 / 80_80
> and the peer itself does also send his own nss capabilities which is used if available. if not ,it uses the fallback.
>>
>> If a 2x2 peer connects to the AP, will it have peer_num_spatial_streams set to 2?
> yes. i had some debug code in my initial early versions. the peer does transmit his own nss capabilities.
>> If so,
>> then your code will configure the peer_bw_rxnss_override to indicate it can send at 160Mhz
>> 2x2 as well, right? And if so, then that is incorrect.
> no. since nss_override is only set if peer phymode is 160 mhz as well
The peer can run at 2x2 80Mhz and 1x1 160Mhz at the same time, so it can advertise
2x2 nss and phy-mode of 160Mhz when associating to the 160Mhz AP, but the peer can only do 1x1 at 160Mhz. There
is no standard way I know of for the peer to tell you specifically that it can only do
1x1 at 160Mhz and also 2x2 at 80Mhz in this case.
That is why this rxnns_override exists, to hack around this problem.
Your patch is going to break in this case as far as I can tell.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-27 16:07 ` Ben Greear
0 siblings, 0 replies; 22+ messages in thread
From: Ben Greear @ 2018-04-27 16:07 UTC (permalink / raw)
To: Sebastian Gottschall, ath10k, linux-wireless; +Cc: kvalo
On 04/26/2018 09:40 PM, Sebastian Gottschall wrote:
> Am 26.04.2018 um 22:35 schrieb Ben Greear:
>> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>
>>>>> current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
>>>>> to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original code
>>>>> initialized the parameter with wrong masked values.
>>>>> This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
>>>>> if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
>>>>> to the QCA sourcecodes.
>>>>>
>>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>
>>>>> v2: remove debug messages
>>>>> ---
>>>>> drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
>>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>>>>> index 5be6386ede8f..df79af89ee71 100644
>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>>
>>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>>> + }
>>>>>
>>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>>> * supports VHT.
>>>>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>>
>>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
>>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>>> + }
>>>>
>>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From what I can tell,
>>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if peer is VHT-160,
>>>> then of course it can only talk at 2x2.
>>>>
>>>> So, I don't think you can just look at the peer_num_spatial_streams here.
>>> no? rxnss_override is only taked if peer phymode is vht160 or vht80_80. vht80 is not considered in that code PEER phy_mode, not HOST phy_mode
>>> this parameter is a assoc per peer parameter as far as i can say here.
>>> consider that the firmware will accept anything except 0 for peer_bw_rxnss_override in vht160 operation mode
>>> if a peer is 80 mhz, the code will be skipped here.
>>
>> From what I can tell, this feature is supposed to configure the rate-ctrl in the firmware to know that
>> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can send at higher NSS if it sends
>> at 80Mhz or lower.
> right. but thats exactly what it should does in case that a peer is connecting with vht160 / 80_80
> and the peer itself does also send his own nss capabilities which is used if available. if not ,it uses the fallback.
>>
>> If a 2x2 peer connects to the AP, will it have peer_num_spatial_streams set to 2?
> yes. i had some debug code in my initial early versions. the peer does transmit his own nss capabilities.
>> If so,
>> then your code will configure the peer_bw_rxnss_override to indicate it can send at 160Mhz
>> 2x2 as well, right? And if so, then that is incorrect.
> no. since nss_override is only set if peer phymode is 160 mhz as well
The peer can run at 2x2 80Mhz and 1x1 160Mhz at the same time, so it can advertise
2x2 nss and phy-mode of 160Mhz when associating to the 160Mhz AP, but the peer can only do 1x1 at 160Mhz. There
is no standard way I know of for the peer to tell you specifically that it can only do
1x1 at 160Mhz and also 2x2 at 80Mhz in this case.
That is why this rxnns_override exists, to hack around this problem.
Your patch is going to break in this case as far as I can tell.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
2018-04-27 16:07 ` Ben Greear
@ 2018-04-27 18:54 ` Sebastian Gottschall
-1 siblings, 0 replies; 22+ messages in thread
From: Sebastian Gottschall @ 2018-04-27 18:54 UTC (permalink / raw)
To: Ben Greear, ath10k, linux-wireless; +Cc: kvalo
Am 27.04.2018 um 18:07 schrieb Ben Greear:
> On 04/26/2018 09:40 PM, Sebastian Gottschall wrote:
>> Am 26.04.2018 um 22:35 schrieb Ben Greear:
>>> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>>>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>
>>>>>> current handling of peer_bw_rxnss_override parameter is based on
>>>>>> guessing the VHT160/8080 capability by rx rate. this is wrong and
>>>>>> may lead
>>>>>> to a non initialized peer_bw_rxnss_override parameter which is
>>>>>> required since VHT160 operation mode only supports 2x2 chainmasks
>>>>>> in addition the original code
>>>>>> initialized the parameter with wrong masked values.
>>>>>> This patch uses the peer phymode and peer nss information for
>>>>>> correct initialisation of the peer_bw_rxnss_override parameter.
>>>>>> if this peer information is not available, we initialize the
>>>>>> parameter by minimum nss which is suggested by QCA as temporary
>>>>>> workaround according
>>>>>> to the QCA sourcecodes.
>>>>>>
>>>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>
>>>>>> v2: remove debug messages
>>>>>> ---
>>>>>> drivers/net/wireless/ath/ath10k/mac.c | 36
>>>>>> +++++++++++++++++++----------------
>>>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>> b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>> index 5be6386ede8f..df79af89ee71 100644
>>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct
>>>>>> ath10k *ar,
>>>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>>>
>>>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>>>> + }
>>>>>>
>>>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>>>> * supports VHT.
>>>>>> @@ -2529,22 +2530,25 @@ static void
>>>>>> ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>>>
>>>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d
>>>>>> flags 0x%x\n",
>>>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode ==
>>>>>> MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>>>> + arg->peer_bw_rxnss_override =
>>>>>> BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>>>> + }
>>>>>
>>>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP.
>>>>> From what I can tell,
>>>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but
>>>>> if peer is VHT-160,
>>>>> then of course it can only talk at 2x2.
>>>>>
>>>>> So, I don't think you can just look at the
>>>>> peer_num_spatial_streams here.
>>>> no? rxnss_override is only taked if peer phymode is vht160 or
>>>> vht80_80. vht80 is not considered in that code PEER phy_mode, not
>>>> HOST phy_mode
>>>> this parameter is a assoc per peer parameter as far as i can say here.
>>>> consider that the firmware will accept anything except 0 for
>>>> peer_bw_rxnss_override in vht160 operation mode
>>>> if a peer is 80 mhz, the code will be skipped here.
>>>
>>> From what I can tell, this feature is supposed to configure the
>>> rate-ctrl in the firmware to know that
>>> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can
>>> send at higher NSS if it sends
>>> at 80Mhz or lower.
>> right. but thats exactly what it should does in case that a peer is
>> connecting with vht160 / 80_80
>> and the peer itself does also send his own nss capabilities which is
>> used if available. if not ,it uses the fallback.
>>>
>>> If a 2x2 peer connects to the AP, will it have
>>> peer_num_spatial_streams set to 2?
>> yes. i had some debug code in my initial early versions. the peer
>> does transmit his own nss capabilities.
>>> If so,
>>> then your code will configure the peer_bw_rxnss_override to indicate
>>> it can send at 160Mhz
>>> 2x2 as well, right? And if so, then that is incorrect.
>> no. since nss_override is only set if peer phymode is 160 mhz as well
>
> The peer can run at 2x2 80Mhz and 1x1 160Mhz at the same time, so it
> can advertise
> 2x2 nss and phy-mode of 160Mhz when associating to the 160Mhz AP, but
> the peer can only do 1x1 at 160Mhz. There
> is no standard way I know of for the peer to tell you specifically
> that it can only do
> 1x1 at 160Mhz and also 2x2 at 80Mhz in this case.
per specification the peer is able to provide max nss to the ap. the
rx_nss property is calculated
from the mcs rateset provided by the peer by mac80211. this is mcs set
provided on on assoc is mandatory.
so there is a way the peer can tell you what it supports and this is
what is used.
if a peer does not provide the mcs rateset (which i have seen for a
single marvell client)
the fallback mechanism will still work, but just with 1x1. the problem
is anything else will crash the firmware.
we have to deal with that.
That is why this rxnns_override exists, to hack around this problem.
i dont think so, because its not just a hack. without providing that
parameter the firmware will crash.
so its a always required parameter and not just a hack. for sure the
firmware can handle it by itself, just someone
at qca should start to work on it.
but again. my implementation is correct from the information i have out
of the qca propertiery wireless driver sources
>
> Your patch is going to break in this case as far as I can tell.
no it isnt. my tests with various different clients from different
vendors shows that its working. with 2x2 and 1x1.
its all well detected by the code and configured as expected
consider that this patch fixes a CRASH. accept that. it wont break. it
repairs.
>
> Thanks,
> Ben
>
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-27 18:54 ` Sebastian Gottschall
0 siblings, 0 replies; 22+ messages in thread
From: Sebastian Gottschall @ 2018-04-27 18:54 UTC (permalink / raw)
To: Ben Greear, ath10k, linux-wireless; +Cc: kvalo
Am 27.04.2018 um 18:07 schrieb Ben Greear:
> On 04/26/2018 09:40 PM, Sebastian Gottschall wrote:
>> Am 26.04.2018 um 22:35 schrieb Ben Greear:
>>> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>>>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>
>>>>>> current handling of peer_bw_rxnss_override parameter is based on
>>>>>> guessing the VHT160/8080 capability by rx rate. this is wrong and
>>>>>> may lead
>>>>>> to a non initialized peer_bw_rxnss_override parameter which is
>>>>>> required since VHT160 operation mode only supports 2x2 chainmasks
>>>>>> in addition the original code
>>>>>> initialized the parameter with wrong masked values.
>>>>>> This patch uses the peer phymode and peer nss information for
>>>>>> correct initialisation of the peer_bw_rxnss_override parameter.
>>>>>> if this peer information is not available, we initialize the
>>>>>> parameter by minimum nss which is suggested by QCA as temporary
>>>>>> workaround according
>>>>>> to the QCA sourcecodes.
>>>>>>
>>>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>
>>>>>> v2: remove debug messages
>>>>>> ---
>>>>>> drivers/net/wireless/ath/ath10k/mac.c | 36
>>>>>> +++++++++++++++++++----------------
>>>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>> b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>> index 5be6386ede8f..df79af89ee71 100644
>>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct
>>>>>> ath10k *ar,
>>>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>>>
>>>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>>>> + }
>>>>>>
>>>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>>>> * supports VHT.
>>>>>> @@ -2529,22 +2530,25 @@ static void
>>>>>> ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>>>
>>>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d
>>>>>> flags 0x%x\n",
>>>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode ==
>>>>>> MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>>>> + arg->peer_bw_rxnss_override =
>>>>>> BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>>>> + }
>>>>>
>>>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP.
>>>>> From what I can tell,
>>>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but
>>>>> if peer is VHT-160,
>>>>> then of course it can only talk at 2x2.
>>>>>
>>>>> So, I don't think you can just look at the
>>>>> peer_num_spatial_streams here.
>>>> no? rxnss_override is only taked if peer phymode is vht160 or
>>>> vht80_80. vht80 is not considered in that code PEER phy_mode, not
>>>> HOST phy_mode
>>>> this parameter is a assoc per peer parameter as far as i can say here.
>>>> consider that the firmware will accept anything except 0 for
>>>> peer_bw_rxnss_override in vht160 operation mode
>>>> if a peer is 80 mhz, the code will be skipped here.
>>>
>>> From what I can tell, this feature is supposed to configure the
>>> rate-ctrl in the firmware to know that
>>> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can
>>> send at higher NSS if it sends
>>> at 80Mhz or lower.
>> right. but thats exactly what it should does in case that a peer is
>> connecting with vht160 / 80_80
>> and the peer itself does also send his own nss capabilities which is
>> used if available. if not ,it uses the fallback.
>>>
>>> If a 2x2 peer connects to the AP, will it have
>>> peer_num_spatial_streams set to 2?
>> yes. i had some debug code in my initial early versions. the peer
>> does transmit his own nss capabilities.
>>> If so,
>>> then your code will configure the peer_bw_rxnss_override to indicate
>>> it can send at 160Mhz
>>> 2x2 as well, right? And if so, then that is incorrect.
>> no. since nss_override is only set if peer phymode is 160 mhz as well
>
> The peer can run at 2x2 80Mhz and 1x1 160Mhz at the same time, so it
> can advertise
> 2x2 nss and phy-mode of 160Mhz when associating to the 160Mhz AP, but
> the peer can only do 1x1 at 160Mhz. There
> is no standard way I know of for the peer to tell you specifically
> that it can only do
> 1x1 at 160Mhz and also 2x2 at 80Mhz in this case.
per specification the peer is able to provide max nss to the ap. the
rx_nss property is calculated
from the mcs rateset provided by the peer by mac80211. this is mcs set
provided on on assoc is mandatory.
so there is a way the peer can tell you what it supports and this is
what is used.
if a peer does not provide the mcs rateset (which i have seen for a
single marvell client)
the fallback mechanism will still work, but just with 1x1. the problem
is anything else will crash the firmware.
we have to deal with that.
That is why this rxnns_override exists, to hack around this problem.
i dont think so, because its not just a hack. without providing that
parameter the firmware will crash.
so its a always required parameter and not just a hack. for sure the
firmware can handle it by itself, just someone
at qca should start to work on it.
but again. my implementation is correct from the information i have out
of the qca propertiery wireless driver sources
>
> Your patch is going to break in this case as far as I can tell.
no it isnt. my tests with various different clients from different
vendors shows that its working. with 2x2 and 1x1.
its all well detected by the code and configured as expected
consider that this patch fixes a CRASH. accept that. it wont break. it
repairs.
>
> Thanks,
> Ben
>
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
2018-04-27 18:54 ` Sebastian Gottschall
@ 2018-04-27 21:57 ` Ben Greear
-1 siblings, 0 replies; 22+ messages in thread
From: Ben Greear @ 2018-04-27 21:57 UTC (permalink / raw)
To: Sebastian Gottschall, ath10k, linux-wireless; +Cc: kvalo
On 04/27/2018 11:54 AM, Sebastian Gottschall wrote:
> Am 27.04.2018 um 18:07 schrieb Ben Greear:
>> On 04/26/2018 09:40 PM, Sebastian Gottschall wrote:
>>> Am 26.04.2018 um 22:35 schrieb Ben Greear:
>>>> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>>>>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>>>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>
>>>>>>> current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
>>>>>>> to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original
>>>>>>> code
>>>>>>> initialized the parameter with wrong masked values.
>>>>>>> This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
>>>>>>> if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
>>>>>>> to the QCA sourcecodes.
>>>>>>>
>>>>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>
>>>>>>> v2: remove debug messages
>>>>>>> ---
>>>>>>> drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
>>>>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>> index 5be6386ede8f..df79af89ee71 100644
>>>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>>>>
>>>>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>>>>> + }
>>>>>>>
>>>>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>>>>> * supports VHT.
>>>>>>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>>>>
>>>>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
>>>>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>>>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>>>>> + }
>>>>>>
>>>>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From what I can tell,
>>>>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if peer is VHT-160,
>>>>>> then of course it can only talk at 2x2.
>>>>>>
>>>>>> So, I don't think you can just look at the peer_num_spatial_streams here.
>>>>> no? rxnss_override is only taked if peer phymode is vht160 or vht80_80. vht80 is not considered in that code PEER phy_mode, not HOST phy_mode
>>>>> this parameter is a assoc per peer parameter as far as i can say here.
>>>>> consider that the firmware will accept anything except 0 for peer_bw_rxnss_override in vht160 operation mode
>>>>> if a peer is 80 mhz, the code will be skipped here.
>>>>
>>>> From what I can tell, this feature is supposed to configure the rate-ctrl in the firmware to know that
>>>> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can send at higher NSS if it sends
>>>> at 80Mhz or lower.
>>> right. but thats exactly what it should does in case that a peer is connecting with vht160 / 80_80
>>> and the peer itself does also send his own nss capabilities which is used if available. if not ,it uses the fallback.
>>>>
>>>> If a 2x2 peer connects to the AP, will it have peer_num_spatial_streams set to 2?
>>> yes. i had some debug code in my initial early versions. the peer does transmit his own nss capabilities.
>>>> If so,
>>>> then your code will configure the peer_bw_rxnss_override to indicate it can send at 160Mhz
>>>> 2x2 as well, right? And if so, then that is incorrect.
>>> no. since nss_override is only set if peer phymode is 160 mhz as well
>>
>> The peer can run at 2x2 80Mhz and 1x1 160Mhz at the same time, so it can advertise
>> 2x2 nss and phy-mode of 160Mhz when associating to the 160Mhz AP, but the peer can only do 1x1 at 160Mhz. There
>> is no standard way I know of for the peer to tell you specifically that it can only do
>> 1x1 at 160Mhz and also 2x2 at 80Mhz in this case.
> per specification the peer is able to provide max nss to the ap. the rx_nss property is calculated
> from the mcs rateset provided by the peer by mac80211. this is mcs set provided on on assoc is mandatory.
> so there is a way the peer can tell you what it supports and this is what is used.
> if a peer does not provide the mcs rateset (which i have seen for a single marvell client)
> the fallback mechanism will still work, but just with 1x1. the problem is anything else will crash the firmware.
> we have to deal with that.
>
> That is why this rxnns_override exists, to hack around this problem.
>
> i dont think so, because its not just a hack. without providing that parameter the firmware will crash.
> so its a always required parameter and not just a hack. for sure the firmware can handle it by itself, just someone
> at qca should start to work on it.
> but again. my implementation is correct from the information i have out of the qca propertiery wireless driver sources
>>
>> Your patch is going to break in this case as far as I can tell.
> no it isnt. my tests with various different clients from different vendors shows that its working. with 2x2 and 1x1.
> its all well detected by the code and configured as expected
> consider that this patch fixes a CRASH. accept that. it wont break. it repairs.
I did some testing with the patch below.
The CHAIMASK_ERR is a debug log from FW that I added to help make sure the patch is
acting as desired. The first hex is an identifier, second is the value passed in,
third is phymode, 4th is the tx-chain-mask for 160Mhz frames.
On station side, when associating a 4x4 9984 station to 9984 configured for nss4, 160Mhz, I see:
[86376.620303] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2 spatial-streams: 4
ath10k-fw: ts: 15229 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
On station side, when associating a 4x4 9984 station with chain-mask of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
[86147.569319] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 780 rxnss_override: 0x80000000 nss160: 1 spatial-streams: 2
ath10k-fw: ts: 6807 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
On AP side, when associating a 4x4 9984 station to 9984 configured for 160Mhz, I see:
[11167.635324] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2 spatial-streams: 4
ath10k-fw: ts: 72917 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
On AP side, when associating a 4x4 9984 station with chain-mask of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
[11422.887181] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 780 rxnss_override: 0x80000000 nss160: 1 spatial-streams: 2
ath10k-fw: ts: 334266 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
On AP side, when associating a 4x4 9984 station configured for 80Mhz instead of 160,
then logging from firmware indicates full 4x4 rates are supported for 80Mhz and below,
and the rxnss_override does not have the (1<<31) bit set:
ath10k-fw: ts: 519211 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x00000000 0x0000000a 0x00000001
So, I think this might be a better fix for this problem (included inline for discussion,
probably white-space damaged by email client:
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index e1ad983..8bce916 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2860,18 +2860,39 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
arg->peer_vht_rates.rx_max_rate, arg->peer_vht_rates.rx_mcs_set,
arg->peer_vht_rates.tx_max_rate, arg->peer_vht_rates.tx_mcs_set);
- if (arg->peer_vht_rates.rx_max_rate &&
- (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
- switch (arg->peer_vht_rates.rx_max_rate) {
+ if (arg->peer_phymode == MODE_11AC_VHT80_80 ||
+ arg->peer_phymode == MODE_11AC_VHT160) {
+ int nss160;
+ int rx = arg->peer_vht_rates.rx_max_rate;
+ /* Deal with cases where chainmask has been decreased.
+ * All known chips that support 160Mhz can do only 1/2 of
+ * the available chains at 160Mhz.
+ */
+ rx = min((int)(arg->peer_num_spatial_streams * 390), rx);
+
+ switch (rx) {
+ /* When a NIC shows up that can do 4x4 at 160Mhz, its
+ * max-rate should be higher, and we can set nss160
+ * to 4 here.
+ */
case 1560:
/* Must be 2x2 at 160Mhz is all it can do. */
- arg->peer_bw_rxnss_override = 2;
+ nss160 = 2;
break;
- case 780:
- /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
- arg->peer_bw_rxnss_override = 1;
+ default:
+ /* Assume we can only do 1x1 at 160Mhz */
+ nss160 = 1;
break;
}
+
+ arg->peer_bw_rxnss_override = ((nss160 - 1) | /* 160Mhz nss */
+ ((nss160 - 1) << 3) | /* 80+80 nss */
+ BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
+
+ ath10k_warn(ar, "NIC rx-max-rate: %d calculated-max: %d rxnss_override: 0x%x nss160: %d spatial-streams: %d\n",
+ arg->peer_vht_rates.rx_max_rate, rx,
+ arg->peer_bw_rxnss_override, nss160,
+ arg->peer_num_spatial_streams);
}
}
@@ -3115,9 +3136,9 @@ static int ath10k_peer_assoc_prepare(struct ath10k *ar,
ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
+ ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
- ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
ath10k_peer_assoc_h_rate_overrides(ar, vif, sta, arg);
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 8eeeab0..365d509 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -7442,12 +7442,8 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k *ar, void *buf,
struct wmi_10_4_peer_assoc_complete_cmd *cmd = buf;
ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
- if (arg->peer_bw_rxnss_override)
- cmd->peer_bw_rxnss_override =
- __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
- BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
- else
- cmd->peer_bw_rxnss_override = 0;
+
+ cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
}
static int
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-27 21:57 ` Ben Greear
0 siblings, 0 replies; 22+ messages in thread
From: Ben Greear @ 2018-04-27 21:57 UTC (permalink / raw)
To: Sebastian Gottschall, ath10k, linux-wireless; +Cc: kvalo
On 04/27/2018 11:54 AM, Sebastian Gottschall wrote:
> Am 27.04.2018 um 18:07 schrieb Ben Greear:
>> On 04/26/2018 09:40 PM, Sebastian Gottschall wrote:
>>> Am 26.04.2018 um 22:35 schrieb Ben Greear:
>>>> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>>>>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>>>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>
>>>>>>> current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
>>>>>>> to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original
>>>>>>> code
>>>>>>> initialized the parameter with wrong masked values.
>>>>>>> This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
>>>>>>> if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
>>>>>>> to the QCA sourcecodes.
>>>>>>>
>>>>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>
>>>>>>> v2: remove debug messages
>>>>>>> ---
>>>>>>> drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
>>>>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>> index 5be6386ede8f..df79af89ee71 100644
>>>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>>>>
>>>>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>>>>> + }
>>>>>>>
>>>>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>>>>> * supports VHT.
>>>>>>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>>>>
>>>>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
>>>>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>>>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>>>>> + }
>>>>>>
>>>>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From what I can tell,
>>>>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if peer is VHT-160,
>>>>>> then of course it can only talk at 2x2.
>>>>>>
>>>>>> So, I don't think you can just look at the peer_num_spatial_streams here.
>>>>> no? rxnss_override is only taked if peer phymode is vht160 or vht80_80. vht80 is not considered in that code PEER phy_mode, not HOST phy_mode
>>>>> this parameter is a assoc per peer parameter as far as i can say here.
>>>>> consider that the firmware will accept anything except 0 for peer_bw_rxnss_override in vht160 operation mode
>>>>> if a peer is 80 mhz, the code will be skipped here.
>>>>
>>>> From what I can tell, this feature is supposed to configure the rate-ctrl in the firmware to know that
>>>> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can send at higher NSS if it sends
>>>> at 80Mhz or lower.
>>> right. but thats exactly what it should does in case that a peer is connecting with vht160 / 80_80
>>> and the peer itself does also send his own nss capabilities which is used if available. if not ,it uses the fallback.
>>>>
>>>> If a 2x2 peer connects to the AP, will it have peer_num_spatial_streams set to 2?
>>> yes. i had some debug code in my initial early versions. the peer does transmit his own nss capabilities.
>>>> If so,
>>>> then your code will configure the peer_bw_rxnss_override to indicate it can send at 160Mhz
>>>> 2x2 as well, right? And if so, then that is incorrect.
>>> no. since nss_override is only set if peer phymode is 160 mhz as well
>>
>> The peer can run at 2x2 80Mhz and 1x1 160Mhz at the same time, so it can advertise
>> 2x2 nss and phy-mode of 160Mhz when associating to the 160Mhz AP, but the peer can only do 1x1 at 160Mhz. There
>> is no standard way I know of for the peer to tell you specifically that it can only do
>> 1x1 at 160Mhz and also 2x2 at 80Mhz in this case.
> per specification the peer is able to provide max nss to the ap. the rx_nss property is calculated
> from the mcs rateset provided by the peer by mac80211. this is mcs set provided on on assoc is mandatory.
> so there is a way the peer can tell you what it supports and this is what is used.
> if a peer does not provide the mcs rateset (which i have seen for a single marvell client)
> the fallback mechanism will still work, but just with 1x1. the problem is anything else will crash the firmware.
> we have to deal with that.
>
> That is why this rxnns_override exists, to hack around this problem.
>
> i dont think so, because its not just a hack. without providing that parameter the firmware will crash.
> so its a always required parameter and not just a hack. for sure the firmware can handle it by itself, just someone
> at qca should start to work on it.
> but again. my implementation is correct from the information i have out of the qca propertiery wireless driver sources
>>
>> Your patch is going to break in this case as far as I can tell.
> no it isnt. my tests with various different clients from different vendors shows that its working. with 2x2 and 1x1.
> its all well detected by the code and configured as expected
> consider that this patch fixes a CRASH. accept that. it wont break. it repairs.
I did some testing with the patch below.
The CHAIMASK_ERR is a debug log from FW that I added to help make sure the patch is
acting as desired. The first hex is an identifier, second is the value passed in,
third is phymode, 4th is the tx-chain-mask for 160Mhz frames.
On station side, when associating a 4x4 9984 station to 9984 configured for nss4, 160Mhz, I see:
[86376.620303] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2 spatial-streams: 4
ath10k-fw: ts: 15229 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
On station side, when associating a 4x4 9984 station with chain-mask of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
[86147.569319] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 780 rxnss_override: 0x80000000 nss160: 1 spatial-streams: 2
ath10k-fw: ts: 6807 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
On AP side, when associating a 4x4 9984 station to 9984 configured for 160Mhz, I see:
[11167.635324] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2 spatial-streams: 4
ath10k-fw: ts: 72917 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
On AP side, when associating a 4x4 9984 station with chain-mask of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
[11422.887181] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 780 rxnss_override: 0x80000000 nss160: 1 spatial-streams: 2
ath10k-fw: ts: 334266 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
On AP side, when associating a 4x4 9984 station configured for 80Mhz instead of 160,
then logging from firmware indicates full 4x4 rates are supported for 80Mhz and below,
and the rxnss_override does not have the (1<<31) bit set:
ath10k-fw: ts: 519211 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x00000000 0x0000000a 0x00000001
So, I think this might be a better fix for this problem (included inline for discussion,
probably white-space damaged by email client:
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index e1ad983..8bce916 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2860,18 +2860,39 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
arg->peer_vht_rates.rx_max_rate, arg->peer_vht_rates.rx_mcs_set,
arg->peer_vht_rates.tx_max_rate, arg->peer_vht_rates.tx_mcs_set);
- if (arg->peer_vht_rates.rx_max_rate &&
- (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
- switch (arg->peer_vht_rates.rx_max_rate) {
+ if (arg->peer_phymode == MODE_11AC_VHT80_80 ||
+ arg->peer_phymode == MODE_11AC_VHT160) {
+ int nss160;
+ int rx = arg->peer_vht_rates.rx_max_rate;
+ /* Deal with cases where chainmask has been decreased.
+ * All known chips that support 160Mhz can do only 1/2 of
+ * the available chains at 160Mhz.
+ */
+ rx = min((int)(arg->peer_num_spatial_streams * 390), rx);
+
+ switch (rx) {
+ /* When a NIC shows up that can do 4x4 at 160Mhz, its
+ * max-rate should be higher, and we can set nss160
+ * to 4 here.
+ */
case 1560:
/* Must be 2x2 at 160Mhz is all it can do. */
- arg->peer_bw_rxnss_override = 2;
+ nss160 = 2;
break;
- case 780:
- /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
- arg->peer_bw_rxnss_override = 1;
+ default:
+ /* Assume we can only do 1x1 at 160Mhz */
+ nss160 = 1;
break;
}
+
+ arg->peer_bw_rxnss_override = ((nss160 - 1) | /* 160Mhz nss */
+ ((nss160 - 1) << 3) | /* 80+80 nss */
+ BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
+
+ ath10k_warn(ar, "NIC rx-max-rate: %d calculated-max: %d rxnss_override: 0x%x nss160: %d spatial-streams: %d\n",
+ arg->peer_vht_rates.rx_max_rate, rx,
+ arg->peer_bw_rxnss_override, nss160,
+ arg->peer_num_spatial_streams);
}
}
@@ -3115,9 +3136,9 @@ static int ath10k_peer_assoc_prepare(struct ath10k *ar,
ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
+ ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
- ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
ath10k_peer_assoc_h_rate_overrides(ar, vif, sta, arg);
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 8eeeab0..365d509 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -7442,12 +7442,8 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k *ar, void *buf,
struct wmi_10_4_peer_assoc_complete_cmd *cmd = buf;
ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
- if (arg->peer_bw_rxnss_override)
- cmd->peer_bw_rxnss_override =
- __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
- BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
- else
- cmd->peer_bw_rxnss_override = 0;
+
+ cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
}
static int
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
2018-04-27 21:57 ` Ben Greear
@ 2018-04-28 0:24 ` Sebastian Gottschall
-1 siblings, 0 replies; 22+ messages in thread
From: Sebastian Gottschall @ 2018-04-28 0:24 UTC (permalink / raw)
To: Ben Greear, ath10k, linux-wireless; +Cc: kvalo
Am 27.04.2018 um 23:57 schrieb Ben Greear:
> On 04/27/2018 11:54 AM, Sebastian Gottschall wrote:
>> Am 27.04.2018 um 18:07 schrieb Ben Greear:
>>> On 04/26/2018 09:40 PM, Sebastian Gottschall wrote:
>>>> Am 26.04.2018 um 22:35 schrieb Ben Greear:
>>>>> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>>>>>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>>>>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>>
>>>>>>>> current handling of peer_bw_rxnss_override parameter is based
>>>>>>>> on guessing the VHT160/8080 capability by rx rate. this is
>>>>>>>> wrong and may lead
>>>>>>>> to a non initialized peer_bw_rxnss_override parameter which is
>>>>>>>> required since VHT160 operation mode only supports 2x2
>>>>>>>> chainmasks in addition the original
>>>>>>>> code
>>>>>>>> initialized the parameter with wrong masked values.
>>>>>>>> This patch uses the peer phymode and peer nss information for
>>>>>>>> correct initialisation of the peer_bw_rxnss_override parameter.
>>>>>>>> if this peer information is not available, we initialize the
>>>>>>>> parameter by minimum nss which is suggested by QCA as temporary
>>>>>>>> workaround according
>>>>>>>> to the QCA sourcecodes.
>>>>>>>>
>>>>>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>>
>>>>>>>> v2: remove debug messages
>>>>>>>> ---
>>>>>>>> drivers/net/wireless/ath/ath10k/mac.c | 36
>>>>>>>> +++++++++++++++++++----------------
>>>>>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>>>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>>>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>> b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>> index 5be6386ede8f..df79af89ee71 100644
>>>>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>> @@ -2505,8 +2505,9 @@ static void
>>>>>>>> ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>>>>>
>>>>>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>>>>>> + }
>>>>>>>>
>>>>>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>>>>>> * supports VHT.
>>>>>>>> @@ -2529,22 +2530,25 @@ static void
>>>>>>>> ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>>> arg->peer_vht_rates.tx_mcs_set =
>>>>>>>> ath10k_peer_assoc_h_vht_limit(
>>>>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>>>>>
>>>>>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu
>>>>>>>> %d flags 0x%x\n",
>>>>>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>>>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode ==
>>>>>>>> MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>>>>>> + arg->peer_bw_rxnss_override =
>>>>>>>> BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>>>>>> + }
>>>>>>>
>>>>>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160
>>>>>>> AP. From what I can tell,
>>>>>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case,
>>>>>>> but if peer is VHT-160,
>>>>>>> then of course it can only talk at 2x2.
>>>>>>>
>>>>>>> So, I don't think you can just look at the
>>>>>>> peer_num_spatial_streams here.
>>>>>> no? rxnss_override is only taked if peer phymode is vht160 or
>>>>>> vht80_80. vht80 is not considered in that code PEER phy_mode, not
>>>>>> HOST phy_mode
>>>>>> this parameter is a assoc per peer parameter as far as i can say
>>>>>> here.
>>>>>> consider that the firmware will accept anything except 0 for
>>>>>> peer_bw_rxnss_override in vht160 operation mode
>>>>>> if a peer is 80 mhz, the code will be skipped here.
>>>>>
>>>>> From what I can tell, this feature is supposed to configure the
>>>>> rate-ctrl in the firmware to know that
>>>>> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it
>>>>> can send at higher NSS if it sends
>>>>> at 80Mhz or lower.
>>>> right. but thats exactly what it should does in case that a peer is
>>>> connecting with vht160 / 80_80
>>>> and the peer itself does also send his own nss capabilities which
>>>> is used if available. if not ,it uses the fallback.
>>>>>
>>>>> If a 2x2 peer connects to the AP, will it have
>>>>> peer_num_spatial_streams set to 2?
>>>> yes. i had some debug code in my initial early versions. the peer
>>>> does transmit his own nss capabilities.
>>>>> If so,
>>>>> then your code will configure the peer_bw_rxnss_override to
>>>>> indicate it can send at 160Mhz
>>>>> 2x2 as well, right? And if so, then that is incorrect.
>>>> no. since nss_override is only set if peer phymode is 160 mhz as well
>>>
>>> The peer can run at 2x2 80Mhz and 1x1 160Mhz at the same time, so it
>>> can advertise
>>> 2x2 nss and phy-mode of 160Mhz when associating to the 160Mhz AP,
>>> but the peer can only do 1x1 at 160Mhz. There
>>> is no standard way I know of for the peer to tell you specifically
>>> that it can only do
>>> 1x1 at 160Mhz and also 2x2 at 80Mhz in this case.
>> per specification the peer is able to provide max nss to the ap. the
>> rx_nss property is calculated
>> from the mcs rateset provided by the peer by mac80211. this is mcs
>> set provided on on assoc is mandatory.
>> so there is a way the peer can tell you what it supports and this is
>> what is used.
>> if a peer does not provide the mcs rateset (which i have seen for a
>> single marvell client)
>> the fallback mechanism will still work, but just with 1x1. the
>> problem is anything else will crash the firmware.
>> we have to deal with that.
>>
>> That is why this rxnns_override exists, to hack around this problem.
>>
>> i dont think so, because its not just a hack. without providing that
>> parameter the firmware will crash.
>> so its a always required parameter and not just a hack. for sure the
>> firmware can handle it by itself, just someone
>> at qca should start to work on it.
>> but again. my implementation is correct from the information i have
>> out of the qca propertiery wireless driver sources
>>>
>>> Your patch is going to break in this case as far as I can tell.
>> no it isnt. my tests with various different clients from different
>> vendors shows that its working. with 2x2 and 1x1.
>> its all well detected by the code and configured as expected
>> consider that this patch fixes a CRASH. accept that. it wont break.
>> it repairs.
>
> I did some testing with the patch below.
>
> The CHAIMASK_ERR is a debug log from FW that I added to help make sure
> the patch is
> acting as desired. The first hex is an identifier, second is the
> value passed in,
> third is phymode, 4th is the tx-chain-mask for 160Mhz frames.
>
> On station side, when associating a 4x4 9984 station to 9984
> configured for nss4, 160Mhz, I see:
> [86376.620303] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2
> spatial-streams: 4
> ath10k-fw: ts: 15229 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>
> On station side, when associating a 4x4 9984 station with chain-mask
> of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
>
> [86147.569319] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 780 rxnss_override: 0x80000000 nss160: 1
> spatial-streams: 2
> ath10k-fw: ts: 6807 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03)
> 0x000000af 0x80000000 0x0000000f 0x00000001
>
>
> On AP side, when associating a 4x4 9984 station to 9984 configured for
> 160Mhz, I see:
>
> [11167.635324] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2
> spatial-streams: 4
> ath10k-fw: ts: 72917 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>
> On AP side, when associating a 4x4 9984 station with chain-mask of 0x3
> (2x2) to 9984 configured for nss4, 160Mhz, I see:
>
> [11422.887181] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 780 rxnss_override: 0x80000000 nss160: 1
> spatial-streams: 2
> ath10k-fw: ts: 334266 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
>
> On AP side, when associating a 4x4 9984 station configured for 80Mhz
> instead of 160,
> then logging from firmware indicates full 4x4 rates are supported for
> 80Mhz and below,
> and the rxnss_override does not have the (1<<31) bit set:
>
> ath10k-fw: ts: 519211 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x00000000 0x0000000a 0x00000001
>
>
> So, I think this might be a better fix for this problem (included
> inline for discussion,
> probably white-space damaged by email client:
i will review this tomorrow and check if all the weired math is okay.
the rx_max_rate is no good idea
from what i see. the marvell client i tested has the problem that it has
the mcs rate set so i can calculcate the max nss
value, but rx_max_rate is empty. that would result in 1x1, even if it
can do 2x2.
in your code peer_num_spatial_streams is also always zero
since peer_num_spatial_streams must be set before with min(sta->rx_nss,
max_nss)
maybe the solution could be to combine both methods. so if rx_max_rate
is set, we use your way and if not we
try to use nss_max. or we calculate nssvht160_max based on the mcs
rateset, which is also a solution since the rateset should not contain
any 3x3 or 4x4 values
for vht160 rates
>
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
> b/drivers/net/wireless/ath/ath10k/mac.c
> index e1ad983..8bce916 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -2860,18 +2860,39 @@ static void ath10k_peer_assoc_h_vht(struct
> ath10k *ar,
> arg->peer_vht_rates.rx_max_rate,
> arg->peer_vht_rates.rx_mcs_set,
> arg->peer_vht_rates.tx_max_rate,
> arg->peer_vht_rates.tx_mcs_set);
>
> - if (arg->peer_vht_rates.rx_max_rate &&
> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
> - switch (arg->peer_vht_rates.rx_max_rate) {
> + if (arg->peer_phymode == MODE_11AC_VHT80_80 ||
> + arg->peer_phymode == MODE_11AC_VHT160) {
> + int nss160;
> + int rx = arg->peer_vht_rates.rx_max_rate;
> + /* Deal with cases where chainmask has been decreased.
> + * All known chips that support 160Mhz can do only 1/2 of
> + * the available chains at 160Mhz.
> + */
> + rx = min((int)(arg->peer_num_spatial_streams * 390), rx);
> +
> + switch (rx) {
> + /* When a NIC shows up that can do 4x4 at 160Mhz, its
> + * max-rate should be higher, and we can set nss160
> + * to 4 here.
> + */
> case 1560:
> /* Must be 2x2 at 160Mhz is all it can do. */
> - arg->peer_bw_rxnss_override = 2;
> + nss160 = 2;
> break;
> - case 780:
> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
> - arg->peer_bw_rxnss_override = 1;
> + default:
> + /* Assume we can only do 1x1 at 160Mhz */
> + nss160 = 1;
> break;
> }
> +
> + arg->peer_bw_rxnss_override = ((nss160 - 1) | /* 160Mhz nss */
> + ((nss160 - 1) << 3) | /* 80+80 nss */
> + BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
> +
> + ath10k_warn(ar, "NIC rx-max-rate: %d calculated-max: %d
> rxnss_override: 0x%x nss160: %d spatial-streams: %d\n",
> + arg->peer_vht_rates.rx_max_rate, rx,
> + arg->peer_bw_rxnss_override, nss160,
> + arg->peer_num_spatial_streams);
> }
> }
>
> @@ -3115,9 +3136,9 @@ static int ath10k_peer_assoc_prepare(struct
> ath10k *ar,
> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>
> ath10k_peer_assoc_h_rate_overrides(ar, vif, sta, arg);
>
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
> b/drivers/net/wireless/ath/ath10k/wmi.c
> index 8eeeab0..365d509 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -7442,12 +7442,8 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k
> *ar, void *buf,
> struct wmi_10_4_peer_assoc_complete_cmd *cmd = buf;
>
> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
> - if (arg->peer_bw_rxnss_override)
> - cmd->peer_bw_rxnss_override =
> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
> - else
> - cmd->peer_bw_rxnss_override = 0;
> +
> + cmd->peer_bw_rxnss_override =
> __cpu_to_le32(arg->peer_bw_rxnss_override);
> }
>
> static int
>
>
>
> Thanks,
> Ben
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-28 0:24 ` Sebastian Gottschall
0 siblings, 0 replies; 22+ messages in thread
From: Sebastian Gottschall @ 2018-04-28 0:24 UTC (permalink / raw)
To: Ben Greear, ath10k, linux-wireless; +Cc: kvalo
Am 27.04.2018 um 23:57 schrieb Ben Greear:
> On 04/27/2018 11:54 AM, Sebastian Gottschall wrote:
>> Am 27.04.2018 um 18:07 schrieb Ben Greear:
>>> On 04/26/2018 09:40 PM, Sebastian Gottschall wrote:
>>>> Am 26.04.2018 um 22:35 schrieb Ben Greear:
>>>>> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>>>>>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>>>>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>>
>>>>>>>> current handling of peer_bw_rxnss_override parameter is based
>>>>>>>> on guessing the VHT160/8080 capability by rx rate. this is
>>>>>>>> wrong and may lead
>>>>>>>> to a non initialized peer_bw_rxnss_override parameter which is
>>>>>>>> required since VHT160 operation mode only supports 2x2
>>>>>>>> chainmasks in addition the original
>>>>>>>> code
>>>>>>>> initialized the parameter with wrong masked values.
>>>>>>>> This patch uses the peer phymode and peer nss information for
>>>>>>>> correct initialisation of the peer_bw_rxnss_override parameter.
>>>>>>>> if this peer information is not available, we initialize the
>>>>>>>> parameter by minimum nss which is suggested by QCA as temporary
>>>>>>>> workaround according
>>>>>>>> to the QCA sourcecodes.
>>>>>>>>
>>>>>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>>
>>>>>>>> v2: remove debug messages
>>>>>>>> ---
>>>>>>>> drivers/net/wireless/ath/ath10k/mac.c | 36
>>>>>>>> +++++++++++++++++++----------------
>>>>>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>>>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>>>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>> b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>> index 5be6386ede8f..df79af89ee71 100644
>>>>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>> @@ -2505,8 +2505,9 @@ static void
>>>>>>>> ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>>>>>
>>>>>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>>>>>> + }
>>>>>>>>
>>>>>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>>>>>> * supports VHT.
>>>>>>>> @@ -2529,22 +2530,25 @@ static void
>>>>>>>> ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>>> arg->peer_vht_rates.tx_mcs_set =
>>>>>>>> ath10k_peer_assoc_h_vht_limit(
>>>>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>>>>>
>>>>>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu
>>>>>>>> %d flags 0x%x\n",
>>>>>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>>>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode ==
>>>>>>>> MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>>>>>> + arg->peer_bw_rxnss_override =
>>>>>>>> BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>>>>>> + }
>>>>>>>
>>>>>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160
>>>>>>> AP. From what I can tell,
>>>>>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case,
>>>>>>> but if peer is VHT-160,
>>>>>>> then of course it can only talk at 2x2.
>>>>>>>
>>>>>>> So, I don't think you can just look at the
>>>>>>> peer_num_spatial_streams here.
>>>>>> no? rxnss_override is only taked if peer phymode is vht160 or
>>>>>> vht80_80. vht80 is not considered in that code PEER phy_mode, not
>>>>>> HOST phy_mode
>>>>>> this parameter is a assoc per peer parameter as far as i can say
>>>>>> here.
>>>>>> consider that the firmware will accept anything except 0 for
>>>>>> peer_bw_rxnss_override in vht160 operation mode
>>>>>> if a peer is 80 mhz, the code will be skipped here.
>>>>>
>>>>> From what I can tell, this feature is supposed to configure the
>>>>> rate-ctrl in the firmware to know that
>>>>> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it
>>>>> can send at higher NSS if it sends
>>>>> at 80Mhz or lower.
>>>> right. but thats exactly what it should does in case that a peer is
>>>> connecting with vht160 / 80_80
>>>> and the peer itself does also send his own nss capabilities which
>>>> is used if available. if not ,it uses the fallback.
>>>>>
>>>>> If a 2x2 peer connects to the AP, will it have
>>>>> peer_num_spatial_streams set to 2?
>>>> yes. i had some debug code in my initial early versions. the peer
>>>> does transmit his own nss capabilities.
>>>>> If so,
>>>>> then your code will configure the peer_bw_rxnss_override to
>>>>> indicate it can send at 160Mhz
>>>>> 2x2 as well, right? And if so, then that is incorrect.
>>>> no. since nss_override is only set if peer phymode is 160 mhz as well
>>>
>>> The peer can run at 2x2 80Mhz and 1x1 160Mhz at the same time, so it
>>> can advertise
>>> 2x2 nss and phy-mode of 160Mhz when associating to the 160Mhz AP,
>>> but the peer can only do 1x1 at 160Mhz. There
>>> is no standard way I know of for the peer to tell you specifically
>>> that it can only do
>>> 1x1 at 160Mhz and also 2x2 at 80Mhz in this case.
>> per specification the peer is able to provide max nss to the ap. the
>> rx_nss property is calculated
>> from the mcs rateset provided by the peer by mac80211. this is mcs
>> set provided on on assoc is mandatory.
>> so there is a way the peer can tell you what it supports and this is
>> what is used.
>> if a peer does not provide the mcs rateset (which i have seen for a
>> single marvell client)
>> the fallback mechanism will still work, but just with 1x1. the
>> problem is anything else will crash the firmware.
>> we have to deal with that.
>>
>> That is why this rxnns_override exists, to hack around this problem.
>>
>> i dont think so, because its not just a hack. without providing that
>> parameter the firmware will crash.
>> so its a always required parameter and not just a hack. for sure the
>> firmware can handle it by itself, just someone
>> at qca should start to work on it.
>> but again. my implementation is correct from the information i have
>> out of the qca propertiery wireless driver sources
>>>
>>> Your patch is going to break in this case as far as I can tell.
>> no it isnt. my tests with various different clients from different
>> vendors shows that its working. with 2x2 and 1x1.
>> its all well detected by the code and configured as expected
>> consider that this patch fixes a CRASH. accept that. it wont break.
>> it repairs.
>
> I did some testing with the patch below.
>
> The CHAIMASK_ERR is a debug log from FW that I added to help make sure
> the patch is
> acting as desired. The first hex is an identifier, second is the
> value passed in,
> third is phymode, 4th is the tx-chain-mask for 160Mhz frames.
>
> On station side, when associating a 4x4 9984 station to 9984
> configured for nss4, 160Mhz, I see:
> [86376.620303] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2
> spatial-streams: 4
> ath10k-fw: ts: 15229 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>
> On station side, when associating a 4x4 9984 station with chain-mask
> of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
>
> [86147.569319] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 780 rxnss_override: 0x80000000 nss160: 1
> spatial-streams: 2
> ath10k-fw: ts: 6807 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03)
> 0x000000af 0x80000000 0x0000000f 0x00000001
>
>
> On AP side, when associating a 4x4 9984 station to 9984 configured for
> 160Mhz, I see:
>
> [11167.635324] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2
> spatial-streams: 4
> ath10k-fw: ts: 72917 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>
> On AP side, when associating a 4x4 9984 station with chain-mask of 0x3
> (2x2) to 9984 configured for nss4, 160Mhz, I see:
>
> [11422.887181] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 780 rxnss_override: 0x80000000 nss160: 1
> spatial-streams: 2
> ath10k-fw: ts: 334266 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
>
> On AP side, when associating a 4x4 9984 station configured for 80Mhz
> instead of 160,
> then logging from firmware indicates full 4x4 rates are supported for
> 80Mhz and below,
> and the rxnss_override does not have the (1<<31) bit set:
>
> ath10k-fw: ts: 519211 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x00000000 0x0000000a 0x00000001
>
>
> So, I think this might be a better fix for this problem (included
> inline for discussion,
> probably white-space damaged by email client:
i will review this tomorrow and check if all the weired math is okay.
the rx_max_rate is no good idea
from what i see. the marvell client i tested has the problem that it has
the mcs rate set so i can calculcate the max nss
value, but rx_max_rate is empty. that would result in 1x1, even if it
can do 2x2.
in your code peer_num_spatial_streams is also always zero
since peer_num_spatial_streams must be set before with min(sta->rx_nss,
max_nss)
maybe the solution could be to combine both methods. so if rx_max_rate
is set, we use your way and if not we
try to use nss_max. or we calculate nssvht160_max based on the mcs
rateset, which is also a solution since the rateset should not contain
any 3x3 or 4x4 values
for vht160 rates
>
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
> b/drivers/net/wireless/ath/ath10k/mac.c
> index e1ad983..8bce916 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -2860,18 +2860,39 @@ static void ath10k_peer_assoc_h_vht(struct
> ath10k *ar,
> arg->peer_vht_rates.rx_max_rate,
> arg->peer_vht_rates.rx_mcs_set,
> arg->peer_vht_rates.tx_max_rate,
> arg->peer_vht_rates.tx_mcs_set);
>
> - if (arg->peer_vht_rates.rx_max_rate &&
> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
> - switch (arg->peer_vht_rates.rx_max_rate) {
> + if (arg->peer_phymode == MODE_11AC_VHT80_80 ||
> + arg->peer_phymode == MODE_11AC_VHT160) {
> + int nss160;
> + int rx = arg->peer_vht_rates.rx_max_rate;
> + /* Deal with cases where chainmask has been decreased.
> + * All known chips that support 160Mhz can do only 1/2 of
> + * the available chains at 160Mhz.
> + */
> + rx = min((int)(arg->peer_num_spatial_streams * 390), rx);
> +
> + switch (rx) {
> + /* When a NIC shows up that can do 4x4 at 160Mhz, its
> + * max-rate should be higher, and we can set nss160
> + * to 4 here.
> + */
> case 1560:
> /* Must be 2x2 at 160Mhz is all it can do. */
> - arg->peer_bw_rxnss_override = 2;
> + nss160 = 2;
> break;
> - case 780:
> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
> - arg->peer_bw_rxnss_override = 1;
> + default:
> + /* Assume we can only do 1x1 at 160Mhz */
> + nss160 = 1;
> break;
> }
> +
> + arg->peer_bw_rxnss_override = ((nss160 - 1) | /* 160Mhz nss */
> + ((nss160 - 1) << 3) | /* 80+80 nss */
> + BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
> +
> + ath10k_warn(ar, "NIC rx-max-rate: %d calculated-max: %d
> rxnss_override: 0x%x nss160: %d spatial-streams: %d\n",
> + arg->peer_vht_rates.rx_max_rate, rx,
> + arg->peer_bw_rxnss_override, nss160,
> + arg->peer_num_spatial_streams);
> }
> }
>
> @@ -3115,9 +3136,9 @@ static int ath10k_peer_assoc_prepare(struct
> ath10k *ar,
> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>
> ath10k_peer_assoc_h_rate_overrides(ar, vif, sta, arg);
>
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
> b/drivers/net/wireless/ath/ath10k/wmi.c
> index 8eeeab0..365d509 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -7442,12 +7442,8 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k
> *ar, void *buf,
> struct wmi_10_4_peer_assoc_complete_cmd *cmd = buf;
>
> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
> - if (arg->peer_bw_rxnss_override)
> - cmd->peer_bw_rxnss_override =
> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
> - else
> - cmd->peer_bw_rxnss_override = 0;
> +
> + cmd->peer_bw_rxnss_override =
> __cpu_to_le32(arg->peer_bw_rxnss_override);
> }
>
> static int
>
>
>
> Thanks,
> Ben
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
2018-04-27 21:57 ` Ben Greear
@ 2018-04-28 0:35 ` Sebastian Gottschall
-1 siblings, 0 replies; 22+ messages in thread
From: Sebastian Gottschall @ 2018-04-28 0:35 UTC (permalink / raw)
To: Ben Greear, ath10k, linux-wireless; +Cc: kvalo
> I did some testing with the patch below.
i mean you tested your own patch here but i dont see results for mine.
i tested my patch in the same way, just not on firmware side
but i logged the values for rxnss_override and the looked correct all
the time
>
> The CHAIMASK_ERR is a debug log from FW that I added to help make sure
> the patch is
> acting as desired. The first hex is an identifier, second is the
> value passed in,
> third is phymode, 4th is the tx-chain-mask for 160Mhz frames.
>
> On station side, when associating a 4x4 9984 station to 9984
> configured for nss4, 160Mhz, I see:
> [86376.620303] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2
> spatial-streams: 4
> ath10k-fw: ts: 15229 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>
> On station side, when associating a 4x4 9984 station with chain-mask
> of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
>
> [86147.569319] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 780 rxnss_override: 0x80000000 nss160: 1
> spatial-streams: 2
> ath10k-fw: ts: 6807 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03)
> 0x000000af 0x80000000 0x0000000f 0x00000001
>
>
> On AP side, when associating a 4x4 9984 station to 9984 configured for
> 160Mhz, I see:
>
> [11167.635324] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2
> spatial-streams: 4
> ath10k-fw: ts: 72917 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>
> On AP side, when associating a 4x4 9984 station with chain-mask of 0x3
> (2x2) to 9984 configured for nss4, 160Mhz, I see:
>
> [11422.887181] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 780 rxnss_override: 0x80000000 nss160: 1
> spatial-streams: 2
> ath10k-fw: ts: 334266 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
>
> On AP side, when associating a 4x4 9984 station configured for 80Mhz
> instead of 160,
> then logging from firmware indicates full 4x4 rates are supported for
> 80Mhz and below,
> and the rxnss_override does not have the (1<<31) bit set:
>
> ath10k-fw: ts: 519211 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x00000000 0x0000000a 0x00000001
>
>
> So, I think this might be a better fix for this problem (included
> inline for discussion,
> probably white-space damaged by email client:
>
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
> b/drivers/net/wireless/ath/ath10k/mac.c
> index e1ad983..8bce916 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -2860,18 +2860,39 @@ static void ath10k_peer_assoc_h_vht(struct
> ath10k *ar,
> arg->peer_vht_rates.rx_max_rate,
> arg->peer_vht_rates.rx_mcs_set,
> arg->peer_vht_rates.tx_max_rate,
> arg->peer_vht_rates.tx_mcs_set);
>
> - if (arg->peer_vht_rates.rx_max_rate &&
> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
> - switch (arg->peer_vht_rates.rx_max_rate) {
> + if (arg->peer_phymode == MODE_11AC_VHT80_80 ||
> + arg->peer_phymode == MODE_11AC_VHT160) {
> + int nss160;
> + int rx = arg->peer_vht_rates.rx_max_rate;
> + /* Deal with cases where chainmask has been decreased.
> + * All known chips that support 160Mhz can do only 1/2 of
> + * the available chains at 160Mhz.
> + */
> + rx = min((int)(arg->peer_num_spatial_streams * 390), rx);
> +
> + switch (rx) {
> + /* When a NIC shows up that can do 4x4 at 160Mhz, its
> + * max-rate should be higher, and we can set nss160
> + * to 4 here.
> + */
> case 1560:
> /* Must be 2x2 at 160Mhz is all it can do. */
> - arg->peer_bw_rxnss_override = 2;
> + nss160 = 2;
> break;
> - case 780:
> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
> - arg->peer_bw_rxnss_override = 1;
> + default:
> + /* Assume we can only do 1x1 at 160Mhz */
> + nss160 = 1;
> break;
> }
> +
> + arg->peer_bw_rxnss_override = ((nss160 - 1) | /* 160Mhz nss */
> + ((nss160 - 1) << 3) | /* 80+80 nss */
> + BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
> +
> + ath10k_warn(ar, "NIC rx-max-rate: %d calculated-max: %d
> rxnss_override: 0x%x nss160: %d spatial-streams: %d\n",
> + arg->peer_vht_rates.rx_max_rate, rx,
> + arg->peer_bw_rxnss_override, nss160,
> + arg->peer_num_spatial_streams);
> }
> }
>
> @@ -3115,9 +3136,9 @@ static int ath10k_peer_assoc_prepare(struct
> ath10k *ar,
> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>
> ath10k_peer_assoc_h_rate_overrides(ar, vif, sta, arg);
>
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
> b/drivers/net/wireless/ath/ath10k/wmi.c
> index 8eeeab0..365d509 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -7442,12 +7442,8 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k
> *ar, void *buf,
> struct wmi_10_4_peer_assoc_complete_cmd *cmd = buf;
>
> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
> - if (arg->peer_bw_rxnss_override)
> - cmd->peer_bw_rxnss_override =
> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
> - else
> - cmd->peer_bw_rxnss_override = 0;
> +
> + cmd->peer_bw_rxnss_override =
> __cpu_to_le32(arg->peer_bw_rxnss_override);
> }
>
> static int
>
>
>
> Thanks,
> Ben
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-28 0:35 ` Sebastian Gottschall
0 siblings, 0 replies; 22+ messages in thread
From: Sebastian Gottschall @ 2018-04-28 0:35 UTC (permalink / raw)
To: Ben Greear, ath10k, linux-wireless; +Cc: kvalo
> I did some testing with the patch below.
i mean you tested your own patch here but i dont see results for mine.
i tested my patch in the same way, just not on firmware side
but i logged the values for rxnss_override and the looked correct all
the time
>
> The CHAIMASK_ERR is a debug log from FW that I added to help make sure
> the patch is
> acting as desired. The first hex is an identifier, second is the
> value passed in,
> third is phymode, 4th is the tx-chain-mask for 160Mhz frames.
>
> On station side, when associating a 4x4 9984 station to 9984
> configured for nss4, 160Mhz, I see:
> [86376.620303] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2
> spatial-streams: 4
> ath10k-fw: ts: 15229 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>
> On station side, when associating a 4x4 9984 station with chain-mask
> of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
>
> [86147.569319] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 780 rxnss_override: 0x80000000 nss160: 1
> spatial-streams: 2
> ath10k-fw: ts: 6807 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03)
> 0x000000af 0x80000000 0x0000000f 0x00000001
>
>
> On AP side, when associating a 4x4 9984 station to 9984 configured for
> 160Mhz, I see:
>
> [11167.635324] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2
> spatial-streams: 4
> ath10k-fw: ts: 72917 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>
> On AP side, when associating a 4x4 9984 station with chain-mask of 0x3
> (2x2) to 9984 configured for nss4, 160Mhz, I see:
>
> [11422.887181] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560
> calculated-max: 780 rxnss_override: 0x80000000 nss160: 1
> spatial-streams: 2
> ath10k-fw: ts: 334266 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
>
> On AP side, when associating a 4x4 9984 station configured for 80Mhz
> instead of 160,
> then logging from firmware indicates full 4x4 rates are supported for
> 80Mhz and below,
> and the rxnss_override does not have the (1<<31) bit set:
>
> ath10k-fw: ts: 519211 args: 4 RATE_CTRL(19) vid: 255
> CHAINMASK_ERR(03) 0x000000af 0x00000000 0x0000000a 0x00000001
>
>
> So, I think this might be a better fix for this problem (included
> inline for discussion,
> probably white-space damaged by email client:
>
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
> b/drivers/net/wireless/ath/ath10k/mac.c
> index e1ad983..8bce916 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -2860,18 +2860,39 @@ static void ath10k_peer_assoc_h_vht(struct
> ath10k *ar,
> arg->peer_vht_rates.rx_max_rate,
> arg->peer_vht_rates.rx_mcs_set,
> arg->peer_vht_rates.tx_max_rate,
> arg->peer_vht_rates.tx_mcs_set);
>
> - if (arg->peer_vht_rates.rx_max_rate &&
> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
> - switch (arg->peer_vht_rates.rx_max_rate) {
> + if (arg->peer_phymode == MODE_11AC_VHT80_80 ||
> + arg->peer_phymode == MODE_11AC_VHT160) {
> + int nss160;
> + int rx = arg->peer_vht_rates.rx_max_rate;
> + /* Deal with cases where chainmask has been decreased.
> + * All known chips that support 160Mhz can do only 1/2 of
> + * the available chains at 160Mhz.
> + */
> + rx = min((int)(arg->peer_num_spatial_streams * 390), rx);
> +
> + switch (rx) {
> + /* When a NIC shows up that can do 4x4 at 160Mhz, its
> + * max-rate should be higher, and we can set nss160
> + * to 4 here.
> + */
> case 1560:
> /* Must be 2x2 at 160Mhz is all it can do. */
> - arg->peer_bw_rxnss_override = 2;
> + nss160 = 2;
> break;
> - case 780:
> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
> - arg->peer_bw_rxnss_override = 1;
> + default:
> + /* Assume we can only do 1x1 at 160Mhz */
> + nss160 = 1;
> break;
> }
> +
> + arg->peer_bw_rxnss_override = ((nss160 - 1) | /* 160Mhz nss */
> + ((nss160 - 1) << 3) | /* 80+80 nss */
> + BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
> +
> + ath10k_warn(ar, "NIC rx-max-rate: %d calculated-max: %d
> rxnss_override: 0x%x nss160: %d spatial-streams: %d\n",
> + arg->peer_vht_rates.rx_max_rate, rx,
> + arg->peer_bw_rxnss_override, nss160,
> + arg->peer_num_spatial_streams);
> }
> }
>
> @@ -3115,9 +3136,9 @@ static int ath10k_peer_assoc_prepare(struct
> ath10k *ar,
> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>
> ath10k_peer_assoc_h_rate_overrides(ar, vif, sta, arg);
>
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
> b/drivers/net/wireless/ath/ath10k/wmi.c
> index 8eeeab0..365d509 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -7442,12 +7442,8 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k
> *ar, void *buf,
> struct wmi_10_4_peer_assoc_complete_cmd *cmd = buf;
>
> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
> - if (arg->peer_bw_rxnss_override)
> - cmd->peer_bw_rxnss_override =
> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
> - else
> - cmd->peer_bw_rxnss_override = 0;
> +
> + cmd->peer_bw_rxnss_override =
> __cpu_to_le32(arg->peer_bw_rxnss_override);
> }
>
> static int
>
>
>
> Thanks,
> Ben
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
2018-04-28 0:24 ` Sebastian Gottschall
@ 2018-04-28 15:08 ` Ben Greear
-1 siblings, 0 replies; 22+ messages in thread
From: Ben Greear @ 2018-04-28 15:08 UTC (permalink / raw)
To: Sebastian Gottschall, ath10k, linux-wireless; +Cc: kvalo
On 04/27/2018 05:24 PM, Sebastian Gottschall wrote:
> Am 27.04.2018 um 23:57 schrieb Ben Greear:
>> On 04/27/2018 11:54 AM, Sebastian Gottschall wrote:
>>> Am 27.04.2018 um 18:07 schrieb Ben Greear:
>>>> On 04/26/2018 09:40 PM, Sebastian Gottschall wrote:
>>>>> Am 26.04.2018 um 22:35 schrieb Ben Greear:
>>>>>> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>>>>>>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>>>>>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>>>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>>>
>>>>>>>>> current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
>>>>>>>>> to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original
>>>>>>>>> code
>>>>>>>>> initialized the parameter with wrong masked values.
>>>>>>>>> This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
>>>>>>>>> if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
>>>>>>>>> to the QCA sourcecodes.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>>>
>>>>>>>>> v2: remove debug messages
>>>>>>>>> ---
>>>>>>>>> drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
>>>>>>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>>>>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>>>>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>>> index 5be6386ede8f..df79af89ee71 100644
>>>>>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>>>>>>
>>>>>>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>>>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>>>>>>> + }
>>>>>>>>>
>>>>>>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>>>>>>> * supports VHT.
>>>>>>>>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>>>>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>>>>>>
>>>>>>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
>>>>>>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>>>>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>>>>>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>>>>>>> + }
>>>>>>>>
>>>>>>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From what I can tell,
>>>>>>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if peer is VHT-160,
>>>>>>>> then of course it can only talk at 2x2.
>>>>>>>>
>>>>>>>> So, I don't think you can just look at the peer_num_spatial_streams here.
>>>>>>> no? rxnss_override is only taked if peer phymode is vht160 or vht80_80. vht80 is not considered in that code PEER phy_mode, not HOST phy_mode
>>>>>>> this parameter is a assoc per peer parameter as far as i can say here.
>>>>>>> consider that the firmware will accept anything except 0 for peer_bw_rxnss_override in vht160 operation mode
>>>>>>> if a peer is 80 mhz, the code will be skipped here.
>>>>>>
>>>>>> From what I can tell, this feature is supposed to configure the rate-ctrl in the firmware to know that
>>>>>> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can send at higher NSS if it sends
>>>>>> at 80Mhz or lower.
>>>>> right. but thats exactly what it should does in case that a peer is connecting with vht160 / 80_80
>>>>> and the peer itself does also send his own nss capabilities which is used if available. if not ,it uses the fallback.
>>>>>>
>>>>>> If a 2x2 peer connects to the AP, will it have peer_num_spatial_streams set to 2?
>>>>> yes. i had some debug code in my initial early versions. the peer does transmit his own nss capabilities.
>>>>>> If so,
>>>>>> then your code will configure the peer_bw_rxnss_override to indicate it can send at 160Mhz
>>>>>> 2x2 as well, right? And if so, then that is incorrect.
>>>>> no. since nss_override is only set if peer phymode is 160 mhz as well
>>>>
>>>> The peer can run at 2x2 80Mhz and 1x1 160Mhz at the same time, so it can advertise
>>>> 2x2 nss and phy-mode of 160Mhz when associating to the 160Mhz AP, but the peer can only do 1x1 at 160Mhz. There
>>>> is no standard way I know of for the peer to tell you specifically that it can only do
>>>> 1x1 at 160Mhz and also 2x2 at 80Mhz in this case.
>>> per specification the peer is able to provide max nss to the ap. the rx_nss property is calculated
>>> from the mcs rateset provided by the peer by mac80211. this is mcs set provided on on assoc is mandatory.
>>> so there is a way the peer can tell you what it supports and this is what is used.
>>> if a peer does not provide the mcs rateset (which i have seen for a single marvell client)
>>> the fallback mechanism will still work, but just with 1x1. the problem is anything else will crash the firmware.
>>> we have to deal with that.
>>>
>>> That is why this rxnns_override exists, to hack around this problem.
>>>
>>> i dont think so, because its not just a hack. without providing that parameter the firmware will crash.
>>> so its a always required parameter and not just a hack. for sure the firmware can handle it by itself, just someone
>>> at qca should start to work on it.
>>> but again. my implementation is correct from the information i have out of the qca propertiery wireless driver sources
>>>>
>>>> Your patch is going to break in this case as far as I can tell.
>>> no it isnt. my tests with various different clients from different vendors shows that its working. with 2x2 and 1x1.
>>> its all well detected by the code and configured as expected
>>> consider that this patch fixes a CRASH. accept that. it wont break. it repairs.
>>
>> I did some testing with the patch below.
>>
>> The CHAIMASK_ERR is a debug log from FW that I added to help make sure the patch is
>> acting as desired. The first hex is an identifier, second is the value passed in,
>> third is phymode, 4th is the tx-chain-mask for 160Mhz frames.
>>
>> On station side, when associating a 4x4 9984 station to 9984 configured for nss4, 160Mhz, I see:
>> [86376.620303] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2 spatial-streams: 4
>> ath10k-fw: ts: 15229 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>>
>> On station side, when associating a 4x4 9984 station with chain-mask of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
>>
>> [86147.569319] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 780 rxnss_override: 0x80000000 nss160: 1 spatial-streams: 2
>> ath10k-fw: ts: 6807 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
>>
>>
>> On AP side, when associating a 4x4 9984 station to 9984 configured for 160Mhz, I see:
>>
>> [11167.635324] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2 spatial-streams: 4
>> ath10k-fw: ts: 72917 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>>
>> On AP side, when associating a 4x4 9984 station with chain-mask of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
>>
>> [11422.887181] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 780 rxnss_override: 0x80000000 nss160: 1 spatial-streams: 2
>> ath10k-fw: ts: 334266 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
>>
>> On AP side, when associating a 4x4 9984 station configured for 80Mhz instead of 160,
>> then logging from firmware indicates full 4x4 rates are supported for 80Mhz and below,
>> and the rxnss_override does not have the (1<<31) bit set:
>>
>> ath10k-fw: ts: 519211 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x00000000 0x0000000a 0x00000001
>>
>>
>> So, I think this might be a better fix for this problem (included inline for discussion,
>> probably white-space damaged by email client:
> i will review this tomorrow and check if all the weired math is okay. the rx_max_rate is no good idea
> from what i see. the marvell client i tested has the problem that it has the mcs rate set so i can calculcate the max nss
> value, but rx_max_rate is empty. that would result in 1x1, even if it can do 2x2.
> in your code peer_num_spatial_streams is also always zero
I logged my output, and peer_num_spatial_streams was at the expected values. I originally chose
the max-rate test because some day there might be a NIC that does 4x4 at 80 and also 4x4 at 160, it would
have same nss, but it's max rate would be higher.
> since peer_num_spatial_streams must be set before with min(sta->rx_nss, max_nss)
> maybe the solution could be to combine both methods. so if rx_max_rate is set, we use your way and if not we
> try to use nss_max. or we calculate nssvht160_max based on the mcs rateset, which is also a solution since the rateset should not contain any 3x3 or 4x4 values
> for vht160 rates
Using the rateset like that might work. My FW logging does show that at least my FW properly calculates the
rateset to disable the 4x4 and 3x3 at 160. I am not certain that part is done in the driver though.
Thanks,
Ben
>>
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index e1ad983..8bce916 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -2860,18 +2860,39 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>> arg->peer_vht_rates.rx_max_rate, arg->peer_vht_rates.rx_mcs_set,
>> arg->peer_vht_rates.tx_max_rate, arg->peer_vht_rates.tx_mcs_set);
>>
>> - if (arg->peer_vht_rates.rx_max_rate &&
>> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
>> - switch (arg->peer_vht_rates.rx_max_rate) {
>> + if (arg->peer_phymode == MODE_11AC_VHT80_80 ||
>> + arg->peer_phymode == MODE_11AC_VHT160) {
>> + int nss160;
>> + int rx = arg->peer_vht_rates.rx_max_rate;
>> + /* Deal with cases where chainmask has been decreased.
>> + * All known chips that support 160Mhz can do only 1/2 of
>> + * the available chains at 160Mhz.
>> + */
>> + rx = min((int)(arg->peer_num_spatial_streams * 390), rx);
>> +
>> + switch (rx) {
>> + /* When a NIC shows up that can do 4x4 at 160Mhz, its
>> + * max-rate should be higher, and we can set nss160
>> + * to 4 here.
>> + */
>> case 1560:
>> /* Must be 2x2 at 160Mhz is all it can do. */
>> - arg->peer_bw_rxnss_override = 2;
>> + nss160 = 2;
>> break;
>> - case 780:
>> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
>> - arg->peer_bw_rxnss_override = 1;
>> + default:
>> + /* Assume we can only do 1x1 at 160Mhz */
>> + nss160 = 1;
>> break;
>> }
>> +
>> + arg->peer_bw_rxnss_override = ((nss160 - 1) | /* 160Mhz nss */
>> + ((nss160 - 1) << 3) | /* 80+80 nss */
>> + BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
>> +
>> + ath10k_warn(ar, "NIC rx-max-rate: %d calculated-max: %d rxnss_override: 0x%x nss160: %d spatial-streams: %d\n",
>> + arg->peer_vht_rates.rx_max_rate, rx,
>> + arg->peer_bw_rxnss_override, nss160,
>> + arg->peer_num_spatial_streams);
>> }
>> }
>>
>> @@ -3115,9 +3136,9 @@ static int ath10k_peer_assoc_prepare(struct ath10k *ar,
>> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
>> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
>> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>
>> ath10k_peer_assoc_h_rate_overrides(ar, vif, sta, arg);
>>
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
>> index 8eeeab0..365d509 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>> @@ -7442,12 +7442,8 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k *ar, void *buf,
>> struct wmi_10_4_peer_assoc_complete_cmd *cmd = buf;
>>
>> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
>> - if (arg->peer_bw_rxnss_override)
>> - cmd->peer_bw_rxnss_override =
>> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
>> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
>> - else
>> - cmd->peer_bw_rxnss_override = 0;
>> +
>> + cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
>> }
>>
>> static int
>>
>>
>>
>> Thanks,
>> Ben
>>
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter
@ 2018-04-28 15:08 ` Ben Greear
0 siblings, 0 replies; 22+ messages in thread
From: Ben Greear @ 2018-04-28 15:08 UTC (permalink / raw)
To: Sebastian Gottschall, ath10k, linux-wireless; +Cc: kvalo
On 04/27/2018 05:24 PM, Sebastian Gottschall wrote:
> Am 27.04.2018 um 23:57 schrieb Ben Greear:
>> On 04/27/2018 11:54 AM, Sebastian Gottschall wrote:
>>> Am 27.04.2018 um 18:07 schrieb Ben Greear:
>>>> On 04/26/2018 09:40 PM, Sebastian Gottschall wrote:
>>>>> Am 26.04.2018 um 22:35 schrieb Ben Greear:
>>>>>> On 04/26/2018 01:21 PM, Sebastian Gottschall wrote:
>>>>>>> Am 26.04.2018 um 17:30 schrieb Ben Greear:
>>>>>>>> On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote:
>>>>>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>>>
>>>>>>>>> current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
>>>>>>>>> to a non initialized peer_bw_rxnss_override parameter which is required since VHT160 operation mode only supports 2x2 chainmasks in addition the original
>>>>>>>>> code
>>>>>>>>> initialized the parameter with wrong masked values.
>>>>>>>>> This patch uses the peer phymode and peer nss information for correct initialisation of the peer_bw_rxnss_override parameter.
>>>>>>>>> if this peer information is not available, we initialize the parameter by minimum nss which is suggested by QCA as temporary workaround according
>>>>>>>>> to the QCA sourcecodes.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>>>
>>>>>>>>> v2: remove debug messages
>>>>>>>>> ---
>>>>>>>>> drivers/net/wireless/ath/ath10k/mac.c | 36 +++++++++++++++++++----------------
>>>>>>>>> drivers/net/wireless/ath/ath10k/wmi.c | 4 +---
>>>>>>>>> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++-
>>>>>>>>> 3 files changed, 34 insertions(+), 20 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>>> index 5be6386ede8f..df79af89ee71 100644
>>>>>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>>>>>> @@ -2505,8 +2505,9 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>>>> if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>>>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw80;
>>>>>>>>>
>>>>>>>>> - if (sta->bandwidth == IEEE80211_STA_RX_BW_160)
>>>>>>>>> + if (sta->bandwidth == IEEE80211_STA_RX_BW_160) {
>>>>>>>>> arg->peer_flags |= ar->wmi.peer_flags->bw160;
>>>>>>>>> + }
>>>>>>>>>
>>>>>>>>> /* Calculate peer NSS capability from VHT capabilities if STA
>>>>>>>>> * supports VHT.
>>>>>>>>> @@ -2529,22 +2530,25 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>>>>>>>>> arg->peer_vht_rates.tx_mcs_set = ath10k_peer_assoc_h_vht_limit(
>>>>>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask);
>>>>>>>>>
>>>>>>>>> - ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mpdu %d flags 0x%x\n",
>>>>>>>>> - sta->addr, arg->peer_max_mpdu, arg->peer_flags);
>>>>>>>>> + if (arg->peer_num_spatial_streams && (arg->peer_phymode == MODE_11AC_VHT160 || arg->peer_phymode == MODE_11AC_VHT80_80)) {
>>>>>>>>> + arg->peer_bw_rxnss_override = BW_NSS_FWCONF_160(arg->peer_num_spatial_streams);
>>>>>>>>> + }
>>>>>>>>
>>>>>>>> So, an 80Mhz peer could be 4x4 and could connect to a VHT-160 AP. From what I can tell,
>>>>>>>> the VHT-160 AP can talk 4x4 @ 80Mhz to the peer in this case, but if peer is VHT-160,
>>>>>>>> then of course it can only talk at 2x2.
>>>>>>>>
>>>>>>>> So, I don't think you can just look at the peer_num_spatial_streams here.
>>>>>>> no? rxnss_override is only taked if peer phymode is vht160 or vht80_80. vht80 is not considered in that code PEER phy_mode, not HOST phy_mode
>>>>>>> this parameter is a assoc per peer parameter as far as i can say here.
>>>>>>> consider that the firmware will accept anything except 0 for peer_bw_rxnss_override in vht160 operation mode
>>>>>>> if a peer is 80 mhz, the code will be skipped here.
>>>>>>
>>>>>> From what I can tell, this feature is supposed to configure the rate-ctrl in the firmware to know that
>>>>>> it can only send 1x1 or 2x2 when sending at 160Mhz, but that it can send at higher NSS if it sends
>>>>>> at 80Mhz or lower.
>>>>> right. but thats exactly what it should does in case that a peer is connecting with vht160 / 80_80
>>>>> and the peer itself does also send his own nss capabilities which is used if available. if not ,it uses the fallback.
>>>>>>
>>>>>> If a 2x2 peer connects to the AP, will it have peer_num_spatial_streams set to 2?
>>>>> yes. i had some debug code in my initial early versions. the peer does transmit his own nss capabilities.
>>>>>> If so,
>>>>>> then your code will configure the peer_bw_rxnss_override to indicate it can send at 160Mhz
>>>>>> 2x2 as well, right? And if so, then that is incorrect.
>>>>> no. since nss_override is only set if peer phymode is 160 mhz as well
>>>>
>>>> The peer can run at 2x2 80Mhz and 1x1 160Mhz at the same time, so it can advertise
>>>> 2x2 nss and phy-mode of 160Mhz when associating to the 160Mhz AP, but the peer can only do 1x1 at 160Mhz. There
>>>> is no standard way I know of for the peer to tell you specifically that it can only do
>>>> 1x1 at 160Mhz and also 2x2 at 80Mhz in this case.
>>> per specification the peer is able to provide max nss to the ap. the rx_nss property is calculated
>>> from the mcs rateset provided by the peer by mac80211. this is mcs set provided on on assoc is mandatory.
>>> so there is a way the peer can tell you what it supports and this is what is used.
>>> if a peer does not provide the mcs rateset (which i have seen for a single marvell client)
>>> the fallback mechanism will still work, but just with 1x1. the problem is anything else will crash the firmware.
>>> we have to deal with that.
>>>
>>> That is why this rxnns_override exists, to hack around this problem.
>>>
>>> i dont think so, because its not just a hack. without providing that parameter the firmware will crash.
>>> so its a always required parameter and not just a hack. for sure the firmware can handle it by itself, just someone
>>> at qca should start to work on it.
>>> but again. my implementation is correct from the information i have out of the qca propertiery wireless driver sources
>>>>
>>>> Your patch is going to break in this case as far as I can tell.
>>> no it isnt. my tests with various different clients from different vendors shows that its working. with 2x2 and 1x1.
>>> its all well detected by the code and configured as expected
>>> consider that this patch fixes a CRASH. accept that. it wont break. it repairs.
>>
>> I did some testing with the patch below.
>>
>> The CHAIMASK_ERR is a debug log from FW that I added to help make sure the patch is
>> acting as desired. The first hex is an identifier, second is the value passed in,
>> third is phymode, 4th is the tx-chain-mask for 160Mhz frames.
>>
>> On station side, when associating a 4x4 9984 station to 9984 configured for nss4, 160Mhz, I see:
>> [86376.620303] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2 spatial-streams: 4
>> ath10k-fw: ts: 15229 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>>
>> On station side, when associating a 4x4 9984 station with chain-mask of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
>>
>> [86147.569319] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 780 rxnss_override: 0x80000000 nss160: 1 spatial-streams: 2
>> ath10k-fw: ts: 6807 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
>>
>>
>> On AP side, when associating a 4x4 9984 station to 9984 configured for 160Mhz, I see:
>>
>> [11167.635324] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 1560 rxnss_override: 0x80000009 nss160: 2 spatial-streams: 4
>> ath10k-fw: ts: 72917 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000009 0x0000000f 0x00000003
>>
>> On AP side, when associating a 4x4 9984 station with chain-mask of 0x3 (2x2) to 9984 configured for nss4, 160Mhz, I see:
>>
>> [11422.887181] ath10k_pci 0000:04:00.0: NIC rx-max-rate: 1560 calculated-max: 780 rxnss_override: 0x80000000 nss160: 1 spatial-streams: 2
>> ath10k-fw: ts: 334266 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x80000000 0x0000000f 0x00000001
>>
>> On AP side, when associating a 4x4 9984 station configured for 80Mhz instead of 160,
>> then logging from firmware indicates full 4x4 rates are supported for 80Mhz and below,
>> and the rxnss_override does not have the (1<<31) bit set:
>>
>> ath10k-fw: ts: 519211 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03) 0x000000af 0x00000000 0x0000000a 0x00000001
>>
>>
>> So, I think this might be a better fix for this problem (included inline for discussion,
>> probably white-space damaged by email client:
> i will review this tomorrow and check if all the weired math is okay. the rx_max_rate is no good idea
> from what i see. the marvell client i tested has the problem that it has the mcs rate set so i can calculcate the max nss
> value, but rx_max_rate is empty. that would result in 1x1, even if it can do 2x2.
> in your code peer_num_spatial_streams is also always zero
I logged my output, and peer_num_spatial_streams was at the expected values. I originally chose
the max-rate test because some day there might be a NIC that does 4x4 at 80 and also 4x4 at 160, it would
have same nss, but it's max rate would be higher.
> since peer_num_spatial_streams must be set before with min(sta->rx_nss, max_nss)
> maybe the solution could be to combine both methods. so if rx_max_rate is set, we use your way and if not we
> try to use nss_max. or we calculate nssvht160_max based on the mcs rateset, which is also a solution since the rateset should not contain any 3x3 or 4x4 values
> for vht160 rates
Using the rateset like that might work. My FW logging does show that at least my FW properly calculates the
rateset to disable the 4x4 and 3x3 at 160. I am not certain that part is done in the driver though.
Thanks,
Ben
>>
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index e1ad983..8bce916 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -2860,18 +2860,39 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
>> arg->peer_vht_rates.rx_max_rate, arg->peer_vht_rates.rx_mcs_set,
>> arg->peer_vht_rates.tx_max_rate, arg->peer_vht_rates.tx_mcs_set);
>>
>> - if (arg->peer_vht_rates.rx_max_rate &&
>> - (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK)) {
>> - switch (arg->peer_vht_rates.rx_max_rate) {
>> + if (arg->peer_phymode == MODE_11AC_VHT80_80 ||
>> + arg->peer_phymode == MODE_11AC_VHT160) {
>> + int nss160;
>> + int rx = arg->peer_vht_rates.rx_max_rate;
>> + /* Deal with cases where chainmask has been decreased.
>> + * All known chips that support 160Mhz can do only 1/2 of
>> + * the available chains at 160Mhz.
>> + */
>> + rx = min((int)(arg->peer_num_spatial_streams * 390), rx);
>> +
>> + switch (rx) {
>> + /* When a NIC shows up that can do 4x4 at 160Mhz, its
>> + * max-rate should be higher, and we can set nss160
>> + * to 4 here.
>> + */
>> case 1560:
>> /* Must be 2x2 at 160Mhz is all it can do. */
>> - arg->peer_bw_rxnss_override = 2;
>> + nss160 = 2;
>> break;
>> - case 780:
>> - /* Can only do 1x1 at 160Mhz (Long Guard Interval) */
>> - arg->peer_bw_rxnss_override = 1;
>> + default:
>> + /* Assume we can only do 1x1 at 160Mhz */
>> + nss160 = 1;
>> break;
>> }
>> +
>> + arg->peer_bw_rxnss_override = ((nss160 - 1) | /* 160Mhz nss */
>> + ((nss160 - 1) << 3) | /* 80+80 nss */
>> + BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
>> +
>> + ath10k_warn(ar, "NIC rx-max-rate: %d calculated-max: %d rxnss_override: 0x%x nss160: %d spatial-streams: %d\n",
>> + arg->peer_vht_rates.rx_max_rate, rx,
>> + arg->peer_bw_rxnss_override, nss160,
>> + arg->peer_num_spatial_streams);
>> }
>> }
>>
>> @@ -3115,9 +3136,9 @@ static int ath10k_peer_assoc_prepare(struct ath10k *ar,
>> ath10k_peer_assoc_h_crypto(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_rates(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_ht(ar, vif, sta, arg);
>> + ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_vht(ar, vif, sta, arg);
>> ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
>> - ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);
>>
>> ath10k_peer_assoc_h_rate_overrides(ar, vif, sta, arg);
>>
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
>> index 8eeeab0..365d509 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>> @@ -7442,12 +7442,8 @@ ath10k_wmi_peer_assoc_fill_10_4(struct ath10k *ar, void *buf,
>> struct wmi_10_4_peer_assoc_complete_cmd *cmd = buf;
>>
>> ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg);
>> - if (arg->peer_bw_rxnss_override)
>> - cmd->peer_bw_rxnss_override =
>> - __cpu_to_le32((arg->peer_bw_rxnss_override - 1) |
>> - BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET));
>> - else
>> - cmd->peer_bw_rxnss_override = 0;
>> +
>> + cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
>> }
>>
>> static int
>>
>>
>>
>> Thanks,
>> Ben
>>
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-04-28 15:08 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-26 9:28 [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter s.gottschall
2018-04-26 9:28 ` s.gottschall
2018-04-26 15:30 ` Ben Greear
2018-04-26 15:30 ` Ben Greear
2018-04-26 20:21 ` Sebastian Gottschall
2018-04-26 20:21 ` Sebastian Gottschall
2018-04-26 20:35 ` Ben Greear
2018-04-26 20:35 ` Ben Greear
2018-04-27 4:40 ` Sebastian Gottschall
2018-04-27 4:40 ` Sebastian Gottschall
2018-04-27 16:07 ` Ben Greear
2018-04-27 16:07 ` Ben Greear
2018-04-27 18:54 ` Sebastian Gottschall
2018-04-27 18:54 ` Sebastian Gottschall
2018-04-27 21:57 ` Ben Greear
2018-04-27 21:57 ` Ben Greear
2018-04-28 0:24 ` Sebastian Gottschall
2018-04-28 0:24 ` Sebastian Gottschall
2018-04-28 15:08 ` Ben Greear
2018-04-28 15:08 ` Ben Greear
2018-04-28 0:35 ` Sebastian Gottschall
2018-04-28 0:35 ` Sebastian Gottschall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.