Incorrect local->wmm_acm bits were set for AC_BK and AC_BE. Fix this
and add some comments to make it easier to understand the AC-to-UP(pair)
mapping. Set the wmm_acm bits (and show WMM debug) even if the driver
does not implement conf_tx() handler.
In addition, fix the ACM-based AC downgrade code to not use the
highest priority in error cases. We need to break the loop to get the
correct AC_BK value (3) instead of returning 0 (which would indicate
AC_VO). The comment here was not really very useful either, so let's
provide somewhat more helpful description of the situation.
Since it is very unlikely that the ACM flag would be set for AC_BK and
AC_BE, these bugs are not likely to be seen in real life networks.
Anyway, better do these things correctly should someone really use
silly AP configuration (and to pass some functionality tests, too).
Remove the TODO comment about handling ACM. Downgrading AC is
perfectly valid mechanism for ACM. Eventually, we may add support for
WMM-AC and send a request for a TS, but anyway, that functionality
won't be here at the location of this TODO comment.
Signed-off-by: Jouni Malinen <[email protected]>
---
net/mac80211/mlme.c | 24 ++++++++++--------------
net/mac80211/wme.c | 9 ++++++---
2 files changed, 16 insertions(+), 17 deletions(-)
--- wireless-testing.orig/net/mac80211/mlme.c 2009-03-05 16:52:36.000000000 +0200
+++ wireless-testing/net/mac80211/mlme.c 2009-03-05 17:16:53.000000000 +0200
@@ -417,9 +417,6 @@ static void ieee80211_sta_wmm_params(str
memset(¶ms, 0, sizeof(params));
- if (!local->ops->conf_tx)
- return;
-
local->wmm_acm = 0;
for (; left >= 4; left -= 4, pos += 4) {
int aci = (pos[0] >> 5) & 0x03;
@@ -427,26 +424,26 @@ static void ieee80211_sta_wmm_params(str
int queue;
switch (aci) {
- case 1:
+ case 1: /* AC_BK */
queue = 3;
if (acm)
- local->wmm_acm |= BIT(0) | BIT(3);
+ local->wmm_acm |= BIT(1) | BIT(2); /* BK/- */
break;
- case 2:
+ case 2: /* AC_VI */
queue = 1;
if (acm)
- local->wmm_acm |= BIT(4) | BIT(5);
+ local->wmm_acm |= BIT(4) | BIT(5); /* CL/VI */
break;
- case 3:
+ case 3: /* AC_VO */
queue = 0;
if (acm)
- local->wmm_acm |= BIT(6) | BIT(7);
+ local->wmm_acm |= BIT(6) | BIT(7); /* VO/NC */
break;
- case 0:
+ case 0: /* AC_BE */
default:
queue = 2;
if (acm)
- local->wmm_acm |= BIT(1) | BIT(2);
+ local->wmm_acm |= BIT(0) | BIT(3); /* BE/EE */
break;
}
@@ -460,9 +457,8 @@ static void ieee80211_sta_wmm_params(str
local->mdev->name, queue, aci, acm, params.aifs, params.cw_min,
params.cw_max, params.txop);
#endif
- /* TODO: handle ACM (block TX, fallback to next lowest allowed
- * AC for now) */
- if (local->ops->conf_tx(local_to_hw(local), queue, ¶ms)) {
+ if (local->ops->conf_tx &&
+ local->ops->conf_tx(local_to_hw(local), queue, ¶ms)) {
printk(KERN_DEBUG "%s: failed to set TX queue "
"parameters for queue %d\n", local->mdev->name, queue);
}
--- wireless-testing.orig/net/mac80211/wme.c 2009-03-05 17:01:26.000000000 +0200
+++ wireless-testing/net/mac80211/wme.c 2009-03-05 17:03:25.000000000 +0200
@@ -99,10 +99,13 @@ static u16 classify80211(struct ieee8021
/* in case we are a client verify acm is not set for this ac */
while (unlikely(local->wmm_acm & BIT(skb->priority))) {
if (wme_downgrade_ac(skb)) {
- /* The old code would drop the packet in this
- * case.
+ /*
+ * This should not really happen. The AP has marked all
+ * lower ACs to require admission control which is not
+ * a reasonable configuration. Allow the frame to be
+ * transmitted using AC_BK as a workaround.
*/
- return 0;
+ break;
}
}
--
Jouni Malinen PGP id EFC895FA
On Thu, 2009-03-05 at 17:23 +0200, Jouni Malinen wrote:
> @@ -99,10 +99,13 @@ static u16 classify80211(struct ieee8021
> /* in case we are a client verify acm is not set for this ac */
> while (unlikely(local->wmm_acm & BIT(skb->priority))) {
> if (wme_downgrade_ac(skb)) {
> - /* The old code would drop the packet in this
> - * case.
> + /*
> + * This should not really happen. The AP has marked all
> + * lower ACs to require admission control which is not
> + * a reasonable configuration. Allow the frame to be
> + * transmitted using AC_BK as a workaround.
> */
> - return 0;
> + break;
It seems to me that return 0 here was incorrect, or wme_downgrade_ac
needs changes?
johannes
On Sun, 2009-03-08 at 16:17 +0200, Jouni Malinen wrote:
> On Sun, Mar 08, 2009 at 01:28:07PM +0100, Johannes Berg wrote:
> > On Thu, 2009-03-05 at 17:23 +0200, Jouni Malinen wrote:
> >
> > > @@ -99,10 +99,13 @@ static u16 classify80211(struct ieee8021
> > > /* in case we are a client verify acm is not set for this ac */
> > > while (unlikely(local->wmm_acm & BIT(skb->priority))) {
> > > if (wme_downgrade_ac(skb)) {
> > > - /* The old code would drop the packet in this
> > > - * case.
> > > + /*
> > > + * This should not really happen. The AP has marked all
> > > + * lower ACs to require admission control which is not
> > > + * a reasonable configuration. Allow the frame to be
> > > + * transmitted using AC_BK as a workaround.
> > > */
> > > - return 0;
> > > + break;
> >
> > It seems to me that return 0 here was incorrect, or wme_downgrade_ac
> > needs changes?
>
> Yes, this return 0 was incorrect and that's why I'm fixing it to not
> return 0 in this patch.. ;-)
Sorry, I meant correct. Why is it not correct? wme_downgrade_ac doesn't
modify the skb when it returns an error.
> In theory, the correct behavior would be to
> drop the frame if the AP is using mandatory admission control for all
> ACs, but in practice, it is probably better not to break the connection
> completely if the AP is misconfigured. The behavior after this patch is
> to downgrade the AC until one is found without ACM and if no such AC
> exists, use the lowest priority (AC_BK).
True. I'm just questioning that this really is what's happening with
this patch :)
johannes
On Sun, Mar 08, 2009 at 01:28:07PM +0100, Johannes Berg wrote:
> On Thu, 2009-03-05 at 17:23 +0200, Jouni Malinen wrote:
>
> > @@ -99,10 +99,13 @@ static u16 classify80211(struct ieee8021
> > /* in case we are a client verify acm is not set for this ac */
> > while (unlikely(local->wmm_acm & BIT(skb->priority))) {
> > if (wme_downgrade_ac(skb)) {
> > - /* The old code would drop the packet in this
> > - * case.
> > + /*
> > + * This should not really happen. The AP has marked all
> > + * lower ACs to require admission control which is not
> > + * a reasonable configuration. Allow the frame to be
> > + * transmitted using AC_BK as a workaround.
> > */
> > - return 0;
> > + break;
>
> It seems to me that return 0 here was incorrect, or wme_downgrade_ac
> needs changes?
Yes, this return 0 was incorrect and that's why I'm fixing it to not
return 0 in this patch.. ;-) In theory, the correct behavior would be to
drop the frame if the AP is using mandatory admission control for all
ACs, but in practice, it is probably better not to break the connection
completely if the AP is misconfigured. The behavior after this patch is
to downgrade the AC until one is found without ACM and if no such AC
exists, use the lowest priority (AC_BK).
--
Jouni Malinen PGP id EFC895FA
On Sun, Mar 08, 2009 at 03:25:33PM +0100, Johannes Berg wrote:
> On Sun, 2009-03-08 at 16:17 +0200, Jouni Malinen wrote:
> > On Sun, Mar 08, 2009 at 01:28:07PM +0100, Johannes Berg wrote:
> > > On Thu, 2009-03-05 at 17:23 +0200, Jouni Malinen wrote:
> > > > @@ -99,10 +99,13 @@ static u16 classify80211(struct ieee8021
> > > > while (unlikely(local->wmm_acm & BIT(skb->priority))) {
> > > > if (wme_downgrade_ac(skb)) {
> > > > - return 0;
> > > > + break;
> > > It seems to me that return 0 here was incorrect, or wme_downgrade_ac
> > > needs changes?
> > Yes, this return 0 was incorrect and that's why I'm fixing it to not
> > return 0 in this patch.. ;-)
> Sorry, I meant correct. Why is it not correct? wme_downgrade_ac doesn't
> modify the skb when it returns an error.
Ah, okay, that's a more sensible comment and I can even try to provide a
more useful reply after having understood your point ;-). The failed
call to wme_downgrade_ac() does not modify the skb, but the previous
call(s) did (if there was one and there likely was). In other words, the
loop of calling wme_downgrade_ac() will end up downgrading the skb all
the way to using skb->priority = 2 which maps to AC_BK.
classify80211() returns the queue number based on the 802.1d tag (= UP)
and the old code was returning 0 in the error case (could not
downgrade). If you take a look at the ieee802_1d_to_ac[] array in the
beginning of wme.c you can see that the value 0 is used for UP values 6
and 7, i.e., something that maps to AC_VO, while we really should return
ieee802_1d_to_ac[2] = 3 (AC_BK) here. Breaking the loop on error makes
the classify80211() look up the proper queue number from
ieee802_1d_to_ac[] based on the downgraded skb->priority, not by using a
hardcoded value here, and ends up returning the desired value here
(lowest priority queue, not highest as the current code does).
--
Jouni Malinen PGP id EFC895FA
On Sun, 2009-03-08 at 19:59 +0200, Jouni Malinen wrote:
> > Sorry, I meant correct. Why is it not correct? wme_downgrade_ac doesn't
> > modify the skb when it returns an error.
>
> Ah, okay, that's a more sensible comment and I can even try to provide a
> more useful reply after having understood your point ;-).
[snip]
Ok, yes, that makes a lot more sense now that I see your comments and
the code, and your fix does seem correct to me now after having
understood it :)
johannes