* [PATCH v2 0/3] wcn36xx: software scanning improvements
@ 2021-10-27 17:03 Benjamin Li
2021-10-27 17:03 ` [PATCH v2 1/3] wcn36xx: add debug prints for sw_scan start/complete Benjamin Li
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Benjamin Li @ 2021-10-27 17:03 UTC (permalink / raw)
To: Kalle Valo
Cc: Joseph Gates, Bryan O'Donoghue, Loic Poulain, linux-arm-msm,
Benjamin Li, David S. Miller, Jakub Kicinski, John W. Linville,
Eugene Krasnikov, wcn36xx, linux-wireless, netdev, linux-kernel
v2:
Fix compiler warning (int flags -> unsigned long flags in patch 2).
v1:
Less important now, given Loic's breakthrough with FW scan offload on
5GHz channels, but downstream does do software scanning so these fixes
for that path may still be valuable to someone.
Benjamin Li (3):
wcn36xx: add debug prints for sw_scan start/complete
wcn36xx: implement flush op to speed up connected scan
wcn36xx: ensure pairing of init_scan/finish_scan and
start_scan/end_scan
drivers/net/wireless/ath/wcn36xx/dxe.c | 47 +++++++++++++++++++++
drivers/net/wireless/ath/wcn36xx/dxe.h | 1 +
drivers/net/wireless/ath/wcn36xx/main.c | 49 ++++++++++++++++++----
drivers/net/wireless/ath/wcn36xx/smd.c | 4 ++
drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 +
5 files changed, 95 insertions(+), 7 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2 1/3] wcn36xx: add debug prints for sw_scan start/complete 2021-10-27 17:03 [PATCH v2 0/3] wcn36xx: software scanning improvements Benjamin Li @ 2021-10-27 17:03 ` Benjamin Li 2021-11-01 14:18 ` Kalle Valo 2021-10-27 17:03 ` [PATCH v2 2/3] wcn36xx: implement flush op to speed up connected scan Benjamin Li 2021-10-27 17:03 ` [PATCH v2 3/3] wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan Benjamin Li 2 siblings, 1 reply; 7+ messages in thread From: Benjamin Li @ 2021-10-27 17:03 UTC (permalink / raw) To: Kalle Valo Cc: Joseph Gates, Bryan O'Donoghue, Loic Poulain, linux-arm-msm, Benjamin Li, David S. Miller, Jakub Kicinski, John W. Linville, Eugene Krasnikov, wcn36xx, linux-wireless, netdev, linux-kernel Add some MAC debug prints for more easily demarcating a software scan when parsing logs. Signed-off-by: Benjamin Li <benl@squareup.com> --- drivers/net/wireless/ath/wcn36xx/main.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c index 42153305f6d84..0b0f8ce729dd1 100644 --- a/drivers/net/wireless/ath/wcn36xx/main.c +++ b/drivers/net/wireless/ath/wcn36xx/main.c @@ -704,6 +704,8 @@ static void wcn36xx_sw_scan_start(struct ieee80211_hw *hw, struct wcn36xx *wcn = hw->priv; struct wcn36xx_vif *vif_priv = wcn36xx_vif_to_priv(vif); + wcn36xx_dbg(WCN36XX_DBG_MAC, "sw_scan_start"); + wcn->sw_scan = true; wcn->sw_scan_vif = vif; wcn->sw_scan_channel = 0; @@ -718,6 +720,8 @@ static void wcn36xx_sw_scan_complete(struct ieee80211_hw *hw, { struct wcn36xx *wcn = hw->priv; + wcn36xx_dbg(WCN36XX_DBG_MAC, "sw_scan_complete"); + /* ensure that any scan session is finished */ wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, wcn->sw_scan_vif); wcn->sw_scan = false; -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/3] wcn36xx: add debug prints for sw_scan start/complete 2021-10-27 17:03 ` [PATCH v2 1/3] wcn36xx: add debug prints for sw_scan start/complete Benjamin Li @ 2021-11-01 14:18 ` Kalle Valo 0 siblings, 0 replies; 7+ messages in thread From: Kalle Valo @ 2021-11-01 14:18 UTC (permalink / raw) To: Benjamin Li Cc: Joseph Gates, Bryan O'Donoghue, Loic Poulain, linux-arm-msm, Benjamin Li, David S. Miller, Jakub Kicinski, John W. Linville, Eugene Krasnikov, wcn36xx, linux-wireless, netdev, linux-kernel Benjamin Li <benl@squareup.com> wrote: > Add some MAC debug prints for more easily demarcating a software scan > when parsing logs. > > Signed-off-by: Benjamin Li <benl@squareup.com> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> 3 patches applied to ath-next branch of ath.git, thanks. df008741dd62 wcn36xx: add debug prints for sw_scan start/complete f02e1cc2a846 wcn36xx: implement flush op to speed up connected scan 8f1ba8b0ee26 wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan -- https://patchwork.kernel.org/project/linux-wireless/patch/20211027170306.555535-2-benl@squareup.com/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] wcn36xx: implement flush op to speed up connected scan 2021-10-27 17:03 [PATCH v2 0/3] wcn36xx: software scanning improvements Benjamin Li 2021-10-27 17:03 ` [PATCH v2 1/3] wcn36xx: add debug prints for sw_scan start/complete Benjamin Li @ 2021-10-27 17:03 ` Benjamin Li 2021-10-27 17:03 ` [PATCH v2 3/3] wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan Benjamin Li 2 siblings, 0 replies; 7+ messages in thread From: Benjamin Li @ 2021-10-27 17:03 UTC (permalink / raw) To: Kalle Valo Cc: Joseph Gates, Bryan O'Donoghue, Loic Poulain, linux-arm-msm, Benjamin Li, David S. Miller, Jakub Kicinski, Eugene Krasnikov, John W. Linville, wcn36xx, linux-wireless, netdev, linux-kernel Without ieee80211_ops->flush implemented to empty HW queues, mac80211 will do a 100ms dead wait after stopping SW queues, before leaving the operating channel to resume a software connected scan[1]. (see ieee80211_scan_state_resume) This wait is correctly included in the calculation for whether or not we've exceeded max off-channel time, as it occurs after sending the null frame with PS bit set. Thus, with 125 ms max off-channel time we only have 25 ms of scan time, which technically isn't even enough to scan one channel (although mac80211 always scans at least one channel per off- channel window). Moreover, for passive probes we end up spending at least 100 ms + 111 ms (IEEE80211_PASSIVE_CHANNEL_TIME) "off-channel"[2], which exceeds the listen interval of 200 ms that we provide in our association request frame. That's technically out-of-spec. [1]: Until recently, wcn36xx performed software (rather than FW-offloaded) scanning when 5GHz channels are requested. This apparent limitation is now resolved -- see commit 1395f8a6a4d5 ("wcn36xx: Enable hardware scan offload for 5Ghz band"). [2]: in quotes because about 100 ms of it is still on-channel but with PS set Signed-off-by: Benjamin Li <benl@squareup.com> --- drivers/net/wireless/ath/wcn36xx/dxe.c | 47 +++++++++++++++++++++++++ drivers/net/wireless/ath/wcn36xx/dxe.h | 1 + drivers/net/wireless/ath/wcn36xx/main.c | 11 ++++++ 3 files changed, 59 insertions(+) diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c index aff04ef662663..fd627c9f3d409 100644 --- a/drivers/net/wireless/ath/wcn36xx/dxe.c +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c @@ -834,6 +834,53 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn, return ret; } +static bool _wcn36xx_dxe_tx_channel_is_empty(struct wcn36xx_dxe_ch *ch) +{ + unsigned long flags; + struct wcn36xx_dxe_ctl *ctl_bd_start, *ctl_skb_start; + struct wcn36xx_dxe_ctl *ctl_bd, *ctl_skb; + bool ret = true; + + spin_lock_irqsave(&ch->lock, flags); + + /* Loop through ring buffer looking for nonempty entries. */ + ctl_bd_start = ch->head_blk_ctl; + ctl_bd = ctl_bd_start; + ctl_skb_start = ctl_bd_start->next; + ctl_skb = ctl_skb_start; + do { + if (ctl_skb->skb) { + ret = false; + goto unlock; + } + ctl_bd = ctl_skb->next; + ctl_skb = ctl_bd->next; + } while (ctl_skb != ctl_skb_start); + +unlock: + spin_unlock_irqrestore(&ch->lock, flags); + return ret; +} + +int wcn36xx_dxe_tx_flush(struct wcn36xx *wcn) +{ + int i = 0; + + /* Called with mac80211 queues stopped. Wait for empty HW queues. */ + do { + if (_wcn36xx_dxe_tx_channel_is_empty(&wcn->dxe_tx_l_ch) && + _wcn36xx_dxe_tx_channel_is_empty(&wcn->dxe_tx_h_ch)) { + return 0; + } + /* This ieee80211_ops callback is specifically allowed to + * sleep. + */ + usleep_range(1000, 1100); + } while (++i < 100); + + return -EBUSY; +} + int wcn36xx_dxe_init(struct wcn36xx *wcn) { int reg_data = 0, ret; diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.h b/drivers/net/wireless/ath/wcn36xx/dxe.h index 31b81b7547a32..26a31edf52e99 100644 --- a/drivers/net/wireless/ath/wcn36xx/dxe.h +++ b/drivers/net/wireless/ath/wcn36xx/dxe.h @@ -466,5 +466,6 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn, struct wcn36xx_tx_bd *bd, struct sk_buff *skb, bool is_low); +int wcn36xx_dxe_tx_flush(struct wcn36xx *wcn); void wcn36xx_dxe_tx_ack_ind(struct wcn36xx *wcn, u32 status); #endif /* _DXE_H_ */ diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c index 0b0f8ce729dd1..18383d0fc0933 100644 --- a/drivers/net/wireless/ath/wcn36xx/main.c +++ b/drivers/net/wireless/ath/wcn36xx/main.c @@ -1278,6 +1278,16 @@ static void wcn36xx_ipv6_addr_change(struct ieee80211_hw *hw, } #endif +static void wcn36xx_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif, + u32 queues, bool drop) +{ + struct wcn36xx *wcn = hw->priv; + + if (wcn36xx_dxe_tx_flush(wcn)) { + wcn36xx_err("Failed to flush hardware tx queues\n"); + } +} + static const struct ieee80211_ops wcn36xx_ops = { .start = wcn36xx_start, .stop = wcn36xx_stop, @@ -1305,6 +1315,7 @@ static const struct ieee80211_ops wcn36xx_ops = { #if IS_ENABLED(CONFIG_IPV6) .ipv6_addr_change = wcn36xx_ipv6_addr_change, #endif + .flush = wcn36xx_flush, CFG80211_TESTMODE_CMD(wcn36xx_tm_cmd) }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan 2021-10-27 17:03 [PATCH v2 0/3] wcn36xx: software scanning improvements Benjamin Li 2021-10-27 17:03 ` [PATCH v2 1/3] wcn36xx: add debug prints for sw_scan start/complete Benjamin Li 2021-10-27 17:03 ` [PATCH v2 2/3] wcn36xx: implement flush op to speed up connected scan Benjamin Li @ 2021-10-27 17:03 ` Benjamin Li 2021-10-27 22:30 ` Bryan O'Donoghue 2 siblings, 1 reply; 7+ messages in thread From: Benjamin Li @ 2021-10-27 17:03 UTC (permalink / raw) To: Kalle Valo Cc: Joseph Gates, Bryan O'Donoghue, Loic Poulain, linux-arm-msm, Benjamin Li, David S. Miller, Jakub Kicinski, Eugene Krasnikov, John W. Linville, wcn36xx, linux-wireless, netdev, linux-kernel An SMD capture from the downstream prima driver on WCN3680B shows the following command sequence for connected scans: - init_scan_req - start_scan_req, channel 1 - end_scan_req, channel 1 - start_scan_req, channel 2 - ... - end_scan_req, channel 3 - finish_scan_req - init_scan_req - start_scan_req, channel 4 - ... - end_scan_req, channel 6 - finish_scan_req - ... - end_scan_req, channel 165 - finish_scan_req Upstream currently never calls wcn36xx_smd_end_scan, and in some cases[1] still sends finish_scan_req twice in a row or before init_scan_req. A typical connected scan looks like this: - init_scan_req - start_scan_req, channel 1 - finish_scan_req - init_scan_req - start_scan_req, channel 2 - ... - start_scan_req, channel 165 - finish_scan_req - finish_scan_req This patch cleans up scanning so that init/finish and start/end are always paired together and correctly nested. - init_scan_req - start_scan_req, channel 1 - end_scan_req, channel 1 - finish_scan_req - init_scan_req - start_scan_req, channel 2 - end_scan_req, channel 2 - ... - start_scan_req, channel 165 - end_scan_req, channel 165 - finish_scan_req Note that upstream will not do batching of 3 active-probe scans before returning to the operating channel, and this patch does not change that. To match downstream in this aspect, adjust IEEE80211_PROBE_DELAY and/or the 125ms max off-channel time in ieee80211_scan_state_decision. [1]: commit d195d7aac09b ("wcn36xx: Ensure finish scan is not requested before start scan") addressed one case of finish_scan_req being sent without a preceding init_scan_req (the case of the operating channel coinciding with the first scan channel); two other cases are: 1) if SW scan is started and aborted immediately, without scanning any channels, we send a finish_scan_req without ever sending init_scan_req, and 2) as SW scan logic always returns us to the operating channel before calling wcn36xx_sw_scan_complete, finish_scan_req is always sent twice at the end of a SW scan Fixes: 8e84c2582169 ("wcn36xx: mac80211 driver for Qualcomm WCN3660/WCN3680 hardware") Signed-off-by: Benjamin Li <benl@squareup.com> --- drivers/net/wireless/ath/wcn36xx/main.c | 34 +++++++++++++++++----- drivers/net/wireless/ath/wcn36xx/smd.c | 4 +++ drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 + 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c index 18383d0fc0933..37b4016f020c9 100644 --- a/drivers/net/wireless/ath/wcn36xx/main.c +++ b/drivers/net/wireless/ath/wcn36xx/main.c @@ -400,6 +400,7 @@ static void wcn36xx_change_opchannel(struct wcn36xx *wcn, int ch) static int wcn36xx_config(struct ieee80211_hw *hw, u32 changed) { struct wcn36xx *wcn = hw->priv; + int ret; wcn36xx_dbg(WCN36XX_DBG_MAC, "mac config changed 0x%08x\n", changed); @@ -415,17 +416,31 @@ static int wcn36xx_config(struct ieee80211_hw *hw, u32 changed) * want to receive/transmit regular data packets, then * simply stop the scan session and exit PS mode. */ - wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, - wcn->sw_scan_vif); - wcn->sw_scan_channel = 0; + if (wcn->sw_scan_channel) + wcn36xx_smd_end_scan(wcn, wcn->sw_scan_channel); + if (wcn->sw_scan_init) { + wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, + wcn->sw_scan_vif); + } } else if (wcn->sw_scan) { /* A scan is ongoing, do not change the operating * channel, but start a scan session on the channel. */ - wcn36xx_smd_init_scan(wcn, HAL_SYS_MODE_SCAN, - wcn->sw_scan_vif); + if (wcn->sw_scan_channel) + wcn36xx_smd_end_scan(wcn, wcn->sw_scan_channel); + if (!wcn->sw_scan_init) { + /* This can fail if we are unable to notify the + * operating channel. + */ + ret = wcn36xx_smd_init_scan(wcn, + HAL_SYS_MODE_SCAN, + wcn->sw_scan_vif); + if (ret) { + mutex_unlock(&wcn->conf_mutex); + return -EIO; + } + } wcn36xx_smd_start_scan(wcn, ch); - wcn->sw_scan_channel = ch; } else { wcn36xx_change_opchannel(wcn, ch); } @@ -723,7 +738,12 @@ static void wcn36xx_sw_scan_complete(struct ieee80211_hw *hw, wcn36xx_dbg(WCN36XX_DBG_MAC, "sw_scan_complete"); /* ensure that any scan session is finished */ - wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, wcn->sw_scan_vif); + if (wcn->sw_scan_channel) + wcn36xx_smd_end_scan(wcn, wcn->sw_scan_channel); + if (wcn->sw_scan_init) { + wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, + wcn->sw_scan_vif); + } wcn->sw_scan = false; wcn->sw_scan_opchannel = 0; } diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c index 3cecc8f9c9647..830341be72673 100644 --- a/drivers/net/wireless/ath/wcn36xx/smd.c +++ b/drivers/net/wireless/ath/wcn36xx/smd.c @@ -721,6 +721,7 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode, wcn36xx_err("hal_init_scan response failed err=%d\n", ret); goto out; } + wcn->sw_scan_init = true; out: mutex_unlock(&wcn->hal_mutex); return ret; @@ -751,6 +752,7 @@ int wcn36xx_smd_start_scan(struct wcn36xx *wcn, u8 scan_channel) wcn36xx_err("hal_start_scan response failed err=%d\n", ret); goto out; } + wcn->sw_scan_channel = scan_channel; out: mutex_unlock(&wcn->hal_mutex); return ret; @@ -781,6 +783,7 @@ int wcn36xx_smd_end_scan(struct wcn36xx *wcn, u8 scan_channel) wcn36xx_err("hal_end_scan response failed err=%d\n", ret); goto out; } + wcn->sw_scan_channel = 0; out: mutex_unlock(&wcn->hal_mutex); return ret; @@ -822,6 +825,7 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn, wcn36xx_err("hal_finish_scan response failed err=%d\n", ret); goto out; } + wcn->sw_scan_init = false; out: mutex_unlock(&wcn->hal_mutex); return ret; diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h index 1c8d918137da2..fbd0558c2c196 100644 --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h @@ -248,6 +248,7 @@ struct wcn36xx { struct cfg80211_scan_request *scan_req; bool sw_scan; u8 sw_scan_opchannel; + bool sw_scan_init; u8 sw_scan_channel; struct ieee80211_vif *sw_scan_vif; struct mutex scan_lock; -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan 2021-10-27 17:03 ` [PATCH v2 3/3] wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan Benjamin Li @ 2021-10-27 22:30 ` Bryan O'Donoghue 2021-10-28 7:28 ` Kalle Valo 0 siblings, 1 reply; 7+ messages in thread From: Bryan O'Donoghue @ 2021-10-27 22:30 UTC (permalink / raw) To: Benjamin Li, Kalle Valo Cc: Joseph Gates, Loic Poulain, linux-arm-msm, David S. Miller, Jakub Kicinski, Eugene Krasnikov, John W. Linville, wcn36xx, linux-wireless, netdev, linux-kernel On 27/10/2021 18:03, Benjamin Li wrote: > An SMD capture from the downstream prima driver on WCN3680B shows the > following command sequence for connected scans: > > - init_scan_req > - start_scan_req, channel 1 > - end_scan_req, channel 1 > - start_scan_req, channel 2 > - ... > - end_scan_req, channel 3 > - finish_scan_req > - init_scan_req > - start_scan_req, channel 4 > - ... > - end_scan_req, channel 6 > - finish_scan_req > - ... > - end_scan_req, channel 165 > - finish_scan_req > > Upstream currently never calls wcn36xx_smd_end_scan, and in some cases[1] > still sends finish_scan_req twice in a row or before init_scan_req. A > typical connected scan looks like this: > > - init_scan_req > - start_scan_req, channel 1 > - finish_scan_req > - init_scan_req > - start_scan_req, channel 2 > - ... > - start_scan_req, channel 165 > - finish_scan_req > - finish_scan_req > > This patch cleans up scanning so that init/finish and start/end are always > paired together and correctly nested. > > - init_scan_req > - start_scan_req, channel 1 > - end_scan_req, channel 1 > - finish_scan_req > - init_scan_req > - start_scan_req, channel 2 > - end_scan_req, channel 2 > - ... > - start_scan_req, channel 165 > - end_scan_req, channel 165 > - finish_scan_req > > Note that upstream will not do batching of 3 active-probe scans before > returning to the operating channel, and this patch does not change that. > To match downstream in this aspect, adjust IEEE80211_PROBE_DELAY and/or > the 125ms max off-channel time in ieee80211_scan_state_decision. > > [1]: commit d195d7aac09b ("wcn36xx: Ensure finish scan is not requested > before start scan") addressed one case of finish_scan_req being sent > without a preceding init_scan_req (the case of the operating channel > coinciding with the first scan channel); two other cases are: > 1) if SW scan is started and aborted immediately, without scanning any > channels, we send a finish_scan_req without ever sending init_scan_req, > and > 2) as SW scan logic always returns us to the operating channel before > calling wcn36xx_sw_scan_complete, finish_scan_req is always sent twice > at the end of a SW scan > > Fixes: 8e84c2582169 ("wcn36xx: mac80211 driver for Qualcomm WCN3660/WCN3680 hardware") > Signed-off-by: Benjamin Li <benl@squareup.com> > --- > drivers/net/wireless/ath/wcn36xx/main.c | 34 +++++++++++++++++----- > drivers/net/wireless/ath/wcn36xx/smd.c | 4 +++ > drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 + > 3 files changed, 32 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c > index 18383d0fc0933..37b4016f020c9 100644 > --- a/drivers/net/wireless/ath/wcn36xx/main.c > +++ b/drivers/net/wireless/ath/wcn36xx/main.c > @@ -400,6 +400,7 @@ static void wcn36xx_change_opchannel(struct wcn36xx *wcn, int ch) > static int wcn36xx_config(struct ieee80211_hw *hw, u32 changed) > { > struct wcn36xx *wcn = hw->priv; > + int ret; > > wcn36xx_dbg(WCN36XX_DBG_MAC, "mac config changed 0x%08x\n", changed); > > @@ -415,17 +416,31 @@ static int wcn36xx_config(struct ieee80211_hw *hw, u32 changed) > * want to receive/transmit regular data packets, then > * simply stop the scan session and exit PS mode. > */ > - wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, > - wcn->sw_scan_vif); > - wcn->sw_scan_channel = 0; > + if (wcn->sw_scan_channel) > + wcn36xx_smd_end_scan(wcn, wcn->sw_scan_channel); > + if (wcn->sw_scan_init) { > + wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, > + wcn->sw_scan_vif); > + } > } else if (wcn->sw_scan) { > /* A scan is ongoing, do not change the operating > * channel, but start a scan session on the channel. > */ > - wcn36xx_smd_init_scan(wcn, HAL_SYS_MODE_SCAN, > - wcn->sw_scan_vif); > + if (wcn->sw_scan_channel) > + wcn36xx_smd_end_scan(wcn, wcn->sw_scan_channel); > + if (!wcn->sw_scan_init) { > + /* This can fail if we are unable to notify the > + * operating channel. > + */ > + ret = wcn36xx_smd_init_scan(wcn, > + HAL_SYS_MODE_SCAN, > + wcn->sw_scan_vif); > + if (ret) { > + mutex_unlock(&wcn->conf_mutex); > + return -EIO; > + } > + } > wcn36xx_smd_start_scan(wcn, ch); > - wcn->sw_scan_channel = ch; > } else { > wcn36xx_change_opchannel(wcn, ch); > } > @@ -723,7 +738,12 @@ static void wcn36xx_sw_scan_complete(struct ieee80211_hw *hw, > wcn36xx_dbg(WCN36XX_DBG_MAC, "sw_scan_complete"); > > /* ensure that any scan session is finished */ > - wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, wcn->sw_scan_vif); > + if (wcn->sw_scan_channel) > + wcn36xx_smd_end_scan(wcn, wcn->sw_scan_channel); > + if (wcn->sw_scan_init) { > + wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, > + wcn->sw_scan_vif); > + } > wcn->sw_scan = false; > wcn->sw_scan_opchannel = 0; > } > diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c > index 3cecc8f9c9647..830341be72673 100644 > --- a/drivers/net/wireless/ath/wcn36xx/smd.c > +++ b/drivers/net/wireless/ath/wcn36xx/smd.c > @@ -721,6 +721,7 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode, > wcn36xx_err("hal_init_scan response failed err=%d\n", ret); > goto out; > } > + wcn->sw_scan_init = true; > out: > mutex_unlock(&wcn->hal_mutex); > return ret; > @@ -751,6 +752,7 @@ int wcn36xx_smd_start_scan(struct wcn36xx *wcn, u8 scan_channel) > wcn36xx_err("hal_start_scan response failed err=%d\n", ret); > goto out; > } > + wcn->sw_scan_channel = scan_channel; > out: > mutex_unlock(&wcn->hal_mutex); > return ret; > @@ -781,6 +783,7 @@ int wcn36xx_smd_end_scan(struct wcn36xx *wcn, u8 scan_channel) > wcn36xx_err("hal_end_scan response failed err=%d\n", ret); > goto out; > } > + wcn->sw_scan_channel = 0; > out: > mutex_unlock(&wcn->hal_mutex); > return ret; > @@ -822,6 +825,7 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn, > wcn36xx_err("hal_finish_scan response failed err=%d\n", ret); > goto out; > } > + wcn->sw_scan_init = false; > out: > mutex_unlock(&wcn->hal_mutex); > return ret; > diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > index 1c8d918137da2..fbd0558c2c196 100644 > --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > @@ -248,6 +248,7 @@ struct wcn36xx { > struct cfg80211_scan_request *scan_req; > bool sw_scan; > u8 sw_scan_opchannel; > + bool sw_scan_init; > u8 sw_scan_channel; > struct ieee80211_vif *sw_scan_vif; > struct mutex scan_lock; > LGTM Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan 2021-10-27 22:30 ` Bryan O'Donoghue @ 2021-10-28 7:28 ` Kalle Valo 0 siblings, 0 replies; 7+ messages in thread From: Kalle Valo @ 2021-10-28 7:28 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Benjamin Li, Joseph Gates, Loic Poulain, linux-arm-msm, David S. Miller, Jakub Kicinski, Eugene Krasnikov, John W. Linville, wcn36xx, linux-wireless, netdev, linux-kernel Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > On 27/10/2021 18:03, Benjamin Li wrote: >> An SMD capture from the downstream prima driver on WCN3680B shows the >> following command sequence for connected scans: >> >> - init_scan_req >> - start_scan_req, channel 1 >> - end_scan_req, channel 1 >> - start_scan_req, channel 2 >> - ... >> - end_scan_req, channel 3 >> - finish_scan_req >> - init_scan_req >> - start_scan_req, channel 4 >> - ... >> - end_scan_req, channel 6 >> - finish_scan_req >> - ... >> - end_scan_req, channel 165 >> - finish_scan_req >> >> Upstream currently never calls wcn36xx_smd_end_scan, and in some cases[1] >> still sends finish_scan_req twice in a row or before init_scan_req. A >> typical connected scan looks like this: >> >> - init_scan_req >> - start_scan_req, channel 1 >> - finish_scan_req >> - init_scan_req >> - start_scan_req, channel 2 >> - ... >> - start_scan_req, channel 165 >> - finish_scan_req >> - finish_scan_req >> >> This patch cleans up scanning so that init/finish and start/end are always >> paired together and correctly nested. >> >> - init_scan_req >> - start_scan_req, channel 1 >> - end_scan_req, channel 1 >> - finish_scan_req >> - init_scan_req >> - start_scan_req, channel 2 >> - end_scan_req, channel 2 >> - ... >> - start_scan_req, channel 165 >> - end_scan_req, channel 165 >> - finish_scan_req >> >> Note that upstream will not do batching of 3 active-probe scans before >> returning to the operating channel, and this patch does not change that. >> To match downstream in this aspect, adjust IEEE80211_PROBE_DELAY and/or >> the 125ms max off-channel time in ieee80211_scan_state_decision. >> >> [1]: commit d195d7aac09b ("wcn36xx: Ensure finish scan is not requested >> before start scan") addressed one case of finish_scan_req being sent >> without a preceding init_scan_req (the case of the operating channel >> coinciding with the first scan channel); two other cases are: >> 1) if SW scan is started and aborted immediately, without scanning any >> channels, we send a finish_scan_req without ever sending init_scan_req, >> and >> 2) as SW scan logic always returns us to the operating channel before >> calling wcn36xx_sw_scan_complete, finish_scan_req is always sent twice >> at the end of a SW scan >> >> Fixes: 8e84c2582169 ("wcn36xx: mac80211 driver for Qualcomm WCN3660/WCN3680 hardware") >> Signed-off-by: Benjamin Li <benl@squareup.com> >> --- >> drivers/net/wireless/ath/wcn36xx/main.c | 34 +++++++++++++++++----- >> drivers/net/wireless/ath/wcn36xx/smd.c | 4 +++ >> drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 + >> 3 files changed, 32 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c >> index 18383d0fc0933..37b4016f020c9 100644 >> --- a/drivers/net/wireless/ath/wcn36xx/main.c >> +++ b/drivers/net/wireless/ath/wcn36xx/main.c >> @@ -400,6 +400,7 @@ static void wcn36xx_change_opchannel(struct wcn36xx *wcn, int ch) >> static int wcn36xx_config(struct ieee80211_hw *hw, u32 changed) >> { >> struct wcn36xx *wcn = hw->priv; >> + int ret; >> wcn36xx_dbg(WCN36XX_DBG_MAC, "mac config changed 0x%08x\n", >> changed); >> @@ -415,17 +416,31 @@ static int wcn36xx_config(struct >> ieee80211_hw *hw, u32 changed) >> * want to receive/transmit regular data packets, then >> * simply stop the scan session and exit PS mode. >> */ >> - wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, >> - wcn->sw_scan_vif); >> - wcn->sw_scan_channel = 0; >> + if (wcn->sw_scan_channel) >> + wcn36xx_smd_end_scan(wcn, wcn->sw_scan_channel); >> + if (wcn->sw_scan_init) { >> + wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, >> + wcn->sw_scan_vif); >> + } >> } else if (wcn->sw_scan) { >> /* A scan is ongoing, do not change the operating >> * channel, but start a scan session on the channel. >> */ >> - wcn36xx_smd_init_scan(wcn, HAL_SYS_MODE_SCAN, >> - wcn->sw_scan_vif); >> + if (wcn->sw_scan_channel) >> + wcn36xx_smd_end_scan(wcn, wcn->sw_scan_channel); >> + if (!wcn->sw_scan_init) { >> + /* This can fail if we are unable to notify the >> + * operating channel. >> + */ >> + ret = wcn36xx_smd_init_scan(wcn, >> + HAL_SYS_MODE_SCAN, >> + wcn->sw_scan_vif); >> + if (ret) { >> + mutex_unlock(&wcn->conf_mutex); >> + return -EIO; >> + } >> + } >> wcn36xx_smd_start_scan(wcn, ch); >> - wcn->sw_scan_channel = ch; >> } else { >> wcn36xx_change_opchannel(wcn, ch); >> } >> @@ -723,7 +738,12 @@ static void wcn36xx_sw_scan_complete(struct ieee80211_hw *hw, >> wcn36xx_dbg(WCN36XX_DBG_MAC, "sw_scan_complete"); >> /* ensure that any scan session is finished */ >> - wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, wcn->sw_scan_vif); >> + if (wcn->sw_scan_channel) >> + wcn36xx_smd_end_scan(wcn, wcn->sw_scan_channel); >> + if (wcn->sw_scan_init) { >> + wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, >> + wcn->sw_scan_vif); >> + } >> wcn->sw_scan = false; >> wcn->sw_scan_opchannel = 0; >> } >> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c >> index 3cecc8f9c9647..830341be72673 100644 >> --- a/drivers/net/wireless/ath/wcn36xx/smd.c >> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c >> @@ -721,6 +721,7 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode, >> wcn36xx_err("hal_init_scan response failed err=%d\n", ret); >> goto out; >> } >> + wcn->sw_scan_init = true; >> out: >> mutex_unlock(&wcn->hal_mutex); >> return ret; >> @@ -751,6 +752,7 @@ int wcn36xx_smd_start_scan(struct wcn36xx *wcn, u8 scan_channel) >> wcn36xx_err("hal_start_scan response failed err=%d\n", ret); >> goto out; >> } >> + wcn->sw_scan_channel = scan_channel; >> out: >> mutex_unlock(&wcn->hal_mutex); >> return ret; >> @@ -781,6 +783,7 @@ int wcn36xx_smd_end_scan(struct wcn36xx *wcn, u8 scan_channel) >> wcn36xx_err("hal_end_scan response failed err=%d\n", ret); >> goto out; >> } >> + wcn->sw_scan_channel = 0; >> out: >> mutex_unlock(&wcn->hal_mutex); >> return ret; >> @@ -822,6 +825,7 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn, >> wcn36xx_err("hal_finish_scan response failed err=%d\n", ret); >> goto out; >> } >> + wcn->sw_scan_init = false; >> out: >> mutex_unlock(&wcn->hal_mutex); >> return ret; >> diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h >> index 1c8d918137da2..fbd0558c2c196 100644 >> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h >> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h >> @@ -248,6 +248,7 @@ struct wcn36xx { >> struct cfg80211_scan_request *scan_req; >> bool sw_scan; >> u8 sw_scan_opchannel; >> + bool sw_scan_init; >> u8 sw_scan_channel; >> struct ieee80211_vif *sw_scan_vif; >> struct mutex scan_lock; >> > > LGTM > > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Thanks, all review and testing is very much appreciated. But please trim your replies, including the whole patch makes reading your replies and using patchwork much harder: https://patchwork.kernel.org/project/linux-wireless/patch/20211027170306.555535-4-benl@squareup.com/ I recommend just including the commit log and dropping the rest. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-01 14:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-27 17:03 [PATCH v2 0/3] wcn36xx: software scanning improvements Benjamin Li 2021-10-27 17:03 ` [PATCH v2 1/3] wcn36xx: add debug prints for sw_scan start/complete Benjamin Li 2021-11-01 14:18 ` Kalle Valo 2021-10-27 17:03 ` [PATCH v2 2/3] wcn36xx: implement flush op to speed up connected scan Benjamin Li 2021-10-27 17:03 ` [PATCH v2 3/3] wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan Benjamin Li 2021-10-27 22:30 ` Bryan O'Donoghue 2021-10-28 7:28 ` Kalle Valo
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).