[PATCH 0/7] ath9k/mac80211: Power save fixes

[PATCH 0/7] ath9k/mac80211: Power save fixes

[Jouni Malinen]

This set of patches addresses some more PS mode issues in ath9k and
mac80211. Some corner case issues are likely to remain even after this,
but at least all my basic tests passed with the end result; both with
timeout=500 (the default) and timeout=0 (PS-Poll).

-- 
Jouni Malinen                                            PGP id EFC895FA

[PATCH 1/7] ath9k: Wake up for TX in mac80211 timeout=0 sleep mode

[Jouni Malinen]

When using timeout=0 (PS-Poll) with mac80211, the driver will need to
wake up for TX requests and remain awake until the TX has been
completed (ACK received or timeout) or until the buffer frame(s) have
been received (in case the TX is for a PS-Poll frame).

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 drivers/net/wireless/ath/ath9k/ath9k.h |    6 +++++-
 drivers/net/wireless/ath/ath9k/main.c  |   30 +++++++++++++++++++++++++++++-
 drivers/net/wireless/ath/ath9k/recv.c  |   16 ++++++++++++++--
 drivers/net/wireless/ath/ath9k/xmit.c  |   10 ++++++++++
 4 files changed, 58 insertions(+), 4 deletions(-)

--- wireless-testing.orig/drivers/net/wireless/ath/ath9k/main.c	2009-05-19 16:32:28.000000000 +0300
+++ wireless-testing/drivers/net/wireless/ath/ath9k/main.c	2009-05-19 16:32:29.000000000 +0300
@@ -2070,6 +2070,31 @@ static int ath9k_tx(struct ieee80211_hw 
 		goto exit;
 	}
 
+	if (unlikely(sc->sc_ah->power_mode != ATH9K_PM_AWAKE)) {
+		/*
+		 * We are using PS-Poll and mac80211 can request TX while in
+		 * power save mode. Need to wake up hardware for the TX to be
+		 * completed and if needed, also for RX of buffered frames.
+		 */
+		struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
+		ath9k_ps_wakeup(sc);
+		ath9k_hw_setrxabort(sc->sc_ah, 0);
+		if (ieee80211_is_pspoll(hdr->frame_control)) {
+			DPRINTF(sc, ATH_DBG_PS, "Sending PS-Poll to pick a "
+				"buffered frame\n");
+			sc->sc_flags |= SC_OP_WAIT_FOR_PSPOLL_DATA;
+		} else {
+			DPRINTF(sc, ATH_DBG_PS, "Wake up to complete TX\n");
+			sc->sc_flags |= SC_OP_WAIT_FOR_TX_ACK;
+		}
+		/*
+		 * The actual restore operation will happen only after
+		 * the sc_flags bit is cleared. We are just dropping
+		 * the ps_usecount here.
+		 */
+		ath9k_ps_restore(sc);
+	}
+
 	memset(&txctl, 0, sizeof(struct ath_tx_control));
 
 	/*
@@ -2307,7 +2332,10 @@ static int ath9k_config(struct ieee80211
 			if (!(ah->caps.hw_caps &
 			      ATH9K_HW_CAP_AUTOSLEEP)) {
 				ath9k_hw_setrxabort(sc->sc_ah, 0);
-				sc->sc_flags &= ~SC_OP_WAIT_FOR_BEACON;
+				sc->sc_flags &= ~(SC_OP_WAIT_FOR_BEACON |
+						  SC_OP_WAIT_FOR_CAB |
+						  SC_OP_WAIT_FOR_PSPOLL_DATA |
+						  SC_OP_WAIT_FOR_TX_ACK);
 				if (sc->imask & ATH9K_INT_TIM_TIMER) {
 					sc->imask &= ~ATH9K_INT_TIM_TIMER;
 					ath9k_hw_set_interrupts(sc->sc_ah,
--- wireless-testing.orig/drivers/net/wireless/ath/ath9k/ath9k.h	2009-05-19 16:32:27.000000000 +0300
+++ wireless-testing/drivers/net/wireless/ath/ath9k/ath9k.h	2009-05-19 16:32:29.000000000 +0300
@@ -520,6 +520,8 @@ struct ath_rfkill {
 #define SC_OP_SCANNING          BIT(14)
 #define SC_OP_TSF_RESET         BIT(15)
 #define SC_OP_WAIT_FOR_CAB      BIT(16)
+#define SC_OP_WAIT_FOR_PSPOLL_DATA BIT(17)
+#define SC_OP_WAIT_FOR_TX_ACK   BIT(18)
 
 struct ath_bus_ops {
 	void		(*read_cachesize)(struct ath_softc *sc, int *csz);
@@ -682,7 +684,9 @@ static inline void ath9k_ps_restore(stru
 {
 	if (atomic_dec_and_test(&sc->ps_usecount))
 		if ((sc->hw->conf.flags & IEEE80211_CONF_PS) &&
-		    !(sc->sc_flags & SC_OP_WAIT_FOR_BEACON))
+		    !(sc->sc_flags & (SC_OP_WAIT_FOR_BEACON |
+				      SC_OP_WAIT_FOR_PSPOLL_DATA |
+				      SC_OP_WAIT_FOR_TX_ACK)))
 			ath9k_hw_setpower(sc->sc_ah,
 					  sc->sc_ah->restore_mode);
 }
--- wireless-testing.orig/drivers/net/wireless/ath/ath9k/recv.c	2009-05-19 16:32:27.000000000 +0300
+++ wireless-testing/drivers/net/wireless/ath/ath9k/recv.c	2009-05-19 16:32:29.000000000 +0300
@@ -560,7 +560,8 @@ static void ath_rx_ps(struct ath_softc *
 	hdr = (struct ieee80211_hdr *)skb->data;
 
 	/* Process Beacon and CAB receive in PS state */
