RE: [RFC PATCH v1 0/9] wifi: rtw88: add RTL8723B/RTL8723BS support

[Luka Gejak <luka.gejak@linux.dev>]

Hi Ping-Ke,

Thanks for the review, and for taking the time on a series this size.

Before I send v2 I want to report what changed, because the result of
your comments is that the series gets noticeably smaller. I would
rather tell you now than have you spend review time on patches I am
about to delete.

Answers to your questions from the cover letter first.

1. The link. Michael Straube's original work is now referenced in the
   chip patch:

     https://github.com/mistraube/rtw88/tree/rtl8723bs

   The current state of my series is here, if it is easier to read than
   mail:

     https://github.com/MocLG/rtw/tree/8723bs-upstream

2. sparse and smatch. Both re-run on the new series, clean, no findings
   in the 8723b/8723bs files.

3. Bluetooth coexistence. Tested as you asked: WiFi download while an
   A2DP sink plays music, shared antenna, 2.4 GHz HT40.

   The audio side is good. Music played for the whole run with no
   underruns reported by the audio stack. The WiFi side is not good:
   download throughput went from 42.0 Mbit/s with Bluetooth idle to
   0.21 Mbit/s with A2DP active, a 99.5% drop. So BT wins the air
   almost completely rather than sharing it.

   While looking into that I found two chip config values of mine were
   wrong, both now fixed:

     - scbd_support was true. The vendor driver has no scoreboard
       implementation for 8723b at all (it exists only for 8703b,
       8723d and 8821c), so this is now false. With it false the TDMA
       case moves off case-0, which it never did before.

     - bt_desired_ver was 0x6f, which is glcoex_ver_8723b_1ant, the
       driver's own coex version rather than the desired BT version.
       It is now 0x6d (glcoex_ver_btdesired_8723b_1ant, shared
       antenna).

   With those two fixed the coex mechanism itself is clearly running.
   During the A2DP run it sits on TDMA case-3 (61 30 03 11 11), coex
   table 10, and the run reason is BTINFO, so it is reacting to BT
   status rather than sitting idle.

   What it is reacting to looks wrong, and this is the part I cannot
   answer from outside. With music actively streaming, coex_info shows:

     Coex Ver/ BT Dez/ BT Rpt   = 0133ece9/ 0x6d/ 0x00000069 (Mismatch)
     BT status/ rssi/ retry/pop = conn-idle/ -54dBm/ 0/ 9
     Profiles                   =  (multi-link 0)
     BT_RSP                     = 01 01 40 12 00 00 00
     Hi-Pri TX/RX, Lo-Pri TX/RX = 0/3, 440/440

   So the BT side reports conn-idle with an empty profile list while it
   is demonstrably moving traffic (440 low priority packets each way),
   and the profile byte stays 0x01 (CONNECTION) with A2DP (BIT 6) and
   ACL_BUSY (BIT 3) never set. Coex is therefore arbitrating for an
   idle link that is not idle, which would explain why WiFi gets
   almost nothing.

   The reported BT coex version is 0x69, below the 0x6d the 1ant
   parameters expect. Is the rtl8723bs BT firmware currently shipped in
   linux-firmware older than the coex protocol these parameters were
   written for, and should I be using a different blob? If this is a
   known gap I am happy to document the limitation rather than chase it
   in the WiFi driver.

Now the larger change.

Several of your comments were variants of "does the existing logic not
work, why do you need this". Rather than argue from the vendor driver I
tested it. For each of those special cases I reverted it to the generic
rtw88 path, rebuilt, reloaded and measured association, WPA2 handshake
and throughput in both directions.

Every one of them turned out to be unnecessary. I have dropped these
five patches:

  - apply the firmware-reported TX rate
  - program the management TX descriptor contract
  - add first-segment and retry-limit TX descriptor bits
  - select the management and EAPOL TX rate
  - keep the chip powered between scan and connect (soft IPS)

