* [PATCH v2 0/2] wcn36xx: Two one line fixes for Antenna Diveristy Switching @ 2021-09-09 14:44 Bryan O'Donoghue 2021-09-09 14:44 ` [PATCH v2 1/2] wcn36xx: Fix Antenna Diversity Switching Bryan O'Donoghue 2021-09-09 14:44 ` [PATCH v2 2/2] wcn36xx: Add ability for wcn36xx_smd_dump_cmd_req to pass two's complement Bryan O'Donoghue 0 siblings, 2 replies; 6+ messages in thread From: Bryan O'Donoghue @ 2021-09-09 14:44 UTC (permalink / raw) To: kvalo, linux-wireless, wcn36xx; +Cc: loic.poulain, benl, bryan.odonoghue V2: Ammends the commit log of the first patch - Clarifies NV is the location of antenna GPIO settings - Adds some text about feature bit ANTENNA_DIVERSITY_SELECTION - Tidies up language around "measured antenna" - Adds Tested-by: Benjamin Li <benl@squareup.com> V1: Here are two one liners. The first fixes a fairly major bug that is apparent with Antenna Diveristy Switching (ADS) but, is not limited to ADS. The bug is a basic firmware table corruption problem, which just happens to manifest itself clearly with the ADS logic. The second fix is a fairly trivial one-liner which prevents us from sending negative, two's complement integers to the firmware via the SMD backdoor "dump" command. Bryan O'Donoghue (2): wcn36xx: Fix Antenna Diversity Switching wcn36xx: Add ability for wcn36xx_smd_dump_cmd_req to pass two's complement drivers/net/wireless/ath/wcn36xx/debug.c | 2 +- drivers/net/wireless/ath/wcn36xx/main.c | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) -- 2.33.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] wcn36xx: Fix Antenna Diversity Switching 2021-09-09 14:44 [PATCH v2 0/2] wcn36xx: Two one line fixes for Antenna Diveristy Switching Bryan O'Donoghue @ 2021-09-09 14:44 ` Bryan O'Donoghue 2021-09-09 15:39 ` Loic Poulain 2021-09-21 13:33 ` Kalle Valo 2021-09-09 14:44 ` [PATCH v2 2/2] wcn36xx: Add ability for wcn36xx_smd_dump_cmd_req to pass two's complement Bryan O'Donoghue 1 sibling, 2 replies; 6+ messages in thread From: Bryan O'Donoghue @ 2021-09-09 14:44 UTC (permalink / raw) To: kvalo, linux-wireless, wcn36xx; +Cc: loic.poulain, benl, bryan.odonoghue 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 NV area. 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. F. A firmware feature bit also exists ANTENNA_DIVERSITY_SELECTION. This feature bit is used by the firmware to report if ANTENNA_DIVERSITY_SELECTION is supported. The host is not required to toggle this bit to enable or disable ADS. 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 RSSI on the measured antenna is better than ASD_TRIGGER_THRESHOLD + the active antenna's RSSI then the measured 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 alternative 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@linaro.org> Tested-by: Benjamin Li <benl@squareup.com> --- 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 9e4fa55d8118..65b4e618d61e 100644 --- a/drivers/net/wireless/ath/wcn36xx/main.c +++ b/drivers/net/wireless/ath/wcn36xx/main.c @@ -580,12 +580,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, -- 2.33.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] wcn36xx: Fix Antenna Diversity Switching 2021-09-09 14:44 ` [PATCH v2 1/2] wcn36xx: Fix Antenna Diversity Switching Bryan O'Donoghue @ 2021-09-09 15:39 ` Loic Poulain 2021-09-21 13:33 ` Kalle Valo 1 sibling, 0 replies; 6+ messages in thread From: Loic Poulain @ 2021-09-09 15:39 UTC (permalink / raw) To: Bryan O'Donoghue; +Cc: Kalle Valo, linux-wireless, wcn36xx, Benjamin Li On Thu, 9 Sept 2021 at 16:42, Bryan O'Donoghue <bryan.odonoghue@linaro.org> 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 NV area. > 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. > > F. A firmware feature bit also exists ANTENNA_DIVERSITY_SELECTION. > This feature bit is used by the firmware to report if > ANTENNA_DIVERSITY_SELECTION is supported. The host is not required to > toggle this bit to enable or disable ADS. > > 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 RSSI on the measured antenna is better than > ASD_TRIGGER_THRESHOLD + the active antenna's RSSI then the > measured 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 alternative > 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@linaro.org> > Tested-by: Benjamin Li <benl@squareup.com> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] wcn36xx: Fix Antenna Diversity Switching 2021-09-09 14:44 ` [PATCH v2 1/2] wcn36xx: Fix Antenna Diversity Switching Bryan O'Donoghue 2021-09-09 15:39 ` Loic Poulain @ 2021-09-21 13:33 ` Kalle Valo 1 sibling, 0 replies; 6+ messages in thread From: Kalle Valo @ 2021-09-21 13:33 UTC (permalink / raw) To: Bryan O'Donoghue Cc: linux-wireless, wcn36xx, loic.poulain, benl, bryan.odonoghue Bryan O'Donoghue <bryan.odonoghue@linaro.org> 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 NV area. > 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. > > F. A firmware feature bit also exists ANTENNA_DIVERSITY_SELECTION. > This feature bit is used by the firmware to report if > ANTENNA_DIVERSITY_SELECTION is supported. The host is not required to > toggle this bit to enable or disable ADS. > > 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 RSSI on the measured antenna is better than > ASD_TRIGGER_THRESHOLD + the active antenna's RSSI then the > measured 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 alternative > 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@linaro.org> > Tested-by: Benjamin Li <benl@squareup.com> > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> 2 patches applied to ath-next branch of ath.git, thanks. 701668d3bfa0 wcn36xx: Fix Antenna Diversity Switching c0c2eb20c79e wcn36xx: Add ability for wcn36xx_smd_dump_cmd_req to pass two's complement -- https://patchwork.kernel.org/project/linux-wireless/patch/20210909144428.2564650-2-bryan.odonoghue@linaro.org/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] wcn36xx: Add ability for wcn36xx_smd_dump_cmd_req to pass two's complement 2021-09-09 14:44 [PATCH v2 0/2] wcn36xx: Two one line fixes for Antenna Diveristy Switching Bryan O'Donoghue 2021-09-09 14:44 ` [PATCH v2 1/2] wcn36xx: Fix Antenna Diversity Switching Bryan O'Donoghue @ 2021-09-09 14:44 ` Bryan O'Donoghue 2021-09-09 15:39 ` Loic Poulain 1 sibling, 1 reply; 6+ messages in thread From: Bryan O'Donoghue @ 2021-09-09 14:44 UTC (permalink / raw) To: kvalo, linux-wireless, wcn36xx; +Cc: loic.poulain, benl, bryan.odonoghue Qcom documents suggest passing of negative values to the dump command, however currently we convert from string to u32 not s32, so we cannot pass a two's complement value to the firmware in this way. There is in fact only one parameter which takes a two's complement value <tigger threshold> in the antenna diversity switch command. Downstream: iwpriv wlan0 dump 71 3 <schedule period> <trigger threshold> <hysteresis value> Upstream: echo "71 3 <schedule period> <trigger threshold> <hysteresis value>" > /sys/kernel/debug/ieee80211/phy0/wcn36xx/dump Fixes: 8e84c2582169 ("wcn36xx: mac80211 driver for Qualcomm WCN3660/WCN3680 hardware") Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/net/wireless/ath/wcn36xx/debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/wcn36xx/debug.c b/drivers/net/wireless/ath/wcn36xx/debug.c index 389b5e7129a6..6af306ae41ad 100644 --- a/drivers/net/wireless/ath/wcn36xx/debug.c +++ b/drivers/net/wireless/ath/wcn36xx/debug.c @@ -120,7 +120,7 @@ static ssize_t write_file_dump(struct file *file, if (begin == NULL) break; - if (kstrtou32(begin, 0, &arg[i]) != 0) + if (kstrtos32(begin, 0, &arg[i]) != 0) break; } -- 2.33.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] wcn36xx: Add ability for wcn36xx_smd_dump_cmd_req to pass two's complement 2021-09-09 14:44 ` [PATCH v2 2/2] wcn36xx: Add ability for wcn36xx_smd_dump_cmd_req to pass two's complement Bryan O'Donoghue @ 2021-09-09 15:39 ` Loic Poulain 0 siblings, 0 replies; 6+ messages in thread From: Loic Poulain @ 2021-09-09 15:39 UTC (permalink / raw) To: Bryan O'Donoghue; +Cc: Kalle Valo, linux-wireless, wcn36xx, Benjamin Li On Thu, 9 Sept 2021 at 16:42, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > Qcom documents suggest passing of negative values to the dump command, > however currently we convert from string to u32 not s32, so we cannot pass > a two's complement value to the firmware in this way. > > There is in fact only one parameter which takes a two's complement value > <tigger threshold> in the antenna diversity switch command. > > Downstream: > iwpriv wlan0 dump 71 3 <schedule period> <trigger threshold> <hysteresis value> > > Upstream: > echo "71 3 <schedule period> <trigger threshold> <hysteresis value>" > /sys/kernel/debug/ieee80211/phy0/wcn36xx/dump > > Fixes: 8e84c2582169 ("wcn36xx: mac80211 driver for Qualcomm WCN3660/WCN3680 hardware") > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-21 13:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-09 14:44 [PATCH v2 0/2] wcn36xx: Two one line fixes for Antenna Diveristy Switching Bryan O'Donoghue 2021-09-09 14:44 ` [PATCH v2 1/2] wcn36xx: Fix Antenna Diversity Switching Bryan O'Donoghue 2021-09-09 15:39 ` Loic Poulain 2021-09-21 13:33 ` Kalle Valo 2021-09-09 14:44 ` [PATCH v2 2/2] wcn36xx: Add ability for wcn36xx_smd_dump_cmd_req to pass two's complement Bryan O'Donoghue 2021-09-09 15:39 ` Loic Poulain
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).