On 9/8/21 6:30 AM, Bryan O'Donoghue wrote:
> We have been tracking a strange bug with Antenna Diversity Switching (ADS)
> on wcn3680b for a while.
>
> ADS is configured like this:
> A. Via a firmware configuration table baked into the firmware binary.
in the NV, not the firmware binary
> 1. Defines if ADS is enabled.
> 2. Defines which GPIOs are connected to which antenna enable pin.
> 3. Defines which antenna/GPIO is primary and which is secondary.
>
> B. WCN36XX_CFG_VAL(ANTENNA_DIVERSITY, N)
> N is a bitmask of available antenna.
>
> Setting N to 3 indicates a bitmask of enabled antenna (1 | 2).
>
> Obviously then we can set N to 1 or N to 2 to fix to a particular
> antenna and disable antenna diversity.
>
> C. WCN36XX_CFG_VAL(ASD_PROBE_INTERVAL, XX)
> XX is the number of beacons between each antenna RSSI check.
> Setting this value to 50 means, every 50 received beacons, run the
> ADS algorithm.
>
> D. WCN36XX_CFG_VAL(ASD_TRIGGER_THRESHOLD, YY)
> YY is a two's complement integer which specifies the RSSI decibel
> threshold below which ADS will run.
> We default to -60db here, meaning a measured RSSI <= -60db will
> trigger an ADS probe.
>
> E. WCN36XX_CFG_VAL(ASD_RTT_RSSI_HYST_THRESHOLD, Z)
> Z is a hysteresis value, indicating a delta which the RSSI must
> exceed for the antenna switch to be valid.
>
> For example if HYST_THRESHOLD == 3 AntennaId1-RSSI == -60db and
> AntennaId-2-RSSI == -58db then firmware will not switch antenna.
> The threshold needs to be -57db or better to satisfy the criteria.
Maybe also worth mentioning there's a feat_cap for
ANTENNA_DIVERSITY_SELECTION, although from what we saw FW doesn't actually
check it.
>
> ADS works like this:
>
> A. Every XX beacons the firmware switches to or remains on the primary
> antenna.
>
> B. The firmware then sends a Request-To-Send (RTS) packet to the AP.
>
> C. The firmware waits for a Clear-To-Send (CTS) response from the AP.
>
> D. The firmware then notes the received RSSI on the CTS packet.
>
> E. The firmware then repeats steps A-D on the secondary antenna.
>
> F. Subsequently if the measured RSSI on the primary or secondary
> antenna is better than ASD_TRIGGER_THRESHOLD +
better than the active antenna's RSSI +
> ASD_RTT_RSSI_HYST_THRESHOLD then that antenna becomes the active
> antenna.
>
> G. If RSSI rises past ASD_TRIGGER_THRESHOLD then ADS doesn't run at
> all even if there is a substantially better RSSI on the other
> antenna.
>
> What we have been observing is that the RTS packet is being sent but the
> MAC address is a byte-swapped version of the target MAC. The ADS/RTS MAC is
> corrupted only when the link is encrypted, if the AP is open the RTS MAC is
> correct. Similarly if we configure the firmware to an RTS/CTS sequence for
> regular data - the transmitted RTS MAC is correctly formatted.
>
> Internally the wcn36xx firmware uses the indexes in the SMD commands to
> populate and extract data from specific entries in an STA lookup table. The
> AP's MAC appears a number of times in different indexes within this lookup
> table, so the MAC address extracted for the data-transmit RTS and the MAC
> address extracted for the ADS/RTS packet are not the same STA table index.
>
> Our analysis indicates the relevant firmware STA table index is
> "bssSelfStaIdx".
>
> There is an STA populate function responsible for formatting the MAC
> address of the bssSelfStaIdx including byte-swapping the MAC address.
>
> Its clear then that the required STA populate command did not run for
> bssSelfStaIdx.
>
> So taking a look at the sequence of SMD commands sent to the firmware we
> see the following downstream when moving from an unencrypted to encrypted
> BSS setup.
>
> - WLAN_HAL_CONFIG_BSS_REQ
> - WLAN_HAL_CONFIG_STA_REQ
> - WLAN_HAL_SET_STAKEY_REQ
>
> Upstream in wcn36xx we have
>
> - WLAN_HAL_CONFIG_BSS_REQ
> - WLAN_HAL_SET_STAKEY_REQ
>
> The solution then is to add the missing WLAN_HAL_CONFIG_STA_REQ between
> WLAN_HAL_CONFIG_BSS_REQ and WLAN_HAL_SET_STAKEY_REQ.
>
> No surprise WLAN_HAL_CONFIG_STA_REQ is the routine responsible for
> populating the STA lookup table in the firmware and once done the MAC sent
> by the ADS routine is in the correct byte-order.
>
> This bug is apparent with ADS but it is also the case that any other
> firmware routine that depends on the "bssSelfStaIdx" would retrieve
> malformed data on an encrypted link.
>
> Fixes: 3e977c5c523d ("wcn36xx: Define wcn3680 specific firmware parameters")
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> ---
> drivers/net/wireless/ath/wcn36xx/main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
> index 2ccf7a8924a0..60cf0516e1bc 100644
> --- a/drivers/net/wireless/ath/wcn36xx/main.c
> +++ b/drivers/net/wireless/ath/wcn36xx/main.c
> @@ -567,12 +567,14 @@ static int wcn36xx_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> if (IEEE80211_KEY_FLAG_PAIRWISE & key_conf->flags) {
> sta_priv->is_data_encrypted = true;
> /* Reconfigure bss with encrypt_type */
> - if (NL80211_IFTYPE_STATION == vif->type)
> + if (NL80211_IFTYPE_STATION == vif->type) {
> wcn36xx_smd_config_bss(wcn,
> vif,
> sta,
> sta->addr,
> true);
> + wcn36xx_smd_config_sta(wcn, vif, sta);
> + }
>
> wcn36xx_smd_set_stakey(wcn,
> vif_priv->encrypt_type,
>
Nice write-up!
Tested-by: Benjamin Li <benl@xxxxxxxxxxxx>
on Square Terminal (2018) with FW-side workaround reverted
