[PATCH] wcn36xx: Don't run scan_init multiple times

[PATCH] wcn36xx: Don't run scan_init multiple times

[Bryan O'Donoghue]

Run scan_init only once. There's no need to run this command multiple times
if it has already been run once.

The software scan algorithm can end up repeatedly calling scan_init on each
loop resulting in between four and eight milliseconds of lost time on each
callout.

Subtract the overhead now.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/net/wireless/ath/wcn36xx/smd.c     | 6 ++++++
 drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index acf533fae46a..ec082cf3ab09 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -706,6 +706,10 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode,
 	int ret;
 
 	mutex_lock(&wcn->hal_mutex);
+	if (wcn->scan_init) {
+		ret = 0;
+		goto out;
+	}
 	INIT_HAL_MSG(msg_body, WCN36XX_HAL_INIT_SCAN_REQ);
 
 	msg_body.mode = mode;
@@ -731,6 +735,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->scan_init = true;
 out:
 	mutex_unlock(&wcn->hal_mutex);
 	return ret;
@@ -761,6 +766,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->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 71fa9992b118..156df6d184c8 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
@@ -235,6 +235,7 @@ struct wcn36xx {
 	struct ieee80211_vif	*sw_scan_vif;
 	struct mutex		scan_lock;
 	bool			scan_aborted;
+	bool			scan_init;
 
 	/* DXE channels */
 	struct wcn36xx_dxe_ch	dxe_tx_l_ch;	/* TX low */
-- 
2.28.0

[PATCH] wcn36xx: Send NULL data packet when exiting BMPS

[Bryan O'Donoghue]

This commit updates the BMPS exit path to be consistent with downstream in
terms of exiting BMPS mode. Downstream sets the flag to send a NULL data
frame to the host on exiting BMPS.

This will tell the AP to send any queued frames to the STA immediately.
Verified the relevant bit toggle in wireshark.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/net/wireless/ath/wcn36xx/smd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 8ff1eda8f942..acf533fae46a 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2176,6 +2176,7 @@ int wcn36xx_smd_exit_bmps(struct wcn36xx *wcn, struct ieee80211_vif *vif)
 	INIT_HAL_MSG(msg_body, WCN36XX_HAL_EXIT_BMPS_REQ);
 
 	msg_body.bss_index = vif_priv->bss_index;
+	msg_body.send_data_null = 1;
 
 	PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
 
-- 
2.28.0

Re: [PATCH] wcn36xx: Send NULL data packet when exiting BMPS

[Loic Poulain]

On Fri, 20 Nov 2020 at 03:13, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> This commit updates the BMPS exit path to be consistent with downstream in
> terms of exiting BMPS mode. Downstream sets the flag to send a NULL data
> frame to the host on exiting BMPS.
>
> This will tell the AP to send any queued frames to the STA immediately.
> Verified the relevant bit toggle in wireshark.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/net/wireless/ath/wcn36xx/smd.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 8ff1eda8f942..acf533fae46a 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2176,6 +2176,7 @@ int wcn36xx_smd_exit_bmps(struct wcn36xx *wcn, struct ieee80211_vif *vif)
>         INIT_HAL_MSG(msg_body, WCN36XX_HAL_EXIT_BMPS_REQ);
>
>         msg_body.bss_index = vif_priv->bss_index;
> +       msg_body.send_data_null = 1;

I'm quite sure I've seen null data packet wakeup (PS=0) when sniffing
wcn3620, but maybe it was submitted by mac80211, have you then checked
you do not end with double null packets with that patch (one from
firmware and one from mac layer)?


>
>         PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
>
> --
> 2.28.0
>

Re: [PATCH] wcn36xx: Send NULL data packet when exiting BMPS

[Bryan O'Donoghue]

On 20/11/2020 08:15, Loic Poulain wrote:
> On Fri, 20 Nov 2020 at 03:13, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> wrote:
>>
>> This commit updates the BMPS exit path to be consistent with downstream in
>> terms of exiting BMPS mode. Downstream sets the flag to send a NULL data
>> frame to the host on exiting BMPS.
>>
>> This will tell the AP to send any queued frames to the STA immediately.
>> Verified the relevant bit toggle in wireshark.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   drivers/net/wireless/ath/wcn36xx/smd.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
>> index 8ff1eda8f942..acf533fae46a 100644
>> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
>> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
>> @@ -2176,6 +2176,7 @@ int wcn36xx_smd_exit_bmps(struct wcn36xx *wcn, struct ieee80211_vif *vif)
>>          INIT_HAL_MSG(msg_body, WCN36XX_HAL_EXIT_BMPS_REQ);
>>
>>          msg_body.bss_index = vif_priv->bss_index;
>> +       msg_body.send_data_null = 1;
> 
> I'm quite sure I've seen null data packet wakeup (PS=0) when sniffing
> wcn3620, but maybe it was submitted by mac80211, have you then checked
> you do not end with double null packets with that patch (one from
> firmware and one from mac layer)?

Just did.

Definitely not double nulling

Re: [PATCH] wcn36xx: Send NULL data packet when exiting BMPS

[Kalle Valo]

Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:

> This commit updates the BMPS exit path to be consistent with downstream in
> terms of exiting BMPS mode. Downstream sets the flag to send a NULL data
> frame to the host on exiting BMPS.
> 
> This will tell the AP to send any queued frames to the STA immediately.
> Verified the relevant bit toggle in wireshark.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

9bc3a55f4ae5 wcn36xx: Send NULL data packet when exiting BMPS

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20201120021403.2646574-2-bryan.odonoghue@linaro.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wcn36xx: Don't run scan_init multiple times

[Loic Poulain]

On Fri, 20 Nov 2020 at 03:13, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> Run scan_init only once. There's no need to run this command multiple times
> if it has already been run once.
>
> The software scan algorithm can end up repeatedly calling scan_init on each
> loop resulting in between four and eight milliseconds of lost time on each
> callout.
>
> Subtract the overhead now.

This command defines parameters like the BSSID we want to inform,
etc... So this can change depending on the scan is done while
connected or not. Moreover in the connected case, the scans are
interleaved with normal data listening period, and AFAIU, init/stop
scan allow to submit a null data packet with PS/non-PS bit when
mac80211 leaves the operating channel to scanning another one (so that
AP does no submit packet to it). So at first glance, this patch would
break that, right?



>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/net/wireless/ath/wcn36xx/smd.c     | 6 ++++++
>  drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 1 +
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index acf533fae46a..ec082cf3ab09 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -706,6 +706,10 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode,
>         int ret;
>
>         mutex_lock(&wcn->hal_mutex);
> +       if (wcn->scan_init) {
> +               ret = 0;
> +               goto out;
> +       }
>         INIT_HAL_MSG(msg_body, WCN36XX_HAL_INIT_SCAN_REQ);
>
>         msg_body.mode = mode;
> @@ -731,6 +735,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->scan_init = true;
>  out:
>         mutex_unlock(&wcn->hal_mutex);
>         return ret;
> @@ -761,6 +766,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->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 71fa9992b118..156df6d184c8 100644
> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
> @@ -235,6 +235,7 @@ struct wcn36xx {
>         struct ieee80211_vif    *sw_scan_vif;
>         struct mutex            scan_lock;
>         bool                    scan_aborted;
> +       bool                    scan_init;
>
>         /* DXE channels */
>         struct wcn36xx_dxe_ch   dxe_tx_l_ch;    /* TX low */
> --
> 2.28.0
>

Re: [PATCH] wcn36xx: Don't run scan_init multiple times

[Bryan O'Donoghue]

On 20/11/2020 08:12, Loic Poulain wrote:
> On Fri, 20 Nov 2020 at 03:13, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> wrote:
>>
>> Run scan_init only once. There's no need to run this command multiple times
>> if it has already been run once.
>>
>> The software scan algorithm can end up repeatedly calling scan_init on each
>> loop resulting in between four and eight milliseconds of lost time on each
>> callout.
>>
>> Subtract the overhead now.
> 
> This command defines parameters like the BSSID we want to inform,
> etc... So this can change depending on the scan is done while
> connected or not. 

So you're saying a scan is started and our connection state toggles from 
non-connected to connected.

Possible I guess.

> Moreover in the connected case, the scans are
> interleaved with normal data listening period, and AFAIU, init/stop
> scan allow to submit a null data packet with PS/non-PS bit when
> mac80211 leaves the operating channel to scanning another one (so that
> AP does no submit packet to it). So at first glance, this patch would
> break that, right?

I agree with that logic, and actually looking at downstream - we see 
that downstream doesn't set the notification byte before starting a scan 
on a new channel

connected:
[   63.475897] BOD WDI_SendMsg/23794 message = 0x04 version = 0x01 len 
0x00000030 // WLAN_HAL_INIT_SCAN_REQ
[   63.475902] SMD <<< 00000000: 04 00 01 00 30 00 00 00 02 00 00 00 00 
00 00 00 00 00 00 00 00 ff ff ff c8 9f 7a 21 c0 ff ff ff
[   63.475907] SMD <<< 00000020: d4 2e 7c 00 c0 ff ff ff 00 54 93 6d c0 
22 22 00
[   63.478242] SMD >>> 00000000: 05 00 00 00 0c 00 00 00 00 00 00 00

type=04 00 version=01 00 length=30 00 00 00
mode=02 00 00 00
bssid=00 000 00 00 00 00
notify=00
...

which I accept is actually a bug downstream, conceptually at any rate.

I need to ensure a scan isn't in process when we go into suspend but, 
there's no reason to skip the scan_init() command for that.

flagging != skipping

Let's forget this one.

---
bod