wifi: rtw88: preparations for RTL8723B/RTL8723BS

From: Luka Gejak <[email protected]>

This is the first of two series adding support for the Realtek RTL8723B 802.11n chipset and its RTL8723BS SDIO variant to rtw88. It contains only the changes to the shared rtw88 core that the chip driver depends on. The chip itself, the build glue and the MAINTAINERS entry are a second series.

v1 had 19 patches, v2 had 11, v3 had 5, v4 had 7, this has 6.

There is no rtw88-style firmware for this chip and no documentation for the vendor blob it does use, so while bringing it up I deliberately kept the driver as close to the vendor driver's behaviour as I could. With that many unknowns at once, matching the vendor exactly was the only way to tell which difference actually mattered when something did not work, rather than guessing. A number of the v1 patches came from that: they reproduced vendor behaviour that was useful to hold fixed during bring up, not behaviour the chip turns out to require.

Bitterblue Smith's review of v1 prompted me to go back and remove each of those on hardware to find which were actually load bearing; eight were not. Ping-Ke Shih's review of v2 did the same for a further six, and the changelog below gives the measurement for each. In four of those cases I had told the list, or claimed in a commit message, that the chip needed something it does not, on the strength of bring-up notes I had not re-measured. All four corrections are in the changelog.

What is left is there because removing it made something measurably worse, not because the vendor driver does it.

What is left falls into these groups:

- patches 1 and 2: the chip type and helper plus a one-line receive

    fix;
  - patch 3: a longer TX-report purge timeout, needed on slower SDIO
    hosts (see below);

- SDIO (patches 4, 5, 6): software free-page and output-queue accounting, RX aggregation and interrupt setup, and TX back-pressure with a retry on page starvation.

This series no longer touches coex.c, mac80211.c or fw.c at all.

Scope
=====

Everything is gated on rtw_is_8723bs(), which is false for every chip currently supported, so behaviour for existing devices is unchanged. There is one exception, deliberate: patch 2 extends an existing RTL8703B zero-length-packet check and is gated on the SDIO interface rather than the chip id, since that is the only place the behaviour has been observed. Ping-Ke has suggested that check may not need a chip test at all; I would rather measure that on RTL8703B hardware than assume it, and I do not have that part, so it is unchanged here.

Patch 5 changes how the SDIO interrupt status is acknowledged. In v1 that applied to all SDIO parts; it is scoped to this chip only, and the others keep writing the status word back unchanged.

Chip specific code in the common path
=====================================

Ping-Ke asked whether this needs a set of ops rather than a chip test sprinkled through the core. Working through that question is what removed most of this series: once the association and coexistence patches turned out not to be needed, the chip tests they carried went with them.

What remains is one test in rx.c, one in tx.c and eleven in sdio.c. There are none in fw.c, mac80211.c or coex.c. The SDIO ones are in the bus driver rather than the chip driver, and rtw_sdio has no per-chip ops table today; Ping-Ke confirmed an inline chip id check there is fine.

Fixed in the second series
==========================

Two of Bitterblue's review points are addressed in the chip driver rather than here, because that is the right place for both:

- the hardware capability is filled in from a chip-specific read_efuse, instead of special casing rtw_dump_hw_feature() in the core;
- the receive FCS handling is a chip configuration fix. v1 stopped the core advertising RX_INCLUDES_FCS for this chip. The real cause was that the chip cleared BIT_APP_FCS by assigning WLAN_RCR_CFG over hal.rcr wholesale, where the 8723x siblings only write the register and leave hal.rcr alone. Setting that bit in the chip's WLAN_RCR_CFG makes the descriptor carry the FCS like every other rtw88 chip, so the core needs no special case and both hunks are gone from this series.

Neither change is visible here. I planned to send that series once this one is applied to rtw-next, but I can send it now alongside if you would rather review them together, or fold the two into one series if that is easier.

Testing
=======

Tested on RTL8723BS hardware together with the second series: repeated scan, authentication, association, WPA2-PSK/CCMP handshake, DHCP, bidirectional traffic, reconnects and module reloads.

The full driver, this preparation series plus the chip support that follows in a second series, is available for testing at:

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

Throughput is measured against an iperf3 server one hop behind the AP, with the wlan0 byte counters as ground truth because the test machine's wired interface shares the same subnet. On one AP at strong signal, 2.4 GHz HT40, it runs at roughly 40 Mbit/s down and 20 Mbit/s up, with UDP showing no loss in either direction. The exact figures move by several Mbit/s from one day to the next with the radio environment, so they are a range rather than a benchmark.

Uplink on this band drifts by several Mbit/s from day to day, enough to swamp the differences I was trying to measure, so where the changelog below compares two builds the comparison is a paired test: the two builds alternate within one session, rebuilt and reinstalled each time, rather than being measured on different days.

Suspend and resume are not covered: this machine only offers s2idle and does not reliably come back from it.

A five minute soak: 1495/1495 pings at 0% loss, 15/15 reconnects, 5/5 link up/down cycles, 3/3 module reloads, 5/5 scans under traffic, 24 group rekeys, no deauthentication, no SDIO errors, no warnings in the log. Scanning saw the target AP in 18 of 18 attempts across 6 cold module reloads, with 6/6 associations.

The soak and scan figures were taken on a build that also carried the reserved page patch dropped in v3; the throughput figures above are from the tree posted here.

Every patch builds with W=1 with no warnings on its own, sparse is clean at the tip, and smatch reports nothing in any file this series touches. checkpatch --strict is clean across this series. The other rtw88 bus and chip modules continue to build.

Known limitation
================

After a long idle period the firmware intermittently fails to leave LPS, which rtw88 reports as "firmware failed to leave lps state". That check is the REG_TCR poll in __rtw_fw_leave_lps_check_reg(), and the failures arrive in long runs rather than singly, so it does not look like a timeout that is merely too short. On one occurrence here the warning was followed fourteen seconds later by a full reauthentication, so it is not only log noise.

Power save is now enabled on my test machine and it reproduces there:
five occurrences over three days of mostly idle uptime. It was first reported on ARM SDIO boards, but it is not specific to slow hosts.

It is not caused by anything in this series, and it needs the chip driver from the second series to be reachable at all, but it is open and I am chasing it.

Changes in v5:
- Dropped "fw: handle the RTL8723BS management TX reports", which v4 had restored. It cannot do what its commit message claimed. This firmware does not advertise FW_FEATURE_LPS_C2H, so rtw_fw_leave_lps_check() takes the REG_TCR polling path and nothing ever waits on the C2H completion the patch rerouted; and with the driver instrumented, 93 TX reports over three scans all arrive as C2H id 0x03, with no event ever arriving as a top level 0x12 or 0x32. Bitterblue Smith made exactly this point on v1 and was right. A tester on a Rockchip RK3288 board confirmed the timeout patch in this series is enough on its own. This is the fourth correction of something I had claimed on the list; restoring it in v4 was based on a tester reporting that a branch containing it cleared the warnings, which showed the branch helped, not that this patch in it did. With it gone the series no longer touches fw.c.
- Power save is now enabled on the test machine, so LPS is exercised there. The v2 entry below dropped a patch that gated LPS entry on smoothed throughput, partly on the grounds that LPS never engaged on this setup at all, which was never a sound reason to drop it. It is measured now, paired against this series in one session with power save on: 60 idle pings average 5.1 ms without the gating and 6.0 ms with it, and TCP is 30.0 down and 19.1 up against 32.3 and 18.9. It makes no difference, because the gate tests the smoothed throughput and interactive traffic rounds to zero there, so it never fires in the case it was meant to help. It stays dropped, now on a measurement rather than for want of one.
- sdio: fixed a transmit stall in the back-pressure patch, found while auditing a tester report of transmit stopping under bidirectional load with receive unaffected and nothing in the log. The TX work item was only re-armed when a transfer failed with a page shortage. Once the mac80211 queue can be stopped, that is not enough: a stopped queue is handed no further frames, so on any other failure nothing would kick the worker again and the affected access category would stay stopped for good, with the link still up. It now also re-arms on a failed skb expansion, which together with the page shortage covers both of the failures that produce no log message. The remaining errors are each logged where they happen, so unlike those two they are visible rather than an unexplained hang, and they keep the existing behaviour rather than being retried forever.
- rx: the zero length test now evaluates pkt_stat->pkt_len first, so the unlikely case short circuits before the chip test.
- sdio: the commit message now says what OQT is, as far as the vendor driver reveals it: the vendor calls the register the OQT free space and never expands the acronym, and it holds the number of further transfers the SDIO output queue will accept.
- sdio: rtw_sdio_8723bs_sync_free_txpg() now returns whether the chip reported anything and is the only caller of rtw_sdio_8723bs_store_free_txpg(), and rtw_sdio_8723bs_init_free_txpg() returns an error rather than nothing. Working through that suggestion found a real bug: the public pool size was computed as acq_pg_num minus the reserved queues with no check, so a chip that came up with no transmit page allocation at all would underflow a u16 and leave the driver believing it had about 65000 free pages. That calculation is now rtw_sdio_8723bs_pubq_num(), shared with the queue page allocation repair path, and it fails cleanly instead.
- sdio: the output queue wait is now bounded by a jiffies deadline rather than a loop count, so RTW_SDIO_OQT_TIMEOUT_MS really is a timeout in milliseconds. It was 1000 iterations of a 1 to 2 ms sleep before, so the name was only approximately true.
- sdio: rtw_sdio_8723bs_check_rqpn() returns an error instead of silently doing nothing when the transmit page pool cannot cover the reserved queues, and rtw_sdio_start() propagates it. Both early returns are now explained: a non-zero free page count means the allocation latched during power on and must be left alone.
- sdio: fixed the interrupt acknowledgment. It masked the status word with both irq_mask and RTW_SDIO_HISR_CLEAR_MASK, but for this chip irq_mask is REG_SDIO_HIMR_RX_REQUEST alone and that bit is not in the clear mask, so the two had no bits in common and the driver acknowledged nothing at all. It now masks with the defined bits only, which is what the comment described. Thanks to Ping-Ke for catching this. Retested on hardware.
- sdio: the back-pressure and wake conditions moved into rtw_sdio_8723bs_stop_tx_queue() and _wake_tx_queue(), and the two requeue sites into rtw_sdio_reschedule_tx_work(), instead of long conditions inline.
- sdio: rtw_sdio_process_tx_queue() now returns 0 on success and 1 for the empty queue case, rather than the other way round.
- sdio: queue_stopped[] renamed to tx_queue_stopped[].
- RTW_SDIO_TX_RETRY_DELAY is left as msecs_to_jiffies(1). Ping-Ke asked whether it becomes 0 when HZ is below 1000; it does not, since msecs_to_jiffies() rounds up for HZ < 1000 and returns one jiffy.

