luka.gejak@linux.dev <luka.gejak@linux.dev> wrote:
> 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.

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,

> 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.

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