[PATCH v2 0/3] wcn36xx: software scanning improvements

[PATCH v2 0/3] wcn36xx: software scanning improvements

[Benjamin Li]

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

[PATCH v2 1/3] wcn36xx: add debug prints for sw_scan start/complete

[Benjamin Li]

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

[PATCH v2 2/3] wcn36xx: implement flush op to speed up connected scan

[Benjamin Li]

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

[PATCH v2 3/3] wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan

[Benjamin Li]

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

Re: [PATCH v2 3/3] wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan

[Bryan O'Donoghue]

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>

Re: [PATCH v2 3/3] wcn36xx: ensure pairing of init_scan/finish_scan and start_scan/end_scan

[Kalle Valo]

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

Re: [PATCH v2 1/3] wcn36xx: add debug prints for sw_scan start/complete

[Kalle Valo]

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