Date: Mon, 21 Apr 2014 01:02:49 +0000 (UTC) From: Adrian Chadd <adrian@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r264708 - head/sys/dev/ath Message-ID: <201404210102.s3L12nWm039424@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: adrian Date: Mon Apr 21 01:02:49 2014 New Revision: 264708 URL: http://svnweb.freebsd.org/changeset/base/264708 Log: Fix a cleanup hang if cleanup gets called _during_ an active cleanup. During power save testing I noticed that the cleanup code is being called during a RUN->RUN state transition. It's because the net80211 stack is treating that (for reasons I don't quitey know yet) as a reassociation and this calls the node cleanup code. The reason it's seeing a RUN->RUN transition is because during active power save stuff it's possible that the RUN->SLEEP and SLEEP->RUN transitions happen so quickly that the deferred net80211 vap state code "loses" a transition, namely the intermediary SLEEP transition. So, this was causing the node reassociation code to sometimes be called twice in quick succession and this would result in ath_tx_tid_cleanup() to be called again. The code calling it would always call pause, and then only call resume if the TID didn't have "cleanup_inprogress" set. Unfortunately it didn't check if it was already set on entry, so it would pause but not call resume. Thus, paused would be called more than once (once before each entry into ath-tx_tid_cleanup()) but resume would only be called once when the cleanup state was finished. This doesn't entirely fix all of the issues seen in the cleanup path but it's a necessary first step. Since this is a stability fix, it should be merged to stable/10 at some point. Tested: * AR5416, STA mode MFC after: 7 days Modified: head/sys/dev/ath/if_ath_tx.c Modified: head/sys/dev/ath/if_ath_tx.c ============================================================================== --- head/sys/dev/ath/if_ath_tx.c Sun Apr 20 23:01:56 2014 (r264707) +++ head/sys/dev/ath/if_ath_tx.c Mon Apr 21 01:02:49 2014 (r264708) @@ -5789,12 +5789,26 @@ ath_addba_stop(struct ieee80211_node *ni */ TAILQ_INIT(&bf_cq); ATH_TX_LOCK(sc); - ath_tx_tid_cleanup(sc, an, tid, &bf_cq); + /* - * Unpause the TID if no cleanup is required. + * In case there's a followup call to this, only call it + * if we don't have a cleanup in progress. + * + * Since we've paused the queue above, we need to make + * sure we unpause if there's already a cleanup in + * progress - it means something else is also doing + * this stuff, so we don't need to also keep it paused. */ - if (! atid->cleanup_inprogress) + if (atid->cleanup_inprogress) { ath_tx_tid_resume(sc, atid); + } else { + ath_tx_tid_cleanup(sc, an, tid, &bf_cq); + /* + * Unpause the TID if no cleanup is required. + */ + if (! atid->cleanup_inprogress) + ath_tx_tid_resume(sc, atid); + } ATH_TX_UNLOCK(sc); /* Handle completing frames and fail them */ @@ -5828,19 +5842,25 @@ ath_tx_node_reassoc(struct ath_softc *sc tid = &an->an_tid[i]; if (tid->hwq_depth == 0) continue; - ath_tx_tid_pause(sc, tid); DPRINTF(sc, ATH_DEBUG_NODE, "%s: %6D: TID %d: cleaning up TID\n", __func__, an->an_node.ni_macaddr, ":", i); - ath_tx_tid_cleanup(sc, an, i, &bf_cq); /* - * Unpause the TID if no cleanup is required. + * In case there's a followup call to this, only call it + * if we don't have a cleanup in progress. */ - if (! tid->cleanup_inprogress) - ath_tx_tid_resume(sc, tid); + if (! tid->cleanup_inprogress) { + ath_tx_tid_pause(sc, tid); + ath_tx_tid_cleanup(sc, an, i, &bf_cq); + /* + * Unpause the TID if no cleanup is required. + */ + if (! tid->cleanup_inprogress) + ath_tx_tid_resume(sc, tid); + } } ATH_TX_UNLOCK(sc);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201404210102.s3L12nWm039424>