LinuxLists.cc - [PATCH] wcn36xx: Fix software-driven scan

2020-08-24 02:36:14

by Bryan O'Donoghue

[permalink] [raw]

Subject: Re: [PATCH] wcn36xx: Fix software-driven scan

On 17/08/2020 17:01, Loic Poulain wrote:
> For software-driven scan, rely on mac80211 software scan instead
> of internal driver implementation. The internal implementation
> cause connection trouble since it keep the antenna busy during
> the entire scan duration, moreover it's only a passive scanning
> (no probe request). Therefore, let mac80211 manages sw scan.
>
> Note: we fallback to software scan if firmware does not report
> scan offload support or if we need to scan the 5Ghz band (currently
> not supported by the offload scan...).
>
> Signed-off-by: Loic Poulain <[email protected]>
> Signed-off-by: Bryan O'Donoghue <[email protected]>

OK.

TL;DR included diff fixes the performance issue.

I've poked about with the old scan, and this new scan and I've managed
to find to find where the new scan is killing performance.

Please see below.

@@ -792,6 +792,7 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
{
struct wcn36xx_vif *vif_priv = wcn36xx_vif_to_priv(vif);
struct wcn36xx_hal_finish_scan_req_msg msg_body;
+ u8 oper_channel;
int ret;

mutex_lock(&wcn->hal_mutex);
@@ -802,7 +803,9 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
/* Notify BSSID with null data packet */
msg_body.notify = 1;
msg_body.frame_type = 2;
- msg_body.oper_channel = channel;
+ oper_channel = WCN36XX_HW_CHANNEL(wcn);
+ if (oper_channel != channel)
+ msg_body.oper_channel = channel;
msg_body.scan_entry.bss_index[0] = vif_priv->bss_index;
msg_body.scan_entry.active_bss_count = 1;
}

Happily, I'm finding with this modification on top of your patch

1. Running a continuous Android scan
As in when you have the network scan window open
A simultaneous "iperf -c xxx.xxx.xxx.xxx -i 1 -t 1800"
2. Performance is as expected
I see throughput oscillate between 30 mbit and 100 mbit
- Android runs a scan every 15 seconds
- Software scan takes 11 seconds to complete
- So this is expected behavior
3. During quiescent periods we have the expected throughput
4. We gain from the additional stability the rest of your patch provides

Specifically on #4 what I find with the old scan is that with some APs
we can get a disassocation after a scan completes.

My test-case AP for that behavior is a Ubiquiti Nano HD on a 5ghz frequency.

Its funny that rewriting the operating channel in the finish_scan path
should make a difference.

All I can guess at here is that the PHY mode doesn't get updated correctly

Loic, can you make sure you are happy with the change and V2 this patch?

You can retain my SoB.

---
bod

2020-08-24 07:47:13

by Loic Poulain

[permalink] [raw]

Subject: Re: [PATCH] wcn36xx: Fix software-driven scan

Hi Bryan,

On Mon, 24 Aug 2020 at 03:45, Bryan O'Donoghue
<[email protected]> wrote:
>
> On 17/08/2020 17:01, Loic Poulain wrote:
> > For software-driven scan, rely on mac80211 software scan instead
> > of internal driver implementation. The internal implementation
> > cause connection trouble since it keep the antenna busy during
> > the entire scan duration, moreover it's only a passive scanning
> > (no probe request). Therefore, let mac80211 manages sw scan.
> >
> > Note: we fallback to software scan if firmware does not report
> > scan offload support or if we need to scan the 5Ghz band (currently
> > not supported by the offload scan...).
> >
> > Signed-off-by: Loic Poulain <[email protected]>
> > Signed-off-by: Bryan O'Donoghue <[email protected]>
>
> OK.
>
> TL;DR included diff fixes the performance issue.
>
> I've poked about with the old scan, and this new scan and I've managed
> to find to find where the new scan is killing performance.
>
> Please see below.
>
> @@ -792,6 +792,7 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
> {
> struct wcn36xx_vif *vif_priv = wcn36xx_vif_to_priv(vif);
> struct wcn36xx_hal_finish_scan_req_msg msg_body;
> + u8 oper_channel;
> int ret;
>
> mutex_lock(&wcn->hal_mutex);
> @@ -802,7 +803,9 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
> /* Notify BSSID with null data packet */
> msg_body.notify = 1;
> msg_body.frame_type = 2;
> - msg_body.oper_channel = channel;
> + oper_channel = WCN36XX_HW_CHANNEL(wcn);
> + if (oper_channel != channel)
> + msg_body.oper_channel = channel;

Is this condition ever satisfied? I mean, finish_scan is normally always
called with operating-channel as channel parameter.

> msg_body.scan_entry.bss_index[0] = vif_priv->bss_index;
> msg_body.scan_entry.active_bss_count = 1;
> }
>
> Happily, I'm finding with this modification on top of your patch
>
> 1. Running a continuous Android scan
> As in when you have the network scan window open
> A simultaneous "iperf -c xxx.xxx.xxx.xxx -i 1 -t 1800"
> 2. Performance is as expected
> I see throughput oscillate between 30 mbit and 100 mbit
> - Android runs a scan every 15 seconds

