Re: [PATCH 2/2] wifi: ath12k: implement custom wake_tx_queue with flow control

[Jose Ignacio Tornos Martinez <jtornosm@redhat.com>]

Hi Tamizh,

Thank you for the review.

> This assignment is wrong and all traffic is incorrectly steered to
> radio[0] as ah->radio is a flexible array, not a pointer

You are right, I will fix it in v2.

> tx_ring/tcl_ring selection should be corrected.

Yes, as also pointed out by Zhi-Jun You in the ath11k review, the ring
selection in wake_tx_queue needs to match dp_tx in a general way.
I will address this for both ath11k and ath12k in v2.

> Do we need this spin_lock_bh?

Yes. Without wake_tx_lock, there is a TOCTOU race between checking ring
space and dequeuing:

CPU A: num_free = 1   (sees 1 free slot)
CPU B: num_free = 1   (sees same slot)
CPU A: dequeue -> dp_tx (succeeds, fills last slot)
CPU B: dequeue -> dp_tx (fails -> -ENOMEM, frame dropped)

The tcl_ring->lock inside dp_tx only protects the descriptor write, not the
check-before-dequeue sequence.

> Do we need this check and spin_lock here? already ath12k_wifi7_dp_tx()
> has this lock and fetches the next entry. Can we check those return
> value here and break the loop?

We need to check before dequeuing. Once ieee80211_tx_dequeue removes the skb
from mac80211, a dp_tx failure means a dropped frame. Checking num_free first
avoids dequeuing when the ring is full, which is the whole point of the flow
control.
In addition, tcl_ring->lock is required by ath12k_hal_srng_src_num_free
itself, which asserts the lock is held.
The brief lock also ensures an accurate count synchronized with dp_tx on other
CPUs.

Best regards,
Jose Ignacio