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

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

[Miaoqing Pan]

This series of patches is to address the issue reported in
https://bugzilla.kernel.org/show_bug.cgi?id=218623.

ath11k_pci 0006:01:00.0: HTC Rx: insufficient length, got 1488, expected 1492
ath11k_pci 0006:01:00.0: HTC Rx: insufficient length, got 1460, expected 1484

---
v2: add Reported-by tag.
v3:
 - update commit message.
 - revert the modifications to ath11k_hal_srng_dst_get_next_entry to ensure efficiency.
 - change warning to dbg when see zero-length descriptor.
v4:
 - update commit message.
 - remove is_desc_len0 flag.
 - skip updating the length field of the descriptor when it is 0.
---
Miaoqing Pan (2):
  wifi: ath11k: add function to get next srng desc
  wifi: ath11k: fix HTC rx insufficient length

 drivers/net/wireless/ath/ath11k/ce.c  | 14 +++++++++++---
 drivers/net/wireless/ath/ath11k/hal.c | 23 +++++++++++++++++++++--
 drivers/net/wireless/ath/ath11k/hal.h |  3 ++-
 3 files changed, 34 insertions(+), 6 deletions(-)


base-commit: 42aa76e608ca845c98e79f9e23af0bdb07b2eb1d
-- 
2.25.1

[PATCH v4 ath-next 1/2] wifi: ath11k: add function to get next srng desc

[Miaoqing Pan]

Adding the ath11k_hal_srng_dst_next() function allows for the
separate invocation of ath11k_hal_srng_dst_peek() and
ath11k_hal_srng_dst_next() in certain situations, instead of
calling the ath11k_hal_srng_dst_get_next_entry() function alone.

Tested-on: QCA6698AQ hw2.1 PCI WLAN.HSP.1.1-04546-QCAHSPSWPL_V1_V2_SILICONZ_IOE-1

Signed-off-by: Miaoqing Pan <quic_miaoqing@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/hal.c | 17 ++++++++++++++++-
 drivers/net/wireless/ath/ath11k/hal.h |  3 ++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/hal.c b/drivers/net/wireless/ath/ath11k/hal.c
index 61f4b6dd5380..211c085921b6 100644
--- a/drivers/net/wireless/ath/ath11k/hal.c
+++ b/drivers/net/wireless/ath/ath11k/hal.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: BSD-3-Clause-Clear
 /*
  * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 #include <linux/dma-mapping.h>
 #include "hal_tx.h"
@@ -656,6 +656,21 @@ static void ath11k_hal_srng_prefetch_desc(struct ath11k_base *ab,
 	}
 }
 
+void ath11k_hal_srng_dst_next(struct ath11k_base *ab, struct hal_srng *srng)
+{
+	lockdep_assert_held(&srng->lock);
+
+	srng->u.dst_ring.tp += srng->entry_size;
+
+	/* wrap around to start of ring*/
+	if (srng->u.dst_ring.tp == srng->ring_size)
+		srng->u.dst_ring.tp = 0;
+
+	/* Try to prefetch the next descriptor in the ring */
+	if (srng->flags & HAL_SRNG_FLAGS_CACHED)
+		ath11k_hal_srng_prefetch_desc(ab, srng);
+}
+
 u32 *ath11k_hal_srng_dst_get_next_entry(struct ath11k_base *ab,
 					struct hal_srng *srng)
 {
diff --git a/drivers/net/wireless/ath/ath11k/hal.h b/drivers/net/wireless/ath/ath11k/hal.h
index 601542410c75..317b09ec6c35 100644
--- a/drivers/net/wireless/ath/ath11k/hal.h
+++ b/drivers/net/wireless/ath/ath11k/hal.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause-Clear */
 /*
  * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2022, 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2022, 2024-2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef ATH11K_HAL_H
@@ -941,6 +941,7 @@ int ath11k_hal_srng_get_entrysize(struct ath11k_base *ab, u32 ring_type);
 int ath11k_hal_srng_get_max_entries(struct ath11k_base *ab, u32 ring_type);
 void ath11k_hal_srng_get_params(struct ath11k_base *ab, struct hal_srng *srng,
 				struct hal_srng_params *params);
+void ath11k_hal_srng_dst_next(struct ath11k_base *ab, struct hal_srng *srng);
 u32 *ath11k_hal_srng_dst_get_next_entry(struct ath11k_base *ab,
 					struct hal_srng *srng);
 u32 *ath11k_hal_srng_dst_peek(struct ath11k_base *ab, struct hal_srng *srng);
-- 
2.25.1

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

[Miaoqing Pan]

A relatively unusual race condition occurs between host software
and hardware, where the host sees the updated destination ring head
pointer before the hardware updates the corresponding descriptor.
When this situation occurs, the length of the descriptor returns 0.

The current error handling method is to increment descriptor tail
pointer by 1, but 'sw_index' is not updated, causing descriptor and
skb to not correspond one-to-one, resulting in the following error:

ath11k_pci 0006:01:00.0: HTC Rx: insufficient length, got 1488, expected 1492
ath11k_pci 0006:01:00.0: HTC Rx: insufficient length, got 1460, expected 1484

To address this problem and work around the broken hardware,
temporarily skip processing the current descriptor and handle it
again next time. Also, skip updating the length field of the
descriptor when it is 0, because there's a racing update, may
never see the updated length.

Tested-on: QCA6698AQ hw2.1 PCI WLAN.HSP.1.1-04546-QCAHSPSWPL_V1_V2_SILICONZ_IOE-1

Reported-by: Johan Hovold <johan+linaro@kernel.org>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218623
Signed-off-by: Miaoqing Pan <quic_miaoqing@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/ce.c  | 14 +++++++++++---
 drivers/net/wireless/ath/ath11k/hal.c |  6 +++++-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
index e66e86bdec20..2bc8bc97165a 100644
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: BSD-3-Clause-Clear
 /*
  * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2023, 2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #include "dp_rx.h"
@@ -387,18 +387,26 @@ static int ath11k_ce_completed_recv_next(struct ath11k_ce_pipe *pipe,
 
 	ath11k_hal_srng_access_begin(ab, srng);
 
-	desc = ath11k_hal_srng_dst_get_next_entry(ab, srng);
+	desc = ath11k_hal_srng_dst_peek(ab, srng);
 	if (!desc) {
 		ret = -EIO;
 		goto err;
 	}
 
 	*nbytes = ath11k_hal_ce_dst_status_get_length(desc);
-	if (*nbytes == 0) {
+	if (unlikely(*nbytes == 0)) {
+		/* A relatively unusual race condition occurs between host
+		 * software and hardware, where the host sees the updated
+		 * destination ring head pointer before the hardware updates
+		 * the corresponding descriptor. Temporarily skip processing
+		 * the current descriptor and handle it again next time.
+		 */
 		ret = -EIO;
 		goto err;
 	}
 
