[PATCH 1/4] wcn36xx: Fix multiple AMPDU sessions support

[PATCH 1/4] wcn36xx: Fix multiple AMPDU sessions support

[Loic Poulain]

Several AMPDU sessions can be started, e.g. for different TIDs.
Currently the driver does not take care of the session ID when
requesting block-ack (statically set to 0), which leads to never
block-acked packet with sessions other than 0.

Fix this by saving the session id when creating the ba session and
use it in subsequent ba operations.

This issue can be reproduced with iperf in two steps (tid 0 strem
then tid 6 stream).

1.0 iperf -s                                # wcn36xx side
1.1 iperf -c ${IP_ADDR}                     # host side

Then

2.0 iperf -s -u -S 0xC0                     # wcn36xx side
2.1 iperf -c ${IP_ADDR} -u -S 0xC0 -l 2000  # host side

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/net/wireless/ath/wcn36xx/main.c | 10 ++++++----
 drivers/net/wireless/ath/wcn36xx/smd.c  | 32 ++++++++++++++++++++++++++------
 drivers/net/wireless/ath/wcn36xx/smd.h  |  4 ++--
 3 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index 702b689..af32bd6 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -1083,6 +1083,7 @@ static int wcn36xx_ampdu_action(struct ieee80211_hw *hw,
 	u16 tid = params->tid;
 	u16 *ssn = &params->ssn;
 	int ret = 0;
+	u8 session;
 
 	wcn36xx_dbg(WCN36XX_DBG_MAC, "mac ampdu action action %d tid %d\n",
 		    action, tid);
@@ -1092,10 +1093,11 @@ static int wcn36xx_ampdu_action(struct ieee80211_hw *hw,
 	switch (action) {
 	case IEEE80211_AMPDU_RX_START:
 		sta_priv->tid = tid;
-		wcn36xx_smd_add_ba_session(wcn, sta, tid, ssn, 0,
-			get_sta_index(vif, sta_priv));
-		wcn36xx_smd_add_ba(wcn);
-		wcn36xx_smd_trigger_ba(wcn, get_sta_index(vif, sta_priv));
+		session = wcn36xx_smd_add_ba_session(wcn, sta, tid, ssn, 0,
+						     get_sta_index(vif, sta_priv));
+		wcn36xx_smd_add_ba(wcn, session);
+		wcn36xx_smd_trigger_ba(wcn, get_sta_index(vif, sta_priv), tid,
+				       session);
 		break;
 	case IEEE80211_AMPDU_RX_STOP:
 		wcn36xx_smd_del_ba(wcn, tid, get_sta_index(vif, sta_priv));
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 77269ac..0ad605f 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2102,6 +2102,22 @@ int wcn36xx_smd_feature_caps_exchange(struct wcn36xx *wcn)
 	return ret;
 }
 
+static int wcn36xx_smd_add_ba_session_rsp(void *buf, int len, u8 *session)
+{
+	struct wcn36xx_hal_add_ba_session_rsp_msg *rsp;
+
+	if (len < sizeof(*rsp))
+		return -EINVAL;
+
+	rsp = (struct wcn36xx_hal_add_ba_session_rsp_msg *) buf;
+	if (WCN36XX_FW_MSG_RESULT_SUCCESS != rsp->status)
+		return rsp->status;
+
+	*session = rsp->ba_session_id;
+
+	return 0;
+}
+
 int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn,
 		struct ieee80211_sta *sta,
 		u16 tid,
@@ -2110,6 +2126,7 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn,
 		u8 sta_index)
 {
 	struct wcn36xx_hal_add_ba_session_req_msg msg_body;
+	u8 session_id;
 	int ret;
 
 	mutex_lock(&wcn->hal_mutex);
@@ -2135,17 +2152,20 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn,
 		wcn36xx_err("Sending hal_add_ba_session failed\n");
 		goto out;
 	}
-	ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
+	ret = wcn36xx_smd_add_ba_session_rsp(wcn->hal_buf, wcn->hal_rsp_len,
+					     &session_id);
 	if (ret) {
 		wcn36xx_err("hal_add_ba_session response failed err=%d\n", ret);
 		goto out;
 	}
+
+	ret = session_id;
 out:
 	mutex_unlock(&wcn->hal_mutex);
 	return ret;
 }
 
-int wcn36xx_smd_add_ba(struct wcn36xx *wcn)
+int wcn36xx_smd_add_ba(struct wcn36xx *wcn, u8 session_id)
 {
 	struct wcn36xx_hal_add_ba_req_msg msg_body;
 	int ret;
@@ -2153,7 +2173,7 @@ int wcn36xx_smd_add_ba(struct wcn36xx *wcn)
 	mutex_lock(&wcn->hal_mutex);
 	INIT_HAL_MSG(msg_body, WCN36XX_HAL_ADD_BA_REQ);
 
-	msg_body.session_id = 0;
+	msg_body.session_id = session_id;
 	msg_body.win_size = WCN36XX_AGGR_BUFFER_SIZE;
 
 	PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
@@ -2212,7 +2232,7 @@ static int wcn36xx_smd_trigger_ba_rsp(void *buf, int len)
 	return rsp->status;
 }
 
-int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index)
+int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index, u16 tid, u8 session_id)
 {
 	struct wcn36xx_hal_trigger_ba_req_msg msg_body;
 	struct wcn36xx_hal_trigger_ba_req_candidate *candidate;
@@ -2221,7 +2241,7 @@ int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index)
 	mutex_lock(&wcn->hal_mutex);
 	INIT_HAL_MSG(msg_body, WCN36XX_HAL_TRIGGER_BA_REQ);
 
-	msg_body.session_id = 0;
+	msg_body.session_id = session_id;
 	msg_body.candidate_cnt = 1;
 	msg_body.header.len += sizeof(*candidate);
 	PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
@@ -2229,7 +2249,7 @@ int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index)
 	candidate = (struct wcn36xx_hal_trigger_ba_req_candidate *)
 		(wcn->hal_buf + sizeof(msg_body));
 	candidate->sta_index = sta_index;
