RE: [PATCH v5 6/6] wifi: rtw88: sdio: add TX back-pressure and retry on page starvation

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

Hi Ping-Ke,

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

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.

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

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