netdev - Re: [PATCH v2] rtl8xxxu: add bluetooth co-existence support for single antenna

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: <0c049f46-fb15-693e-affe-a84ea759b5d7@gmail.com>
Date:   Wed, 2 Oct 2019 11:03:51 -0400
From:   Jes Sorensen <jes.sorensen@...il.com>
To:     Chris Chiu <chiu@...lessm.com>, kvalo@...eaurora.org,
        davem@...emloft.net
Cc:     linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux@...lessm.com
Subject: Re: [PATCH v2] rtl8xxxu: add bluetooth co-existence support for
 single antenna

On 9/10/19 10:50 PM, Chris Chiu wrote:
> The RTL8723BU suffers the wifi disconnection problem while bluetooth
> device connected. While wifi is doing tx/rx, the bluetooth will scan
> without results. This is due to the wifi and bluetooth share the same
> single antenna for RF communication and they need to have a mechanism
> to collaborate.
> 
> BT information is provided via the packet sent from co-processor to
> host (C2H). It contains the status of BT but the rtl8723bu_handle_c2h
> dose not really handle it. And there's no bluetooth coexistence
> mechanism to deal with it.
> 
> This commit adds a workqueue to set the tdma configurations and
> coefficient table per the parsed bluetooth link status and given
> wifi connection state. The tdma/coef table comes from the vendor
> driver code of the RTL8192EU and RTL8723BU. However, this commit is
> only for single antenna scenario which RTL8192EU is default dual
> antenna. The rtl8xxxu_parse_rxdesc24 which invokes the handle_c2h
> is only for 8723b and 8192e so the mechanism is expected to work
> on both chips with single antenna. Note RTL8192EU dual antenna is
> not supported.
> 
> Signed-off-by: Chris Chiu <chiu@...lessm.com>
> ---
> 
> Notes:
>    v2:
>     - Add helper functions to replace bunch of tdma settings
>     - Reformat some lines to meet kernel coding style
> 
> 
>   .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  37 +++
>   .../realtek/rtl8xxxu/rtl8xxxu_8723b.c         |   2 -
>   .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 262 +++++++++++++++++-
>   3 files changed, 294 insertions(+), 7 deletions(-)

In general I think it looks good! One nit below:

Sorry I have been traveling for the last three weeks, so just catching up.


> +void rtl8723bu_set_coex_with_type(struct rtl8xxxu_priv *priv, u8 type)
> +{
> +	switch (type) {
> +	case 0:
> +		rtl8xxxu_write32(priv, REG_BT_COEX_TABLE1, 0x55555555);
> +		rtl8xxxu_write32(priv, REG_BT_COEX_TABLE2, 0x55555555);
> +		rtl8xxxu_write32(priv, REG_BT_COEX_TABLE3, 0x00ffffff);
> +		rtl8xxxu_write8(priv, REG_BT_COEX_TABLE4, 0x03);
> +		break;
> +	case 1:
> +	case 3:

The one item here, I would prefer introducing some defined types to 
avoid the hard coded type numbers. It's much easier to read and debug 
when named.

If you shortened the name of the function to rtl8723bu_set_coex() you 
won't have problems with line lengths at the calling point.

Cheers,
Jes