From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Shepard Date: Sun, 03 Jul 2016 03:53:18 -0000 Subject: [ath9k-devel] [PATCH] ath9k: Switch to using mac80211 intermediate software queues. In-Reply-To: Your message of Sat, 18 Jun 2016 21:05:05 +0200. <20160618190505.24038-1-toke@toke.dk> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: ath9k-devel@lists.ath9k.org Toke, I've been tesing your ath9k patch (using it instead of my earlier ath9k patch) and plan to continue testing it. Thanks for unconfusing me a couple weeks ago, and cluing me into how the limit on ->pending_frames is not really relevant for the data packets that go through the new intermediate queues. But I'm not sure if this would allow us to remove the limit on pending_frames because even though normal data packets would not normally build up that many packets, there are other packet types which bypass the intermediate queues and are transmitted directly (also in most cases bypassing the ath9k internal queues in the way ath9k worked before we patched it to use the mac80211 intermediate queues). Along similar lines, from reading the code I think your patch has introduced a bug (but I don't know how to demonstrate it at runtime). Looking in the body of ath_tx_start() at the result of applying your patch, we now see this: [...] /* Force queueing of all frames that belong to a virtual interface on * a different channel context, to ensure that they are sent on the * correct channel. */ if (((avp && avp->chanctx != sc->cur_chan) || sc->cur_chan->stopped) && !txctl->force_channel) { if (!txctl->an) txctl->an = &avp->mcast_node; queue = true; skip_uapsd = true; } if (!skip_uapsd && ps_resp) { ath_txq_unlock(sc, txq); txq = sc->tx.uapsdq; ath_txq_lock(sc, txq); } else if(WARN_ON(txctl->an && queue)) ath_txq_unlock(sc, txq); return -EINVAL; } [...] In the case where the first if body above is run to force queuing of all packets (not just normal data packets), then the else case of the second if statement above will surely run and its if statement will surely be true, so your new WARN_ON will happen. This is why I left the previous ath9k internal queueing mechanisms in place. I couldn't figure out how to handle the above case without leaving the ath9k internal queueing mechanisms. I'm not sure how to test for this though... I don't know what sort of configuration scenario I would need to set up to generate the above situation and trigger the warning. (Presumably, it involves multiple vifs on different channels.) But unless the first if statement body is dead code that can never happen, I think you've introduced a bug here (with a good WARN_ON to make it obvious when it happens). Earlier Felix said: > Channel context based queue handling can be dealt with by > stopping/starting relevant queues on channel context changes. But I don't see how to handle the case here where packets get passed to the driver with ath_tx_start() and wind up in the scenario above. -Tim Shepard shep at alum.mit.edu