[PATCH] wcn36xx: RFC self-limit sw_scan to 166ms to avoid link stalls
Eugene Krasnikov
k.eugene.e at gmail.com
Tue Jan 27 11:52:27 PST 2015
Why does it take longe to find AP? SMD command is sent almost
immediately. Do we spend less time on each channel?
2015-01-24 15:11 GMT+00:00 Andy Green <andy.green at linaro.org>:
> On wcn3620 with wpa_supplicant set to defaults, it can asks for a
> 10s sw_scan as often as every 40s.
>
> During the 10s scan, the link is basically stopped for the whole
> time, leading to behaviours like this on ping
>
> ...
> 64 bytes from 198.41.208.143: icmp_seq=242 ttl=53 time=40.8 ms
> 64 bytes from 198.41.208.143: icmp_seq=243 ttl=53 time=31.6 ms
> 64 bytes from 198.41.208.143: icmp_seq=244 ttl=53 time=32.2 ms
> 64 bytes from 198.41.208.143: icmp_seq=245 ttl=53 time=8968 ms
> 64 bytes from 198.41.208.143: icmp_seq=246 ttl=53 time=7977 ms
> 64 bytes from 198.41.208.143: icmp_seq=247 ttl=53 time=6977 ms
> 64 bytes from 198.41.208.143: icmp_seq=248 ttl=53 time=6018 ms
> 64 bytes from 198.41.208.143: icmp_seq=249 ttl=53 time=5018 ms
> 64 bytes from 198.41.208.143: icmp_seq=250 ttl=53 time=4019 ms
> 64 bytes from 198.41.208.143: icmp_seq=251 ttl=53 time=3019 ms
> 64 bytes from 198.41.208.143: icmp_seq=252 ttl=53 time=2019 ms
> 64 bytes from 198.41.208.143: icmp_seq=253 ttl=53 time=1019 ms
> 64 bytes from 198.41.208.143: icmp_seq=254 ttl=53 time=34.3 ms
> 64 bytes from 198.41.208.143: icmp_seq=255 ttl=53 time=30.9 ms
> 64 bytes from 198.41.208.143: icmp_seq=256 ttl=53 time=32.9 ms
> 64 bytes from 198.41.208.143: icmp_seq=257 ttl=53 time=30.8 ms
> 64 bytes from 198.41.208.143: icmp_seq=258 ttl=53 time=30.9 ms
> 64 bytes from 198.41.208.143: icmp_seq=259 ttl=53 time=33.7 ms
> ...
>
> As was mentioned on the wcn36xx list, the prima driver does not
> use this kind of scan behaviour, as captured here in the
> wcn36xx-dissector example dumps
>
> https://raw.githubusercontent.com/kanstrup/wcn36xx-dissector/master/examples/prima-3660-sta-connect-11g-wpa2-wmm-screen-off.txt
>
> Instead he scans for 150ms channel by channel, with explicit
> scan start and stop SMD commands for each channel.
>
> This patch decouples the scan action of the driver from the
> sw_scan start and stop ops, in this initial version of the
> patch the driver now does one 166ms scan and stops / finishes
> the scan action itself and waits until the sw_scan stop.
>
> This removes the 10s blocking behaviour, while still
> introducing problems with the link, they are far more
> tolerable than 10s
>
> ...
> 64 bytes from 198.41.209.142: icmp_seq=57 ttl=54 time=31.2 ms
> 64 bytes from 198.41.209.142: icmp_seq=58 ttl=54 time=31.9 ms
> 64 bytes from 198.41.209.142: icmp_seq=59 ttl=54 time=31.2 ms
> 64 bytes from 198.41.209.142: icmp_seq=60 ttl=54 time=31.1 ms
> 64 bytes from 198.41.209.142: icmp_seq=61 ttl=54 time=37.9 ms
> 64 bytes from 198.41.209.142: icmp_seq=62 ttl=54 time=31.5 ms
> 64 bytes from 198.41.209.142: icmp_seq=63 ttl=54 time=198 ms
> 64 bytes from 198.41.209.142: icmp_seq=64 ttl=54 time=31.7 ms
> 64 bytes from 198.41.209.142: icmp_seq=65 ttl=54 time=33.0 ms
> 64 bytes from 198.41.209.142: icmp_seq=66 ttl=54 time=80.2 ms
> 64 bytes from 198.41.209.142: icmp_seq=67 ttl=54 time=159 ms
> 64 bytes from 198.41.209.142: icmp_seq=68 ttl=54 time=34.3 ms
> 64 bytes from 198.41.209.142: icmp_seq=69 ttl=54 time=32.7 ms
> 64 bytes from 198.41.209.142: icmp_seq=70 ttl=54 time=39.3 ms
> 64 bytes from 198.41.209.142: icmp_seq=71 ttl=54 time=118 ms
> 64 bytes from 198.41.209.142: icmp_seq=72 ttl=54 time=31.7 ms
> 64 bytes from 198.41.209.142: icmp_seq=73 ttl=54 time=35.5 ms
> 64 bytes from 198.41.209.142: icmp_seq=74 ttl=54 time=32.0 ms
> 64 bytes from 198.41.209.142: icmp_seq=75 ttl=54 time=33.0 ms
> 64 bytes from 198.41.209.142: icmp_seq=76 ttl=54 time=35.1 ms
> 64 bytes from 198.41.209.142: icmp_seq=77 ttl=54 time=31.8 ms
> 64 bytes from 198.41.209.142: icmp_seq=78 ttl=54 time=32.0 ms
> 64 bytes from 198.41.209.142: icmp_seq=79 ttl=54 time=31.9 ms
> 64 bytes from 198.41.209.142: icmp_seq=80 ttl=54 time=46.0 ms
> 64 bytes from 198.41.209.142: icmp_seq=81 ttl=54 time=38.3 ms
> ...
>
> It takes a bit longer to associate probably because it takes
> longer to find the AP beacon.
>
> For bulk transfer, a download that was interrupted regularly and
> averaged 150KBps before is able to complete at average 1.0MBps
>
> # wget -Obbb.mp4 http://download.blender.org/peach/bigbuckbunny_movies/BigBuckBunny_320x180.mp4
> --1970-01-01 00:03:13-- http://download.blender.org/peach/bigbuckbunny_movies/BigBuckBunny_320x180.mp4
> Resolving download.blender.org (download.blender.org)... 82.94.213.221
> Connecting to download.blender.org (download.blender.org)|82.94.213.221|:80... connected.
> HTTP request sent, awaiting response... 200 OK
> Length: 64657027 (62M) [application/octet-stream]
> Saving to: ‘bbb.mp4’
>
> 100%[======================================>] 64,657,027 1.02MB/s in 61s
>
> 2015-01-24 13:50:11 (1.00 MB/s) - ‘bbb.mp4’ saved [64657027/64657027]
>
> It seems wpa_supplicant is asking for the scans, according to dynamic
> average rssi. That makes the scan requests unpredictable, eg
>
> [ 261.144852] wcn36xx wcn36xx: wcn36xx_scan_work: ch 11
> [ 300.940232] wcn36xx wcn36xx: wcn36xx_scan_work: ch 1
> [ 340.726914] wcn36xx wcn36xx: wcn36xx_scan_work: ch 11
> [ 380.529928] wcn36xx wcn36xx: wcn36xx_scan_work: ch 1
> [ 420.314379] wcn36xx wcn36xx: wcn36xx_scan_work: ch 11
> [ 460.102097] wcn36xx wcn36xx: wcn36xx_scan_work: ch 11
> [ 499.892589] wcn36xx wcn36xx: wcn36xx_scan_work: ch 11
> [ 809.681723] wcn36xx wcn36xx: wcn36xx_scan_work: ch 11
> [ 1119.474430] wcn36xx wcn36xx: wcn36xx_scan_work: ch 11
> [ 1429.264266] wcn36xx wcn36xx: wcn36xx_scan_work: ch 11
> [ 1739.055172] wcn36xx wcn36xx: wcn36xx_scan_work: ch 11
>
> the channel it wants to scan is also unpredictable (this is while
> associated to the only AP with the BSSID on ch11).
>
> Any advice about if this is a legitimate way to cover the
> problem, if there's a better way and if the other wcn36xx
> variants also have this problem (and could use the solution)
> are welcome.
>
> Signed-off-by: Andy Green <andy.green at linaro.org>
> ---
> drivers/net/wireless/ath/wcn36xx/main.c | 9 +-
> drivers/net/wireless/ath/wcn36xx/smd.c | 131 +++++++++++++++++++---------
> drivers/net/wireless/ath/wcn36xx/smd.h | 5 -
> drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 7 +
> 4 files changed, 100 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
> index aff1ea0..9f67c98 100644
> --- a/drivers/net/wireless/ath/wcn36xx/main.c
> +++ b/drivers/net/wireless/ath/wcn36xx/main.c
> @@ -19,6 +19,7 @@
> #include <linux/module.h>
> #include <linux/firmware.h>
> #include <linux/platform_device.h>
> +#include <linux/workqueue.h>
> #include "wcn36xx.h"
>
> unsigned int wcn36xx_dbg_mask;
> @@ -501,8 +502,7 @@ static void wcn36xx_sw_scan_start(struct ieee80211_hw *hw,
> {
> struct wcn36xx *wcn = hw->priv;
>
> - wcn36xx_smd_init_scan(wcn, HAL_SYS_MODE_SCAN);
> - wcn36xx_smd_start_scan(wcn);
> + wcn36xx_smd_init_scan(wcn);
> }
>
> static void wcn36xx_sw_scan_complete(struct ieee80211_hw *hw,
> @@ -510,8 +510,7 @@ static void wcn36xx_sw_scan_complete(struct ieee80211_hw *hw,
> {
> struct wcn36xx *wcn = hw->priv;
>
> - wcn36xx_smd_end_scan(wcn);
> - wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN);
> + wcn36xx_smd_finish_scan(wcn);
> }
>
> static void wcn36xx_update_allowed_rates(struct ieee80211_sta *sta,
> @@ -1035,7 +1034,7 @@ static int wcn36xx_probe(struct platform_device *pdev)
> wcn->chip_version = wcn->ctrl_ops->get_chip_type();
>
> mutex_init(&wcn->hal_mutex);
> -
> + INIT_DELAYED_WORK(&wcn->scan_work, wcn36xx_scan_work);
> if (!wcn->ctrl_ops->get_hw_mac(addr)) {
> wcn36xx_info("mac address: %pM\n", addr);
> SET_IEEE80211_PERM_ADDR(wcn->hw, addr);
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index e9bf8bd..91b7a1e 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -19,6 +19,7 @@
> #include <linux/etherdevice.h>
> #include <linux/firmware.h>
> #include <linux/bitops.h>
> +#include <linux/workqueue.h>
> #include "smd.h"
>
> static int put_cfg_tlv_u32(struct wcn36xx *wcn, size_t *len, u32 id, u32 value)
> @@ -432,126 +433,168 @@ out:
> return ret;
> }
>
> -int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode)
> +int _wcn36xx_smd_finish_scan(struct wcn36xx *wcn)
> {
> - struct wcn36xx_hal_init_scan_req_msg msg_body;
> - int ret = 0;
> + struct wcn36xx_hal_finish_scan_req_msg msg_body;
> + int ret = 0;
>
> - mutex_lock(&wcn->hal_mutex);
> - INIT_HAL_MSG(msg_body, WCN36XX_HAL_INIT_SCAN_REQ);
> + INIT_HAL_MSG(msg_body, WCN36XX_HAL_FINISH_SCAN_REQ);
>
> - msg_body.mode = mode;
> + msg_body.mode = HAL_SYS_MODE_SCAN;
>
> PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
>
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "hal init scan mode %d\n", msg_body.mode);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "hal finish scan mode %d\n",
> + msg_body.mode);
>
> ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
> if (ret) {
> - wcn36xx_err("Sending hal_init_scan failed\n");
> - goto out;
> + wcn36xx_err("Sending hal_finish_scan failed\n");
> + return ret;
> }
> ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
> if (ret) {
> - wcn36xx_err("hal_init_scan response failed err=%d\n", ret);
> - goto out;
> + wcn36xx_err("hal_finish_scan response failed err=%d\n", ret);
> + return ret;
> }
> -out:
> - mutex_unlock(&wcn->hal_mutex);
> - return ret;
> +
> + return 0;
> }
>
> -int wcn36xx_smd_start_scan(struct wcn36xx *wcn)
> +int _wcn36xx_smd_end_scan(struct wcn36xx *wcn)
> {
> - struct wcn36xx_hal_start_scan_req_msg msg_body;
> + struct wcn36xx_hal_end_scan_req_msg msg_body;
> int ret = 0;
>
> - mutex_lock(&wcn->hal_mutex);
> - INIT_HAL_MSG(msg_body, WCN36XX_HAL_START_SCAN_REQ);
> + INIT_HAL_MSG(msg_body, WCN36XX_HAL_END_SCAN_REQ);
>
> - msg_body.scan_channel = WCN36XX_HW_CHANNEL(wcn);
> + msg_body.scan_channel = wcn->scan_channel;
>
> PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
>
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "hal start scan channel %d\n",
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "hal end scan channel %d\n",
> msg_body.scan_channel);
>
> ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
> if (ret) {
> - wcn36xx_err("Sending hal_start_scan failed\n");
> - goto out;
> + wcn36xx_err("Sending hal_end_scan failed\n");
> + return ret;
> }
> ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
> if (ret) {
> - wcn36xx_err("hal_start_scan response failed err=%d\n", ret);
> - goto out;
> + wcn36xx_err("hal_end_scan response failed err=%d\n", ret);
> + return ret;
> }
> -out:
> - mutex_unlock(&wcn->hal_mutex);
> - return ret;
> +
> + wcn->scan_started = false;
> +
> + return 0;
> }
>
> -int wcn36xx_smd_end_scan(struct wcn36xx *wcn)
> +void wcn36xx_scan_work(struct work_struct *work)
> {
> - struct wcn36xx_hal_end_scan_req_msg msg_body;
> + struct wcn36xx *wcn = container_of(work, struct wcn36xx, scan_work.work);
> + struct wcn36xx_hal_start_scan_req_msg msg_body;
> int ret = 0;
>
> mutex_lock(&wcn->hal_mutex);
> - INIT_HAL_MSG(msg_body, WCN36XX_HAL_END_SCAN_REQ);
>
> - msg_body.scan_channel = WCN36XX_HW_CHANNEL(wcn);
> + if (wcn->scan_started) {
> + ret = _wcn36xx_smd_end_scan(wcn);
> + if (ret)
> + goto out;
> +
> + /* we just do only the one start -> stop then finish scan */
> + wcn->scan_finishing = true;
> + }
> + if (wcn->scan_finishing) {
> + ret = _wcn36xx_smd_finish_scan(wcn);
> + goto out;
> + }
> +
> + INIT_HAL_MSG(msg_body, WCN36XX_HAL_START_SCAN_REQ);
> +
> + wcn->scan_channel = WCN36XX_HW_CHANNEL(wcn);
> + msg_body.scan_channel = wcn->scan_channel;
> + dev_dbg(wcn->dev, "%s: ch %d\n", __func__, wcn->scan_channel);
>
> PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
>
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "hal end scan channel %d\n",
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "hal start scan channel %d\n",
> msg_body.scan_channel);
>
> ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
> if (ret) {
> - wcn36xx_err("Sending hal_end_scan failed\n");
> + wcn36xx_err("Sending hal_start_scan failed\n");
> goto out;
> }
> ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
> if (ret) {
> - wcn36xx_err("hal_end_scan response failed err=%d\n", ret);
> + wcn36xx_err("hal_start_scan response failed err=%d\n", ret);
> goto out;
> }
> +
> + wcn->scan_started = true;
> + /* we will come back in around 166ms and stop the scan */
> + schedule_delayed_work(&wcn->scan_work, HZ / 6);
> +
> out:
> mutex_unlock(&wcn->hal_mutex);
> - return ret;
> }
>
> -int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
> - enum wcn36xx_hal_sys_mode mode)
> +int wcn36xx_smd_init_scan(struct wcn36xx *wcn)
> {
> - struct wcn36xx_hal_finish_scan_req_msg msg_body;
> + struct wcn36xx_hal_init_scan_req_msg msg_body;
> int ret = 0;
>
> mutex_lock(&wcn->hal_mutex);
> - INIT_HAL_MSG(msg_body, WCN36XX_HAL_FINISH_SCAN_REQ);
>
> - msg_body.mode = mode;
> + wcn->scan_started = false;
> + wcn->scan_finishing = false;
> +
> + INIT_HAL_MSG(msg_body, WCN36XX_HAL_INIT_SCAN_REQ);
> +
> + msg_body.mode = HAL_SYS_MODE_SCAN;
>
> PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
>
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "hal finish scan mode %d\n",
> - msg_body.mode);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "hal init scan mode %d\n", msg_body.mode);
>
> ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
> if (ret) {
> - wcn36xx_err("Sending hal_finish_scan failed\n");
> + wcn36xx_err("Sending hal_init_scan failed\n");
> goto out;
> }
> ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
> if (ret) {
> - wcn36xx_err("hal_finish_scan response failed err=%d\n", ret);
> + wcn36xx_err("hal_init_scan response failed err=%d\n", ret);
> goto out;
> }
> +
> + schedule_delayed_work(&wcn->scan_work, 0);
> +
> out:
> mutex_unlock(&wcn->hal_mutex);
> return ret;
> }
>
> +int wcn36xx_smd_finish_scan(struct wcn36xx *wcn)
> +{
> + int ret = 0;
> +
> + mutex_lock(&wcn->hal_mutex);
> +
> + cancel_delayed_work(&wcn->scan_work);
> +
> + if (wcn->scan_started) {
> + wcn->scan_finishing = true;
> + ret = _wcn36xx_smd_end_scan(wcn);
> + }
> +
> + mutex_unlock(&wcn->hal_mutex);
> + return ret;
> +}
> +
> static int wcn36xx_smd_switch_channel_rsp(void *buf, size_t len)
> {
> struct wcn36xx_hal_switch_channel_rsp_msg *rsp;
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
> index 8361f9e..299d60e 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.h
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.h
> @@ -67,11 +67,10 @@ void wcn36xx_smd_close(struct wcn36xx *wcn);
> int wcn36xx_smd_load_nv(struct wcn36xx *wcn);
> int wcn36xx_smd_start(struct wcn36xx *wcn);
> int wcn36xx_smd_stop(struct wcn36xx *wcn);
> -int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode);
> +int wcn36xx_smd_init_scan(struct wcn36xx *wcn);
> int wcn36xx_smd_start_scan(struct wcn36xx *wcn);
> int wcn36xx_smd_end_scan(struct wcn36xx *wcn);
> -int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
> - enum wcn36xx_hal_sys_mode mode);
> +int wcn36xx_smd_finish_scan(struct wcn36xx *wcn);
> int wcn36xx_smd_update_scan_params(struct wcn36xx *wcn);
> int wcn36xx_smd_add_sta_self(struct wcn36xx *wcn, struct ieee80211_vif *vif);
> int wcn36xx_smd_delete_sta_self(struct wcn36xx *wcn, u8 *addr);
> diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
> index 04793c6..82d96bb 100644
> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
> @@ -228,6 +228,12 @@ struct wcn36xx {
>
> struct sk_buff *tx_ack_skb;
>
> + /* we restrict the amount of time scan can block normal operation */
> + struct delayed_work scan_work;
> + bool scan_started;
> + bool scan_finishing;
> + u8 scan_channel;
> +
> #ifdef CONFIG_WCN36XX_DEBUGFS
> /* Debug file system entry */
> struct wcn36xx_dfs_entry dfs;
> @@ -247,5 +253,6 @@ static inline bool wcn36xx_is_fw_version(struct wcn36xx *wcn,
> wcn->fw_revision == revision);
> }
> void wcn36xx_set_default_rates(struct wcn36xx_hal_supported_rates *rates);
> +void wcn36xx_scan_work(struct work_struct *work);
>
> #endif /* _WCN36XX_H_ */
>
--
Best regards,
Eugene
More information about the wcn36xx
mailing list