Specifically, to your questions:

  - "Why does it need report?" and the mac_id question. It does not.
    With SPE_RPT not set and mac_id taken from si/vif as you suggested,
    association still completes normally through the generic management
    path. My claim that the firmware will not schedule a management
    frame without SPE_RPT was wrong.

  - First/last segment and the four-way handshake. You were right that
    these are unrelated. With the first-segment bit removed the WPA2
    four-way completes and data flows unchanged.

  - The two rate patches. Not needed. The generic rtw_get_mgmt_rate
    path carries auth, assoc and EAPOL fine, and without forcing the
    firmware-reported rate the TX rate still climbs to MCS7. The CCK
    floor behaviour I described does not reproduce.

  - IPS. You were right to question this too, though not for the reason
    I expected. IPS is entered here. With the soft-IPS special case
    removed the chip does a real power off and on, which I confirmed by
    tracing the power state across the transition, and it still
    reconnects every time and passes full throughput. So the power
    cycle does not corrupt the management TX path the way I claimed.

Result, measured on the reduced series: association fine, WPA2
reconnect 3/3, 20 Mbit/s up and about 32 Mbit/s down, no warnings and
no TX report failures. That matches the full series, so nothing was
lost by removing them. The series goes from 15 patches to 10.

Two caveats I want to state plainly rather than have you find them.

First, this was measured on one AP at strong signal (-32 dBm), 2.4 GHz
HT40, on a single board. If any of those five patches guards a case I
cannot reach here, a weaker link, a different AP, another firmware, you
will know it and I will not. Please say so and I will put it back.

Second, I kept one you asked about: the auth_sync mechanism in the SDIO
association sequence. It also tested removable here, association and
repeated rejoins all succeeded without it, but its whole purpose is to
wait for a beacon or probe response before authenticating, which is
exactly the thing a strong-signal AP will hide. I did not want to drop
it on the strength of a test that cannot show the problem it solves. If
you think the driver has no business doing that and mac80211 should
handle the timing, I will drop it too.

The remaining changes since v1, in short. Details will be in the v2
per-patch changelogs:

  - rtw_is_8723bs_sdio() renamed to rtw_is_8723bs().
  - Kconfig and Makefile moved to the end of the series, before
    MAINTAINERS, so the driver only becomes buildable once the
    supporting patches are in.
  - Struct fields, flags and function declarations moved out of the
    chip patch into the patch that first uses them.
  - The vendor-reference comments removed throughout. You were right
    that they were notes to myself.
  - Local register defines moved to reg.h, and RF_RCK_OS / RF_TXPA_G1 /
    RF_TXPA_G2 replaced with the existing RF_MODE_TABLE_* definitions.
  - Copyright headers corrected to include Realtek, includes reordered,
    comment style switched to the current form, forward declarations
    removed, reverse christmas tree and the alignment and mdelay
    issues fixed.
  - The old patch 2 split into smaller patches as you asked. What is
    left of it after the removals above is the TX report purge timeout
    change.
  - Peter Robinson tested the RFC and reported two warnings. The
    "failed to get tx report" one is what the purge timeout patch
    addresses: the 500 ms default is too short for this firmware, the
    same reasoning as the 8723D USB case already in tree. The
    "firmware failed to leave lps state" one I could not reproduce
    here, including with forced LPS enter and leave cycles, so I have
    not added a workaround for something I cannot demonstrate. Peter is
    on CC and I have asked him about it at the end of this mail.
  - checkpatch --strict is clean apart from one TRANS_SEQ_END macro
    warning, which is byte for byte the same as the existing one in
    rtw8703b.c.
  - Every commit builds with W=1 with no warnings.

I will hold v2 until you have had a chance to react to the removals, in
case any of them should stay, and to pick up whatever else you find in
the patches that remain.


Peter, the rest of this is for you, hence the CC.

Since your Tested-by the series has lost the five patches listed above,
so what you tested is not quite what v2 will be. I would rather not
carry the tag forward without asking. Say so if you would prefer I drop
it until you have had a look at v2.

There is a better reason to ask you than tag hygiene though. Both
warnings you hit are timing races I cannot reproduce here, and the
difference looks like host speed: your Pine64 SDIO hosts are slower than
the board I test on. That also makes them the right place to check the
five removals, because I validated those on a single fast host against a
strong signal AP, which is the least likely setup to expose a timing
regression. If any of those patches was quietly covering for a slow
host, your boards will show it and mine will not.

The tx report warning should be gone with the longer purge timeout.

For the LPS one, the driver gives the firmware 100 ms to send its null
packet, get the ACK back from the AP and restore REG_TCR. If that
warning still fires on v2, could you say roughly how often it happens,
and if it is easy to tell, how far past 100 ms it goes? I would rather
extend that poll by a measured amount than guess a number, and I do not
want to just silence the message.

Best regards,
Luka Gejak