[PATCH 1/2] ath9k: fix powersave frame filtering/buffering in AP mode

[PATCH 1/2] ath9k: fix powersave frame filtering/buffering in AP mode

[Felix Fietkau]

This patch fixes a long standing issue of pending packets in the queue being
sent (and retransmitted many times) to sleeping stations.
This was made worse by aggregation through driver-internal retransmitting
of A-MDPU subframes.
Previously the hardware tx filter was cleared unconditionally for every
single packet - with this patch it uses the IEEE80211_TX_CTL_CLEAR_PS_FILT
for unaggregated frames.
A sta_notify driver op is added to stop aggregation for stations when they
enter powersave mode. Subframes stay buffered inside the driver, to ensure
that the BlockAck window keeps a sane state.
Since the driver uses software aggregation, the clearing of the tx filter
needs to be handled by the driver instead of mac80211 for aggregated frames.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/wireless/ath/ath9k/ar9002_mac.c |   12 +++-
 drivers/net/wireless/ath/ath9k/ar9003_mac.c |   12 +++-
 drivers/net/wireless/ath/ath9k/ath9k.h      |    6 ++
 drivers/net/wireless/ath/ath9k/hw-ops.h     |    5 ++
 drivers/net/wireless/ath/ath9k/hw.h         |    1 +
 drivers/net/wireless/ath/ath9k/mac.h        |    1 -
 drivers/net/wireless/ath/ath9k/main.c       |   22 ++++++
 drivers/net/wireless/ath/ath9k/xmit.c       |  101 +++++++++++++++++++++++---
 8 files changed, 145 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9002_mac.c b/drivers/net/wireless/ath/ath9k/ar9002_mac.c
index 8dd8f63..c338efb 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_mac.c
+++ b/drivers/net/wireless/ath/ath9k/ar9002_mac.c
@@ -290,7 +290,6 @@ static void ar9002_hw_set11n_txdesc(struct ath_hw *ah, void *ds,
 		| (flags & ATH9K_TXDESC_VMF ? AR_VirtMoreFrag : 0)
 		| SM(txPower, AR_XmitPower)
 		| (flags & ATH9K_TXDESC_VEOL ? AR_VEOL : 0)
-		| (flags & ATH9K_TXDESC_CLRDMASK ? AR_ClrDestMask : 0)
 		| (flags & ATH9K_TXDESC_INTREQ ? AR_TxIntrReq : 0)
 		| (keyIx != ATH9K_TXKEYIX_INVALID ? AR_DestIdxValid : 0);
 
@@ -311,6 +310,16 @@ static void ar9002_hw_set11n_txdesc(struct ath_hw *ah, void *ds,
 	}
 }
 
