Return-path: Received: from mail2.candelatech.com ([208.74.158.173]:55460 "EHLO mail2.candelatech.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755998AbeDZPau (ORCPT ); Thu, 26 Apr 2018 11:30:50 -0400 Subject: Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter To: s.gottschall@dd-wrt.com, ath10k@lists.infradead.org, linux-wireless@vger.kernel.org References: <20180426092848.23069-1-s.gottschall@dd-wrt.com> Cc: kvalo@codeaurora.org From: Ben Greear Message-ID: (sfid-20180426_173055_994761_8FE8C87F) Date: Thu, 26 Apr 2018 08:30:47 -0700 MIME-Version: 1.0 In-Reply-To: <20180426092848.23069-1-s.gottschall@dd-wrt.com> Content-Type: text/plain; charset=windows-1252; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: On 04/26/2018 02:28 AM, s.gottschall@dd-wrt.com wrote: > From: Sebastian Gottschall > > 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 > > 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 Candela Technologies Inc http://www.candelatech.com