diff mbox series

[2/4] wcn36xx: Add TX ack support

Message ID 1593524821-32115-2-git-send-email-loic.poulain@linaro.org
State Superseded
Headers show
Series [1/4] wcn36xx: Fix multiple AMPDU sessions support | expand

Commit Message

Loic Poulain June 30, 2020, 1:46 p.m. UTC 
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(-)

-- 
2.7.4

Comments

Kalle Valo July 20, 2020, 5:19 p.m. UTC | #1 
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
Loic Poulain July 21, 2020, 10:16 a.m. UTC | #2 
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
diff mbox series

Patch

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;