-	if (ieee80211_is_beacon(hdr->frame_control))
+	if ((sc->sc_flags & SC_OP_WAIT_FOR_BEACON) &&
+	    ieee80211_is_beacon(hdr->frame_control))
 		ath_rx_ps_beacon(sc, skb);
 	else if ((sc->sc_flags & SC_OP_WAIT_FOR_CAB) &&
 		 (ieee80211_is_data(hdr->frame_control) ||
@@ -574,6 +575,16 @@ static void ath_rx_ps(struct ath_softc *
 		 * point.
 		 */
 		ath_rx_ps_back_to_sleep(sc);
+	} else if ((sc->sc_flags & SC_OP_WAIT_FOR_PSPOLL_DATA) &&
+		   !is_multicast_ether_addr(hdr->addr1) &&
+		   !ieee80211_has_morefrags(hdr->frame_control)) {
+		sc->sc_flags &= ~SC_OP_WAIT_FOR_PSPOLL_DATA;
+		DPRINTF(sc, ATH_DBG_PS, "Going back to sleep after having "
+			"received PS-Poll data (0x%x)\n",
+			sc->sc_flags & (SC_OP_WAIT_FOR_BEACON |
+					SC_OP_WAIT_FOR_CAB |
+					SC_OP_WAIT_FOR_PSPOLL_DATA |
+					SC_OP_WAIT_FOR_TX_ACK));
 	}
 }
 
@@ -798,7 +809,8 @@ int ath_rx_tasklet(struct ath_softc *sc,
 			sc->rx.rxotherant = 0;
 		}
 
-		if (unlikely(sc->sc_flags & SC_OP_WAIT_FOR_BEACON))
+		if (unlikely(sc->sc_flags & (SC_OP_WAIT_FOR_BEACON |
+					     SC_OP_WAIT_FOR_PSPOLL_DATA)))
 			ath_rx_ps(sc, skb);
 
 		ath_rx_send_to_mac80211(sc, skb, &rx_status);
--- wireless-testing.orig/drivers/net/wireless/ath/ath9k/xmit.c	2009-05-19 16:32:27.000000000 +0300
+++ wireless-testing/drivers/net/wireless/ath/ath9k/xmit.c	2009-05-19 16:32:29.000000000 +0300
@@ -1790,6 +1790,16 @@ static void ath_tx_complete(struct ath_s
 		skb_pull(skb, padsize);
 	}
 
+	if (sc->sc_flags & SC_OP_WAIT_FOR_TX_ACK) {
+		sc->sc_flags &= ~SC_OP_WAIT_FOR_TX_ACK;
+		DPRINTF(sc, ATH_DBG_PS, "Going back to sleep after having "
+			"received TX status (0x%x)\n",
+			sc->sc_flags & (SC_OP_WAIT_FOR_BEACON |
+					SC_OP_WAIT_FOR_CAB |
+					SC_OP_WAIT_FOR_PSPOLL_DATA |
+					SC_OP_WAIT_FOR_TX_ACK));
+	}
+
 	if (frame_type == ATH9K_NOT_INTERNAL)
 		ieee80211_tx_status(hw, skb);
 	else

-- 

-- 
Jouni Malinen                                            PGP id EFC895FA

[PATCH 2/7] ath9k: Do not try to calibrate radio when in sleep mode

[Jouni Malinen]

When the chip is in sleep mode, there is no point trying to calibrate
the radio since it will just results in incorrect values being read
from registers and other potential issues. In addition, if we actually
start processing calibrate, do not allow the chip to be put into sleep
until we have completed the calibration step.

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 drivers/net/wireless/ath/ath9k/main.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- wireless-testing.orig/drivers/net/wireless/ath/ath9k/main.c	2009-05-19 16:32:29.000000000 +0300
+++ wireless-testing/drivers/net/wireless/ath/ath9k/main.c	2009-05-19 16:32:31.000000000 +0300
@@ -329,6 +329,12 @@ static void ath_ani_calibrate(unsigned l
 	if (sc->sc_flags & SC_OP_SCANNING)
 		goto set_timer;
 
+	/* Only calibrate if awake */
+	if (sc->sc_ah->power_mode != ATH9K_PM_AWAKE)
+		goto set_timer;
+
+	ath9k_ps_wakeup(sc);
+
 	/* Long calibration runs independently of short calibration. */
 	if ((timestamp - sc->ani.longcal_timer) >= ATH_LONG_CALINTERVAL) {
 		longcal = true;
@@ -380,6 +386,8 @@ static void ath_ani_calibrate(unsigned l
 		}
 	}
 
+	ath9k_ps_restore(sc);
+
 set_timer:
 	/*
 	* Set timer interval based on previous results.

-- 

-- 
Jouni Malinen                                            PGP id EFC895FA

[PATCH 3/7] ath9k: Use TSFOOR interrupt to trigger TSF sync with next Beacon

[Jouni Malinen]

If the chip complains about TSF sync, make sure we remain awake to
sync with the next Beacon frame. In theory, this should not be needed
since we are currently trying to receive all Beacon frames, anyway,
better have this code ready should we ever change that.

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 drivers/net/wireless/ath/ath9k/main.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- wireless-testing.orig/drivers/net/wireless/ath/ath9k/main.c	2009-05-19 16:32:31.000000000 +0300
+++ wireless-testing/drivers/net/wireless/ath/ath9k/main.c	2009-05-19 16:32:33.000000000 +0300
@@ -480,6 +480,16 @@ static void ath9k_tasklet(unsigned long 
 	if (status & ATH9K_INT_TX)
 		ath_tx_tasklet(sc);
 
+	if ((status & ATH9K_INT_TSFOOR) &&
+	    (sc->hw->conf.flags & IEEE80211_CONF_PS)) {
+		/*
+		 * TSF sync does not look correct; remain awake to sync with
+		 * the next Beacon.
+		 */
+		DPRINTF(sc, ATH_DBG_PS, "TSFOOR - Sync with next Beacon\n");
+		sc->sc_flags |= SC_OP_WAIT_FOR_BEACON;
+	}
+
 	/* re-enable hardware interrupt */
 	ath9k_hw_set_interrupts(sc->sc_ah, sc->imask);
 	ath9k_ps_restore(sc);

-- 

-- 
Jouni Malinen                                            PGP id EFC895FA

[PATCH 4/7] ath9k: Wake up for RX filter changes

[Jouni Malinen]

We must make sure the chip is awake when changing the RX filter
parameters. This could have caused problems, e.g., when changing the
interface to promiscuous mode while in sleep mode.

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 drivers/net/wireless/ath/ath9k/main.c |    2 ++
 1 file changed, 2 insertions(+)

--- wireless-testing.orig/drivers/net/wireless/ath/ath9k/main.c	2009-05-19 16:32:33.000000000 +0300
+++ wireless-testing/drivers/net/wireless/ath/ath9k/main.c	2009-05-19 16:32:35.000000000 +0300
@@ -2428,8 +2428,10 @@ static void ath9k_configure_filter(struc
 	*total_flags &= SUPPORTED_FILTERS;
 
 	sc->rx.rxfilter = *total_flags;
+	ath9k_ps_wakeup(sc);
 	rfilt = ath_calcrxfilter(sc);
 	ath9k_hw_setrxfilter(sc->sc_ah, rfilt);
+	ath9k_ps_restore(sc);
 
 	DPRINTF(sc, ATH_DBG_CONFIG, "Set HW RX filter: 0x%x\n", sc->rx.rxfilter);
 }

-- 

-- 
Jouni Malinen                                            PGP id EFC895FA

[PATCH 5/7] ath9k: Set PM field in frame control when in PS mode

[Jouni Malinen]

mac80211 does not set PM field for normal data frames, so we need to
update that based on the current PS mode when using PS-Poll
(timeout=0) power save mode.

This allows the AP to remain in sync with our PS state. However, there
is still a potential race condition between PS state changes when
multiple TX queues are used and nullfunc and PS-Poll frames use
different queue. That corner case may need to be handled separately by
changing which queue is used either in ath9k or mac80211.

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 drivers/net/wireless/ath/ath9k/main.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

--- wireless-testing.orig/drivers/net/wireless/ath/ath9k/main.c	2009-05-19 16:32:35.000000000 +0300
+++ wireless-testing/drivers/net/wireless/ath/ath9k/main.c	2009-05-19 16:32:37.000000000 +0300
@@ -2088,6 +2088,21 @@ static int ath9k_tx(struct ieee80211_hw 
 		goto exit;
 	}
 
+	if (sc->hw->conf.flags & IEEE80211_CONF_PS) {
+		struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
+		/*
+		 * mac80211 does not set PM field for normal data frames, so we
+		 * need to update that based on the current PS mode.
+		 */
+		if (ieee80211_is_data(hdr->frame_control) &&
+		    !ieee80211_is_nullfunc(hdr->frame_control) &&
+		    !ieee80211_has_pm(hdr->frame_control)) {
+			DPRINTF(sc, ATH_DBG_PS, "Add PM=1 for a TX frame "
+				"while in PS mode\n");
+			hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PM);
+		}
+	}
+
 	if (unlikely(sc->sc_ah->power_mode != ATH9K_PM_AWAKE)) {
 		/*
 		 * We are using PS-Poll and mac80211 can request TX while in

-- 

-- 
Jouni Malinen                                            PGP id EFC895FA

[PATCH 6/7] mac80211: PS processing for every Beacon with our AID in TIM

[Jouni Malinen]

If the AP includes our AID in the TIM IE, we need to process the
Beacon frame as far as PS is concerned (send PS-Poll or nullfunc data
with PM=0). The previous code skipped this in cases where the CRC
value did not change and it would not change if the AP continues
including our AID in the TIM..

There is no need to count the crc32 value for directed_tim with this
change, so we can remove that part. In order not to change the order
of operations (i.e., update WMM parameters prior to sending PS-Poll),
the CRC match is checked twice as only after the PS processing step,
the rest of the function is skipped if nothing changed in the Beacon.

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 net/mac80211/mlme.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

--- wireless-testing.orig/net/mac80211/mlme.c	2009-05-19 16:32:25.000000000 +0300
+++ wireless-testing/net/mac80211/mlme.c	2009-05-19 16:32:39.000000000 +0300
@@ -1958,16 +1958,13 @@ static void ieee80211_rx_mgmt_beacon(str
 		directed_tim = ieee80211_check_tim(elems.tim, elems.tim_len,
 						   ifmgd->aid);
 
-	ncrc = crc32_be(ncrc, (void *)&directed_tim, sizeof(directed_tim));
+	if (ncrc != ifmgd->beacon_crc) {
+		ieee80211_rx_bss_info(sdata, mgmt, len, rx_status, &elems,
+				      true);
 
-	if (ncrc == ifmgd->beacon_crc)
-		return;
-	ifmgd->beacon_crc = ncrc;
-
-	ieee80211_rx_bss_info(sdata, mgmt, len, rx_status, &elems, true);
-
-	ieee80211_sta_wmm_params(local, ifmgd, elems.wmm_param,
-				 elems.wmm_param_len);
+		ieee80211_sta_wmm_params(local, ifmgd, elems.wmm_param,
+					 elems.wmm_param_len);
+	}
 
 	if (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) {
 		if (directed_tim) {
@@ -1992,6 +1989,10 @@ static void ieee80211_rx_mgmt_beacon(str
 		}
 	}
 
+	if (ncrc == ifmgd->beacon_crc)
+		return;
+	ifmgd->beacon_crc = ncrc;
+
 	if (elems.erp_info && elems.erp_info_len >= 1) {
 		erp_valid = true;
 		erp_value = elems.erp_info[0];

-- 

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 6/7] mac80211: PS processing for every Beacon with our AID in TIM

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

On Tue, 2009-05-19 at 17:01 +0300, Jouni Malinen wrote:
> plain text document attachment
> (mac80211-pspoll-on-every-beacon-tim.patch)
> If the AP includes our AID in the TIM IE, we need to process the
> Beacon frame as far as PS is concerned (send PS-Poll or nullfunc data
> with PM=0). The previous code skipped this in cases where the CRC
> value did not change and it would not change if the AP continues
> including our AID in the TIM..
> 
> There is no need to count the crc32 value for directed_tim with this
> change, so we can remove that part. In order not to change the order
> of operations (i.e., update WMM parameters prior to sending PS-Poll),
> the CRC match is checked twice as only after the PS processing step,
> the rest of the function is skipped if nothing changed in the Beacon.
> 
> Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

Makes sense, thanks.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 7/7] mac80211: Do not override AID in the duration field

[Jouni Malinen]

When updating the duration field for TX frames, skip the update for
PS-Poll frames that use this field for other purposes (AID).

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 net/mac80211/tx.c |    2 ++
 1 file changed, 2 insertions(+)

--- wireless-testing.orig/net/mac80211/tx.c	2009-05-19 16:32:25.000000000 +0300
+++ wireless-testing/net/mac80211/tx.c	2009-05-19 16:32:42.000000000 +0300
@@ -872,6 +872,8 @@ ieee80211_tx_h_calculate_duration(struct
 
 	do {
 		hdr = (void *) skb->data;
+		if (ieee80211_is_pspoll(hdr->frame_control))
+			continue; /* must not overwrite AID */
 		next_len = skb->next ? skb->next->len : 0;
 		group_addr = is_multicast_ether_addr(hdr->addr1);
 

-- 

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 7/7] mac80211: Do not override AID in the duration field

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

