On Tue, Jun 03, 2025 at 06:52:37PM +0800, Baochen Qiang wrote:
On 6/2/2025 4:03 PM, Johan Hovold wrote:
No, the barrier is needed between reading the head pointer and accessing descriptor fields, that's what matters.
You can still end up with reading stale descriptor data even when ath11k_hal_srng_dst_get_next_entry() returns non-NULL due to speculation (that's what happens on the X13s).
The fact is that a dma_rmb() does not even prevent speculation, no matter where it is placed, right?
It prevents the speculated load from being used.
If so the whole point of dma_rmb() is to prevent from compiler reordering or CPU reordering, but is it really possible?
The sequence is
1# reading HP srng->u.dst_ring.cached_hp = READ_ONCE(*srng->u.dst_ring.hp_addr);
2# validate HP if (srng->u.dst_ring.tp == srng->u.dst_ring.cached_hp) return NULL;
3# get desc desc = srng->ring_base_vaddr + srng->u.dst_ring.tp;
4# accessing desc ath11k_hal_desc_reo_parse_err(... desc, ...)
Clearly each step depends on the results of previous steps. In this case the compiler/CPU is expected to be smart enough to not do any reordering, isn't it?
Steps 3 and 4 can be done speculatively before the load in step 1 is complete as long as the result is discarded if it turns out not to be needed.
Johan