Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] wcn36xx: Fix software-driven scan
To:     Loic Poulain <loic.poulain@linaro.org>
Cc:     Kalle Valo <kvalo@codeaurora.org>, wcn36xx@lists.infradead.org,
        linux-wireless@vger.kernel.org
References: <1597680075-18723-1-git-send-email-loic.poulain@linaro.org>
 <db8adbab-bf1a-0552-0dce-af4acee9719e@linaro.org>
 <CAMZdPi8KRJ3Jb129xAqR8fnfc7tKaVvLZH00DF_z+Vx+wOD3Ow@mail.gmail.com>
From:   Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Message-ID: <5fc88e36-385c-68eb-091f-f53edac7ca29@linaro.org>
Date:   Mon, 24 Aug 2020 10:57:16 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <CAMZdPi8KRJ3Jb129xAqR8fnfc7tKaVvLZH00DF_z+Vx+wOD3Ow@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 24/08/2020 08:51, Loic Poulain wrote:
> Hi Bryan,
> 
> On Mon, 24 Aug 2020 at 03:45, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> 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 <loic.poulain@linaro.org>
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>
>> 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.