[RFC v2 1/2] mac80211: add a function for setting the TIM bit for a specific station

[RFC v2 1/2] mac80211: add a function for setting the TIM bit for a specific station

[Felix Fietkau]

When a driver wants to buffer some frames internally without feeding them
back to mac80211, this allows it to still wake up the station

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 include/net/mac80211.h  |   14 ++++++++++++++
 net/mac80211/sta_info.c |    8 ++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 8fcd169..9a0babe 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2194,6 +2194,20 @@ static inline int ieee80211_sta_ps_transition_ni(struct ieee80211_sta *sta,
 #define IEEE80211_TX_STATUS_HEADROOM	13
 
 /**
+ * ieee80211_sta_set_tim - set the TIM bit for a sleeping station
+ *
+ * If a driver buffers frames for a powersave station instead of passing
+ * them back to mac80211 for retransmission, the station needs to be told
+ * to wake up using the TIM bitmap in the beacon.
+ *
+ * This function sets the station's TIM bit - it will be cleared automatically
+ * either when the station wakes up (and mac80211 has flushed out its
+ * buffered frames), or if all remaining buffered frames in mac80211 have
+ * timed out.
+ */
+void ieee80211_sta_set_tim(struct ieee80211_sta *sta);
+
+/**
  * ieee80211_tx_status - transmit status callback
  *
  * Call this function for all transmitted frames after they have been
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 5a11078..3458091 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -991,3 +991,11 @@ void ieee80211_sta_block_awake(struct ieee80211_hw *hw,
 		ieee80211_queue_work(hw, &sta->drv_unblock_wk);
 }
 EXPORT_SYMBOL(ieee80211_sta_block_awake);
+
+void ieee80211_sta_set_tim(struct ieee80211_sta *pubsta)
+{
+	struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
+
+	sta_info_set_tim_bit(sta);
+}
+EXPORT_SYMBOL(ieee80211_sta_set_tim);
-- 
1.7.3.2

[RFC v2 2/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       |   83 ++++++++++++++++++++++++++-
 8 files changed, 137 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9002_mac.c b/drivers/net/wireless/ath/ath9k/ar9002_mac.c
index 399ab3b..ccfe2c9 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,
@@ -460,4 +469,5 @@ void ar9002_hw_attach_mac_ops(struct ath_hw *ah)
 	ops->clr11n_aggr = ar9002_hw_clr11n_aggr;
 	ops->set11n_burstduration = ar9002_hw_set11n_burstduration;
 	ops->set11n_virtualmorefrag = ar9002_hw_set11n_virtualmorefrag;
+	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 038a0cb..349df33 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,
@@ -522,6 +531,7 @@ void ar9003_hw_attach_mac_ops(struct ath_hw *hw)
 	ops->clr11n_aggr = ar9003_hw_clr11n_aggr;
 	ops->set11n_burstduration = ar9003_hw_set11n_burstduration;
 	ops->set11n_virtualmorefrag = ar9003_hw_set11n_virtualmorefrag;
+	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 9272278..1207b0d 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -205,6 +205,7 @@ struct ath_atx_ac {
 	int sched;
 	struct list_head list;
 	struct list_head tid_q;
+	bool clear_ps_filter;
 };
 
 struct ath_frame_info {
@@ -262,6 +263,8 @@ struct ath_node {
 	struct ath_atx_ac ac[WME_NUM_AC];
 	u16 maxampdu;
 	u8 mpdudensity;
+
+	bool sleeping;
 };
 
 #define AGGR_CLEANUP         BIT(1)
@@ -343,6 +346,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 c8f254f..f64e86e 100644
--- a/drivers/net/wireless/ath/ath9k/hw-ops.h
+++ b/drivers/net/wireless/ath/ath9k/hw-ops.h
@@ -128,6 +128,11 @@ static inline void ath9k_hw_set11n_virtualmorefrag(struct ath_hw *ah, void *ds,
 	ath9k_hw_ops(ah)->set11n_virtualmorefrag(ah, ds, vmf);
 }
 
+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 ef79f4c..77da61e 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -642,6 +642,7 @@ struct ath_hw_ops {
 				     u32 burstDuration);
 	void (*set11n_virtualmorefrag)(struct ath_hw *ah, void *ds,
 				       u32 vmf);
+	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 04d58ae..e3cda955 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 4ed43b2..0772852 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1794,6 +1794,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)
 {
@@ -2133,6 +2154,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 9f4e755..865fcd0 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;
@@ -442,7 +443,11 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
 			acked_cnt++;
 		} else {
 			if (!(tid->state & AGGR_CLEANUP) && retry) {
-				if (fi->retries < ATH_MAX_SW_RETRIES) {
+				if (ts->ts_status & ATH9K_TXERR_FILT) {
+					if (!an->sleeping)
+						clear_filter = true;
+					txpending = 1;
+				} else if (fi->retries < ATH_MAX_SW_RETRIES) {
 					ath_tx_set_retry(sc, txq, bf->bf_mpdu);
 					txpending = 1;
 				} else {
@@ -496,6 +501,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 +552,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 +827,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 +912,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;
@@ -1492,7 +1569,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)
@@ -1755,6 +1831,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

Re: [RFC v2 1/2] mac80211: add a function for setting the TIM bit for a specific station

[Johannes Berg]

On Wed, 2011-02-09 at 00:32 +0100, Felix Fietkau wrote:
> When a driver wants to buffer some frames internally without feeding them
> back to mac80211, this allows it to still wake up the station

Why would the driver want to do this though?

johannes

Re: [RFC v2 1/2] mac80211: add a function for setting the TIM bit for a specific station

[Felix Fietkau]

On 2011-02-14 1:09 PM, Johannes Berg wrote:
> On Wed, 2011-02-09 at 00:32 +0100, Felix Fietkau wrote:
>> When a driver wants to buffer some frames internally without feeding them
>> back to mac80211, this allows it to still wake up the station
> 
> Why would the driver want to do this though?
If a station enters powersave and the AP still has buffered frames for
aggregation, those frames need to stay in the queue, since the driver
controls the block ack window and keeps some state per frame (e.g.
number of software retransmissions).
If ath9k would send those frames back to mac80211, and mac80211 would
drop some of them, then the client's reorder window would get messed up
when the client comes back. This would also make managing frame sequence
numbers more complicated, since the driver would have to skip assigning
sequence numbers for frames where tx_info->flags has the
IEEE80211_TX_INTFL_RETRANSMISSION bit set, but then also look for gaps
caused by expired frames.

- Felix

Re: [RFC v2 1/2] mac80211: add a function for setting the TIM bit for a specific station

[Johannes Berg]

On Mon, 2011-02-14 at 15:06 +0100, Felix Fietkau wrote:
> On 2011-02-14 1:09 PM, Johannes Berg wrote:
> > On Wed, 2011-02-09 at 00:32 +0100, Felix Fietkau wrote:
> >> When a driver wants to buffer some frames internally without feeding them
> >> back to mac80211, this allows it to still wake up the station
> > 
> > Why would the driver want to do this though?
> If a station enters powersave and the AP still has buffered frames for
> aggregation, those frames need to stay in the queue, since the driver
> controls the block ack window and keeps some state per frame (e.g.
> number of software retransmissions).
> If ath9k would send those frames back to mac80211, and mac80211 would
> drop some of them, then the client's reorder window would get messed up
> when the client comes back. This would also make managing frame sequence
> numbers more complicated, since the driver would have to skip assigning
> sequence numbers for frames where tx_info->flags has the
> IEEE80211_TX_INTFL_RETRANSMISSION bit set, but then also look for gaps
> caused by expired frames.

Ok, that makes sense. In fact, I think iwlwifi will have the same issue
since it buffers in hw queues... will have to check that.

johannes

Re: [RFC v2 1/2] mac80211: add a function for setting the TIM bit for a specific station

[Johannes Berg]

On Wed, 2011-02-09 at 00:32 +0100, Felix Fietkau wrote:
> When a driver wants to buffer some frames internally without feeding them
> back to mac80211, this allows it to still wake up the station
> 
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> ---
>  include/net/mac80211.h  |   14 ++++++++++++++
>  net/mac80211/sta_info.c |    8 ++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 8fcd169..9a0babe 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -2194,6 +2194,20 @@ static inline int ieee80211_sta_ps_transition_ni(struct ieee80211_sta *sta,
>  #define IEEE80211_TX_STATUS_HEADROOM	13
>  
>  /**
> + * ieee80211_sta_set_tim - set the TIM bit for a sleeping station
> + *
> + * If a driver buffers frames for a powersave station instead of passing
> + * them back to mac80211 for retransmission, the station needs to be told
> + * to wake up using the TIM bitmap in the beacon.
> + *
> + * This function sets the station's TIM bit - it will be cleared automatically
> + * either when the station wakes up (and mac80211 has flushed out its
> + * buffered frames), or if all remaining buffered frames in mac80211 have
> + * timed out.

I'm wondering ... this doesn't seem right. Even if the frames in
mac80211 time out, there might be more in the driver still, no? It seems
that this should be more of a "I have frames / I no longer have frames"
type API?

johannes