linux-kernel - Re: [PATCH v2] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD8Lp45rYuE5WHx4vSbUhF10hOHam1aBLd52_aDKP8z2eeL4vA@mail.gmail.com>
Date:   Fri, 5 Jul 2019 11:41:41 +0800
From:   Daniel Drake <drake@...lessm.com>
To:     Chris Chiu <chiu@...lessm.com>
Cc:     Jes Sorensen <jes.sorensen@...il.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        David Miller <davem@...emloft.net>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        Linux Upstreaming Team <linux@...lessm.com>
Subject: Re: [PATCH v2] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU

On Thu, Jul 4, 2019 at 6:55 PM Chris Chiu <chiu@...lessm.com> wrote:
> The WiFi tx power of RTL8723BU is extremely low after booting. So
> the WiFi scan gives very limited AP list and it always fails to
> connect to the selected AP. This module only supports 1x1 antenna
> and the antenna is switched to bluetooth due to some incorrect
> register settings.
>
> Compare with the vendor driver https://github.com/lwfinger/rtl8723bu,
> we realized that the 8723bu's enable_rf() does the same thing as
> rtw_btcoex_HAL_Initialize() in vendor driver. And it by default
> sets the antenna path to BTC_ANT_PATH_BT which we verified it's
> the cause of the wifi weak tx power. The vendor driver will set
> the antenna path to BTC_ANT_PATH_PTA in the consequent btcoexist
> mechanism, by the function halbtc8723b1ant_PsTdma.

Checking these details in the vendor driver:
EXhalbtc8723b1ant_PowerOnSetting sets:
        pBoardInfo->btdmAntPos = BTC_ANTENNA_AT_AUX_PORT;

Following the code flow from rtw_btcoex_HAL_Initialize(), this has a
bWifiOnly parameter which will ultimately influence the final register
value.
Continuing the flow, we reach halbtc8723b1ant_SetAntPath() with
bInitHwCfg=TRUE, bWifiOff=FALSE. antPosType is set to WIFI in the
bWifiOnly case, and BT otherwise.

I'm assuming that bUseExtSwitch = FALSE (existing rtl8xxxu code also
seems to make the same assumption).
For the bWifiOnly=FALSE case, it uses BT,
                    pBtCoexist->fBtcWrite4Byte(pBtCoexist, 0x948, 0x0);
and rtl8xxxu seems to do the same - seemingly routing the antenna path
for BT only.

As for halbtc8723b1ant_PsTdma() then being called in a way that causes
it to switch to the PTA path a little later, it's more difficult to
point out how that happens in an email, but I thin kwe can trust you
on that :) There are certainly many callsites that would pass those
parameters.

> +        * Different settings per different antenna position.
> +        *      Antenna Position:   | Normal   Inverse
> +        * --------------------------------------------------
> +        * Antenna switch to BT:    |  0x280,   0x00
> +        * Antenna switch to WiFi:  |  0x0,     0x280
> +        * Antenna switch to PTA:   |  0x200,   0x80
>          */
> -       rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00);
> +       rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80);

I don't really understand what we mean by an "inverse" antenna and my
reading of the vendor driver leads me to another interpretation.

The logic is based around an antenna position - btdmAntPos. It takes
one of two values:
    BTC_ANTENNA_AT_MAIN_PORT                = 0x1,
    BTC_ANTENNA_AT_AUX_PORT                = 0x2,

So the chip has pins to support two antennas - a "main" antenna and an
"aux" one.

We know we're dealing with a single antenna, so the actual module is
going to only be using one of those antenna interfaces. If the chip
tries to use the other antenna interface, it's effectively not using
the antenna. So it's rather important to tell the chip to use the
right interface.

And that's exactly what happens here. btdmAntPos is hardcoded that the
antenna is on the aux port for these devices, and this code is telling
the chip that this is how things are wired up.

The alternative way of calling this an antenna inverse (which the
vendor driver also does in another section), i.e. "antenna is not
connected to the main port but instead it's connected to the other
one", seems less clear to me.

Even if we don't fully understand what's going on here, I'm convinced
that your code change is fixing an inconsistency with the vendor
driver, and most significantly, making the signal level actually
usable on our devices. But if you agree with my interpretation of
these values then maybe you could update the comment here!

Daniel