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

[Luka Gejak]

Hi Ping-Ke,

On August 25, 2026 8:36:30 AM GMT+02:00, Ping-Ke Shih <pkshih@xxxxxxxxxxx> wrote:

> With WRITE_ONCE() and READ_ONCE(), I think it will concurrency work
> well. Did you really encounter problems with smp_mb()?

Not from a failure I hit, no. It came out of reading the code, and I think
it is still needed, so let me lay out the case rather than just assert it.

The flag alone is not enough because of this order:

  producer                              worker
  skb_queue_tail()
  reads len, sees >= HIWATER
                                        drains the queue to empty
                                        reads the flag, still false,
                                        so it does not wake
  writes the flag true
  ieee80211_stop_queue()

The AC is now stopped with an empty queue and nobody left to wake it. That
is what the re-check under the barrier is for: once the flag is set, read
the length again, and if the worker drained it meanwhile, undo the stop.

For that re-check to mean anything the flag store has to be visible before
the length is re-loaded. READ_ONCE and WRITE_ONCE stop the compiler from
tearing or hoisting either access, but they do not order one against the
other, so without smp_mb() the CPU may still perform the length re-load
before the flag store lands. Then the worker can read the old flag, skip
the wake, and the re-check reads a length that is already stale, which puts
us back at the same stuck AC.

So I have kept it, with the blank line you asked for. If you would rather
drop the flag entirely, both watermark helpers become two-line functions
and the barrier goes with it, since ieee80211_stop_queue() and
_wake_queue() are idempotent and the watermarks alone can decide the state.
I did not do that here because it is a bigger change than the one you asked
about and I would rather not touch the concurrency in this patch without
your say-so.

> Can we move below just before the user?

Moved, though not all the way: rtw_sdio_indicate_tx_status() consumes the
skb, either handing it to rtw_tx_report_enqueue() or to
ieee80211_tx_status_irqsafe(), so reading q_map after that call would be a
use after free. It now sits immediately before it, with a comment saying
why it cannot go further down.

Best regards,
Luka Gejak