diff mbox series

wcn36xx: Fix discarded frames due to wrong sequence number

Message ID 1635150336-18736-1-git-send-email-loic.poulain@linaro.org
State New
Headers show
Series wcn36xx: Fix discarded frames due to wrong sequence number | expand

Commit Message

Loic Poulain Oct. 25, 2021, 8:25 a.m. UTC
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(-)

-- 
2.7.4


_______________________________________________
wcn36xx mailing list
wcn36xx@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/wcn36xx

Comments

Bryan O'Donoghue Oct. 25, 2021, 8:36 a.m. UTC | #1
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>


_______________________________________________
wcn36xx mailing list
wcn36xx@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/wcn36xx
Kalle Valo Oct. 27, 2021, 7:45 a.m. UTC | #2
Loic Poulain <loic.poulain@linaro.org> 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>

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

> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>


Patch applied to ath-next branch of ath.git, thanks.

113f304dbc16 wcn36xx: Fix discarded frames due to wrong sequence number

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/1635150336-18736-1-git-send-email-loic.poulain@linaro.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


_______________________________________________
wcn36xx mailing list
wcn36xx@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/wcn36xx
diff mbox series

Patch

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) :