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

[Miaoqing Pan]

On 3/14/2025 4:20 PM, Johan Hovold wrote:
> On Fri, Mar 14, 2025 at 02:13:53PM +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. However, to prevent this descriptor from
> > continuously returning 0, use the skb control block (cb) to set
> > a flag. If the length returns 0 again, this descriptor will be
> > discarded.
> >
> > Tested-on: QCA6698AQ hw2.1 PCI WLAN.HSP.1.1-04546-QCAHSPSWPL_V1_V2_SILICONZ_IOE-1
> >
> > Reported-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218623
> > Signed-off-by: Miaoqing Pan <quic_miaoqing@xxxxxxxxxxx>
>
> > @@ -387,18 +387,36 @@ 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);
>
> As I mentioned elsewhere, this function also sets the length field in
> the descriptor to zero. So if there's a racing update, you may never see
> the updated length.

Will add below check.

--- a/drivers/net/wireless/ath/ath11k/hal.c
+++ b/drivers/net/wireless/ath/ath11k/hal.c
@@ -602,7 +602,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;
+       /* Avoid setting the length field in the descriptor to zero when 
length
+        * is 0, as there's a racing update, may never see the updated 
length.
+        */
+       if (likely(len))
+               desc->flags &= ~HAL_CE_DST_STATUS_DESC_FLAGS_LEN;



> > -	if (*nbytes == 0) {
> > -		ret = -EIO;
> > -		goto err;
> > +	if (unlikely(*nbytes == 0)) {
> > +		struct ath11k_skb_rxcb *rxcb =
> > +			ATH11K_SKB_RXCB(pipe->dest_ring->skb[sw_index]);
> > +
> > +		/* 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. However, to prevent this descriptor from
> > +		 * continuously returning 0, set 'is_desc_len0' flag. If the
> > +		 * length returns 0 again, this descriptor will be discarded.
> > +		 */
> > +		if (!rxcb->is_desc_len0) {
> > +			rxcb->is_desc_len0 = true;
> > +			ret = -EIO;
> > +			goto err;
> > +		}
>
> If you add the memory barrier and make sure not to clear the length
> field above, do you still see the length sometimes always reading zero
> if you retry more than once (i.e. drop the is_desc_len0 flag)?
>
> Perhaps the device is really passing you a zero-length descriptor that
> can simply be discarded straight away?
>
> Johan

Will verify your suggestion, thanks.