Re: [PATCH] wifi: ath12k: flush REO queue extension descriptors before freeing the qdesc

[Sebastian Salmhofer]

On 9/3/2026 5:07 AM, Baochen Qiang wrote:
>> The faulting addresses always fall at offsets 0x100..0x580 of a
>> retired qdesc and never at 0x000 or 0x080, i.e. exactly the ten
>> extension descriptors and never the queue descriptor or the 1K bitmap
>
> the queue desc offset should be 0x000, and the offset of the first 
extension descriptor
> should at 0x080. However the IOMMU warning starts at 0x100, which 
does not make sense ...

You are right that under the driver's struct layout ext_desc[0] sits at
0x080, so that sentence of the commit message was an interpretation on
my part, not an observed fact. The observed facts, across about ten
days of logs and more than a dozen retired qdescs, are:

- every fault falls at an offset in 0x100..0x580 of a retired
  1536-byte qdesc (bases are 2KB-aligned from kmalloc-2k, so the
  offsets are unambiguous),
- no fault was ever seen at 0x000 or 0x080,
- within one qdesc the faulting lines are 0x80-strided runs, sometimes
  the full 0x100..0x580 sweep, sometimes a subset, and lines of one
  qdesc can be written back in separate bursts minutes apart.

Two explanations would fit that pattern: either the hardware expects
the 1K bitmap descriptor directly after the base queue structure (so
extension descriptors effectively start at 0x100 from the hardware's
point of view), or FLUSH_QUEUE_1K_DESC covers the first 256 bytes of
the queue as one object. I don't have documentation for the REO cache
controller, so I can only report that on QCN9274 hw2.0 with
WLAN.WBE.1.6-01243 the single 1K flush demonstrably leaves everything
from 0x100 upward dirty. Could you check internally what the 1K flush
actually covers on this target, and where the hardware expects the 1K
bitmap? I will reword the commit message to stay purely with the
observed offsets in v2.

>> The writes are triggered by later REO activity, typically
>> a new station association, so they can occur minutes or hours after the
>> memory was freed, and the blocked transactions stall the data path for
>> several seconds.
>
> then what happens? the new sta association succeeds?

Yes, the association succeeds. The IOMMU blocks the writes, so nothing
is corrupted; the user-visible symptom is that the data path stalls
for several seconds around each fault burst, for all connected
stations, which is how this was noticed (periodic freezes of
latency-sensitive traffic). With the fix applied the stalls are gone
along with the faults. Without an IOMMU the same writes would land in
freed memory. I will make that explicit in v2.

>> +    while (off > HAL_LINK_DESC_ALIGN) {
>> +        off -= HAL_LINK_DESC_ALIGN;
>> +        memset(&cmd, 0, sizeof(cmd));
>
> unnecessary cleanup since all required fields are refilled in each 
iteration.

Right, and the same for the one before the base flush - cmd is
zero-initialized and both sites set every field the flush-cache
command reads. Will drop both in v2.

>> +    if (rx_tid->tid != HAL_DESC_REO_NON_QOS_TID)
>> +        cmd.flag |= HAL_REO_CMD_FLG_FLUSH_QUEUE_1K_DESC;
>
> HAL_REO_CMD_FLG_FLUSH_QUEUE_1K_DESC is used to flush all in a single 
cmd. since we switch
> back to the per segment flush, do we still need it?

I kept it deliberately. The per-segment loop covers every 128-byte
line from 0x080 upward, and the flag makes the final base flush cover
whatever the "1K descriptor" object is, so the sequence is correct
under either interpretation above, at no extra cost. Dropping the flag
would restore the exact pre-b706fb4e580b sequence; that may well be
sufficient, but if the 1K-window queue is cached as an object larger
than 128 bytes on this target, a plain flush of the base address might
not cover its tail, and I cannot rule that out from here. If you can
confirm that plain 128-byte flushes fully cover a 1K-window queue
descriptor, I am happy to drop the flag in v2.

I will send v2 with the memsets removed and the commit message
clarified once the 1K flush semantics are settled.

Thanks for the review,
Sebastian