-	candidate->tid_bitmap = 1;
+	candidate->tid_bitmap = 1 << tid;
 
 	ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
 	if (ret) {
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
index ff15df8..68c59df 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.h
+++ b/drivers/net/wireless/ath/wcn36xx/smd.h
@@ -132,9 +132,9 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn,
 		u16 *ssn,
 		u8 direction,
 		u8 sta_index);
-int wcn36xx_smd_add_ba(struct wcn36xx *wcn);
+int wcn36xx_smd_add_ba(struct wcn36xx *wcn, u8 session_id);
 int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 sta_index);
-int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index);
+int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index, u16 tid, u8 session_id);
 
 int wcn36xx_smd_update_cfg(struct wcn36xx *wcn, u32 cfg_id, u32 value);
 
-- 
2.7.4

[PATCH 2/4] wcn36xx: Add TX ack support

[Loic Poulain]

The controller is capable of reporting TX indication which can be used
to report TX ack. It was only partially implemented.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/net/wireless/ath/wcn36xx/dxe.c     | 57 ++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/wcn36xx/main.c    |  1 +
 drivers/net/wireless/ath/wcn36xx/smd.c     |  4 +--
 drivers/net/wireless/ath/wcn36xx/txrx.c    | 20 +++++++----
 drivers/net/wireless/ath/wcn36xx/wcn36xx.h |  1 +
 5 files changed, 72 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
index bab30f7..6307923 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.c
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
@@ -334,6 +334,7 @@ void wcn36xx_dxe_tx_ack_ind(struct wcn36xx *wcn, u32 status)
 	spin_lock_irqsave(&wcn->dxe_lock, flags);
 	skb = wcn->tx_ack_skb;
 	wcn->tx_ack_skb = NULL;
+	del_timer(&wcn->tx_ack_timer);
 	spin_unlock_irqrestore(&wcn->dxe_lock, flags);
 
 	if (!skb) {
@@ -345,6 +346,8 @@ void wcn36xx_dxe_tx_ack_ind(struct wcn36xx *wcn, u32 status)
 
 	if (status == 1)
 		info->flags |= IEEE80211_TX_STAT_ACK;
+	else
+		info->flags &= ~IEEE80211_TX_STAT_ACK;
 
 	wcn36xx_dbg(WCN36XX_DBG_DXE, "dxe tx ack status: %d\n", status);
 
@@ -352,6 +355,32 @@ void wcn36xx_dxe_tx_ack_ind(struct wcn36xx *wcn, u32 status)
 	ieee80211_wake_queues(wcn->hw);
 }
 
+static void wcn36xx_dxe_tx_timer(struct timer_list *t)
+{
+	struct wcn36xx *wcn = from_timer(wcn, t, tx_ack_timer);
+	struct ieee80211_tx_info *info;
+	unsigned long flags;
+	struct sk_buff *skb;
+
+	/* TX Timeout */
+	wcn36xx_dbg(WCN36XX_DBG_DXE, "TX timeout\n");
+
+	spin_lock_irqsave(&wcn->dxe_lock, flags);
+	skb = wcn->tx_ack_skb;
+	wcn->tx_ack_skb = NULL;
+	spin_unlock_irqrestore(&wcn->dxe_lock, flags);
+
+	if (!skb)
+		return;
+
+	info = IEEE80211_SKB_CB(skb);
+	info->flags &= ~IEEE80211_TX_STAT_ACK;
+	info->flags &= ~IEEE80211_TX_STAT_NOACK_TRANSMITTED;
+
+	ieee80211_tx_status_irqsafe(wcn->hw, skb);
+	ieee80211_wake_queues(wcn->hw);
+}
+
 static void reap_tx_dxes(struct wcn36xx *wcn, struct wcn36xx_dxe_ch *ch)
 {
 	struct wcn36xx_dxe_ctl *ctl;
@@ -397,6 +426,7 @@ static irqreturn_t wcn36xx_irq_tx_complete(int irq, void *dev)
 {
 	struct wcn36xx *wcn = (struct wcn36xx *)dev;
 	int int_src, int_reason;
+	bool transmitted = false;
 
 	wcn36xx_dxe_read_register(wcn, WCN36XX_DXE_INT_SRC_RAW_REG, &int_src);
 
@@ -434,8 +464,10 @@ static irqreturn_t wcn36xx_irq_tx_complete(int irq, void *dev)
 			    int_reason);
 
 		if (int_reason & (WCN36XX_CH_STAT_INT_DONE_MASK |
-				  WCN36XX_CH_STAT_INT_ED_MASK))
+				  WCN36XX_CH_STAT_INT_ED_MASK)) {
 			reap_tx_dxes(wcn, &wcn->dxe_tx_h_ch);
+			transmitted = true;
+		}
 	}
 
 	if (int_src & WCN36XX_INT_MASK_CHAN_TX_L) {
@@ -473,9 +505,27 @@ static irqreturn_t wcn36xx_irq_tx_complete(int irq, void *dev)
 			    int_reason);
 
 		if (int_reason & (WCN36XX_CH_STAT_INT_DONE_MASK |
-				  WCN36XX_CH_STAT_INT_ED_MASK))
+				  WCN36XX_CH_STAT_INT_ED_MASK)) {
 			reap_tx_dxes(wcn, &wcn->dxe_tx_l_ch);
+			transmitted = true;
+		}
+	}
+
+	spin_lock(&wcn->dxe_lock);
+	if (wcn->tx_ack_skb && transmitted) {
+		struct ieee80211_tx_info *info = IEEE80211_SKB_CB(wcn->tx_ack_skb);
+
+		/* TX complete, no need to wait for 802.11 ack indication */
+		if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS &&
+		    info->flags & IEEE80211_TX_CTL_NO_ACK) {
+			info->flags |= IEEE80211_TX_STAT_NOACK_TRANSMITTED;
+			del_timer(&wcn->tx_ack_timer);
+			ieee80211_tx_status_irqsafe(wcn->hw, wcn->tx_ack_skb);
+			wcn->tx_ack_skb = NULL;
+			ieee80211_wake_queues(wcn->hw);
+		}
 	}