+	ath11k_hal_srng_dst_next(ab, srng);
+
 	*skb = pipe->dest_ring->skb[sw_index];
 	pipe->dest_ring->skb[sw_index] = NULL;
 
diff --git a/drivers/net/wireless/ath/ath11k/hal.c b/drivers/net/wireless/ath/ath11k/hal.c
index 211c085921b6..16e0b5713445 100644
--- a/drivers/net/wireless/ath/ath11k/hal.c
+++ b/drivers/net/wireless/ath/ath11k/hal.c
@@ -600,7 +600,11 @@ u32 ath11k_hal_ce_dst_status_get_length(void *buf)
 	u32 len;
 
 	len = FIELD_GET(HAL_CE_DST_STATUS_DESC_FLAGS_LEN, desc->flags);
-	desc->flags &= ~HAL_CE_DST_STATUS_DESC_FLAGS_LEN;
+	/* Skip updating the length field of the descriptor when it is 0,
+	 * because there's a racing update, may never see the updated length.
+	 */
+	if (likely(len))
+		desc->flags &= ~HAL_CE_DST_STATUS_DESC_FLAGS_LEN;
 
 	return len;
 }
-- 
2.25.1

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

[Johan Hovold]

On Mon, Mar 17, 2025 at 03:20:36PM +0800, Miaoqing Pan wrote:
> A relatively unusual race condition occurs between host software
> and hardware, where the host sees the updated destination ring head
> pointer before the hardware updates the corresponding descriptor.
> When this situation occurs, the length of the descriptor returns 0.
> 
> The current error handling method is to increment descriptor tail
> pointer by 1, but 'sw_index' is not updated, causing descriptor and
> skb to not correspond one-to-one, resulting in the following error:
> 
> ath11k_pci 0006:01:00.0: HTC Rx: insufficient length, got 1488, expected 1492
> ath11k_pci 0006:01:00.0: HTC Rx: insufficient length, got 1460, expected 1484
> 
> To address this problem and work around the broken hardware,
> temporarily skip processing the current descriptor and handle it
> again next time. Also, skip updating the length field of the
> descriptor when it is 0, because there's a racing update, may
> never see the updated length.
> 
> Tested-on: QCA6698AQ hw2.1 PCI WLAN.HSP.1.1-04546-QCAHSPSWPL_V1_V2_SILICONZ_IOE-1
> 
> Reported-by: Johan Hovold <johan+linaro@kernel.org>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218623
> Signed-off-by: Miaoqing Pan <quic_miaoqing@quicinc.com>

As I've argued elsewhere, I think this should be fixed by adding the
missing memory barrier which is needed to prevent ordering issues like
this on aarch64:

	https://lore.kernel.org/lkml/Z90yyrZcORhJJgNU@hovoldconsulting.com/

The fact that this alone does not seem to be sufficient to address the
issue on qcs615 (and qcs8300) suggests that there are further issues
with these platforms that need to be properly understood before adding
workarounds in one place in one driver.

