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

[Miaoqing Pan <quic_miaoqing@quicinc.com>]

On 3/14/2025 4:06 PM, Johan Hovold wrote:
> On Fri, Mar 14, 2025 at 09:01:36AM +0800, Miaoqing Pan wrote:
>> On 3/14/2025 12:14 AM, Johan Hovold wrote:
>>> On Thu, Mar 13, 2025 at 09:31:56PM +0800, Miaoqing Pan wrote:
>>>> On 3/13/2025 12:43 AM, Johan Hovold wrote:
> 
>>>>> +	/* Make sure descriptor is read after the head pointer. */
>>>>> +	dma_rmb();
>>>>> +
>>>>>     	*nbytes = ath11k_hal_ce_dst_status_get_length(desc);
>>>>>     	if (*nbytes == 0) {
>>>>> +		WARN_ON_ONCE(1);	// FIXME: remove
>>>>>     		ret = -EIO;
>>>>>     		goto err;
>>>>>     	}
>>>>
>>>> This issue can still be reproduced.
>>>>
>>>> [ 3283.687469] WARNING: CPU: 0 PID: 0 at
>>>> /drivers/net/wireless/ath/ath11k/ce.c:405
>>>> ath11k_ce_per_engine_service+0x228/0x3e4 [ath11k]
>>>
>>> Thanks for verifying.
>>>
>>> Which platform are you testing on and which kernel are you using?
>>>
>>> I'm still waiting to hear back from some people testing my patch on the
>>> X13s (sc8280xp).
> 
>> qcs615-ride, kernel 6.6.65.
> 
> Ok, so a downstream vendor kernel?
> 
> qcs615-ride does not even have PCIe enabled in mainline yet, but I
> assume that's what you use here?
>   
>> I think the hardware has already ensured synchronization between
>> descriptor and head pointer, which isn't difficult to achieve. The issue
>> is likely caused by something else and requires further debugging.
> 
> Yeah, but you still need memory barriers on the kernel side.
> 
> It could be that we are looking at two different causes for those
> zero-length descriptors.
> 
> The error handling for that obviously needs to be fixed either way, but
> I haven't heard anyone hitting the corruption with the memory barriers
> in place on the X13s yet (even if we'd need some more time to test
> this).
> 
> Johan

After multiple and prolonged verifications, adding dma_rmb() did not 
improve the issue at all. I think this Status Descriptor is updated by 
hardware (Copy Engine) controlled by another system, not involving DMA 
or out-of-order CPU access within the same system, so memory barriers do 
not take effect.