Re: [PATCH v2 ath-next 2/2] wifi: ath11k: fix HTC rx insufficient length

[Miaoqing Pan]

On 3/19/2025 1:42 AM, Johan Hovold wrote:
> On Tue, Mar 18, 2025 at 03:53:39PM +0800, Miaoqing Pan wrote:
> > On 3/17/2025 9:04 PM, Johan Hovold wrote:
>
> > > Then it seems we are looking at two separate root causes for the
> > > corruption as the memory barrier appears to be all that is needed to fix
> > > the X13s issue.
> > >
> > > A user who hit the corruption after 2 h without the fix has been running
> > > over the weekend with the memory barrier without any problems. I'll ask
> > > further users to test, but it certainly looks like it is working as
> > > intended.
> > >
> > > And the memory barrier is de-facto missing as the head pointer and
> > > descriptor are accessed through (two separate) coherent mappings so
> > > there are no ordering guarantees without explicit barriers.
> >
> > This situation should occur when there is only one descriptor in the
> > ring. If, as you mentioned, the CPU tries to load the descriptor first,
> > but the descriptor fetch fails before the HP load because the ring
> > returns empty, it won't trigger the current issue.
>
> It could if the CPU observes the updates out of order due to the missing
> barrier. The driver could be processing an earlier interrupt when the
> new descriptor is added and head pointer updated. If for example the CPU
> speculatively fetches the descriptor before the head pointer is updated,
> then the descriptor length may be zero when the CPU sees the updated
> head pointer.

Sorry, I still think this situation won't happen. Please see the 
following code.

ath11k_hal_srng_access_begin(ab, srng);
  => srng->u.dst_ring.cached_hp = *srng->u.dst_ring.hp_addr;
desc = ath11k_hal_srng_dst_get_next_entry(ab, srng);
  => if (srng->u.dst_ring.tp == srng->u.dst_ring.cached_hp) return NULL;
//dma_rmb();
*nbytes = ath11k_hal_ce_dst_status_get_length(desc);

If the condition 'srng->u.dst_ring.tp == srng->u.dst_ring.cached_hp' is 
true, the descriptor retrieval fails.

> This seems to be what is happening on the X13s since adding the memory
> barrier makes the zero-length descriptors go away.

Hmm, it is indeed a bit strange. Could it be that dma_rmb() introduces 
some delay ?

> > The Copy Engine hardware module copies the metadata to the Status
> > Descriptor after the DMA is complete, then updates the HP to trigger an
> > interrupt. I think there might be some issues in this process, such as
> > the lack of a wmb instruction after the copy is complete, causing the HP
> > to be updated first.
>
> Yeah, possibly. At least it seems there are more issues than the missing
> barrier on the machines you test.
>   
> > > Now obviously there are further issues in your system, which we should
> > > make sure we understand before adding workarounds to the driver.
> > >
> > > Do you have a pointer to the downstream kernel sources you are testing
> > > with? Or even better, can you reproduce the issue with mainline after
> > > adding the PCIe patches that were posted to the lists for these
> > > platforms?
> > https://github.com/qualcomm-linux/meta-qcom-hwe/blob/scarthgap/recipes-kernel/linux/linux-qcom-base_6.6.bb
>
> Thanks for the pointer. That's a lot of out-of-tree patches on top of
> stable so not that easy to check the state of the resulting tree.

Yes, but there are only a few patches for ath11k.

> > > Does it make any difference if you use a full rmb() barrier?
> > I've also tried rmb() and mb(), but they didn't work either.
>
> Thanks for checking.
>
> Just to be sure, you did add the barrier in the same place as my patch
> (i.e. just before the descriptor read)?

Yes.

> > > And after modifying ath11k_hal_ce_dst_status_get_length() so that it
> > > does not clear the length, how many times you need to retry? Does it
> > > always work on the second try?
> >
> > Yes, the test has been running continuously for over 48 hours, always
> > work on the second try, updated in patch v4.
>
> Good, at least the descriptor-length-sometimes-never-updated issue is
> solved.
>
> Johan

Yeah, thanks for pointing out the issue.