+static void ar9002_hw_set_clrdmask(struct ath_hw *ah, void *ds, bool val)
+{
+	struct ar5416_desc *ads = AR5416DESC(ds);
+
+	if (val)
+		ads->ds_ctl0 |= AR_ClrDestMask;
+	else
+		ads->ds_ctl0 &= ~AR_ClrDestMask;
+}
+
 static void ar9002_hw_set11n_ratescenario(struct ath_hw *ah, void *ds,
 					  void *lastds,
 					  u32 durUpdateEn, u32 rtsctsRate,
@@ -448,4 +457,5 @@ void ar9002_hw_attach_mac_ops(struct ath_hw *ah)
 	ops->set11n_aggr_last = ar9002_hw_set11n_aggr_last;
 	ops->clr11n_aggr = ar9002_hw_clr11n_aggr;
 	ops->set11n_burstduration = ar9002_hw_set11n_burstduration;
+	ops->set_clrdmask = ar9002_hw_set_clrdmask;
 }
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_mac.c b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
index 724ac24..c1264d6 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_mac.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
@@ -329,7 +329,6 @@ static void ar9003_hw_set11n_txdesc(struct ath_hw *ah, void *ds,
 		| (flags & ATH9K_TXDESC_VMF ? AR_VirtMoreFrag : 0)
 		| SM(txpower, AR_XmitPower)
 		| (flags & ATH9K_TXDESC_VEOL ? AR_VEOL : 0)
-		| (flags & ATH9K_TXDESC_CLRDMASK ? AR_ClrDestMask : 0)
 		| (keyIx != ATH9K_TXKEYIX_INVALID ? AR_DestIdxValid : 0)
 		| (flags & ATH9K_TXDESC_LOWRXCHAIN ? AR_LowRxChain : 0);
 
@@ -350,6 +349,16 @@ static void ar9003_hw_set11n_txdesc(struct ath_hw *ah, void *ds,
 	ads->ctl22 = 0;
 }
 
+static void ar9003_hw_set_clrdmask(struct ath_hw *ah, void *ds, bool val)
+{
+	struct ar9003_txc *ads = (struct ar9003_txc *) ds;
+
+	if (val)
+		ads->ctl11 |= AR_ClrDestMask;
+	else
+		ads->ctl11 &= ~AR_ClrDestMask;
+}
+
 static void ar9003_hw_set11n_ratescenario(struct ath_hw *ah, void *ds,
 					  void *lastds,
 					  u32 durUpdateEn, u32 rtsctsRate,
@@ -510,6 +519,7 @@ void ar9003_hw_attach_mac_ops(struct ath_hw *hw)
 	ops->set11n_aggr_last = ar9003_hw_set11n_aggr_last;
 	ops->clr11n_aggr = ar9003_hw_clr11n_aggr;
 	ops->set11n_burstduration = ar9003_hw_set11n_burstduration;
+	ops->set_clrdmask = ar9003_hw_set_clrdmask;
 }
 
 void ath9k_hw_set_rx_bufsize(struct ath_hw *ah, u16 buf_size)
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 77ad407..a2ddabf 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -200,6 +200,7 @@ struct ath_atx_ac {
 	int sched;
 	struct list_head list;
 	struct list_head tid_q;
+	bool clear_ps_filter;
 };
 
 struct ath_frame_info {
@@ -257,6 +258,8 @@ struct ath_node {
 	struct ath_atx_ac ac[WME_NUM_AC];
 	u16 maxampdu;
 	u8 mpdudensity;
+
+	bool sleeping;
 };
 
 #define AGGR_CLEANUP         BIT(1)
@@ -338,6 +341,9 @@ int ath_tx_aggr_start(struct ath_softc *sc, struct ieee80211_sta *sta,
 void ath_tx_aggr_stop(struct ath_softc *sc, struct ieee80211_sta *sta, u16 tid);
 void ath_tx_aggr_resume(struct ath_softc *sc, struct ieee80211_sta *sta, u16 tid);
 
+void ath_tx_aggr_wakeup(struct ath_softc *sc, struct ath_node *an);
+bool ath_tx_aggr_sleep(struct ath_softc *sc, struct ath_node *an);
+
 /********/
 /* VIFs */
 /********/
diff --git a/drivers/net/wireless/ath/ath9k/hw-ops.h b/drivers/net/wireless/ath/ath9k/hw-ops.h
index 22ee888..9dd90a8 100644
--- a/drivers/net/wireless/ath/ath9k/hw-ops.h
+++ b/drivers/net/wireless/ath/ath9k/hw-ops.h
@@ -122,6 +122,11 @@ static inline void ath9k_hw_set11n_burstduration(struct ath_hw *ah, void *ds,
 	ath9k_hw_ops(ah)->set11n_burstduration(ah, ds, burstDuration);
 }
 
+static inline void ath9k_hw_set_clrdmask(struct ath_hw *ah, void *ds, bool val)
+{
+	ath9k_hw_ops(ah)->set_clrdmask(ah, ds, val);
+}
+
 /* Private hardware call ops */
 
 /* PHY ops */
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 073bc9e..1018d6c 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -626,6 +626,7 @@ struct ath_hw_ops {
 	void (*clr11n_aggr)(struct ath_hw *ah, void *ds);
 	void (*set11n_burstduration)(struct ath_hw *ah, void *ds,
 				     u32 burstDuration);
+	void (*set_clrdmask)(struct ath_hw *ah, void *ds, bool val);
 };
 
 struct ath_nf_limits {
diff --git a/drivers/net/wireless/ath/ath9k/mac.h b/drivers/net/wireless/ath/ath9k/mac.h
index c2a5938..b60c130 100644
--- a/drivers/net/wireless/ath/ath9k/mac.h
+++ b/drivers/net/wireless/ath/ath9k/mac.h
@@ -239,7 +239,6 @@ struct ath_desc {
 	void *ds_vdata;
 } __packed __aligned(4);
 
-#define ATH9K_TXDESC_CLRDMASK		0x0001
 #define ATH9K_TXDESC_NOACK		0x0002
 #define ATH9K_TXDESC_RTSENA		0x0004
 #define ATH9K_TXDESC_CTSENA		0x0008
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 3580bc1..44f4d59 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1753,6 +1753,27 @@ static int ath9k_sta_remove(struct ieee80211_hw *hw,
 	return 0;
 }
 
+static void ath9k_sta_notify(struct ieee80211_hw *hw,
+			 struct ieee80211_vif *vif,
+			 enum sta_notify_cmd cmd,
+			 struct ieee80211_sta *sta)
+{
+	struct ath_softc *sc = hw->priv;
+	struct ath_node *an = (struct ath_node *) sta->drv_priv;
+
+	switch (cmd) {
+	case STA_NOTIFY_SLEEP:
+		an->sleeping = true;
+		if (ath_tx_aggr_sleep(sc, an))
+			ieee80211_sta_set_tim(sta);
+		break;
+	case STA_NOTIFY_AWAKE:
+		an->sleeping = false;
+		ath_tx_aggr_wakeup(sc, an);
+		break;
+	}
+}
+
 static int ath9k_conf_tx(struct ieee80211_hw *hw, u16 queue,
 			 const struct ieee80211_tx_queue_params *params)
 {
@@ -2238,6 +2259,7 @@ struct ieee80211_ops ath9k_ops = {
 	.configure_filter   = ath9k_configure_filter,
 	.sta_add	    = ath9k_sta_add,
 	.sta_remove	    = ath9k_sta_remove,
+	.sta_notify         = ath9k_sta_notify,
 	.conf_tx 	    = ath9k_conf_tx,
 	.bss_info_changed   = ath9k_bss_info_changed,
 	.set_key            = ath9k_set_key,
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 3cea3f7..a1230c0 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -357,6 +357,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
 	struct ath_frame_info *fi;
 	int nframes;
 	u8 tidno;
+	bool clear_filter;
 
 	skb = bf->bf_mpdu;
 	hdr = (struct ieee80211_hdr *)skb->data;
@@ -441,22 +442,24 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
 			/* transmit completion */
 			acked_cnt++;
 		} else {
-			if (!(tid->state & AGGR_CLEANUP) && retry) {
-				if (fi->retries < ATH_MAX_SW_RETRIES) {
-					ath_tx_set_retry(sc, txq, bf->bf_mpdu);
-					txpending = 1;
-				} else {
-					bf->bf_state.bf_type |= BUF_XRETRY;
-					txfail = 1;
-					sendbar = 1;
-					txfail_cnt++;
-				}
-			} else {
+			if ((tid->state & AGGR_CLEANUP) || !retry) {
 				/*
 				 * cleanup in progress, just fail
 				 * the un-acked sub-frames
 				 */
 				txfail = 1;
+			} else if (fi->retries < ATH_MAX_SW_RETRIES) {
+				if (!(ts->ts_status & ATH9K_TXERR_FILT) ||
+				    !an->sleeping)
+					ath_tx_set_retry(sc, txq, bf->bf_mpdu);
+
+				clear_filter = true;
+				txpending = 1;
+			} else {
+				bf->bf_state.bf_type |= BUF_XRETRY;
+				txfail = 1;
+				sendbar = 1;
+				txfail_cnt++;
 			}
 		}
 
@@ -496,6 +499,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
 				!txfail, sendbar);
 		} else {
 			/* retry the un-acked ones */
+			ath9k_hw_set_clrdmask(sc->sc_ah, bf->bf_desc, false);
 			if (!(sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA)) {
 				if (bf->bf_next == NULL && bf_last->bf_stale) {
 					struct ath_buf *tbf;
@@ -546,7 +550,12 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
 
 	/* prepend un-acked frames to the beginning of the pending frame queue */
 	if (!list_empty(&bf_pending)) {
+		if (an->sleeping)
+			ieee80211_sta_set_tim(sta);
+
 		spin_lock_bh(&txq->axq_lock);
+		if (clear_filter)
+			tid->ac->clear_ps_filter = true;
 		list_splice(&bf_pending, &tid->buf_q);
 		ath_tx_queue_tid(txq, tid);
 		spin_unlock_bh(&txq->axq_lock);
@@ -816,6 +825,11 @@ static void ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
 		bf = list_first_entry(&bf_q, struct ath_buf, list);
 		bf->bf_lastbf = list_entry(bf_q.prev, struct ath_buf, list);
 
+		if (tid->ac->clear_ps_filter) {
+			tid->ac->clear_ps_filter = false;
+			ath9k_hw_set_clrdmask(sc->sc_ah, bf->bf_desc, true);
+		}
+
 		/* if only one frame, send as non-aggregate */
 		if (bf == bf->bf_lastbf) {
 			fi = get_frame_info(bf->bf_mpdu);
@@ -896,6 +910,67 @@ void ath_tx_aggr_stop(struct ath_softc *sc, struct ieee80211_sta *sta, u16 tid)
 	ath_tx_flush_tid(sc, txtid);
 }
 
+bool ath_tx_aggr_sleep(struct ath_softc *sc, struct ath_node *an)
+{
+	struct ath_atx_tid *tid;
+	struct ath_atx_ac *ac;
+	struct ath_txq *txq;
+	bool buffered = false;
+	int tidno;
+
+	for (tidno = 0, tid = &an->tid[tidno];
+	     tidno < WME_NUM_TID; tidno++, tid++) {
+
+		if (!tid->sched)
+			continue;
+
+		ac = tid->ac;
+		txq = ac->txq;
+
+		spin_lock_bh(&txq->axq_lock);
+
+		if (!list_empty(&tid->buf_q))
+			buffered = true;
+
+		tid->sched = false;
+		list_del(&tid->list);
+
+		if (ac->sched) {
+			ac->sched = false;
+			list_del(&ac->list);
+		}
+
+		spin_unlock_bh(&txq->axq_lock);
+	}
+
+	return buffered;
+}
+
+void ath_tx_aggr_wakeup(struct ath_softc *sc, struct ath_node *an)
+{
+	struct ath_atx_tid *tid;
+	struct ath_atx_ac *ac;
+	struct ath_txq *txq;
+	int tidno;
+
+	for (tidno = 0, tid = &an->tid[tidno];
+	     tidno < WME_NUM_TID; tidno++, tid++) {
+
+		ac = tid->ac;
+		txq = ac->txq;
+
+		spin_lock_bh(&txq->axq_lock);
+		ac->clear_ps_filter = true;
+
+		if (!list_empty(&tid->buf_q) && !tid->paused) {
+			ath_tx_queue_tid(txq, tid);
+			ath_txq_schedule(sc, txq);
+		}
+
+		spin_unlock_bh(&txq->axq_lock);
+	}
+}
+
 void ath_tx_aggr_resume(struct ath_softc *sc, struct ieee80211_sta *sta, u16 tid)
 {
 	struct ath_atx_tid *txtid;
@@ -1491,7 +1566,6 @@ static int setup_tx_flags(struct sk_buff *skb)
 	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
 	int flags = 0;
 
-	flags |= ATH9K_TXDESC_CLRDMASK; /* needed for crypto errors */
 	flags |= ATH9K_TXDESC_INTREQ;
 
 	if (tx_info->flags & IEEE80211_TX_CTL_NO_ACK)
@@ -1754,6 +1828,9 @@ static void ath_tx_start_dma(struct ath_softc *sc, struct ath_buf *bf,
 		if (txctl->paprd)
 			bf->bf_state.bfs_paprd_timestamp = jiffies;
 
+		if (tx_info->flags & IEEE80211_TX_CTL_CLEAR_PS_FILT)
+			ath9k_hw_set_clrdmask(sc->sc_ah, bf->bf_desc, true);
+
 		ath_tx_send_normal(sc, txctl->txq, tid, &bf_head);
 	}
 
-- 
1.7.3.2

[PATCH 2/2] ath9k: assign keycache slots to unencrypted stations

[Felix Fietkau]

Frame filtering relies on having a valid destination index (keycache slot),
to keep track of the destination. Assigning a keycache slot (configured
to unencrypted, with no key data attached) improves powersave handling in
AP mode with no encryption.
The dummy keycache entry for a station is cleared, when a real key gets
added.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/wireless/ath/ath9k/ath9k.h |    2 ++
 drivers/net/wireless/ath/ath9k/main.c  |   22 ++++++++++++++++++++++
 drivers/net/wireless/ath/ath9k/xmit.c  |   10 +++++++---
 drivers/net/wireless/ath/key.c         |    6 +++++-
 4 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index a2ddabf..a6b5388 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -256,6 +256,8 @@ struct ath_node {
 #endif
 	struct ath_atx_tid tid[WME_NUM_TID];
 	struct ath_atx_ac ac[WME_NUM_AC];
+	int ps_key;
+
 	u16 maxampdu;
 	u8 mpdudensity;
 
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 44f4d59..a8d9009 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1736,18 +1736,37 @@ static int ath9k_sta_add(struct ieee80211_hw *hw,
 			 struct ieee80211_sta *sta)
 {
 	struct ath_softc *sc = hw->priv;
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	struct ath_node *an = (struct ath_node *) sta->drv_priv;
+	struct ieee80211_key_conf ps_key = { };
 
 	ath_node_attach(sc, sta);
+	an->ps_key = ath_key_config(common, vif, sta, &ps_key);
 
 	return 0;
 }
 
+static void ath9k_del_ps_key(struct ath_softc *sc,
+			     struct ieee80211_vif *vif,
+			     struct ieee80211_sta *sta)
+{
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	struct ath_node *an = (struct ath_node *) sta->drv_priv;
+	struct ieee80211_key_conf ps_key = { .hw_key_idx = an->ps_key };
+
+	if (!an->ps_key)
+	    return;
+
+	ath_key_delete(common, &ps_key);
+}
+
 static int ath9k_sta_remove(struct ieee80211_hw *hw,
 			    struct ieee80211_vif *vif,
 			    struct ieee80211_sta *sta)
 {
 	struct ath_softc *sc = hw->priv;
 
+	ath9k_del_ps_key(sc, vif, sta);
 	ath_node_detach(sc, sta);
 
 	return 0;
@@ -1850,6 +1869,9 @@ static int ath9k_set_key(struct ieee80211_hw *hw,
 
 	switch (cmd) {
 	case SET_KEY:
+		if (sta)
+			ath9k_del_ps_key(sc, vif, sta);
+
 		ret = ath_key_config(common, vif, sta, key);
 		if (ret >= 0) {
 			key->hw_key_idx = ret;
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index a1230c0..e9e99f7 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1526,7 +1526,7 @@ static void setup_frame_info(struct ieee80211_hw *hw, struct sk_buff *skb,
 	struct ieee80211_key_conf *hw_key = tx_info->control.hw_key;
 	struct ieee80211_hdr *hdr;
 	struct ath_frame_info *fi = get_frame_info(skb);
-	struct ath_node *an;
+	struct ath_node *an = NULL;
 	struct ath_atx_tid *tid;
 	enum ath9k_key_type keytype;
 	u16 seqno = 0;
@@ -1534,11 +1534,13 @@ static void setup_frame_info(struct ieee80211_hw *hw, struct sk_buff *skb,
 
 	keytype = ath9k_cmn_get_hw_crypto_keytype(skb);
 
+	if (sta)
+		an = (struct ath_node *) sta->drv_priv;
+
 	hdr = (struct ieee80211_hdr *)skb->data;
-	if (sta && ieee80211_is_data_qos(hdr->frame_control) &&
+	if (an && ieee80211_is_data_qos(hdr->frame_control) &&
 		conf_is_ht(&hw->conf) && (sc->sc_flags & SC_OP_TXAGGR)) {
 
-		an = (struct ath_node *) sta->drv_priv;
 		tidno = ieee80211_get_qos_ctl(hdr)[0] & IEEE80211_QOS_CTL_TID_MASK;
 
 		/*
@@ -1554,6 +1556,8 @@ static void setup_frame_info(struct ieee80211_hw *hw, struct sk_buff *skb,
 	memset(fi, 0, sizeof(*fi));
 	if (hw_key)
 		fi->keyix = hw_key->hw_key_idx;
+	else if (an && ieee80211_is_data(hdr->frame_control) && an->ps_key > 0)
+		fi->keyix = an->ps_key;
 	else
 		fi->keyix = ATH9K_TXKEYIX_INVALID;
 	fi->keytype = keytype;
diff --git a/drivers/net/wireless/ath/key.c b/drivers/net/wireless/ath/key.c
index 0d4f39c..a61ef3d6 100644
--- a/drivers/net/wireless/ath/key.c
+++ b/drivers/net/wireless/ath/key.c
@@ -483,6 +483,9 @@ int ath_key_config(struct ath_common *common,
 	memset(&hk, 0, sizeof(hk));
 
 	switch (key->cipher) {
+	case 0:
+		hk.kv_type = ATH_CIPHER_CLR;
+		break;
 	case WLAN_CIPHER_SUITE_WEP40:
 	case WLAN_CIPHER_SUITE_WEP104:
 		hk.kv_type = ATH_CIPHER_WEP;
@@ -498,7 +501,8 @@ int ath_key_config(struct ath_common *common,
 	}
 
 	hk.kv_len = key->keylen;
-	memcpy(hk.kv_val, key->key, key->keylen);
+	if (key->keylen)
+		memcpy(hk.kv_val, key->key, key->keylen);
 
 	if (!(key->flags & IEEE80211_KEY_FLAG_PAIRWISE)) {
 		switch (vif->type) {
-- 
1.7.3.2

[PATCH 1/2] ath9k: fix powersave frame filtering/buffering in AP mode

[Sujith]

Felix Fietkau wrote:
> diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
> index 77ad407..a2ddabf 100644
> --- a/drivers/net/wireless/ath/ath9k/ath9k.h
> +++ b/drivers/net/wireless/ath/ath9k/ath9k.h
> @@ -200,6 +200,7 @@ struct ath_atx_ac {
>  	int sched;
>  	struct list_head list;
>  	struct list_head tid_q;
> +	bool clear_ps_filter;

The destination mask is per-station, so would ath_node be a better place for this
variable ?

> +static void ath9k_sta_notify(struct ieee80211_hw *hw,
> +			 struct ieee80211_vif *vif,
> +			 enum sta_notify_cmd cmd,
> +			 struct ieee80211_sta *sta)
> +{
> +	struct ath_softc *sc = hw->priv;
> +	struct ath_node *an = (struct ath_node *) sta->drv_priv;
> +
> +	switch (cmd) {
> +	case STA_NOTIFY_SLEEP:
> +		an->sleeping = true;
> +		if (ath_tx_aggr_sleep(sc, an))
> +			ieee80211_sta_set_tim(sta);
> +		break;
> +	case STA_NOTIFY_AWAKE:
> +		an->sleeping = false;
> +		ath_tx_aggr_wakeup(sc, an);
> +		break;
> +	}
> +}
> +

an->sleeping needs to be locked, no ?
It seems to be accessed from the TX tasklet also.

> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index 3cea3f7..a1230c0 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -357,6 +357,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
>  	struct ath_frame_info *fi;
>  	int nframes;
>  	u8 tidno;
> +	bool clear_filter;
>  
>  	skb = bf->bf_mpdu;
>  	hdr = (struct ieee80211_hdr *)skb->data;
> @@ -441,22 +442,24 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
>  			/* transmit completion */
>  			acked_cnt++;
>  		} else {
> -			if (!(tid->state & AGGR_CLEANUP) && retry) {
> -				if (fi->retries < ATH_MAX_SW_RETRIES) {
> -					ath_tx_set_retry(sc, txq, bf->bf_mpdu);
> -					txpending = 1;
> -				} else {
> -					bf->bf_state.bf_type |= BUF_XRETRY;
> -					txfail = 1;
> -					sendbar = 1;
> -					txfail_cnt++;
> -				}
> -			} else {
> +			if ((tid->state & AGGR_CLEANUP) || !retry) {
>  				/*
>  				 * cleanup in progress, just fail
>  				 * the un-acked sub-frames
>  				 */
>  				txfail = 1;
> +			} else if (fi->retries < ATH_MAX_SW_RETRIES) {
> +				if (!(ts->ts_status & ATH9K_TXERR_FILT) ||
> +				    !an->sleeping)
> +					ath_tx_set_retry(sc, txq, bf->bf_mpdu);
> +
> +				clear_filter = true;
> +				txpending = 1;
> +			} else {
> +				bf->bf_state.bf_type |= BUF_XRETRY;
> +				txfail = 1;
> +				sendbar = 1;
> +				txfail_cnt++;
>  			}
>  		}
>  
> @@ -496,6 +499,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
>  				!txfail, sendbar);
>  		} else {
>  			/* retry the un-acked ones */
> +			ath9k_hw_set_clrdmask(sc->sc_ah, bf->bf_desc, false);
>  			if (!(sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA)) {
>  				if (bf->bf_next == NULL && bf_last->bf_stale) {
>  					struct ath_buf *tbf;
> @@ -546,7 +550,12 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
>  
>  	/* prepend un-acked frames to the beginning of the pending frame queue */
>  	if (!list_empty(&bf_pending)) {
> +		if (an->sleeping)
> +			ieee80211_sta_set_tim(sta);
> +
>  		spin_lock_bh(&txq->axq_lock);
> +		if (clear_filter)
> +			tid->ac->clear_ps_filter = true;
>  		list_splice(&bf_pending, &tid->buf_q);
>  		ath_tx_queue_tid(txq, tid);
>  		spin_unlock_bh(&txq->axq_lock);
> @@ -816,6 +825,11 @@ static void ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
>  		bf = list_first_entry(&bf_q, struct ath_buf, list);
>  		bf->bf_lastbf = list_entry(bf_q.prev, struct ath_buf, list);
>  
> +		if (tid->ac->clear_ps_filter) {
> +			tid->ac->clear_ps_filter = false;
> +			ath9k_hw_set_clrdmask(sc->sc_ah, bf->bf_desc, true);
> +		}
> +

Can you explain a bit more on the flow ? How exactly is ieee80211_sta_set_tim() to be used
by a driver ? (I would like to port this fix to ath9k_htc).

>  		/* if only one frame, send as non-aggregate */
>  		if (bf == bf->bf_lastbf) {
>  			fi = get_frame_info(bf->bf_mpdu);
> @@ -896,6 +910,67 @@ void ath_tx_aggr_stop(struct ath_softc *sc, struct ieee80211_sta *sta, u16 tid)
>  	ath_tx_flush_tid(sc, txtid);
>  }
>  
> +bool ath_tx_aggr_sleep(struct ath_softc *sc, struct ath_node *an)
> +{
> +	struct ath_atx_tid *tid;
> +	struct ath_atx_ac *ac;
> +	struct ath_txq *txq;
> +	bool buffered = false;
> +	int tidno;
> +
> +	for (tidno = 0, tid = &an->tid[tidno];
> +	     tidno < WME_NUM_TID; tidno++, tid++) {
> +
> +		if (!tid->sched)
> +			continue;

tid->sched has to be locked ?

Sujith

Re: [PATCH 1/2] ath9k: fix powersave frame filtering/buffering in AP mode

[Felix Fietkau]

On 2011-04-18 6:26 AM, Sujith wrote:
> Felix Fietkau wrote:
>>  diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
>>  index 77ad407..a2ddabf 100644
>>  --- a/drivers/net/wireless/ath/ath9k/ath9k.h
>>  +++ b/drivers/net/wireless/ath/ath9k/ath9k.h
>>  @@ -200,6 +200,7 @@ struct ath_atx_ac {
>>   	int sched;
>>   	struct list_head list;
>>   	struct list_head tid_q;
>>  +	bool clear_ps_filter;
>
> The destination mask is per-station, so would ath_node be a better place for this
> variable ?
That's what I thought at first as well, but the problem with that is 
that when using multiple queues there is no guarantee that the A-MPDU 
enqueued first is also transmitted first, so I decided to do it once per 
AC. Most of the time, only BE is active anyway...

>>  +static void ath9k_sta_notify(struct ieee80211_hw *hw,
>>  +			 struct ieee80211_vif *vif,
>>  +			 enum sta_notify_cmd cmd,
>>  +			 struct ieee80211_sta *sta)
>>  +{
>>  +	struct ath_softc *sc = hw->priv;
>>  +	struct ath_node *an = (struct ath_node *) sta->drv_priv;
>>  +
>>  +	switch (cmd) {
>>  +	case STA_NOTIFY_SLEEP:
>>  +		an->sleeping = true;
>>  +		if (ath_tx_aggr_sleep(sc, an))
>>  +			ieee80211_sta_set_tim(sta);
>>  +		break;
>>  +	case STA_NOTIFY_AWAKE:
>>  +		an->sleeping = false;
>>  +		ath_tx_aggr_wakeup(sc, an);
>>  +		break;
>>  +	}
>>  +}
>>  +
>
> an->sleeping needs to be locked, no ?
> It seems to be accessed from the TX tasklet also.
It is only updated in this function and these updates should be atomic 
anyway, so I don't think locking would be useful here.

>>  diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>>  index 3cea3f7..a1230c0 100644
>>  --- a/drivers/net/wireless/ath/ath9k/xmit.c
>>  +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>>  @@ -357,6 +357,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
>>   	struct ath_frame_info *fi;
>>   	int nframes;
>>   	u8 tidno;
>>  +	bool clear_filter;
>>
>>   	skb = bf->bf_mpdu;
>>   	hdr = (struct ieee80211_hdr *)skb->data;
>>  @@ -816,6 +825,11 @@ static void ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
>>   		bf = list_first_entry(&bf_q, struct ath_buf, list);
>>   		bf->bf_lastbf = list_entry(bf_q.prev, struct ath_buf, list);
>>
>>  +		if (tid->ac->clear_ps_filter) {
>>  +			tid->ac->clear_ps_filter = false;
>>  +			ath9k_hw_set_clrdmask(sc->sc_ah, bf->bf_desc, true);
>>  +		}
>>  +
>
> Can you explain a bit more on the flow ? How exactly is ieee80211_sta_set_tim() to be used
> by a driver ? (I would like to port this fix to ath9k_htc).
The driver calls ieee80211_sta_set_tim on STA_NOTIFY_SLEEP whenever it 
still has some buffered frames that it intends to keep until the client 
wakes up again (i.e. frames that will not be returned to mac80211 as 
filtered).

>>   		/* if only one frame, send as non-aggregate */
>>   		if (bf == bf->bf_lastbf) {
>>   			fi = get_frame_info(bf->bf_mpdu);
>>  @@ -896,6 +910,67 @@ void ath_tx_aggr_stop(struct ath_softc *sc, struct ieee80211_sta *sta, u16 tid)
>>   	ath_tx_flush_tid(sc, txtid);
>>   }
>>
>>  +bool ath_tx_aggr_sleep(struct ath_softc *sc, struct ath_node *an)
>>  +{
>>  +	struct ath_atx_tid *tid;
>>  +	struct ath_atx_ac *ac;
>>  +	struct ath_txq *txq;
>>  +	bool buffered = false;
>>  +	int tidno;
>>  +
>>  +	for (tidno = 0, tid =&an->tid[tidno];
>>  +	     tidno<  WME_NUM_TID; tidno++, tid++) {
>>  +
>>  +		if (!tid->sched)
>>  +			continue;
>
> tid->sched has to be locked ?
Yeah, I think that one might need locking.

- Felix

Re: [PATCH 1/2] ath9k: fix powersave frame filtering/buffering in AP mode

[Sujith]

Felix Fietkau wrote:
> The driver calls ieee80211_sta_set_tim on STA_NOTIFY_SLEEP whenever it 
> still has some buffered frames that it intends to keep until the client 
> wakes up again (i.e. frames that will not be returned to mac80211 as 
> filtered).

Why should ieee80211_sta_set_tim() be called from complete_aggr() ?

Sujith

Re: [PATCH 1/2] ath9k: fix powersave frame filtering/buffering in AP mode

[Felix Fietkau]

On 2011-04-18 1:10 PM, Sujith wrote:
> Felix Fietkau wrote:
>>  The driver calls ieee80211_sta_set_tim on STA_NOTIFY_SLEEP whenever it
>>  still has some buffered frames that it intends to keep until the client
>>  wakes up again (i.e. frames that will not be returned to mac80211 as
>>  filtered).
>
> Why should ieee80211_sta_set_tim() be called from complete_aggr() ?
I should probably add an extra check that tests for tid->buf_q being 
empty - and some comments.
This handles a race condition that occurs if an A-MPDU completes (with 
some pending frames) after the station entered PS and 
ieee80211_sta_set_tim was not called because the buffer queue was empty.

- Felix

Re: [PATCH 1/2] ath9k: fix powersave frame filtering/buffering in AP mode

[Sujith]

Felix Fietkau wrote:
> I should probably add an extra check that tests for tid->buf_q being 
> empty - and some comments.
> This handles a race condition that occurs if an A-MPDU completes (with 
> some pending frames) after the station entered PS and 
> ieee80211_sta_set_tim was not called because the buffer queue was empty.

Alright, thanks for the explanation.

Sujith