On Tue, 2009-05-19 at 17:01 +0300, Jouni Malinen wrote:
> plain text document attachment (mac80211-pspoll-aid.patch)
> When updating the duration field for TX frames, skip the update for
> PS-Poll frames that use this field for other purposes (AID).
> 
> Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>
> 
> ---
>  net/mac80211/tx.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- wireless-testing.orig/net/mac80211/tx.c	2009-05-19 16:32:25.000000000 +0300
> +++ wireless-testing/net/mac80211/tx.c	2009-05-19 16:32:42.000000000 +0300
> @@ -872,6 +872,8 @@ ieee80211_tx_h_calculate_duration(struct
>  
>  	do {
>  		hdr = (void *) skb->data;
> +		if (ieee80211_is_pspoll(hdr->frame_control))
> +			continue; /* must not overwrite AID */

unlikely()? Also, you can 'break' instead of 'continue' or actually put
this in front of the loop since the frame cannot be fragmented.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v2 7/7] mac80211: Do not override AID in the duration field

[Jouni Malinen]

When updating the duration field for TX frames, skip the update for
PS-Poll frames that use this field for other purposes (AID).

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 net/mac80211/tx.c |    2 ++
 1 file changed, 2 insertions(+)

v2: add unlikely() and change continue to break to avoid unnecessary
    check for a fragment of PS-Poll frame

--- wireless-testing.orig/net/mac80211/tx.c	2009-05-14 21:03:49.000000000 +0300
+++ wireless-testing/net/mac80211/tx.c	2009-05-19 19:20:57.000000000 +0300
@@ -872,6 +872,8 @@ ieee80211_tx_h_calculate_duration(struct
 
 	do {
 		hdr = (void *) skb->data;
+		if (unlikely(ieee80211_is_pspoll(hdr->frame_control)))
+			break; /* must not overwrite AID */
 		next_len = skb->next ? skb->next->len : 0;
 		group_addr = is_multicast_ether_addr(hdr->addr1);
 
-- 
Jouni Malinen                                            PGP id EFC895FA