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;
MIME-Version: 1.0
References: <1597680075-18723-1-git-send-email-loic.poulain@linaro.org> <db8adbab-bf1a-0552-0dce-af4acee9719e@linaro.org>
In-Reply-To: <db8adbab-bf1a-0552-0dce-af4acee9719e@linaro.org>
From:   Loic Poulain <loic.poulain@linaro.org>
Date:   Mon, 24 Aug 2020 09:51:22 +0200
Message-ID: <CAMZdPi8KRJ3Jb129xAqR8fnfc7tKaVvLZH00DF_z+Vx+wOD3Ow@mail.gmail.com>
Subject: Re: [PATCH] wcn36xx: Fix software-driven scan
To:     "Bryan O'Donoghue" <bryan.odonoghue@linaro.org>
Cc:     Kalle Valo <kvalo@codeaurora.org>, wcn36xx@lists.infradead.org,
        linux-wireless@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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.

>                  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