Re: [PATCH] wcn36xx: Fix discarded frames due to wrong sequence number

From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To: Loic Poulain <loic.poulain@linaro.org>, kvalo@codeaurora.org
Cc: wcn36xx@lists.infradead.org, linux-wireless@vger.kernel.org
Subject: Re: [PATCH] wcn36xx: Fix discarded frames due to wrong sequence number
Date: Mon, 25 Oct 2021 09:36:37 +0100	[thread overview]
Message-ID: <2a858752-5727-95ca-1121-626e04500900@linaro.org> (raw)
In-Reply-To: <1635150336-18736-1-git-send-email-loic.poulain@linaro.org>

On 25/10/2021 09:25, Loic Poulain wrote:
> The firmware is offering features such as ARP offload, for which
> firmware crafts its own (QoS)packets without waking up the host.
> Point is that the sequence numbers generated by the firmware are
> not in sync with the host mac80211 layer and can cause packets
> such as firmware ARP reponses to be dropped by the AP (too old SN).
> 
> To fix this we need to let the firmware manages the sequence
> numbers by its own (except for QoS null frames). There is a SN
> counter for each QoS queue and one global/baseline counter for
> Non-QoS.
> 
> Fixes: 84aff52e4f57 ("wcn36xx: Use sequence number allocated by mac80211")
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>   drivers/net/wireless/ath/wcn36xx/txrx.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
> index 81db90f..75951cc 100644
> --- a/drivers/net/wireless/ath/wcn36xx/txrx.c
> +++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
> @@ -427,8 +427,6 @@ static void wcn36xx_set_tx_pdu(struct wcn36xx_tx_bd *bd,
>   		bd->pdu.mpdu_header_off;
>   	bd->pdu.mpdu_len = len;
>   	bd->pdu.tid = tid;
> -	/* Use seq number generated by mac80211 */
> -	bd->pdu.bd_ssn = WCN36XX_TXBD_SSN_FILL_HOST;
>   }
>   
>   static inline struct wcn36xx_vif *get_vif_by_addr(struct wcn36xx *wcn,
> @@ -525,6 +523,9 @@ static void wcn36xx_set_tx_data(struct wcn36xx_tx_bd *bd,
>   		tid = ieee80211_get_tid(hdr);
>   		/* TID->QID is one-to-one mapping */
>   		bd->queue_id = tid;
> +		bd->pdu.bd_ssn = WCN36XX_TXBD_SSN_FILL_DPU_QOS;
> +	} else {
> +		bd->pdu.bd_ssn = WCN36XX_TXBD_SSN_FILL_DPU_NON_QOS;
>   	}
>   
>   	if (info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT ||
> @@ -536,6 +537,8 @@ static void wcn36xx_set_tx_data(struct wcn36xx_tx_bd *bd,
>   		/* Don't use a regular queue for null packet (no ampdu) */
>   		bd->queue_id = WCN36XX_TX_U_WQ_ID;
>   		bd->bd_rate = WCN36XX_BD_RATE_CTRL;
> +		if (ieee80211_is_qos_nullfunc(hdr->frame_control))
> +			bd->pdu.bd_ssn = WCN36XX_TXBD_SSN_FILL_HOST;
>   	}
>   
>   	if (bcast) {
> @@ -595,6 +598,8 @@ static void wcn36xx_set_tx_mgmt(struct wcn36xx_tx_bd *bd,
>   		bd->queue_id = WCN36XX_TX_U_WQ_ID;
>   	*vif_priv = __vif_priv;
>   
> +	bd->pdu.bd_ssn = WCN36XX_TXBD_SSN_FILL_DPU_NON_QOS;
> +
>   	wcn36xx_set_tx_pdu(bd,
>   			   ieee80211_is_data_qos(hdr->frame_control) ?
>   			   sizeof(struct ieee80211_qos_hdr) :
> 

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>