Date: Wed, 3 Oct 2012 16:29:54 -0700 From: Adrian Chadd <adrian@freebsd.org> To: freebsd-wireless@freebsd.org Subject: [RFT] power-save TX suspend and resume (was Fwd: svn commit: r241170 - head/sys/dev/ath) Message-ID: <CAJ-VmonFoq%2B0H9maoRnfMDe%2B9BksEmf7rGzdhJZctetg5=DQFA@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
Hi, For those who are testing out 802.11n development - please update and test this for me. This work ties the software TX mechanism into the net80211 node power state, so whenever a wireless node sends a "going to sleep!" frame, the ath driver pauses TX. Same with wakeup/resume. Devices which are doing ps-poll will still be badly broken, unfortunately. And I have to fix TIM handling so the TIM bitmap takes the power save queue _and_ the driver software queue into account when deciding if to set or clear the TIM bitmap. That, and CABQ + MORE data bit stuff being set. Please report any issues you may have. The first thing to do is to turn on power save (wlandebug +power) and see if it's going in/out of powersave, along with whether it mentions ps-poll or not. I'm going to address ps-poll but first everything else has to work (TIM handling, software queue notification and CABQ/MORE data.) Thanks, Adrian ---------- Forwarded message ---------- From: Adrian Chadd <adrian@freebsd.org> Date: 3 October 2012 16:23 Subject: svn commit: r241170 - head/sys/dev/ath To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Author: adrian Date: Wed Oct 3 23:23:45 2012 New Revision: 241170 URL: http://svn.freebsd.org/changeset/base/241170 Log: Pause and unpause the software queues for a given node based on the net80211 node power save state. * Add an ATH_NODE_UNLOCK_ASSERT() check * Add a new node field - an_is_powersave * Pause/unpause the queue based on the node state * Attempt to handle net80211 concurrency issues so the queue doesn't get paused/unpaused more than once at a time from the net80211 power save code. Whilst here (and breaking my usual rule), set CLRDMASK when a queue is unpaused, regardless of whether the queue has some pending traffic. This means the first frame from that TID (now or later) will hvae CLRDMASK set. Also whilst here, bump the swretrymax counters whenever the filtered frames code expires a frame. Again, breaking my rule, but this is just a statistics thing rather than a functional change. This doesn't fix ps-poll (but it doesn't break it too much worse than it is at the present) or correcting the TID updates. That's next on the list. Tested: * AR9220 AP (Atheros AP96 reference design) * Macbook Pro and LG Optimus 1 Android phone, both setting and clearing power save state (but not using PS-POLL.) Modified: head/sys/dev/ath/if_ath.c head/sys/dev/ath/if_ath_debug.h head/sys/dev/ath/if_ath_tx.c head/sys/dev/ath/if_ath_tx.h head/sys/dev/ath/if_athvar.h Modified: head/sys/dev/ath/if_ath.c ============================================================================== --- head/sys/dev/ath/if_ath.c Wed Oct 3 22:02:16 2012 (r241169) +++ head/sys/dev/ath/if_ath.c Wed Oct 3 23:23:45 2012 (r241170) @@ -199,6 +199,7 @@ static void ath_setcurmode(struct ath_so static void ath_announce(struct ath_softc *); static void ath_dfs_tasklet(void *, int); +static void ath_node_powersave(struct ieee80211_node *, int); #ifdef IEEE80211_SUPPORT_TDMA #include <dev/ath/if_ath_tdma.h> @@ -1138,6 +1139,9 @@ ath_vap_create(struct ieee80211com *ic, avp->av_bmiss = vap->iv_bmiss; vap->iv_bmiss = ath_bmiss_vap; + avp->av_node_ps = vap->iv_node_ps; + vap->iv_node_ps = ath_node_powersave; + /* Set default parameters */ /* @@ -5335,6 +5339,37 @@ ath_dfs_tasklet(void *p, int npending) } } +/* + * Enable/disable power save. This must be called with + * no TX driver locks currently held, so it should only + * be called from the RX path (which doesn't hold any + * TX driver locks.) + */ +static void +ath_node_powersave(struct ieee80211_node *ni, int enable) +{ + struct ath_node *an = ATH_NODE(ni); + struct ieee80211com *ic = ni->ni_ic; + struct ath_softc *sc = ic->ic_ifp->if_softc; + struct ath_vap *avp = ATH_VAP(ni->ni_vap); + + ATH_NODE_UNLOCK_ASSERT(an); + /* XXX and no TXQ locks should be held here */ + + DPRINTF(sc, ATH_DEBUG_NODE_PWRSAVE, "%s: ni=%p, enable=%d\n", + __func__, ni, enable); + + /* Suspend or resume software queue handling */ + if (enable) + ath_tx_node_sleep(sc, an); + else + ath_tx_node_wakeup(sc, an); + + /* Update net80211 state */ + avp->av_node_ps(ni, enable); +} + + MODULE_VERSION(if_ath, 1); MODULE_DEPEND(if_ath, wlan, 1, 1, 1); /* 802.11 media layer */ #if defined(IEEE80211_ALQ) || defined(AH_DEBUG_ALQ) Modified: head/sys/dev/ath/if_ath_debug.h ============================================================================== --- head/sys/dev/ath/if_ath_debug.h Wed Oct 3 22:02:16 2012 (r241169) +++ head/sys/dev/ath/if_ath_debug.h Wed Oct 3 23:23:45 2012 (r241170) @@ -66,6 +66,7 @@ enum { ATH_DEBUG_SW_TX_BAR = 0x100000000ULL, /* BAR TX */ ATH_DEBUG_EDMA_RX = 0x200000000ULL, /* RX EDMA state */ ATH_DEBUG_SW_TX_FILT = 0x400000000ULL, /* SW TX FF */ + ATH_DEBUG_NODE_PWRSAVE = 0x800000000ULL, /* node powersave */ ATH_DEBUG_ANY = 0xffffffffffffffffULL }; Modified: head/sys/dev/ath/if_ath_tx.c ============================================================================== --- head/sys/dev/ath/if_ath_tx.c Wed Oct 3 22:02:16 2012 (r241169) +++ head/sys/dev/ath/if_ath_tx.c Wed Oct 3 23:23:45 2012 (r241170) @@ -111,6 +111,9 @@ __FBSDID("$FreeBSD$"); */ #define ATH_NONQOS_TID_AC WME_AC_VO +#if 0 +static int ath_tx_node_is_asleep(struct ath_softc *sc, struct ath_node *an); +#endif static int ath_tx_ampdu_pending(struct ath_softc *sc, struct ath_node *an, int tid); static int ath_tx_ampdu_running(struct ath_softc *sc, struct ath_node *an, @@ -2902,9 +2905,17 @@ ath_tx_tid_resume(struct ath_softc *sc, DPRINTF(sc, ATH_DEBUG_SW_TX_CTRL, "%s: unpaused = %d\n", __func__, tid->paused); - if (tid->paused || tid->axq_depth == 0) { + if (tid->paused) + return; + + /* + * Override the clrdmask configuration for the next frame + * from this TID, just to get the ball rolling. + */ + tid->clrdmask = 1; + + if (tid->axq_depth == 0) return; - } /* XXX isfiltered shouldn't ever be 0 at this point */ if (tid->isfiltered == 1) { @@ -2912,12 +2923,6 @@ ath_tx_tid_resume(struct ath_softc *sc, return; } - /* - * Override the clrdmask configuration for the next frame, - * just to get the ball rolling. - */ - tid->clrdmask = 1; - ath_tx_tid_sched(sc, tid); /* Punt some frames to the hardware if needed */ //ath_txq_sched(sc, sc->sc_ac2q[tid->ac]); @@ -3021,6 +3026,7 @@ ath_tx_tid_filt_comp_single(struct ath_s * Don't allow a filtered frame to live forever. */ if (bf->bf_state.bfs_retries > SWMAX_RETRIES) { + sc->sc_stats.ast_tx_swretrymax++; DPRINTF(sc, ATH_DEBUG_SW_TX_FILT, "%s: bf=%p, seqno=%d, exceeded retries\n", __func__, @@ -3073,6 +3079,7 @@ ath_tx_tid_filt_comp_aggr(struct ath_sof * Don't allow a filtered frame to live forever. */ if (bf->bf_state.bfs_retries > SWMAX_RETRIES) { + sc->sc_stats.ast_tx_swretrymax++; DPRINTF(sc, ATH_DEBUG_SW_TX_FILT, "%s: bf=%p, seqno=%d, exceeded retries\n", __func__, @@ -5282,6 +5289,145 @@ ath_addba_response_timeout(struct ieee80 ATH_TXQ_UNLOCK(sc->sc_ac2q[atid->ac]); } +#if 0 +/* + * Check if a node is asleep or not. + */ +static int +ath_tx_node_is_asleep(struct ath_softc *sc, struct ath_node *an) +{ + + ATH_NODE_LOCK_ASSERT(an); + + return (an->an_is_powersave); +} +#endif + +/* + * Mark a node as currently "in powersaving." + * This suspends all traffic on the node. + * + * This must be called with the node/tx locks free. + * + * XXX TODO: the locking silliness below is due to how the node + * locking currently works. Right now, the node lock is grabbed + * to do rate control lookups and these are done with the TX + * queue lock held. This means the node lock can't be grabbed + * first here or a LOR will occur. + * + * Eventually (hopefully!) the TX path code will only grab + * the TXQ lock when transmitting and the ath_node lock when + * doing node/TID operations. There are other complications - + * the sched/unsched operations involve walking the per-txq + * 'active tid' list and this requires both locks to be held. + */ +void +ath_tx_node_sleep(struct ath_softc *sc, struct ath_node *an) +{ + struct ath_tid *atid; + struct ath_txq *txq; + int tid; + + ATH_NODE_UNLOCK_ASSERT(an); + + /* + * It's possible that a parallel call to ath_tx_node_wakeup() + * will unpause these queues. + * + * The node lock can't just be grabbed here, as there's places + * in the driver where the node lock is grabbed _within_ a + * TXQ lock. + * So, we do this delicately and unwind state if needed. + * + * + Pause all the queues + * + Grab the node lock + * + If the queue is already asleep, unpause and quit + * + else just mark as asleep. + * + * A parallel sleep() call will just pause and then + * find they're already paused, so undo it. + * + * A parallel wakeup() call will check if asleep is 1 + * and if it's not (ie, it's 0), it'll treat it as already + * being awake. If it's 1, it'll mark it as 0 and then + * unpause everything. + * + * (Talk about a delicate hack.) + */ + + /* Suspend all traffic on the node */ + for (tid = 0; tid < IEEE80211_TID_SIZE; tid++) { + atid = &an->an_tid[tid]; + txq = sc->sc_ac2q[atid->ac]; + + ATH_TXQ_LOCK(txq); + ath_tx_tid_pause(sc, atid); + ATH_TXQ_UNLOCK(txq); + } + + ATH_NODE_LOCK(an); + + /* In case of concurrency races from net80211.. */ + if (an->an_is_powersave == 1) { + ATH_NODE_UNLOCK(an); + device_printf(sc->sc_dev, + "%s: an=%p: node was already asleep\n", + __func__, an); + for (tid = 0; tid < IEEE80211_TID_SIZE; tid++) { + atid = &an->an_tid[tid]; + txq = sc->sc_ac2q[atid->ac]; + + ATH_TXQ_LOCK(txq); + ath_tx_tid_resume(sc, atid); + ATH_TXQ_UNLOCK(txq); + } + return; + } + + /* Mark node as in powersaving */ + an->an_is_powersave = 1; + + ATH_NODE_UNLOCK(an); +} + +/* + * Mark a node as currently "awake." + * This resumes all traffic to the node. + */ +void +ath_tx_node_wakeup(struct ath_softc *sc, struct ath_node *an) +{ + struct ath_tid *atid; + struct ath_txq *txq; + int tid; + + ATH_NODE_UNLOCK_ASSERT(an); + ATH_NODE_LOCK(an); + + /* In case of concurrency races from net80211.. */ + if (an->an_is_powersave == 0) { + ATH_NODE_UNLOCK(an); + device_printf(sc->sc_dev, + "%s: an=%p: node was already awake\n", + __func__, an); + return; + } + + /* Mark node as awake */ + an->an_is_powersave = 0; + + ATH_NODE_UNLOCK(an); + + for (tid = 0; tid < IEEE80211_TID_SIZE; tid++) { + atid = &an->an_tid[tid]; + txq = sc->sc_ac2q[atid->ac]; + + ATH_TXQ_LOCK(txq); + ath_tx_tid_resume(sc, atid); + ATH_TXQ_UNLOCK(txq); + } +} + static int ath_legacy_dma_txsetup(struct ath_softc *sc) { Modified: head/sys/dev/ath/if_ath_tx.h ============================================================================== --- head/sys/dev/ath/if_ath_tx.h Wed Oct 3 22:02:16 2012 (r241169) +++ head/sys/dev/ath/if_ath_tx.h Wed Oct 3 23:23:45 2012 (r241170) @@ -124,6 +124,12 @@ extern void ath_addba_response_timeout(s struct ieee80211_tx_ampdu *tap); /* + * AP mode power save handling (of stations) + */ +extern void ath_tx_node_sleep(struct ath_softc *sc, struct ath_node *an); +extern void ath_tx_node_wakeup(struct ath_softc *sc, struct ath_node *an); + +/* * Setup path */ #define ath_txdma_setup(_sc) \ Modified: head/sys/dev/ath/if_athvar.h ============================================================================== --- head/sys/dev/ath/if_athvar.h Wed Oct 3 22:02:16 2012 (r241169) +++ head/sys/dev/ath/if_athvar.h Wed Oct 3 23:23:45 2012 (r241170) @@ -170,6 +170,7 @@ struct ath_node { struct ieee80211_node an_node; /* base class */ u_int8_t an_mgmtrix; /* min h/w rate index */ u_int8_t an_mcastrix; /* mcast h/w rate index */ + uint32_t an_is_powersave; /* node is sleeping */ struct ath_buf *an_ff_buf[WME_NUM_AC]; /* ff staging area */ struct ath_tid an_tid[IEEE80211_TID_SIZE]; /* per-TID state */ char an_name[32]; /* eg "wlan0_a1" */ @@ -332,6 +333,8 @@ struct ath_txq { #define ATH_NODE_LOCK(_an) mtx_lock(&(_an)->an_mtx) #define ATH_NODE_UNLOCK(_an) mtx_unlock(&(_an)->an_mtx) #define ATH_NODE_LOCK_ASSERT(_an) mtx_assert(&(_an)->an_mtx, MA_OWNED) +#define ATH_NODE_UNLOCK_ASSERT(_an) mtx_assert(&(_an)->an_mtx, \ + MA_NOTOWNED) #define ATH_TXQ_LOCK_INIT(_sc, _tq) do { \ snprintf((_tq)->axq_name, sizeof((_tq)->axq_name), "%s_txq%u", \ @@ -379,6 +382,7 @@ struct ath_vap { int (*av_newstate)(struct ieee80211vap *, enum ieee80211_state, int); void (*av_bmiss)(struct ieee80211vap *); + void (*av_node_ps)(struct ieee80211_node *, int); }; #define ATH_VAP(vap) ((struct ath_vap *)(vap))
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-VmonFoq%2B0H9maoRnfMDe%2B9BksEmf7rGzdhJZctetg5=DQFA>