Changes in v4:
- Restored "tx: extend the TX report purge timeout to RTL8723BS", dropped in v2 because I could not reproduce the failure on my test machine, a fast x86 host, so the path never ran for me. Testers on slower ARM SDIO hosts, Peter Robinson on the RFC and Zhang Ning on a Rockchip RK3288 board, both hit "failed to get tx report from firmware". It extends the exact fix from commit c80788f7c5ae ("wifi:
rtw88: increase TX report timeout to fix race condition"), already applied to the RTL8723DU, to the RTL8723BS, since they run the same firmware and hit the same off-channel-scan race.

Changes in v3:
- Dropped "fw: fix the reserved page upload on RTL8723BS". Its commit message said the BIT_BCN_VALID handshake fails on 8723BS SDIO without it. That does not reproduce. The failure path is an explicit rtw_err("error beacon valid") returning -EBUSY, reached from BSS_CHANGED_ASSOC on every association; over three associations on each of five builds with the patch reverted it never fired, associations were 3/3, and there was no packet loss after 30 s idle in power save, which is the path the reserved page's null data and PS-poll frames serve. As a control, rtw_info messages appear in the same captures and rtw_err outranks them, so a failure would have been logged. AP mode and WoWLAN also download the reserved page and are not tested here, so if it turns out to matter there it can come back with evidence behind it.
- Kept "sdio: add TX back-pressure and retry on page starvation", now with a measurement rather than an assertion, since you asked whether the remaining chip tests are necessary. Counting rtw_sdio_tx_handler invocations under an identical 60 Mbit/s UDP flood, because the retry re-arms that same work item: 1798 and 3095 on two baseline runs against 69 with the patch reverted. Achievable transmit rate falls by about a third without it, 25.5 Mbit/s against 38.3 and 40.2. tx_dropped and tx_errors stay at zero throughout, so the failure is not dropping frames, it is failing to push them.
- Kept "sdio: set up RX aggregation and interrupts". The aggregation half makes no measurable difference to throughput here, and on that evidence alone I would have dropped it. The rest of the patch does not show up in a throughput test at all: the chip keeps raising the interrupt after resume if undefined status bits are written back when acknowledging.

I have not been able to validate the suspend and resume path: the test machine only offers s2idle and does not reliably resume from it, so I cannot exercise the interrupt acknowledgment change. It is kept because the failure it addresses was observed during bring up, not because it has been re-measured. If someone with hardware that suspends cleanly finds it unnecessary, it should go.
- Dropped "run the RTL8723BS association register sequence", 440 lines, which was the vendor join sequence, six sites in rtw_ops_bss_info_changed() and both sites in rtw_ops_set_key().

Ping-Ke asked for this to run from rtw_chip_prepare_tx() rather than a chip test in mgd_prepare_tx(), and I had done that with a new chip_ops::prepare_tx. Since the whole patch is gone, so is the callback; the core is left as it was rather than gaining an op with no user.

I had told the list that association fails without these registers. That was wrong, and it was bring-up-era data I repeated without re-measuring. With the whole patch reverted: 18/18 scans saw the AP, 6/6 associations, and the five minute soak above, whose 15 reconnects and 24 rekeys exercise the exact path the sequence ran on, since it ran from mgd_prepare_tx() on every authentication. Uplink measured paired against the full series, 19.0 against 19.2 Mbit/s over ten runs each.

The default key search handling in that patch was also simply wrong:
rtw_sec_enable_sec_engine() sets sec->default_key_search itself and programs all four USE_DK bits on that basis, so the enable path was a no-op and the disable path was clearing bits the core had deliberately set.
- Dropped "coex: add the RTL8723BS scan antenna workaround" and "coex:
reassert the antenna path when associating", and with them "fw: add the GNT_BT firmware command", whose only caller they were.

The commit message on the first claimed the site survey does not hear the AP reliably without it. That is also wrong: 18/18 scans without it. Both were gated on the chip and on coex bt_disabled, so that gate is their entire scope rather than a subset of it, and the measurements above cover it.
- Dropped "record beacons from the target BSSID before authenticating". Ping-Ke doubted the beacon wait affected the connection and he was right: with the wait removed, association succeeded 12/12 from a cold module reload and 12/12 from a warm reconnect, against 10/10 and 10/10 with it, at the same latency. The pre-auth deauth already sleeps 100 ms with the recording window open, so on a 100 ms beacon interval the wait was almost always already satisfied when it was reached. struct rtw_auth_sync and the RX-path hook go with it.
- Dropped the BT_MP version queries and the BT_INFO query at scan start, and the BT_INFO query plus two of three repeated PS-TDMA H2Cs in the pre-auth replay. Measured over cold module reloads with three scans each, 8 reloads with the queries and 10 without: 29/30 scans saw the target AP and 10/10 associations succeeded without them, against 24/24 and 8/8 with them.
- The cached SDIO page counters are clamped at zero. A lost update between the free page check and the accounting could previously drive the public counter negative, and the sum was assigned to an unsigned, which made the check pass unconditionally from then on and suppressed the resync that was supposed to recover it.
- REG_SDIO_FREE_TXPG now has field masks and is decoded with u32_get_bits(); the RX DMA burst count uses BIT_DMA_BURST_CNT; open-coded BIT(0)|BIT(1) on REG_SYS_FUNC_EN uses the existing names.
- The RTL8723BS-specific SDIO helpers are named _8723bs_ and the chip test moved to their callers.
- rtw_sdio_process_tx_queue() returns a value instead of taking a "processed" out-parameter, which makes the retry and back-pressure logic in the TX handler readable.
- mod_delayed_work() in rtw_sdio_tx_kick_off() carries a comment saying why it is not queue_delayed_work(): a page-starvation retry may already be armed with a delay, and the newly queued frame should not wait for it.
- An argument that had a single value at every call site is gone, along with its dead branch.
- Block comments use the general kernel style, per commit 82b8000c28b5 ("net: drop special comment style").

Changes in v2:
- Dropped "tx: extend the TX report purge timeout". Instrumenting the report path shows payload[6] & 0xfc equals the enqueued sequence number, 32 out of 32 times, with rtw88's existing decode, and no report is missed. The longer timeout was covering for the next patch, not for the hardware.
- Dropped "fw: handle the RTL8723BS management TX reports". 0x12 and 0x32 are the first payload byte of C2H_CCX_TX_RPT, not C2H IDs; every C2H event this chip sends carries a known id, so the existing handler already covers them.
- Dropped "fw: send rate adaptation and RSSI info in the vendor layout". The vendor byte layout is equivalent to what the existing macros produce; uplink and the negotiated rate are unchanged without it (16.4/16.5/17.0 against 15.4/16.7/16.6 Mbit/s, MCS7 both ways).
- Dropped "fw: send the media status report in the vendor layout". The role field the vendor sets makes no difference here, as with the other chips.
- Dropped "sdio: handle the RTL8723BS management TX path". Sequence numbers do work for management frames on this chip, so reporting completion at DMA completion is unnecessary.
- Dropped "calibrate and tune the PHY" entirely. All three parts were vendor-matching scaffolding and none survived measurement: the scan-time initial gain override is worse than rtw_phy_dig_set_max_coverage() (five scans found 41 BSSes without it against 34 with it); the IQ calibration works from the normal phy_calibration path; and the hardcoded per-rate TX AGC table overrode the efuse calibration by up to 22 index units and bypassed the regulatory limit the by-rate path applies, which is not something a driver should do.
- Dropped "match the RTL8723BS firmware connect and power save behaviour". Deferring the connect report changes nothing, and the LPS gating cannot be justified from measurement: with RTW_DBG_PS enabled, LPS never engages on this setup at all, so the gating never takes effect here.
- Dropped "advertise the correct receive capabilities". Both halves move into the chip driver, as described above.
- "fw: add the vendor firmware commands" now adds only GNT_BT. rtw88 already implements MACID_CFG and the WL channel info report, and the coexistence antenna select reserve became unused once the PHY patch went.
- The zero-length packet check is gated on SDIO rather than the chip

    id.

- The SDIO interrupt acknowledgment change is scoped to this chip.
- The association sequence no longer re-applies the BSS capability at association time; BSS_CHANGED_ERP_PREAMBLE and BSS_CHANGED_ERP_SLOT are handled later in the same callback.
- Register accesses that had raw addresses now use the existing REG_GNT_BT and REG_BT_COEX_ENH_INTR_CTRL, plus a named define for the BB antenna select register.

The implementation is based on the initial RTL8723B work by Michael Straube <[email protected]>:

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

Based on the rtw-next branch from pkshih/rtw.

Luka Gejak (6):
wifi: rtw88: add the RTL8723B chip type and SDIO helper
wifi: rtw88: rx: mark zero length packets on RTL8723BS
wifi: rtw88: tx: extend the TX report purge timeout to RTL8723BS
wifi: rtw88: sdio: track free TX pages and OQT credits for RTL8723BS
wifi: rtw88: sdio: set up RX aggregation and interrupts for RTL8723BS
wifi: rtw88: sdio: add TX back-pressure and retry on page starvation

 drivers/net/wireless/realtek/rtw88/main.h |   7 +
 drivers/net/wireless/realtek/rtw88/rx.c   |   8 +-
 drivers/net/wireless/realtek/rtw88/sdio.c | 460 ++++++++++++++++++++--
 drivers/net/wireless/realtek/rtw88/sdio.h |  22 +-
 drivers/net/wireless/realtek/rtw88/tx.c   |   5 +-
 5 files changed, 473 insertions(+), 29 deletions(-)

--
2.53.0

From: Luka Gejak <[email protected]>

The RTL8723B is a 802.11n combo chip whose SDIO variant, RTL8723BS, runs a Realtek vendor firmware that differs from the firmware used by the other rtw88 8723 family devices. Supporting it needs a number of small adjustments spread across the shared core, all of which have to be restricted to this one chip and bus combination.

Add the chip type and a helper that tests for it, so the changes that follow can be gated without repeating the chip and bus comparison.

---
 drivers/net/wireless/realtek/rtw88/main.h | 7 +++++++
 1 file changed, 7 insertions(+)

index c6e981ba7986..8f86f7c12de5 100644
@@ -194,6 +194,7 @@ enum rtw_chip_type {
 	RTW_CHIP_TYPE_8723D,
 	RTW_CHIP_TYPE_8821C,
 	RTW_CHIP_TYPE_8703B,
+	RTW_CHIP_TYPE_8723B,
 	RTW_CHIP_TYPE_8821A,
 	RTW_CHIP_TYPE_8812A,
 	RTW_CHIP_TYPE_8814A,

@@ -2194,6 +2195,12 @@ static inline bool rtw_chip_has_tx_stbc(struct rtw_dev *rtwdev)
 	return rtwdev->chip->tx_stbc;
 }
 
+static inline bool rtw_is_8723bs(struct rtw_dev *rtwdev)
+{
+	return rtwdev->chip->id == RTW_CHIP_TYPE_8723B &&
+	       rtwdev->hci.type == RTW_HCI_TYPE_SDIO;
+}
+
 static inline u8 rtw_acquire_macid(struct rtw_dev *rtwdev)
 {
 	unsigned long mac_id;

2.53.0

From: Luka Gejak <[email protected]>

Like the RTL8703B, the RTL8723BS reports receive descriptors with a zero packet length, which the vendor driver drops outright. rtw88 already flags these as having no PSDU for the RTL8703B, so extend the same handling rather than passing an empty frame up.

This is gated on the SDIO interface rather than the chip id, because it has only been observed there; the USB variant is not known to do it.

---
 drivers/net/wireless/realtek/rtw88/rx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

index 01fd299abb7f..fde21c840ac0 100644
@@ -253,10 +253,12 @@ static void rtw_rx_fill_rx_status(struct rtw_dev *rtwdev,
 
 	rtw_rx_addr_match(rtwdev, pkt_stat, hdr);
 
-	/* Rtl8723cs driver checks for size < 14 or size > 8192 and
-	 * simply drops the packet.
+	/*
+	 * Rtl8723cs and rtl8723bs drivers check for size < 14 or size > 8192
+	 * and simply drop the packet.
 	 */
-	if (rtwdev->chip->id == RTW_CHIP_TYPE_8703B && pkt_stat->pkt_len == 0) {
+	if (pkt_stat->pkt_len == 0 &&
+	    (rtwdev->chip->id == RTW_CHIP_TYPE_8703B || rtw_is_8723bs(rtwdev))) {
 		rx_status->flag |= RX_FLAG_NO_PSDU;
 		rtw_dbg(rtwdev, RTW_DBG_RX, "zero length packet");
 	}

2.53.0

By the way, though you have added change log in cover letter. It'd be better to make a copy here (after --- delimiter).

From: Luka Gejak <[email protected]>

Commit c80788f7c5ae ("wifi: rtw88: increase TX report timeout to fix race condition") raised the purge timeout to 2500 ms for the RTL8723DU, because the firmware can stay off channel during background scans for longer than the 500 ms default, which delays the TX reports and lets the purge timer drop the tracking skbs. The host stack then reads the missing status as loss and collapses TCP throughput.

The RTL8723BS runs the same vendor firmware over a slower SDIO host and hits the same race. Testers on ARM SDIO boards see "failed to get tx report from firmware" under load, with the same throughput collapse. Extend the 2500 ms timeout to the RTL8723BS.

---
 drivers/net/wireless/realtek/rtw88/tx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 9d747a060b98..797c1e0402f2 100644
@@ -208,8 +208,9 @@ void rtw_tx_report_enqueue(struct rtw_dev *rtwdev, struct sk_buff *skb, u8 sn)
 	__skb_queue_tail(&tx_report->queue, skb);
 	spin_unlock_irqrestore(&tx_report->q_lock, flags);
 
-	if (rtwdev->chip->id == RTW_CHIP_TYPE_8723D &&
-	    rtwdev->hci.type == RTW_HCI_TYPE_USB)
+	if ((rtwdev->chip->id == RTW_CHIP_TYPE_8723D &&
+	     rtwdev->hci.type == RTW_HCI_TYPE_USB) ||
+	    rtw_is_8723bs(rtwdev))
 		timeout = msecs_to_jiffies(2500);
 
 	mod_timer(&tx_report->purge_timer, jiffies + timeout);

2.53.0

From: Luka Gejak <[email protected]>

The RTL8723BS reports free TX page counts that the generic 8051 path reads back from the chip on every transfer, which is both slow over SDIO and unreliable on this part: the register frequently reads back zero while pages are in fact available. It also gates transmission on a free count in the SDIO output queue, REG_SDIO_OQT_FREE_PG, which rtw88 does not track at all. The vendor driver calls this the OQT free space and never expands the acronym; the register holds the number of further transfers the SDIO output queue can accept, and the chip discards writes that arrive when it has run out.

Mirror the vendor driver and keep the per-queue and public page counts in software, seeded at start and resynchronised from the chip only when the cached counts say there is not enough room. Wait for a free output queue entry before writing, and account for the pages consumed after a successful transfer.

Transfers also have to be padded up to the SDIO block size for this chip rather than using the generic alignment, so size the write separately from the frame and trim the skb back afterwards.

Measured on RTL8723BS hardware against an iperf3 server one hop behind the AP, with the wlan0 byte counters as ground truth. On the generic path the association completes but no data passes at all: TCP and UDP both measure 0 bit/s in either direction. With this patch TCP is 25.3 Mbit/s up and 37.3 Mbit/s down, and UDP is 25.0 Mbit/s up at 0% loss.

---
 drivers/net/wireless/realtek/rtw88/sdio.c | 293 +++++++++++++++++++++-
 drivers/net/wireless/realtek/rtw88/sdio.h |  10 +
 2 files changed, 291 insertions(+), 12 deletions(-)

index 5b40d74b16ee..a8209b9acefd 100644
@@ -20,6 +20,7 @@
 #include "tx.h"
 
 #define RTW_SDIO_INDIRECT_RW_RETRIES			50
+#define RTW_SDIO_OQT_TIMEOUT_MS				1000
 
 static bool rtw_sdio_is_bus_addr(u32 addr)
 {

@@ -548,12 +549,157 @@ static int rtw_sdio_read_port(struct rtw_dev *rtwdev, u8 *buf, size_t count)
 	return ret;
 }
 
+/*
+ * The cached free page counters are a fast path hint only. They are written
+ * from the single threaded TX work and, for H2C and reserved page writes,
+ * from process context, so they are atomic_t; whenever they claim there is
+ * not enough room they are resynchronised from the chip before the caller
+ * gives up, which also absorbs a lost update.
+ */
+static void rtw_sdio_8723bs_store_free_txpg(struct rtw_dev *rtwdev,
+					    u32 free_txpg)
+{
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+
+	atomic_set(&rtwsdio->free_pg_high,
+		   u32_get_bits(free_txpg, BIT_FREE_TXPG_HIGH));
+	atomic_set(&rtwsdio->free_pg_normal,
+		   u32_get_bits(free_txpg, BIT_FREE_TXPG_NORMAL));
+	atomic_set(&rtwsdio->free_pg_low,
+		   u32_get_bits(free_txpg, BIT_FREE_TXPG_LOW));
+	atomic_set(&rtwsdio->free_pg_pub,
+		   u32_get_bits(free_txpg, BIT_FREE_TXPG_PUB));
+}
+
+/*
+ * Refresh the cached counters from the chip. Returns false when the chip
+ * reports no free pages at all, which means the counts cannot be trusted
+ * and the caller has to decide what to do instead.
+ */
+static bool rtw_sdio_8723bs_sync_free_txpg(struct rtw_dev *rtwdev)
+{
+	u32 free_txpg = rtw_read32(rtwdev, REG_SDIO_FREE_TXPG);
+
+	if (!free_txpg)
+		return false;
+
+	rtw_sdio_8723bs_store_free_txpg(rtwdev, free_txpg);
+
+	return true;
+}
+
+/*
+ * Size of the public page pool: whatever the transmit page allocation has
+ * left once the per queue pools are taken out. Fails if the allocation
+ * cannot cover the reserved queues, since the remainder would underflow and
+ * there would be no sensible pool to hand out.
+ */
+static int rtw_sdio_8723bs_pubq_num(struct rtw_dev *rtwdev, u16 *pubq_num)
+{
+	const struct rtw_page_table *pg_tbl = &rtwdev->chip->page_table[0];
+	u16 acq_pg_num = rtwdev->fifo.acq_pg_num;
+	u16 reserved_num;
+
+	reserved_num = pg_tbl->hq_num + pg_tbl->lq_num + pg_tbl->nq_num +
+		       pg_tbl->exq_num + pg_tbl->gapq_num;
+	if (acq_pg_num <= reserved_num) {
+		rtw_err(rtwdev,
+			"no transmit pages left for the public queue: %u of %u reserved\n",
+			reserved_num, acq_pg_num);
+		return -EINVAL;
+	}
+
+	*pubq_num = acq_pg_num - reserved_num;
+
+	return 0;
+}
+
+static int rtw_sdio_8723bs_init_free_txpg(struct rtw_dev *rtwdev)
+{
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+	const struct rtw_page_table *pg_tbl;
+	u16 pubq_num;
+	int ret;
+
+	/* Seed from the page table when the chip has nothing to report yet. */
+	if (!rtw_sdio_8723bs_sync_free_txpg(rtwdev)) {
+		ret = rtw_sdio_8723bs_pubq_num(rtwdev, &pubq_num);
+		if (ret)
+			return ret;
+
+		pg_tbl = &rtwdev->chip->page_table[0];
+		atomic_set(&rtwsdio->free_pg_high, pg_tbl->hq_num);
+		atomic_set(&rtwsdio->free_pg_normal, pg_tbl->nq_num);
+		atomic_set(&rtwsdio->free_pg_low, pg_tbl->lq_num);
+		atomic_set(&rtwsdio->free_pg_pub, pubq_num);
+	}
+
+	atomic_set(&rtwsdio->tx_oqt_free,
+		   rtw_read8(rtwdev, REG_SDIO_OQT_FREE_PG));
+
+	return 0;
+}
+
+/*
+ * Sum of the queue's dedicated counter and the public pool, clamped at zero:
+ * a lost update between the check below and rtw_sdio_8723bs_consume_txpg()
+ * can briefly drive a counter negative, and letting that wrap would hide the
+ * shortage instead of triggering a resync from the chip.
+ */
+static unsigned int rtw_sdio_8723bs_pages_free(struct rtw_dev *rtwdev,
+					       atomic_t *dedicated)
+{
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+	int free;
+
+	free = atomic_read(dedicated) + atomic_read(&rtwsdio->free_pg_pub);
+
+	return free > 0 ? free : 0;
+}
+
+static atomic_t *rtw_sdio_8723bs_free_txpg(struct rtw_dev *rtwdev, u8 queue)
+{
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+
+	switch (queue) {
+	case RTW_TX_QUEUE_VI:
+		return &rtwsdio->free_pg_normal;
+	case RTW_TX_QUEUE_BE:
+	case RTW_TX_QUEUE_BK:
+		return &rtwsdio->free_pg_low;
+	case RTW_TX_QUEUE_BCN:
+	case RTW_TX_QUEUE_H2C:
+	case RTW_TX_QUEUE_HI0:
+	case RTW_TX_QUEUE_MGMT:
+	case RTW_TX_QUEUE_VO:
+		return &rtwsdio->free_pg_high;
+	default:
+		return NULL;
+	}
+}
+
 static int rtw_sdio_check_free_txpg(struct rtw_dev *rtwdev, u8 queue,
 				    size_t count)
 {
 	unsigned int pages_free, pages_needed;
 
-	if (rtw_chip_wcpu_8051(rtwdev)) {
+	if (rtw_is_8723bs(rtwdev)) {
+		atomic_t *dedicated;
+
+		dedicated = rtw_sdio_8723bs_free_txpg(rtwdev, queue);
+		if (!dedicated) {
+			rtw_warn(rtwdev, "Unknown mapping for queue %u\n", queue);
+			return -EINVAL;
+		}
+
+		pages_free = rtw_sdio_8723bs_pages_free(rtwdev, dedicated);
+		pages_needed = DIV_ROUND_UP(count, rtwdev->chip->page_size);
+		if (pages_needed <= pages_free)
+			return 0;
+
+		rtw_sdio_8723bs_sync_free_txpg(rtwdev);
+		pages_free = rtw_sdio_8723bs_pages_free(rtwdev, dedicated);
+	} else if (rtw_chip_wcpu_8051(rtwdev)) {
 		u32 free_txpg;
 
 		free_txpg = rtw_sdio_read32(rtwdev, REG_SDIO_FREE_TXPG);

@@ -632,44 +778,124 @@ static int rtw_sdio_check_free_txpg(struct rtw_dev *rtwdev, u8 queue,
 	return 0;
 }
 
+static int rtw_sdio_8723bs_wait_tx_oqt(struct rtw_dev *rtwdev)
+{
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+	unsigned long deadline;
+	u8 free;
+
+	if (atomic_add_unless(&rtwsdio->tx_oqt_free, -1, 0))
+		return 0;
+
+	deadline = jiffies + msecs_to_jiffies(RTW_SDIO_OQT_TIMEOUT_MS);
+	do {
+		free = rtw_read8(rtwdev, REG_SDIO_OQT_FREE_PG);
+		if (free) {
+			atomic_set(&rtwsdio->tx_oqt_free, free - 1);
+			return 0;
+		}
+		usleep_range(1000, 2000);
+	} while (time_before(jiffies, deadline));
+
+	return -EBUSY;
+}
+
+static void rtw_sdio_8723bs_consume_txpg(struct rtw_dev *rtwdev, u8 queue,
+					 unsigned int pages)
+{
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+	atomic_t *dedicated;
+	unsigned int taken;
+	int free;
+
+	dedicated = rtw_sdio_8723bs_free_txpg(rtwdev, queue);
+	if (!dedicated)
+		return;
+
+	free = atomic_read(dedicated);
+	taken = min_t(unsigned int, pages, free > 0 ? free : 0);
+	atomic_sub(taken, dedicated);
+
+	pages -= taken;
+	if (pages && atomic_sub_return(pages, &rtwsdio->free_pg_pub) < 0)
+		atomic_set(&rtwsdio->free_pg_pub, 0);
+}
+
 static int rtw_sdio_write_port(struct rtw_dev *rtwdev, struct sk_buff *skb,
 			       enum rtw_tx_queue_type queue)
 {
 	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+	bool rtl8723bs = rtw_is_8723bs(rtwdev);
+	unsigned int orig_len = skb->len;
+	unsigned int pages;
+	size_t write_size;
 	bool bus_claim;
 	size_t txsize;
 	u32 txaddr;
 	int ret;
 
-	txaddr = rtw_sdio_get_tx_addr(rtwdev, skb->len, queue);
-	if (!txaddr)
-		return -EINVAL;
+	if (rtl8723bs) {
+		txsize = round_up(orig_len, 4);
+		write_size = txsize > RTW_SDIO_BLOCK_SIZE ?
+			     round_up(txsize, RTW_SDIO_BLOCK_SIZE) : txsize;
+	} else {
+		txsize = sdio_align_size(rtwsdio->sdio_func, orig_len);
+		write_size = txsize;
+	}
 
-	txsize = sdio_align_size(rtwsdio->sdio_func, skb->len);
+	if (write_size > orig_len) {
+		unsigned int padding = write_size - orig_len;
+
+		if (skb_tailroom(skb) < padding) {
+			ret = pskb_expand_head(skb, 0,
+					       padding - skb_tailroom(skb),
+					       GFP_KERNEL);
+			if (ret)
+				return ret;
+		}
+		skb_put_zero(skb, padding);
+	}
+
+	txaddr = rtw_sdio_get_tx_addr(rtwdev, txsize, queue);
+	if (!txaddr) {
+		ret = -EINVAL;
+		goto out_trim;
+	}
 
 	ret = rtw_sdio_check_free_txpg(rtwdev, queue, txsize);
 	if (ret)
-		return ret;
+		goto out_trim;
+
+	if (rtl8723bs) {
+		ret = rtw_sdio_8723bs_wait_tx_oqt(rtwdev);
+		if (ret)
+			goto out_trim;
+	}
 
 	if (!IS_ALIGNED((unsigned long)skb->data, RTW_SDIO_DATA_PTR_ALIGN))
 		rtw_warn(rtwdev, "Got unaligned SKB in %s() for queue %u\n",
 			 __func__, queue);
 
 	bus_claim = rtw_sdio_bus_claim_needed(rtwsdio);
-
 	if (bus_claim)
 		sdio_claim_host(rtwsdio->sdio_func);
-
-	ret = sdio_memcpy_toio(rtwsdio->sdio_func, txaddr, skb->data, txsize);
-
+	ret = sdio_memcpy_toio(rtwsdio->sdio_func, txaddr, skb->data,
+			       write_size);
 	if (bus_claim)
 		sdio_release_host(rtwsdio->sdio_func);
 
-	if (ret)
+	if (ret) {
 		rtw_warn(rtwdev,
 			 "Failed to write %zu byte(s) to SDIO port 0x%08x",
-			 txsize, txaddr);
+			 write_size, txaddr);
+	} else if (rtl8723bs) {
+		pages = DIV_ROUND_UP(txsize, rtwdev->chip->page_size);
+		rtw_sdio_8723bs_consume_txpg(rtwdev, queue, pages);
+	}
 
+out_trim:
+	if (write_size > orig_len)
+		skb_trim(skb, orig_len);
 	return ret;
 }
 

@@ -749,8 +975,51 @@ static int rtw_sdio_setup(struct rtw_dev *rtwdev)
 	return 0;
 }
 
+/*
+ * Reprogram the queue page allocation if the chip came up with none. This is
+ * a repair path, not part of the normal start sequence: a non-zero free page
+ * count means the allocation latched during power on and must be left alone,
+ * and without a transmit page pool there is nothing to divide up either.
+ */
+static int rtw_sdio_8723bs_check_rqpn(struct rtw_dev *rtwdev)
+{
+	const struct rtw_chip_info *chip = rtwdev->chip;
+	struct rtw_fifo_conf *fifo = &rtwdev->fifo;
+	const struct rtw_page_table *pg_tbl;
+	u32 free_txpg;
+	u16 pubq_num;
+	int ret;
+
+	free_txpg = rtw_read32(rtwdev, REG_SDIO_FREE_TXPG);
+	if (free_txpg || !fifo->acq_pg_num)
+		return 0;
+
+	ret = rtw_sdio_8723bs_pubq_num(rtwdev, &pubq_num);
+	if (ret)
+		return ret;
+
+	pg_tbl = &chip->page_table[0];
+	rtw_write32(rtwdev, REG_RQPN_NPQ,
+		    BIT_RQPN_NE(pg_tbl->nq_num, pg_tbl->exq_num));
+	rtw_write32(rtwdev, REG_RQPN,
+		    BIT_RQPN_HLP(pg_tbl->hq_num, pg_tbl->lq_num, pubq_num));
+
+	return 0;
+}
+
 static int rtw_sdio_start(struct rtw_dev *rtwdev)
 {
+	if (rtw_is_8723bs(rtwdev)) {
+		int ret = rtw_sdio_8723bs_check_rqpn(rtwdev);
+
+		if (ret)
+			return ret;
+
+		ret = rtw_sdio_8723bs_init_free_txpg(rtwdev);
+		if (ret)
+			return ret;
+	}
+
 	rtw_sdio_enable_rx_aggregation(rtwdev);
 	rtw_sdio_enable_interrupt(rtwdev);
 

index 457e8b02380e..5b000cb8fac2 100644
@@ -86,6 +86,10 @@
 #define REG_SDIO_OQT_FREE_PG			(SDIO_LOCAL_OFFSET + 0x001E)
 /* Free Tx Buffer Page */
 #define REG_SDIO_FREE_TXPG			(SDIO_LOCAL_OFFSET + 0x0020)
+#define BIT_FREE_TXPG_HIGH			GENMASK(7, 0)
+#define BIT_FREE_TXPG_NORMAL			GENMASK(15, 8)
+#define BIT_FREE_TXPG_LOW			GENMASK(23, 16)
+#define BIT_FREE_TXPG_PUB			GENMASK(31, 24)
 /* HCI Current Power Mode 1 */
 #define REG_SDIO_HCPWM1				(SDIO_LOCAL_OFFSET + 0x0024)
 /* HCI Current Power Mode 2 */

@@ -159,6 +163,12 @@ struct rtw_sdio {
 	struct workqueue_struct *txwq;
 	struct rtw_sdio_work_data *tx_handler_data;
 	struct sk_buff_head tx_queue[RTK_MAX_TX_QUEUE_NUM];
+
+	atomic_t free_pg_high;
+	atomic_t free_pg_normal;
+	atomic_t free_pg_low;
+	atomic_t free_pg_pub;
+	atomic_t tx_oqt_free;
 };
 
 extern const struct dev_pm_ops rtw_sdio_pm_ops;

2.53.0

From: Luka Gejak <[email protected]>

Enable the existing RX aggregation setup for this chip and select the larger DMA burst count it needs. The RTL8723BS does not raise CPWM1, so leave that source out of its interrupt mask, and set the SDIO TX control bit the vendor driver uses to have transfers always recognised.

The chip was also seen to keep raising the interrupt after resume when undefined status bits were written back on acknowledgment, so acknowledge only the bits this driver defines. That observation dates from bring up and I cannot re-measure it: the test machine only offers s2idle and does not reliably return from it, so the resume path is not exercised here. The change is scoped to this chip; the other SDIO parts keep writing the status word back unchanged.

---
 drivers/net/wireless/realtek/rtw88/sdio.c | 28 ++++++++++++++++++++++-
 drivers/net/wireless/realtek/rtw88/sdio.h |  9 ++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

index a8209b9acefd..eaec776dd943 100644
@@ -903,7 +903,11 @@ static void rtw_sdio_init(struct rtw_dev *rtwdev)
 {
 	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 
-	rtwsdio->irq_mask = REG_SDIO_HIMR_RX_REQUEST | REG_SDIO_HIMR_CPWM1;
+	if (rtw_is_8723bs(rtwdev))
+		rtwsdio->irq_mask = REG_SDIO_HIMR_RX_REQUEST;
+	else
+		rtwsdio->irq_mask = REG_SDIO_HIMR_RX_REQUEST |
+				    REG_SDIO_HIMR_CPWM1;
 }
 
 static void rtw_sdio_enable_rx_aggregation(struct rtw_dev *rtwdev)

@@ -911,6 +915,7 @@ static void rtw_sdio_enable_rx_aggregation(struct rtw_dev *rtwdev)
 	u8 size, timeout;
 
 	switch (rtwdev->chip->id) {
+	case RTW_CHIP_TYPE_8723B:
 	case RTW_CHIP_TYPE_8703B:
 	case RTW_CHIP_TYPE_8821A:
 	case RTW_CHIP_TYPE_8812A:

@@ -938,6 +943,8 @@ static void rtw_sdio_enable_rx_aggregation(struct rtw_dev *rtwdev)
 		    FIELD_PREP(BIT_DMA_AGG_TO_V1, timeout));
 
 	rtw_write8_set(rtwdev, REG_RXDMA_MODE, BIT_DMA_MODE);
+	if (rtw_is_8723bs(rtwdev))
+		rtw_write8_mask(rtwdev, REG_RXDMA_MODE, BIT_DMA_BURST_CNT, 0x3);
 }
 
 static void rtw_sdio_enable_interrupt(struct rtw_dev *rtwdev)

@@ -1009,6 +1016,8 @@ static int rtw_sdio_8723bs_check_rqpn(struct rtw_dev *rtwdev)
 
 static int rtw_sdio_start(struct rtw_dev *rtwdev)
 {
+	u32 clear;
+
 	if (rtw_is_8723bs(rtwdev)) {
 		int ret = rtw_sdio_8723bs_check_rqpn(rtwdev);
 

@@ -1021,6 +1030,13 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
 	}
 
 	rtw_sdio_enable_rx_aggregation(rtwdev);
+
+	if (rtw_is_8723bs(rtwdev)) {
+		clear = rtw_read32(rtwdev, REG_SDIO_HISR) & RTW_SDIO_HISR_CLEAR_MASK;
+		if (clear)
+			rtw_write32(rtwdev, REG_SDIO_HISR, clear);
+	}
+
 	rtw_sdio_enable_interrupt(rtwdev);
 
 	return 0;

@@ -1100,6 +1116,8 @@ static void rtw_sdio_interface_cfg(struct rtw_dev *rtwdev)
 
 	val = rtw_read32(rtwdev, REG_SDIO_TX_CTRL);
 	val &= 0xfff8;
+	if (rtw_is_8723bs(rtwdev))
+		val |= BIT_SDIO_TX_CTRL_ALWAYS_RECOGNIZE;
 	rtw_write32(rtwdev, REG_SDIO_TX_CTRL, val);
 }
 

@@ -1364,6 +1382,14 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
 		rtw_sdio_rx_isr(rtwdev);
 	}
 
+	/*
+	 * RTL8723BS keeps raising the interrupt after resume if undefined
+	 * status bits are written back, so acknowledge only the bits this
+	 * driver defines. Other chips keep the existing behaviour.
+	 */
+	if (rtw_is_8723bs(rtwdev))
+		hisr &= RTW_SDIO_HISR_CLEAR_MASK;
+
 	rtw_write32(rtwdev, REG_SDIO_HISR, hisr);
 
 	rtwsdio->irq_thread = NULL;

index 5b000cb8fac2..7474a7511811 100644
@@ -22,6 +22,7 @@
 
 /* SDIO Tx Control */
 #define REG_SDIO_TX_CTRL			(SDIO_LOCAL_OFFSET + 0x0000)
+#define BIT_SDIO_TX_CTRL_ALWAYS_RECOGNIZE	BIT(4)
 
 /*SDIO status timeout*/
 #define REG_SDIO_TIMEOUT			(SDIO_LOCAL_OFFSET + 0x0002)

@@ -77,6 +78,14 @@
 /* the following two are RTL8188 SDIO Specific */
 #define REG_SDIO_HISR_MCU_ERR			BIT(28)
 #define REG_SDIO_HISR_TSF_BIT32_TOGGLE		BIT(29)
+#define RTW_SDIO_HISR_CLEAR_MASK		\
+	(REG_SDIO_HISR_TXERR | REG_SDIO_HISR_RXERR | \
+	 REG_SDIO_HISR_TXFOVW | REG_SDIO_HISR_RXFOVW | \
+	 REG_SDIO_HISR_TXBCNOK | REG_SDIO_HISR_TXBCNERR | \
+	 REG_SDIO_HISR_C2HCMD | REG_SDIO_HISR_CPWM1 | \
+	 REG_SDIO_HISR_CPWM2 | REG_SDIO_HISR_HSISR_IND | \
+	 REG_SDIO_HISR_GTINT3_IND | REG_SDIO_HISR_GTINT4_IND | \
+	 REG_SDIO_HISR_PSTIMEOUT | REG_SDIO_HISR_OCPINT)
 
 /* HCI Current Power Mode */
 #define REG_SDIO_HCPWM				(SDIO_LOCAL_OFFSET + 0x0019)

2.53.0

From: Luka Gejak <[email protected]>

Two problems show up on RTL8723BS uplink. The per-AC software FIFO is unbounded, so mac80211 keeps handing frames down until latency collapses under load. And when a transfer cannot be completed the queue is simply abandoned for that pass, which stalls the AC until something else kicks the worker.

Stop the mac80211 queue once a data AC fills past a high watermark and wake it from the drain path when it falls back to a low one. Convert the TX work item to a delayed work and re-arm it when a transfer fails for a reason that can clear on its own, so it is retried rather than the AC abandoned, and cancel the work on teardown.

Retrying matters once the queue can be stopped. A stopped queue is handed no further frames, so nothing else would kick the worker, and the AC would stay stopped for good with the link still up and receive unaffected. The two retried cases, a transmit page or output queue shortage and a failed skb expansion, are also the two that fail silently; the rest are logged where they happen, so they are visible rather than an unexplained hang, and they keep the existing behaviour rather than being retried indefinitely.

Measured on RTL8723BS hardware, uplink goes from 11.9 Mbit/s with 204 TCP retransmits to 20.1 Mbit/s with 2.

---
 drivers/net/wireless/realtek/rtw88/sdio.c | 139 ++++++++++++++++++++--
 drivers/net/wireless/realtek/rtw88/sdio.h |   3 +-
 2 files changed, 131 insertions(+), 11 deletions(-)

index eaec776dd943..1de104870572 100644
@@ -22,6 +22,16 @@
 #define RTW_SDIO_INDIRECT_RW_RETRIES			50
 #define RTW_SDIO_OQT_TIMEOUT_MS				1000
 
+/*
+ * 8723BS SDIO TX FIFO back-pressure watermarks: stop the mac80211 queue once
+ * the per-AC software FIFO fills past the high watermark, and wake it from the
+ * TX drain path once it falls back to the low one. Bounds the queueing latency
+ * that otherwise causes uplink bufferbloat / congestion collapse.
+ */
+#define RTW_SDIO_TX_FIFO_HIWATER			16
+#define RTW_SDIO_TX_FIFO_LOWATER			8
+#define RTW_SDIO_TX_RETRY_DELAY			msecs_to_jiffies(1)
+
 static bool rtw_sdio_is_bus_addr(u32 addr)
 {
 	return !!(addr & RTW_SDIO_BUS_MSK);

@@ -1100,7 +1110,11 @@ static void rtw_sdio_tx_kick_off(struct rtw_dev *rtwdev)
 {
 	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 
-	queue_work(rtwsdio->txwq, &rtwsdio->tx_handler_data->work);
+	/*
+	 * A retry may already be pending with a delay; re-arm it so a newly
+	 * queued frame is not held back by it.
+	 */
+	mod_delayed_work(rtwsdio->txwq, &rtwsdio->tx_handler_data->work, 0);
 }
 
 static void rtw_sdio_link_ps(struct rtw_dev *rtwdev, bool enter)

@@ -1209,6 +1223,50 @@ static int rtw_sdio_write_data_h2c(struct rtw_dev *rtwdev, u8 *buf, u32 size)
 	return rtw_sdio_write_data(rtwdev, &pkt_info, skb, RTW_TX_QUEUE_H2C);
 }
 
+/*
+ * Back-pressure on the data ACs (BK/BE/VI/VO): once the software FIFO fills
+ * past the high watermark, stop the corresponding mac80211 queue so it stops
+ * handing frames down, which bounds the queueing latency. The queue is woken
+ * again from the TX drain path once the FIFO falls back to the low watermark.
+ */
+static void rtw_sdio_8723bs_stop_tx_queue(struct rtw_dev *rtwdev,
+					  enum rtw_tx_queue_type queue,
+					  u16 q_map)
+{
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+
+	if (!rtw_is_8723bs(rtwdev) || queue >= RTW_TX_QUEUE_BCN)
+		return;
+
+	if (rtwsdio->tx_queue_stopped[queue])
+		return;
+
+	if (skb_queue_len(&rtwsdio->tx_queue[queue]) < RTW_SDIO_TX_FIFO_HIWATER)
+		return;
+
+	rtwsdio->tx_queue_stopped[queue] = true;
+	ieee80211_stop_queue(rtwdev->hw, q_map);
+}
+
+static void rtw_sdio_8723bs_wake_tx_queue(struct rtw_dev *rtwdev,
+					  enum rtw_tx_queue_type queue,
+					  u16 q_map)
+{
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+
+	if (!rtw_is_8723bs(rtwdev) || queue >= RTW_TX_QUEUE_BCN)
+		return;
+
+	if (!rtwsdio->tx_queue_stopped[queue])
+		return;
+
+	if (skb_queue_len(&rtwsdio->tx_queue[queue]) > RTW_SDIO_TX_FIFO_LOWATER)
+		return;
+
+	rtwsdio->tx_queue_stopped[queue] = false;
+	ieee80211_wake_queue(rtwdev->hw, q_map);
+}
+
 static int rtw_sdio_tx_write(struct rtw_dev *rtwdev,
 			     struct rtw_tx_pkt_info *pkt_info,
 			     struct sk_buff *skb)

@@ -1224,6 +1282,8 @@ static int rtw_sdio_tx_write(struct rtw_dev *rtwdev,
 
 	skb_queue_tail(&rtwsdio->tx_queue[queue], skb);
 
+	rtw_sdio_8723bs_stop_tx_queue(rtwdev, queue, skb_get_queue_mapping(skb));
+
 	return 0;
 }
 

@@ -1526,43 +1586,99 @@ static void rtw_sdio_indicate_tx_status(struct rtw_dev *rtwdev,
 	ieee80211_tx_status_irqsafe(hw, skb);
 }
 
-static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
-				      enum rtw_tx_queue_type queue)
+/*
+ * Send one frame from @queue. Returns 0 when a frame was written, 1 when the
+ * queue was empty and a negative errno when the write failed, in which case
+ * the frame is put back at the head of the queue.
+ */
+static int rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
+				     enum rtw_tx_queue_type queue)
 {
 	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 	struct sk_buff *skb;
+	u16 q_map;
 	int ret;
 
 	skb = skb_dequeue(&rtwsdio->tx_queue[queue]);
 	if (!skb)
-		return;
+		return 1;
 
+	q_map = skb_get_queue_mapping(skb);
 	ret = rtw_sdio_write_port(rtwdev, skb, queue);
 	if (ret) {
 		skb_queue_head(&rtwsdio->tx_queue[queue], skb);
-		return;
+		return ret;
 	}
 
 	rtw_sdio_indicate_tx_status(rtwdev, skb);
+
+	rtw_sdio_8723bs_wake_tx_queue(rtwdev, queue, q_map);
+
+	return 0;
+}
+
+static void rtw_sdio_reschedule_tx_work(struct rtw_dev *rtwdev,
+					struct rtw_sdio_work_data *work_data,
+					unsigned long delay)
+{
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+
+	queue_delayed_work(rtwsdio->txwq, &work_data->work, delay);
 }
 
 static void rtw_sdio_tx_handler(struct work_struct *work)
 {
 	struct rtw_sdio_work_data *work_data =
-		container_of(work, struct rtw_sdio_work_data, work);
+		container_of(to_delayed_work(work), struct rtw_sdio_work_data,
+			     work);
 	struct rtw_sdio *rtwsdio;
 	struct rtw_dev *rtwdev;
-	int limit, queue;
+	int limit, queue, ret;
+	bool rtl8723bs;
 
 	rtwdev = work_data->rtwdev;
 	rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+	rtl8723bs = rtw_is_8723bs(rtwdev);
 
 	if (!rtw_fw_feature_check(&rtwdev->fw, FW_FEATURE_TX_WAKE))
 		rtw_sdio_deep_ps_leave(rtwdev);
 
 	for (queue = RTK_MAX_TX_QUEUE_NUM - 1; queue >= 0; queue--) {
 		for (limit = 0; limit < 1000; limit++) {
-			rtw_sdio_process_tx_queue(rtwdev, queue);
+			ret = rtw_sdio_process_tx_queue(rtwdev, queue);
+			if (ret > 0)
+				break;
+
+			if (ret < 0) {
+				/*
+				 * A page or output queue shortage and a failed
+				 * skb expansion are both transient, and the
+				 * frame is still queued, so come back for it.
+				 * That matters once the queue can be stopped:
+				 * a stopped queue is handed no further frames,
+				 * so nothing else would kick this work item and
+				 * the queue would stay stopped for good. The
+				 * remaining errors cannot succeed on a retry
+				 * and each log where they happen.
+				 */
+				if (rtl8723bs &&
+				    (ret == -EBUSY || ret == -ENOMEM)) {
+					rtw_sdio_reschedule_tx_work(rtwdev, work_data,
+								    RTW_SDIO_TX_RETRY_DELAY);
+					return;
+				}
+				break;
+			}
+
+			/*
+			 * Restart from the highest priority queue after every
+			 * management frame so the join sequence is not held up
+			 * behind a data backlog.
+			 */
+			if (rtl8723bs && queue == RTW_TX_QUEUE_MGMT) {
+				rtw_sdio_reschedule_tx_work(rtwdev, work_data, 0);
+				return;
+			}
 
 			if (skb_queue_empty(&rtwsdio->tx_queue[queue]))
 				break;

@@ -1589,14 +1705,16 @@ static int rtw_sdio_init_tx(struct rtw_dev *rtwdev)
 		return -ENOMEM;
 	}
 
-	for (i = 0; i < RTK_MAX_TX_QUEUE_NUM; i++)
+	for (i = 0; i < RTK_MAX_TX_QUEUE_NUM; i++) {
 		skb_queue_head_init(&rtwsdio->tx_queue[i]);
+		rtwsdio->tx_queue_stopped[i] = false;
+	}
 	rtwsdio->tx_handler_data = kmalloc_obj(*rtwsdio->tx_handler_data);
 	if (!rtwsdio->tx_handler_data)
 		goto err_destroy_wq;
 
 	rtwsdio->tx_handler_data->rtwdev = rtwdev;
-	INIT_WORK(&rtwsdio->tx_handler_data->work, rtw_sdio_tx_handler);
+	INIT_DELAYED_WORK(&rtwsdio->tx_handler_data->work, rtw_sdio_tx_handler);
 
 	return 0;
 

@@ -1610,6 +1728,7 @@ static void rtw_sdio_deinit_tx(struct rtw_dev *rtwdev)
 	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 	int i;
 
+	cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
 	destroy_workqueue(rtwsdio->txwq);
 	kfree(rtwsdio->tx_handler_data);
 

index 7474a7511811..cec3dd1c4b2e 100644
@@ -156,7 +156,7 @@ struct rtw_sdio_tx_data {
 };
 
 struct rtw_sdio_work_data {
-	struct work_struct work;
+	struct delayed_work work;
 	struct rtw_dev *rtwdev;
 };
 

@@ -172,6 +172,7 @@ struct rtw_sdio {
 	struct workqueue_struct *txwq;
 	struct rtw_sdio_work_data *tx_handler_data;
 	struct sk_buff_head tx_queue[RTK_MAX_TX_QUEUE_NUM];
+	bool tx_queue_stopped[RTK_MAX_TX_QUEUE_NUM];
 
 	atomic_t free_pg_high;
 	atomic_t free_pg_normal;

2.53.0

Hi Ping-Ke,

Thanks for the acks, and for catching the wrapper.

You are right, and I misread your v4 comment. You asked me to move the chunk into a function and I produced a one line wrapper around queue_delayed_work() instead, which hides the API for no gain. It is gone in v6.

Done. The conditions now live in rtw_sdio_8723bs_reschedule_tx(), which decides whether the work should run again and says why, so the handler loop is just:

	ret = rtw_sdio_process_tx_queue(rtwdev, queue);
	if (ret > 0)
		break;

	if (rtw_sdio_8723bs_reschedule_tx(rtwdev, work_data, queue, ret))
		return;

	if (skb_queue_empty(&rtwsdio->tx_queue[queue]))
		break;

While rewriting that I found something I had got wrong in v4 and v5, and it is worth pointing out because it was not limited to this chip. Both versions had an unconditional

	if (ret < 0)
		break;

in that loop. Upstream requeues the frame on a failed transfer and lets the loop retry, so that break quietly changed every other SDIO part to give up after the first failure. That contradicted the scope claim in my own cover letter. It is dropped in v6: the two errors this chip needs to retry are handled in the helper before that point, and everything else keeps the existing behaviour, so the other parts are untouched again.

Best regards,
Luka Gejak

It looks much better. :)

I didn't get this point. I'll review carefully on this part in v6.

Thanks for pointing this.

If you have another SDIO chip, such as RTL8723CS, please verify your patches on another real hardware, because it is not possible to catch all flaws by reviewer (like me). Using real hardware with a simple throughput test is more important.

Ping-Ke

Hi Ping-Ke,

Easier to see side by side, so here it is in full.

Upstream ignores the return value of rtw_sdio_process_tx_queue():

        rtw_sdio_process_tx_queue(rtwdev, queue);

        if (skb_queue_empty(&rtwsdio->tx_queue[queue]))
                break;

and that function puts the frame back when the write fails:

        ret = rtw_sdio_write_port(rtwdev, skb, queue);
        if (ret) {
                skb_queue_head(&rtwsdio->tx_queue[queue], skb);
                return;
        }

So after a failure the queue is not empty, the loop does not break, and the transfer is retried, up to the limit of 1000. rtw_sdio_write_port() returning -EBUSY from rtw_sdio_check_free_txpg() is the ordinary case here, and upstream just spins until pages free up.

v4 and v5 put this in the same loop:

        ret = rtw_sdio_process_tx_queue(rtwdev, queue);
        if (ret > 0)
                break;

        if (ret < 0) {
                if (rtl8723bs && (ret == -EBUSY || ret == -ENOMEM)) {
                        rtw_sdio_reschedule_tx_work(...);
                        return;
                }
                break;
        }

The inner condition is chip gated. The break is not. On every other SDIO part rtl8723bs is false, so any error, including that ordinary -EBUSY, now leaves the loop and abandons the queue for the rest of the pass instead of retrying it. That is the change I did not intend, and it was not in v3.

v6 has no break there at all:

        ret = rtw_sdio_process_tx_queue(rtwdev, queue);
        if (ret > 0)
                break;

        if (rtw_sdio_8723bs_reschedule_tx(rtwdev, work_data, queue, ret))
                return;

        if (skb_queue_empty(&rtwsdio->tx_queue[queue]))
                break;

rtw_sdio_8723bs_reschedule_tx() returns false on its first line for anything that is not an RTL8723BS, so the other parts get process, requeue on failure, queue not empty, retry, which is upstream's flow.

Agreed, and you are right to ask. I do not have another SDIO part. The only SDIO hardware I have is the RTL8723BS, so I cannot give you a measured result on another chip, and I would rather say so than imply coverage I do not have. This is exactly the bug you would expect that gap to produce.

So that a reviewer knows where to look, after v6 the other SDIO parts see three changes, all structural:

- rtw_sdio_tx_kick_off() uses mod_delayed_work(..., 0) instead of queue_work(). Only the RTL8723BS ever arms a delay, so for the others the work is always queued immediately, as before.
- INIT_WORK() becomes INIT_DELAYED_WORK() on the same handler.
- rtw_sdio_deinit_tx() gains cancel_delayed_work_sync() before destroy_workqueue(), needed once the item is a delayed work, and a no-op where no timer is ever armed.

Everything else is behind rtw_is_8723bs().

If anyone on the list has an RTL8723CS, RTL8822BS or RTL8821CS to hand, the full tree is at:

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

and a plain iperf3 run either way is all it needs. I will take any result on another part over my own reading of the diff.

Best regards,
Luka Gejak

If you can add these notes along my comments, it will be easier to me. Not only your reply, but also I can reference source code again to reconsider my comments if needed.

Hi Ping-Ke,

Done. Every patch in v6 carries its own changelog after the --- delimiter, so the notes sit next to the code they describe. I used git notes for it, so they stay attached if I have to regenerate the series.

Your acks on patches 1 to 5 are carried over. Only patch 6 changed, and I have replied separately on that thread.

The rework there is behaviour preserving for this chip, so I ran the short regression pass rather than a full characterisation: association, idle pings with power save on, bidirectional load, throughput, scans and reconnects. Everything matches v5 and the log is clean.

v6 follows shortly.

Best regards,
Luka Gejak

...

Hi Ping-Ke,

Please do not apply patches 4 and 6 of v6. I found real bugs in both after posting and a v7 is on the way.

Patch 6 touches the skb after handing it to the TX worker:

        skb_queue_tail(&rtwsdio->tx_queue[queue], skb);

        rtw_sdio_8723bs_stop_tx_queue(rtwdev, queue,
                                      skb_get_queue_mapping(skb));

The worker can dequeue, transmit and free that skb before the second statement runs, so reading it there is a use after free. v7 reads the queue index into a local before the enqueue.

Patch 4 concerns you more than me. It passed the padded size to rtw_sdio_get_tx_addr(), where upstream passes skb->len. That value is encoded into the CMD53 address as the transfer length, so on every other SDIO chip the encoded length changed whenever sdio_align_size() padded. For the RTL8723BS both expressions are the same value, so the change bought this chip nothing and only affected the other parts. The padding itself also ran on the generic path, giving those chips an allocation and an -ENOMEM path they did not have. v7 restores skb->len and gates the padding on the chip.

The other v7 changes, all in patches 4 and 6:

- the free page check, the output queue wait and the accounting are serialised. The TX worker and the H2C path both reach rtw_sdio_write_port(), and two writers could each pass the checks and claim the same pages and output queue entry, after which the chip discards one transfer silently. The vendor driver has one transmit thread and never needed this.
- the padding called skb_put_zero() after checking only tailroom, so a cloned skb had its shared buffer written. It uses __skb_pad() now.
- the back-pressure stop could race the drain: the worker could empty the queue while the stop was still in progress, see the flag clear, and never wake it, leaving an access category stopped with nothing left to wake it.

Patches 1, 2, 3 and 5 are unchanged and keep your acks; patch 5 shifts only by hunk offsets because patch 4 grew above it. I dropped your ack from patch 4, since the rework is too substantive to carry it.

Most of these came from Sashiko's automated review of v6. I checked each against the code before acting on it, and one of its eight findings did not hold up.

v7 follows shortly. It is hardware tested: 60 idle pings at 0% loss with power save on, bidirectional load with no stall, 19.6 Mbit/s up and 36.8 down, UDP at 0% loss, three scans, three reconnects and a clean log.

I am posting it rather than waiting for your comments on v6, since patches 4 and 6 changed enough that reviewing the v6 versions would not be a good use of your time.

Best regards,
Luka Gejak

Hi Luka,

I had done some basic testing with v5, and had left a v6 to build overnight. I'll grab this shortly and do a build with v7 and do some testing.It was generally looking stable on the Pine64+ board I was using for testing.

I think I may also have a device(s) with a 8723cs and/or a 8723ds I can possibly test with, I'll see if I can remember/find it over the weekend and get it running for some testing of that too.

Peter

Hi Peter,

Thanks a lot for testing. In my testing changes in v6 and v7 didn't impact performance and v7 should be as stable if not more stable then v5 and v6.

That would be nice, so we can see if other chips are affected, as Ping-Ke said.

Best regards,
Luka Gejak

Hi Luka,

I've literally just booted v7 so it'll take a bit to test it more.

In v5/v6 I saw a bunch of these once it was running for a while, it seems to be stable though.

[ 5594.434616] rtw88_8723bs mmc0:0001:1: failed to get tx report from firmware
[ 5692.223363] rtw88_8723bs mmc0:0001:1: failed to get tx report from firmware
[ 5747.389804] rtw88_8723bs mmc0:0001:1: failed to get tx report from firmware
[ 5767.421101] rtw88_8723bs mmc0:0001:1: failed to get tx report from firmware

Testing on iperf3 I'm getting just over 2Mbits/sec both TX/RX overall.

I'll try and work out what I have over the weekend and will do some testing against v7 on both devices.

Peter

I think these are generic rtw88, not introduced by this driver, but I will investigate it either way.

Hmm, this seems low, are those speeds on staging driver too, as in my testing they are around 30-40 down and ~20 up.

Thanks again.

Best regards,
Luka Gejak

Thanks for your heads-up.
I'll arrange my time to review v7 as soon as possible.

Ping-Ke