From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from smtps.newmedia-net.de ([2a05:a1c0:0:de::167] helo=webmail.newmedia-net.de) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1fCDfW-0002S2-8o for ath10k@lists.infradead.org; Sat, 28 Apr 2018 00:24:53 +0000 Subject: Re: [PATCH v2] ath10k: fix crash in recent 3.5.3 9984 firmware due wrong handling of peer_bw_rxnss_override parameter References: <20180426092848.23069-1-s.gottschall@dd-wrt.com> <96d512b3-3261-c6cf-e0eb-a9f5fe91f3b8@candelatech.com> <8263a720-07a2-dc5d-f8fc-2153574cbafc@dd-wrt.com> <96f0393e-6bab-d397-d2b6-4538da7fc275@candelatech.com> From: Sebastian Gottschall Message-ID: Date: Sat, 28 Apr 2018 02:24:33 +0200 MIME-Version: 1.0 In-Reply-To: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="windows-1252"; Format="flowed" Sender: "ath10k" Errors-To: ath10k-bounces+kvalo=adurom.com@lists.infradead.org To: Ben Greear , ath10k@lists.infradead.org, linux-wireless@vger.kernel.org Cc: kvalo@codeaurora.org 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 >>>>>>>> >>>>>>>> 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 >>>>>>>> --- >>>>>>>> =A0drivers/net/wireless/ath/ath10k/mac.c | 36 = >>>>>>>> +++++++++++++++++++---------------- >>>>>>>> =A0drivers/net/wireless/ath/ath10k/wmi.c |=A0 4 +--- >>>>>>>> =A0drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++++++++- >>>>>>>> =A03 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, >>>>>>>> =A0=A0=A0=A0 if (sta->bandwidth =3D=3D IEEE80211_STA_RX_BW_80) >>>>>>>> =A0=A0=A0=A0=A0=A0=A0=A0 arg->peer_flags |=3D ar->wmi.peer_flags->= bw80; >>>>>>>> >>>>>>>> -=A0=A0=A0 if (sta->bandwidth =3D=3D IEEE80211_STA_RX_BW_160) >>>>>>>> +=A0=A0=A0 if (sta->bandwidth =3D=3D IEEE80211_STA_RX_BW_160) { >>>>>>>> =A0=A0=A0=A0=A0=A0=A0=A0 arg->peer_flags |=3D ar->wmi.peer_flags->= bw160; >>>>>>>> +=A0=A0=A0 } >>>>>>>> >>>>>>>> =A0=A0=A0=A0 /* Calculate peer NSS capability from VHT capabilitie= s if STA >>>>>>>> =A0=A0=A0=A0=A0 * supports VHT. >>>>>>>> @@ -2529,22 +2530,25 @@ static void = >>>>>>>> ath10k_peer_assoc_h_vht(struct ath10k *ar, >>>>>>>> =A0=A0=A0=A0 arg->peer_vht_rates.tx_mcs_set =3D = >>>>>>>> ath10k_peer_assoc_h_vht_limit( >>>>>>>> __le16_to_cpu(vht_cap->vht_mcs.tx_mcs_map), vht_mcs_mask); >>>>>>>> >>>>>>>> -=A0=A0=A0 ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vht peer %pM max_mp= du = >>>>>>>> %d flags 0x%x\n", >>>>>>>> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 sta->addr, arg->peer_max_mpdu, arg= ->peer_flags); >>>>>>>> +=A0=A0=A0 if (arg->peer_num_spatial_streams && (arg->peer_phymode= =3D=3D = >>>>>>>> MODE_11AC_VHT160 || arg->peer_phymode =3D=3D MODE_11AC_VHT80_80)) { >>>>>>>> +=A0=A0=A0=A0=A0=A0=A0 arg->peer_bw_rxnss_override =3D = >>>>>>>> BW_NSS_FWCONF_160(arg->peer_num_spatial_streams); >>>>>>>> +=A0=A0=A0 } >>>>>>> >>>>>>> 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?=A0 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.=A0 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.=A0 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=A0 nss160: 2 = > spatial-streams: 4 > =A0 ath10k-fw: ts: 15229 args: 4 RATE_CTRL(19) vid: 255 = > CHAINMASK_ERR(03)=A0 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=A0 nss160: 1 = > spatial-streams: 2 > ath10k-fw: ts: 6807 args: 4 RATE_CTRL(19) vid: 255 CHAINMASK_ERR(03)=A0 = > 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=A0 nss160: 2 = > spatial-streams: 4 > =A0 ath10k-fw: ts: 72917 args: 4 RATE_CTRL(19) vid: 255 = > CHAINMASK_ERR(03)=A0 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=A0 nss160: 1 = > spatial-streams: 2 > =A0 ath10k-fw: ts: 334266 args: 4 RATE_CTRL(19) vid: 255 = > CHAINMASK_ERR(03)=A0 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: > > =A0 ath10k-fw: ts: 519211 args: 4 RATE_CTRL(19) vid: 255 = > CHAINMASK_ERR(03)=A0 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, > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 arg->peer_vht_rates.rx_max_rate, = > arg->peer_vht_rates.rx_mcs_set, > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 arg->peer_vht_rates.tx_max_rate, = > arg->peer_vht_rates.tx_mcs_set); > > -=A0=A0=A0 if (arg->peer_vht_rates.rx_max_rate && > -=A0=A0=A0=A0=A0=A0=A0 (sta->vht_cap.cap & IEEE80211_VHT_CAP_SUPP_CHAN_WI= DTH_MASK)) { > -=A0=A0=A0=A0=A0=A0=A0 switch (arg->peer_vht_rates.rx_max_rate) { > +=A0=A0=A0 if (arg->peer_phymode =3D=3D MODE_11AC_VHT80_80 || > +=A0=A0=A0=A0=A0=A0=A0 arg->peer_phymode =3D=3D MODE_11AC_VHT160) { > +=A0=A0=A0=A0=A0=A0=A0 int nss160; > +=A0=A0=A0=A0=A0=A0=A0 int rx =3D arg->peer_vht_rates.rx_max_rate; > +=A0=A0=A0=A0=A0=A0=A0 /* Deal with cases where chainmask has been decrea= sed. > +=A0=A0=A0=A0=A0=A0=A0=A0 * All known chips that support 160Mhz can do on= ly 1/2 of > +=A0=A0=A0=A0=A0=A0=A0=A0 * the available chains at 160Mhz. > +=A0=A0=A0=A0=A0=A0=A0=A0 */ > +=A0=A0=A0=A0=A0=A0=A0 rx =3D min((int)(arg->peer_num_spatial_streams * 3= 90), rx); > + > +=A0=A0=A0=A0=A0=A0=A0 switch (rx) { > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 /* When a NIC shows up that can do 4x4= at 160Mhz, its > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * max-rate should be higher, and we= can set nss160 > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * to 4 here. > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 */ > =A0=A0=A0=A0=A0=A0=A0=A0 case 1560: > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 /* Must be 2x2 at 160Mhz is all it c= an do. */ > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 arg->peer_bw_rxnss_override =3D 2; > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 nss160 =3D 2; > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 break; > -=A0=A0=A0=A0=A0=A0=A0 case 780: > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 /* Can only do 1x1 at 160Mhz (Long Gua= rd Interval) */ > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 arg->peer_bw_rxnss_override =3D 1; > +=A0=A0=A0=A0=A0=A0=A0 default: > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 /* Assume we can only do 1x1 at 160Mhz= */ > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 nss160 =3D 1; > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 break; > =A0=A0=A0=A0=A0=A0=A0=A0 } > + > +=A0=A0=A0=A0=A0=A0=A0 arg->peer_bw_rxnss_override =3D ((nss160 - 1) | /*= 160Mhz nss */ > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 ((nss160 - 1) << 3) | /* 80+80 nss */ > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 BIT(PEER_BW_RXNSS_OVERRIDE_OFFSET)); > + > +=A0=A0=A0=A0=A0=A0=A0 ath10k_warn(ar, "NIC rx-max-rate: %d calculated-ma= x: %d = > rxnss_override: 0x%x=A0 nss160: %d=A0 spatial-streams: %d\n", > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 arg->peer_vht_rates.rx_max= _rate, rx, > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 arg->peer_bw_rxnss_overrid= e, nss160, > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 arg->peer_num_spatial_stre= ams); > =A0=A0=A0=A0 } > =A0} > > @@ -3115,9 +3136,9 @@ static int ath10k_peer_assoc_prepare(struct = > ath10k *ar, > =A0=A0=A0=A0 ath10k_peer_assoc_h_crypto(ar, vif, sta, arg); > =A0=A0=A0=A0 ath10k_peer_assoc_h_rates(ar, vif, sta, arg); > =A0=A0=A0=A0 ath10k_peer_assoc_h_ht(ar, vif, sta, arg); > +=A0=A0=A0 ath10k_peer_assoc_h_phymode(ar, vif, sta, arg); > =A0=A0=A0=A0 ath10k_peer_assoc_h_vht(ar, vif, sta, arg); > =A0=A0=A0=A0 ath10k_peer_assoc_h_qos(ar, vif, sta, arg); > -=A0=A0=A0 ath10k_peer_assoc_h_phymode(ar, vif, sta, arg); > > =A0=A0=A0=A0 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, > =A0=A0=A0=A0 struct wmi_10_4_peer_assoc_complete_cmd *cmd =3D buf; > > =A0=A0=A0=A0 ath10k_wmi_peer_assoc_fill_10_2(ar, buf, arg); > -=A0=A0=A0 if (arg->peer_bw_rxnss_override) > -=A0=A0=A0=A0=A0=A0=A0 cmd->peer_bw_rxnss_override =3D > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 __cpu_to_le32((arg->peer_bw_rxnss_over= ride - 1) | > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 BIT(PEER= _BW_RXNSS_OVERRIDE_OFFSET)); > -=A0=A0=A0 else > -=A0=A0=A0=A0=A0=A0=A0 cmd->peer_bw_rxnss_override =3D 0; > + > +=A0=A0=A0 cmd->peer_bw_rxnss_override =3D = > __cpu_to_le32(arg->peer_bw_rxnss_override); > =A0} > > =A0static int > > > > Thanks, > Ben > -- = Mit freundlichen Gr=FCssen / Regards Sebastian Gottschall / CTO NewMedia-NET GmbH - DD-WRT Firmensitz: Stubenwaldallee 21a, 64625 Bensheim Registergericht: Amtsgericht Darmstadt, HRB 25473 Gesch=E4ftsf=FChrer: Peter Steinh=E4user, 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