I've just posted my fix, a version of which users have been running now
for a week without hitting the corruption (that some used to hit
multiple times a daily), here:

	https://lore.kernel.org/lkml/20250321094916.19098-1-johan+linaro@kernel.org/

> @@ -387,18 +387,26 @@ static int ath11k_ce_completed_recv_next(struct ath11k_ce_pipe *pipe,
>  
>  	ath11k_hal_srng_access_begin(ab, srng);
>  
> -	desc = ath11k_hal_srng_dst_get_next_entry(ab, srng);
> +	desc = ath11k_hal_srng_dst_peek(ab, srng);
>  	if (!desc) {
>  		ret = -EIO;
>  		goto err;
>  	}
>  
>  	*nbytes = ath11k_hal_ce_dst_status_get_length(desc);
> -	if (*nbytes == 0) {
> +	if (unlikely(*nbytes == 0)) {
> +		/* A relatively unusual race condition occurs between host
> +		 * software and hardware, where the host sees the updated
> +		 * destination ring head pointer before the hardware updates
> +		 * the corresponding descriptor. Temporarily skip processing
> +		 * the current descriptor and handle it again next time.
> +		 */
>  		ret = -EIO;
>  		goto err;

Your tests suggested that you always see the correct length the next
time you process the ring buffer, but AFAICT that is not guaranteed to
happen (i.e. if you hit this on the last transfer).

Johan

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

[Jeff Johnson]

On 3/21/2025 3:10 AM, Johan Hovold wrote:
> On Mon, Mar 17, 2025 at 03:20:36PM +0800, Miaoqing Pan wrote:
>> A relatively unusual race condition occurs between host software
>> and hardware, where the host sees the updated destination ring head
>> pointer before the hardware updates the corresponding descriptor.
>> When this situation occurs, the length of the descriptor returns 0.
>>
>> The current error handling method is to increment descriptor tail
>> pointer by 1, but 'sw_index' is not updated, causing descriptor and
>> skb to not correspond one-to-one, resulting in the following error:
>>
>> ath11k_pci 0006:01:00.0: HTC Rx: insufficient length, got 1488, expected 1492
>> ath11k_pci 0006:01:00.0: HTC Rx: insufficient length, got 1460, expected 1484
>>
>> To address this problem and work around the broken hardware,
>> temporarily skip processing the current descriptor and handle it
>> again next time. Also, skip updating the length field of the
>> descriptor when it is 0, because there's a racing update, may
>> never see the updated length.
>>
>> Tested-on: QCA6698AQ hw2.1 PCI WLAN.HSP.1.1-04546-QCAHSPSWPL_V1_V2_SILICONZ_IOE-1
>>
>> Reported-by: Johan Hovold <johan+linaro@kernel.org>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218623
>> Signed-off-by: Miaoqing Pan <quic_miaoqing@quicinc.com>
> 
> As I've argued elsewhere, I think this should be fixed by adding the
> missing memory barrier which is needed to prevent ordering issues like
> this on aarch64:
> 
> 	https://lore.kernel.org/lkml/Z90yyrZcORhJJgNU@hovoldconsulting.com/
> 
> The fact that this alone does not seem to be sufficient to address the
> issue on qcs615 (and qcs8300) suggests that there are further issues
> with these platforms that need to be properly understood before adding
> workarounds in one place in one driver.
> 
> I've just posted my fix, a version of which users have been running now
> for a week without hitting the corruption (that some used to hit
> multiple times a daily), here:
> 
> 	https://lore.kernel.org/lkml/20250321094916.19098-1-johan+linaro@kernel.org/
> 
>> @@ -387,18 +387,26 @@ static int ath11k_ce_completed_recv_next(struct ath11k_ce_pipe *pipe,
>>  
>>  	ath11k_hal_srng_access_begin(ab, srng);
>>  
>> -	desc = ath11k_hal_srng_dst_get_next_entry(ab, srng);
>> +	desc = ath11k_hal_srng_dst_peek(ab, srng);
>>  	if (!desc) {
>>  		ret = -EIO;
>>  		goto err;
>>  	}
>>  
>>  	*nbytes = ath11k_hal_ce_dst_status_get_length(desc);
>> -	if (*nbytes == 0) {
>> +	if (unlikely(*nbytes == 0)) {
>> +		/* A relatively unusual race condition occurs between host
>> +		 * software and hardware, where the host sees the updated
>> +		 * destination ring head pointer before the hardware updates
>> +		 * the corresponding descriptor. Temporarily skip processing
>> +		 * the current descriptor and handle it again next time.
>> +		 */
>>  		ret = -EIO;
>>  		goto err;
> 
> Your tests suggested that you always see the correct length the next
> time you process the ring buffer, but AFAICT that is not guaranteed to
> happen (i.e. if you hit this on the last transfer).

I'm going to mark this as Deferred in patchwork.
Let's have Johan's complete set of barrier changes land both in ath11k and
ath12k, and then re-evaluate the need for your workaround after that.

/jeff