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>, kvalo@codeaurora.org
Cc:     wcn36xx@lists.infradead.org, linux-wireless@vger.kernel.org
References: <1597680075-18723-1-git-send-email-loic.poulain@linaro.org>
From:   Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Message-ID: <db8adbab-bf1a-0552-0dce-af4acee9719e@linaro.org>
Date:   Mon, 24 Aug 2020 02:46:24 +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: <1597680075-18723-1-git-send-email-loic.poulain@linaro.org>
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 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;
                 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