With scanning app in the foreground, right?
While connected to an AP, Android 'should' only background scan if
RSSI becomes too low (for roaming).

> - Software scan takes 11 seconds to complete
> - So this is expected behavior
> 3. During quiescent periods we have the expected throughput
> 4. We gain from the additional stability the rest of your patch provides
>
> Specifically on #4 what I find with the old scan is that with some APs
> we can get a disassocation after a scan completes.
>
> My test-case AP for that behavior is a Ubiquiti Nano HD on a 5ghz frequency.
>
> Its funny that rewriting the operating channel in the finish_scan path
> should make a difference.
>
> All I can guess at here is that the PHY mode doesn't get updated correctly
>
> Loic, can you make sure you are happy with the change and V2 this patch?
>
> You can retain my SoB.
>
> ---
> bod

2020-08-24 09:57:17

by Bryan O'Donoghue

[permalink] [raw]

Subject: Re: [PATCH] wcn36xx: Fix software-driven scan

On 24/08/2020 08:51, Loic Poulain wrote:
> Hi Bryan,
>
> On Mon, 24 Aug 2020 at 03:45, Bryan O'Donoghue
> <[email protected]> wrote:
>>
>> On 17/08/2020 17:01, Loic Poulain wrote:
>>> For software-driven scan, rely on mac80211 software scan instead
>>> of internal driver implementation. The internal implementation
>>> cause connection trouble since it keep the antenna busy during
>>> the entire scan duration, moreover it's only a passive scanning
>>> (no probe request). Therefore, let mac80211 manages sw scan.
>>>
>>> Note: we fallback to software scan if firmware does not report
>>> scan offload support or if we need to scan the 5Ghz band (currently
>>> not supported by the offload scan...).
>>>
>>> Signed-off-by: Loic Poulain <[email protected]>
>>> Signed-off-by: Bryan O'Donoghue <[email protected]>
>>
>> OK.
>>
>> TL;DR included diff fixes the performance issue.
>>
>> I've poked about with the old scan, and this new scan and I've managed
>> to find to find where the new scan is killing performance.
>>
>> Please see below.
>>
>> @@ -792,6 +792,7 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
>> {
>> struct wcn36xx_vif *vif_priv = wcn36xx_vif_to_priv(vif);
>> struct wcn36xx_hal_finish_scan_req_msg msg_body;
>> + u8 oper_channel;
>> int ret;
>>
>> mutex_lock(&wcn->hal_mutex);
>> @@ -802,7 +803,9 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
>> /* Notify BSSID with null data packet */
>> msg_body.notify = 1;
>> msg_body.frame_type = 2;
>> - msg_body.oper_channel = channel;
>> + oper_channel = WCN36XX_HW_CHANNEL(wcn);
>> + if (oper_channel != channel)
>> + msg_body.oper_channel = channel;
>
> Is this condition ever satisfied? I mean, finish_scan is normally always
> called with operating-channel as channel parameter.

I had a debug statement and never saw it run, so I think no is probably
the answer to that.

Basically though the msg_body.oper_channel needs to be left at zero.
Whatever the firmware does with non-zero is not correct @ 80Mhz/5ghz

>> 2. Performance is as expected
>> I see throughput oscillate between 30 mbit and 100 mbit
>> - Android runs a scan every 15 seconds
>
> With scanning app in the foreground, right?
> While connected to an AP, Android 'should' only background scan if
> RSSI becomes too low (for roaming).

Foreground scanning yes, I'm forcing a constant interleaving.