+	spin_unlock(&wcn->dxe_lock);
 
 	return IRQ_HANDLED;
 }
@@ -916,6 +966,8 @@ int wcn36xx_dxe_init(struct wcn36xx *wcn)
 	if (ret < 0)
 		goto out_err_irq;
 
+	timer_setup(&wcn->tx_ack_timer, wcn36xx_dxe_tx_timer, 0);
+
 	return 0;
 
 out_err_irq:
@@ -934,6 +986,7 @@ void wcn36xx_dxe_deinit(struct wcn36xx *wcn)
 {
 	free_irq(wcn->tx_irq, wcn);
 	free_irq(wcn->rx_irq, wcn);
+	del_timer(&wcn->tx_ack_timer);
 
 	if (wcn->tx_ack_skb) {
 		ieee80211_tx_status_irqsafe(wcn->hw, wcn->tx_ack_skb);
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index af32bd6..c19648f 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -1175,6 +1175,7 @@ static int wcn36xx_init_ieee80211(struct wcn36xx *wcn)
 	ieee80211_hw_set(wcn->hw, SIGNAL_DBM);
 	ieee80211_hw_set(wcn->hw, HAS_RATE_CONTROL);
 	ieee80211_hw_set(wcn->hw, SINGLE_SCAN_ON_ALL_BANDS);
+	ieee80211_hw_set(wcn->hw, REPORTS_TX_ACK_STATUS);
 
 	wcn->hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
 		BIT(NL80211_IFTYPE_AP) |
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 0ad605f..3fc2d46 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -45,8 +45,8 @@ static struct wcn36xx_cfg_val wcn36xx_cfg_vals[] = {
 	WCN36XX_CFG_VAL(MAX_MEDIUM_TIME, 6000),
 	WCN36XX_CFG_VAL(MAX_MPDUS_IN_AMPDU, 64),
 	WCN36XX_CFG_VAL(RTS_THRESHOLD, 2347),
-	WCN36XX_CFG_VAL(SHORT_RETRY_LIMIT, 6),
-	WCN36XX_CFG_VAL(LONG_RETRY_LIMIT, 6),
+	WCN36XX_CFG_VAL(SHORT_RETRY_LIMIT, 15),
+	WCN36XX_CFG_VAL(LONG_RETRY_LIMIT, 15),
 	WCN36XX_CFG_VAL(FRAGMENTATION_THRESHOLD, 8000),
 	WCN36XX_CFG_VAL(DYNAMIC_THRESHOLD_ZERO, 5),
 	WCN36XX_CFG_VAL(DYNAMIC_THRESHOLD_ONE, 10),
diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
index a690237..274cf58 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.c
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
@@ -191,9 +191,10 @@ static void wcn36xx_set_tx_data(struct wcn36xx_tx_bd *bd,
 		bd->dpu_sign = __vif_priv->self_ucast_dpu_sign;
 	}
 
-	if (ieee80211_is_nullfunc(hdr->frame_control) ||
-	   (sta_priv && !sta_priv->is_data_encrypted))
+	if (ieee80211_is_any_nullfunc(hdr->frame_control) ||
+	   (sta_priv && !sta_priv->is_data_encrypted)) {
 		bd->dpu_ne = 1;
+	}
 
 	if (bcast) {
 		bd->ub = 1;
@@ -287,9 +288,9 @@ int wcn36xx_start_tx(struct wcn36xx *wcn,
 
 	bd.dpu_rf = WCN36XX_BMU_WQ_TX;
 
-	bd.tx_comp = !!(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS);
-	if (bd.tx_comp) {
+	if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
 		wcn36xx_dbg(WCN36XX_DBG_DXE, "TX_ACK status requested\n");
+
 		spin_lock_irqsave(&wcn->dxe_lock, flags);
 		if (wcn->tx_ack_skb) {
 			spin_unlock_irqrestore(&wcn->dxe_lock, flags);
@@ -302,10 +303,15 @@ int wcn36xx_start_tx(struct wcn36xx *wcn,
 
 		/* Only one at a time is supported by fw. Stop the TX queues
 		 * until the ack status gets back.
-		 *
-		 * TODO: Add watchdog in case FW does not answer
 		 */
 		ieee80211_stop_queues(wcn->hw);
+
+		/* TX watchdog if no TX irq or ack indication received  */
+		mod_timer(&wcn->tx_ack_timer, jiffies + HZ / 10);
+
+		/* Request ack indication from the firmware */
+		if (!(info->flags & IEEE80211_TX_CTL_NO_ACK))
+			bd.tx_comp = 1;
 	}
 
 	/* Data frames served first*/
@@ -319,7 +325,7 @@ int wcn36xx_start_tx(struct wcn36xx *wcn,
 	bd.tx_bd_sign = 0xbdbdbdbd;
 
 	ret = wcn36xx_dxe_tx_frame(wcn, vif_priv, &bd, skb, is_low);
-	if (ret && bd.tx_comp) {
+	if (ret && (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)) {
 		/* If the skb has not been transmitted,
 		 * don't keep a reference to it.
 		 */
diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
index a58f313..2d89849 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
@@ -245,6 +245,7 @@ struct wcn36xx {
 	struct wcn36xx_dxe_mem_pool data_mem_pool;
 
 	struct sk_buff		*tx_ack_skb;
+	struct timer_list	tx_ack_timer;
 
 	/* RF module */
 	unsigned		rf_id;
-- 
2.7.4

[PATCH 3/4] wcn36xx: Fix TX data path

[Loic Poulain]

This patch contains the following fixes:

- Use correct queue for submitting QoS packet. The queue id to use
is a one-to-one mapping with the TID.

- Don't encrypt a frame with IEEE80211_TX_INTFL_DONT_ENCRYPT flag.

- Use the 'special queue' for null packets, preventing the firmware
to submit it as AMPDU.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/net/wireless/ath/wcn36xx/txrx.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
index 274cf58..dcc2ec0 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.c
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
@@ -160,9 +160,11 @@ static void wcn36xx_set_tx_data(struct wcn36xx_tx_bd *bd,
 				bool bcast)
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_vif *vif = NULL;
 	struct wcn36xx_vif *__vif_priv = NULL;
-	bool is_data_qos;
+	bool is_data_qos = ieee80211_is_data_qos(hdr->frame_control);
+	u16 tid = 0;
 
 	bd->bd_rate = WCN36XX_BD_RATE_DATA;
 
@@ -191,24 +193,33 @@ static void wcn36xx_set_tx_data(struct wcn36xx_tx_bd *bd,
 		bd->dpu_sign = __vif_priv->self_ucast_dpu_sign;
 	}
 
-	if (ieee80211_is_any_nullfunc(hdr->frame_control) ||
-	   (sta_priv && !sta_priv->is_data_encrypted)) {
+	if (is_data_qos) {
+		tid = ieee80211_get_tid(hdr);
+		/* TID->QID is one-to-one mapping */
+		bd->queue_id = tid;
+	}
+
+	if (info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT ||
+	    (sta_priv && !sta_priv->is_data_encrypted)) {
 		bd->dpu_ne = 1;
 	}
 
+	if (ieee80211_is_any_nullfunc(hdr->frame_control)) {
+		/* Don't use a regular queue for null packet (no ampdu) */
+		bd->queue_id = WCN36XX_TX_U_WQ_ID;
+	}
+
 	if (bcast) {
 		bd->ub = 1;
 		bd->ack_policy = 1;
 	}
 	*vif_priv = __vif_priv;
 
-	is_data_qos = ieee80211_is_data_qos(hdr->frame_control);
-
 	wcn36xx_set_tx_pdu(bd,
 			   is_data_qos ?
 			   sizeof(struct ieee80211_qos_hdr) :
 			   sizeof(struct ieee80211_hdr_3addr),
-			   skb->len, sta_priv ? sta_priv->tid : 0);
+			   skb->len, tid);
 
 	if (sta_priv && is_data_qos)
 		wcn36xx_tx_start_ampdu(wcn, sta_priv, skb);
-- 
2.7.4

[PATCH 4/4] wcn36xx: Fix software-driven scan

[Loic Poulain]

For software-driven scan, rely on mac80211 software scan instead
of internal driver implementation. The internal implementation
cause connection trouble since it keeps the antenna busy during
the entire scan duration, moreover it's only a passive scanning
(no probe request). Therefore, let mac80211 manages sw scan.

Note: we fallback to software scan if firmware does not report
scan offload support or if we need to scan the 5Ghz band (currently
not supported by the offload scan...).

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/net/wireless/ath/wcn36xx/main.c    | 162 +++++++++++++++--------------
 drivers/net/wireless/ath/wcn36xx/smd.c     |  23 +++-
 drivers/net/wireless/ath/wcn36xx/smd.h     |   8 +-
 drivers/net/wireless/ath/wcn36xx/txrx.c    |  11 +-
 drivers/net/wireless/ath/wcn36xx/wcn36xx.h |   6 +-
 5 files changed, 117 insertions(+), 93 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index c19648f..78a4164 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -354,8 +354,6 @@ static void wcn36xx_stop(struct ieee80211_hw *hw)
 
 	wcn36xx_dbg(WCN36XX_DBG_MAC, "mac stop\n");
 
-	cancel_work_sync(&wcn->scan_work);
-
 	mutex_lock(&wcn->scan_lock);
 	if (wcn->scan_req) {
 		struct cfg80211_scan_info scan_info = {
@@ -378,12 +376,37 @@ static void wcn36xx_stop(struct ieee80211_hw *hw)
 	kfree(wcn->hal_buf);
 }
 
-static int wcn36xx_config(struct ieee80211_hw *hw, u32 changed)
+static void wcn36xx_change_ps(struct wcn36xx *wcn, bool enable)
 {
-	struct wcn36xx *wcn = hw->priv;
 	struct ieee80211_vif *vif = NULL;
 	struct wcn36xx_vif *tmp;
 
+	list_for_each_entry(tmp, &wcn->vif_list, list) {
+		vif = wcn36xx_priv_to_vif(tmp);
+		if (enable && !wcn->sw_scan) {
+			if (vif->bss_conf.ps) /* ps allowed ? */
+				wcn36xx_pmc_enter_bmps_state(wcn, vif);
+		} else {
+			wcn36xx_pmc_exit_bmps_state(wcn, vif);
+		}
+	}
+}
+
+static void wcn36xx_change_opchannel(struct wcn36xx *wcn, int ch)
+{
+	struct ieee80211_vif *vif = NULL;
+	struct wcn36xx_vif *tmp;
+
+	list_for_each_entry(tmp, &wcn->vif_list, list) {
+		vif = wcn36xx_priv_to_vif(tmp);
+		wcn36xx_smd_switch_channel(wcn, vif, ch);
+	}
+}
+
+static int wcn36xx_config(struct ieee80211_hw *hw, u32 changed)
+{
+	struct wcn36xx *wcn = hw->priv;
+
 	wcn36xx_dbg(WCN36XX_DBG_MAC, "mac config changed 0x%08x\n", changed);
 
 	mutex_lock(&wcn->conf_mutex);
@@ -392,24 +415,30 @@ static int wcn36xx_config(struct ieee80211_hw *hw, u32 changed)
 		int ch = WCN36XX_HW_CHANNEL(wcn);
 		wcn36xx_dbg(WCN36XX_DBG_MAC, "wcn36xx_config channel switch=%d\n",
 			    ch);
-		list_for_each_entry(tmp, &wcn->vif_list, list) {
-			vif = wcn36xx_priv_to_vif(tmp);
-			wcn36xx_smd_switch_channel(wcn, vif, ch);
-		}
-	}
 
-	if (changed & IEEE80211_CONF_CHANGE_PS) {
-		list_for_each_entry(tmp, &wcn->vif_list, list) {
-			vif = wcn36xx_priv_to_vif(tmp);
-			if (hw->conf.flags & IEEE80211_CONF_PS) {
-				if (vif->bss_conf.ps) /* ps allowed ? */
-					wcn36xx_pmc_enter_bmps_state(wcn, vif);
-			} else {
-				wcn36xx_pmc_exit_bmps_state(wcn, vif);
-			}
+		if (wcn->sw_scan_opchannel == ch) {
+			/* If channel is the initial operating channel, we may
+			 * want to receive/transmit regular data packets, then
+			 * simply stop the scan session and return into
+			 * operating mode.
+			 */
+			wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN,
+						wcn->sw_scan_vif, ch);
+		} else if (wcn->sw_scan) {
+			/* A scan is ongoing, do not change the operating
+			 * channel, but start a scan session on the channel.
+			 */
+			wcn36xx_smd_init_scan(wcn, HAL_SYS_MODE_SCAN,
+					      wcn->sw_scan_vif);
+			wcn36xx_smd_start_scan(wcn, ch);
+		} else {
+			wcn36xx_change_opchannel(wcn, ch);
 		}
 	}
 
+	if (changed & IEEE80211_CONF_CHANGE_PS)
+		wcn36xx_change_ps(wcn, hw->conf.flags & IEEE80211_CONF_PS);
+
 	mutex_unlock(&wcn->conf_mutex);
 
 	return 0;
@@ -614,55 +643,26 @@ static int wcn36xx_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 	return ret;
 }
 
-static void wcn36xx_hw_scan_worker(struct work_struct *work)
+static int wcn36xx_hw_scan(struct ieee80211_hw *hw,
+			   struct ieee80211_vif *vif,
+			   struct ieee80211_scan_request *hw_req)
 {
-	struct wcn36xx *wcn = container_of(work, struct wcn36xx, scan_work);
-	struct cfg80211_scan_request *req = wcn->scan_req;
-	u8 channels[WCN36XX_HAL_PNO_MAX_NETW_CHANNELS_EX];
-	struct cfg80211_scan_info scan_info = {};
-	bool aborted = false;
+	struct wcn36xx *wcn = hw->priv;
 	int i;
 
-	wcn36xx_dbg(WCN36XX_DBG_MAC, "mac80211 scan %d channels worker\n", req->n_channels);
-
-	for (i = 0; i < req->n_channels; i++)
-		channels[i] = req->channels[i]->hw_value;
-
-	wcn36xx_smd_update_scan_params(wcn, channels, req->n_channels);
-
-	wcn36xx_smd_init_scan(wcn, HAL_SYS_MODE_SCAN);
-	for (i = 0; i < req->n_channels; i++) {
-		mutex_lock(&wcn->scan_lock);
-		aborted = wcn->scan_aborted;
-		mutex_unlock(&wcn->scan_lock);
-
-		if (aborted)
-			break;
-
-		wcn->scan_freq = req->channels[i]->center_freq;
-		wcn->scan_band = req->channels[i]->band;
-
-		wcn36xx_smd_start_scan(wcn, req->channels[i]->hw_value);
-		msleep(30);
-		wcn36xx_smd_end_scan(wcn, req->channels[i]->hw_value);
-
-		wcn->scan_freq = 0;
+	if (!get_feat_caps(wcn->fw_feat_caps, SCAN_OFFLOAD)) {
+		/* fallback to mac80211 software scan */
+		return 1;
 	}
-	wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN);
 
-	scan_info.aborted = aborted;
-	ieee80211_scan_completed(wcn->hw, &scan_info);
-
-	mutex_lock(&wcn->scan_lock);
-	wcn->scan_req = NULL;
-	mutex_unlock(&wcn->scan_lock);
-}
+	/* For unknown reason, the hardware offloaded scan only works with
+	 * 2.4Ghz channels, fallback to software scan in other cases.
+	 */
+	for (i = 0; i < hw_req->req.n_channels; i++) {
+		if (hw_req->req.channels[i]->band != NL80211_BAND_2GHZ)
+			return 1;
+	}
 
-static int wcn36xx_hw_scan(struct ieee80211_hw *hw,
-			   struct ieee80211_vif *vif,
-			   struct ieee80211_scan_request *hw_req)
-{
-	struct wcn36xx *wcn = hw->priv;
 	mutex_lock(&wcn->scan_lock);
 	if (wcn->scan_req) {
 		mutex_unlock(&wcn->scan_lock);
@@ -674,12 +674,6 @@ static int wcn36xx_hw_scan(struct ieee80211_hw *hw,
 
 	mutex_unlock(&wcn->scan_lock);
 
-	if (!get_feat_caps(wcn->fw_feat_caps, SCAN_OFFLOAD)) {
-		/* legacy manual/sw scan */
-		schedule_work(&wcn->scan_work);
-		return 0;
-	}
-
 	return wcn36xx_smd_start_hw_scan(wcn, vif, &hw_req->req);
 }
 
@@ -696,16 +690,32 @@ static void wcn36xx_cancel_hw_scan(struct ieee80211_hw *hw,
 		/* ieee80211_scan_completed will be called on FW scan
 		 * indication */
 		wcn36xx_smd_stop_hw_scan(wcn);
-	} else {
-		struct cfg80211_scan_info scan_info = {
-			.aborted = true,
-		};
-
-		cancel_work_sync(&wcn->scan_work);
-		ieee80211_scan_completed(wcn->hw, &scan_info);
 	}
 }
 
+static void wcn36xx_sw_scan_start(struct ieee80211_hw *hw,
+				  struct ieee80211_vif *vif,
+				  const u8 *mac_addr)
+{
+	struct wcn36xx *wcn = hw->priv;
+
+	wcn->sw_scan = true;
+	wcn->sw_scan_vif = vif;
+	wcn->sw_scan_opchannel = WCN36XX_HW_CHANNEL(wcn);
+}
+
+static void wcn36xx_sw_scan_complete(struct ieee80211_hw *hw,
+				     struct ieee80211_vif *vif)
+{
+	struct wcn36xx *wcn = hw->priv;
+
+	/* ensure that any scan session is finished */
+	wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN, wcn->sw_scan_vif,
+				wcn->sw_scan_opchannel);
+	wcn->sw_scan = false;
+	wcn->sw_scan_opchannel = 0;
+}
+
 static void wcn36xx_update_allowed_rates(struct ieee80211_sta *sta,
 					 enum nl80211_band band)
 {
@@ -1151,6 +1161,8 @@ static const struct ieee80211_ops wcn36xx_ops = {
 	.set_key		= wcn36xx_set_key,
 	.hw_scan		= wcn36xx_hw_scan,
 	.cancel_hw_scan		= wcn36xx_cancel_hw_scan,
+	.sw_scan_start		= wcn36xx_sw_scan_start,
+	.sw_scan_complete	= wcn36xx_sw_scan_complete,
 	.bss_info_changed	= wcn36xx_bss_info_changed,
 	.set_rts_threshold	= wcn36xx_set_rts_threshold,
 	.sta_add		= wcn36xx_sta_add,
@@ -1329,8 +1341,6 @@ static int wcn36xx_probe(struct platform_device *pdev)
 		goto out_wq;
 	}
 
-	INIT_WORK(&wcn->scan_work, wcn36xx_hw_scan_worker);
-
 	wcn->smd_channel = qcom_wcnss_open_channel(wcnss, "WLAN_CTRL", wcn36xx_smd_rsp_process, hw);
 	if (IS_ERR(wcn->smd_channel)) {
 		wcn36xx_err("failed to open WLAN_CTRL channel\n");
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 3fc2d46..e58b038 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -517,8 +517,10 @@ int wcn36xx_smd_stop(struct wcn36xx *wcn)
 	return ret;
 }
 
-int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode)
+int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode,
+			  struct ieee80211_vif *vif)
 {
+	struct wcn36xx_vif *vif_priv = wcn36xx_vif_to_priv(vif);
 	struct wcn36xx_hal_init_scan_req_msg msg_body;
 	int ret;
 
@@ -526,6 +528,13 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode)
 	INIT_HAL_MSG(msg_body, WCN36XX_HAL_INIT_SCAN_REQ);
 
 	msg_body.mode = mode;
+	if (vif_priv->bss_index != WCN36XX_HAL_BSS_INVALID_IDX) {
+		/* Notify BSSID with null DATA packet */
+		msg_body.frame_type = 2;
+		msg_body.notify = 1;
+		msg_body.scan_entry.bss_index[0] = vif_priv->bss_index;
+		msg_body.scan_entry.active_bss_count = 1;
+	}
 
 	PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
 
@@ -607,8 +616,10 @@ int wcn36xx_smd_end_scan(struct wcn36xx *wcn, u8 scan_channel)
 }
 
 int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
-			    enum wcn36xx_hal_sys_mode mode)
+			    enum wcn36xx_hal_sys_mode mode,
+			    struct ieee80211_vif *vif, u8 channel)
 {
+	struct wcn36xx_vif *vif_priv = wcn36xx_vif_to_priv(vif);
 	struct wcn36xx_hal_finish_scan_req_msg msg_body;
 	int ret;
 
@@ -616,6 +627,14 @@ int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
 	INIT_HAL_MSG(msg_body, WCN36XX_HAL_FINISH_SCAN_REQ);
 
 	msg_body.mode = mode;
+	if (vif_priv->bss_index != WCN36XX_HAL_BSS_INVALID_IDX) {
+		/* Notify BSSID with null data packet */
+		msg_body.notify = 1;
+		msg_body.frame_type = 2;
+		msg_body.oper_channel = channel;
+		msg_body.scan_entry.bss_index[0] = vif_priv->bss_index;
+		msg_body.scan_entry.active_bss_count = 1;
+	}
 
 	PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
 
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
index 68c59df..ffe8b0c 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.h
+++ b/drivers/net/wireless/ath/wcn36xx/smd.h
@@ -59,11 +59,13 @@ void wcn36xx_smd_close(struct wcn36xx *wcn);
 int wcn36xx_smd_load_nv(struct wcn36xx *wcn);
 int wcn36xx_smd_start(struct wcn36xx *wcn);
 int wcn36xx_smd_stop(struct wcn36xx *wcn);
-int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode);
 int wcn36xx_smd_start_scan(struct wcn36xx *wcn, u8 scan_channel);
 int wcn36xx_smd_end_scan(struct wcn36xx *wcn, u8 scan_channel);
-int wcn36xx_smd_finish_scan(struct wcn36xx *wcn,
-			    enum wcn36xx_hal_sys_mode mode);
+int wcn36xx_smd_finish_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode,
+			    struct ieee80211_vif *vif, u8 channel);
+int wcn36xx_smd_init_scan(struct wcn36xx *wcn, enum wcn36xx_hal_sys_mode mode,
+			  struct ieee80211_vif *vif);
+
 int wcn36xx_smd_update_scan_params(struct wcn36xx *wcn, u8 *channels, size_t channel_count);
 int wcn36xx_smd_start_hw_scan(struct wcn36xx *wcn, struct ieee80211_vif *vif,
 			      struct cfg80211_scan_request *req);
diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
index dcc2ec0..c9cf3db 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.c
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
@@ -49,15 +49,8 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb)
 	fc = __le16_to_cpu(hdr->frame_control);
 	sn = IEEE80211_SEQ_TO_SN(__le16_to_cpu(hdr->seq_ctrl));
 
-	/* When scanning associate beacons to this */
-	if (ieee80211_is_beacon(hdr->frame_control) && wcn->scan_freq) {
-		status.freq = wcn->scan_freq;
-		status.band = wcn->scan_band;
-	} else {
-		status.freq = WCN36XX_CENTER_FREQ(wcn);
-		status.band = WCN36XX_BAND(wcn);
-	}
-
+	status.freq = WCN36XX_CENTER_FREQ(wcn);
+	status.band = WCN36XX_BAND(wcn);
 	status.mactime = 10;
 	status.signal = -get_rssi0(bd);
 	status.antenna = 1;
diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
index 2d89849..3221fed 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
@@ -223,10 +223,10 @@ struct wcn36xx {
 	spinlock_t		hal_ind_lock;
 	struct list_head	hal_ind_queue;
 
-	struct work_struct	scan_work;
 	struct cfg80211_scan_request *scan_req;
-	int			scan_freq;
-	int			scan_band;
+	bool			sw_scan;
+	u8			sw_scan_opchannel;
+	struct ieee80211_vif	*sw_scan_vif;
 	struct mutex		scan_lock;
 	bool			scan_aborted;
 
-- 
2.7.4

Re: [PATCH 1/4] wcn36xx: Fix multiple AMPDU sessions support

[Kalle Valo]

Loic Poulain <loic.poulain@linaro.org> wrote:

> Several AMPDU sessions can be started, e.g. for different TIDs.
> Currently the driver does not take care of the session ID when
> requesting block-ack (statically set to 0), which leads to never
> block-acked packet with sessions other than 0.
> 
> Fix this by saving the session id when creating the ba session and
> use it in subsequent ba operations.
> 
> This issue can be reproduced with iperf in two steps (tid 0 strem
> then tid 6 stream).
> 
> 1.0 iperf -s                                # wcn36xx side
> 1.1 iperf -c ${IP_ADDR}                     # host side
> 
> Then
> 
> 2.0 iperf -s -u -S 0xC0                     # wcn36xx side
> 2.1 iperf -c ${IP_ADDR} -u -S 0xC0 -l 2000  # host side
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

What's the difference from the earlier version:

https://patchwork.kernel.org/patch/11609871/

A changelog would be nice.

-- 
https://patchwork.kernel.org/patch/11633975/

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

Re: [PATCH 4/4] wcn36xx: Fix software-driven scan

[Bryan O'Donoghue]

On 30/06/2020 14:47, Loic Poulain wrote:
> For software-driven scan, rely on mac80211 software scan instead
> of internal driver implementation. The internal implementation
> cause connection trouble since it keeps the antenna busy during
> the entire scan duration, moreover it's only a passive scanning
> (no probe request). Therefore, let mac80211 manages sw scan.
> 
> Note: we fallback to software scan if firmware does not report
> scan offload support or if we need to scan the 5Ghz band (currently
> not supported by the offload scan...).
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

Validated on a aqp8039/wcn3680b

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

Re: [PATCH 1/4] wcn36xx: Fix multiple AMPDU sessions support

[Kalle Valo]

Loic Poulain <loic.poulain@linaro.org> writes:

> On Wed, 15 Jul 2020 at 17:33, Kalle Valo <kvalo@codeaurora.org> wrote:
>
>     Loic Poulain <loic.poulain@linaro.org> wrote:
>     
>     > Several AMPDU sessions can be started, e.g. for different TIDs.
>     > Currently the driver does not take care of the session ID when
>     > requesting block-ack (statically set to 0), which leads to never
>     > block-acked packet with sessions other than 0.
>     > 
>     > Fix this by saving the session id when creating the ba session
>     and
>     > use it in subsequent ba operations.
>     > 
>     > This issue can be reproduced with iperf in two steps (tid 0
>     strem
>     > then tid 6 stream).
>     > 
>     > 1.0 iperf -s # wcn36xx side
>     > 1.1 iperf -c ${IP_ADDR} # host side
>     > 
>     > Then
>     > 
>     > 2.0 iperf -s -u -S 0xC0 # wcn36xx side
>     > 2.1 iperf -c ${IP_ADDR} -u -S 0xC0 -l 2000 # host side
>     > 
>     > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>     
>     What's the difference from the earlier version:
>     
>     https://patchwork.kernel.org/patch/11609871/
>     
>     A changelog would be nice.
>     
>
> There is no change, but I've simply included it in this series.
> I can resend the series without this one if necessary so that you can
> consider only the initial one.

No need to resend, I just wanted to understand if there are any changes.
In the future try to always include a changelog, that way I don't need
to guess if something has changed or not.

And don't send HTML mail, linux-wireless list drops it and then your
mail won't be visible in the patchwork either.

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

Re: [PATCH 1/4] wcn36xx: Fix multiple AMPDU sessions support

[Bryan O'Donoghue]

On 30/06/2020 14:46, Loic Poulain wrote:
> Several AMPDU sessions can be started, e.g. for different TIDs.
> Currently the driver does not take care of the session ID when
> requesting block-ack (statically set to 0), which leads to never
> block-acked packet with sessions other than 0.
> 
> Fix this by saving the session id when creating the ba session and
> use it in subsequent ba operations.
> 
> This issue can be reproduced with iperf in two steps (tid 0 strem
> then tid 6 stream).
> 
> 1.0 iperf -s                                # wcn36xx side
> 1.1 iperf -c ${IP_ADDR}                     # host side
> 
> Then
> 
> 2.0 iperf -s -u -S 0xC0                     # wcn36xx side
> 2.1 iperf -c ${IP_ADDR} -u -S 0xC0 -l 2000  # host side
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---

Getting a few checkpatch errors here

CHECK: No space is necessary after a cast
#44: FILE: drivers/net/wireless/ath/wcn36xx/smd.c:2112:
+	rsp = (struct wcn36xx_hal_add_ba_session_rsp_msg *) buf;

WARNING: Comparisons should place the constant on the right side of the test
#45: FILE: drivers/net/wireless/ath/wcn36xx/smd.c:2113:
+	if (WCN36XX_FW_MSG_RESULT_SUCCESS != rsp->status)

total: 0 errors, 1 warnings, 1 checks, 116 lines checked

Re: [PATCH 2/4] wcn36xx: Add TX ack support

[Kalle Valo]

Loic Poulain <loic.poulain@linaro.org> writes:

> The controller is capable of reporting TX indication which can be used
> to report TX ack. It was only partially implemented.
>
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

[...]

> +static void wcn36xx_dxe_tx_timer(struct timer_list *t)
> +{
> +	struct wcn36xx *wcn = from_timer(wcn, t, tx_ack_timer);
> +	struct ieee80211_tx_info *info;
> +	unsigned long flags;
> +	struct sk_buff *skb;
> +
> +	/* TX Timeout */
> +	wcn36xx_dbg(WCN36XX_DBG_DXE, "TX timeout\n");
> +
> +	spin_lock_irqsave(&wcn->dxe_lock, flags);
> +	skb = wcn->tx_ack_skb;
> +	wcn->tx_ack_skb = NULL;
> +	spin_unlock_irqrestore(&wcn->dxe_lock, flags);
> +
> +	if (!skb)
> +		return;
> +
> +	info = IEEE80211_SKB_CB(skb);
> +	info->flags &= ~IEEE80211_TX_STAT_ACK;
> +	info->flags &= ~IEEE80211_TX_STAT_NOACK_TRANSMITTED;
> +
> +	ieee80211_tx_status_irqsafe(wcn->hw, skb);
> +	ieee80211_wake_queues(wcn->hw);
> +}

What's this timer thing? The commit log mentions nothing about that. To
me this looks like a some kind of TX timeout detection and has nothing
to do ACK handling, but of course I might have misunderstood.

Should it be in a separate patch to follow one logical change per patch
rule?

> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -45,8 +45,8 @@ static struct wcn36xx_cfg_val wcn36xx_cfg_vals[] = {
>  	WCN36XX_CFG_VAL(MAX_MEDIUM_TIME, 6000),
>  	WCN36XX_CFG_VAL(MAX_MPDUS_IN_AMPDU, 64),
>  	WCN36XX_CFG_VAL(RTS_THRESHOLD, 2347),
> -	WCN36XX_CFG_VAL(SHORT_RETRY_LIMIT, 6),
> -	WCN36XX_CFG_VAL(LONG_RETRY_LIMIT, 6),
> +	WCN36XX_CFG_VAL(SHORT_RETRY_LIMIT, 15),
> +	WCN36XX_CFG_VAL(LONG_RETRY_LIMIT, 15),

Also there's no mention of these changes in the commit log. Should these
in a third patch as they are more like a separate change?

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

Re: [PATCH 2/4] wcn36xx: Add TX ack support

[Loic Poulain]

On Mon, 20 Jul 2020 at 19:19, Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Loic Poulain <loic.poulain@linaro.org> writes:
>
> > The controller is capable of reporting TX indication which can be used
> > to report TX ack. It was only partially implemented.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>
> [...]
>
> > +static void wcn36xx_dxe_tx_timer(struct timer_list *t)
> > +{
> > +     struct wcn36xx *wcn = from_timer(wcn, t, tx_ack_timer);
> > +     struct ieee80211_tx_info *info;
> > +     unsigned long flags;
> > +     struct sk_buff *skb;
> > +
> > +     /* TX Timeout */
> > +     wcn36xx_dbg(WCN36XX_DBG_DXE, "TX timeout\n");
> > +
> > +     spin_lock_irqsave(&wcn->dxe_lock, flags);
> > +     skb = wcn->tx_ack_skb;
> > +     wcn->tx_ack_skb = NULL;
> > +     spin_unlock_irqrestore(&wcn->dxe_lock, flags);
> > +
> > +     if (!skb)
> > +             return;
> > +
> > +     info = IEEE80211_SKB_CB(skb);
> > +     info->flags &= ~IEEE80211_TX_STAT_ACK;
> > +     info->flags &= ~IEEE80211_TX_STAT_NOACK_TRANSMITTED;
> > +
> > +     ieee80211_tx_status_irqsafe(wcn->hw, skb);
> > +     ieee80211_wake_queues(wcn->hw);
> > +}
>
> What's this timer thing? The commit log mentions nothing about that. To
> me this looks like a some kind of TX timeout detection and has nothing
> to do ACK handling, but of course I might have misunderstood.
>
> Should it be in a separate patch to follow one logical change per patch
> rule?

This is part of ack management, I could have named it dex_ack_timer though...
When we send a packet requesting explicit ack notification via mac80211
status callback, we ask the firmware to return an ack event and stop tx queue
temporarily until the ack event is received. But if for any reasons the 802.11
packet is not acked, firmware never sends this event, causing stale of the
TX path.

So the timer is here to handle the no-ack case, in order to
1. run the mac80211 status callback (packet not acked)
2. unblock TX queue

> > --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> > +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> > @@ -45,8 +45,8 @@ static struct wcn36xx_cfg_val wcn36xx_cfg_vals[] = {
> >       WCN36XX_CFG_VAL(MAX_MEDIUM_TIME, 6000),
> >       WCN36XX_CFG_VAL(MAX_MPDUS_IN_AMPDU, 64),
> >       WCN36XX_CFG_VAL(RTS_THRESHOLD, 2347),
> > -     WCN36XX_CFG_VAL(SHORT_RETRY_LIMIT, 6),
> > -     WCN36XX_CFG_VAL(LONG_RETRY_LIMIT, 6),
> > +     WCN36XX_CFG_VAL(SHORT_RETRY_LIMIT, 15),
> > +     WCN36XX_CFG_VAL(LONG_RETRY_LIMIT, 15),
>
> Also there's no mention of these changes in the commit log. Should these
> in a third patch as they are more like a separate change?

Correct, this change increases the number of TX retries, to improve robustness,
and have more chances to have a packet acked. 15 is the default value defined
by the downstream driver. I observed much less ack timeout in a noisy
environment with that change.

So I can rework the commit for with ack timer info, and move the
config change in a separate change.

Regards,
Loic