net80211: add helper functions for determining 11n transmit parameters - AMPDU density/size, short-GI

[adrian]

Summary

net80211: add helper functions for determining HT transmit parameters

This adds helper functions to determine a variety of HT related
transmit parameters:

* AMPDU density and size;
* short-GI for 20 and 40MHz;
* Whether 40MHz transmit is allowed;
* Whether HT rates have been negotiated for transmit.

It should be done here rather than drivers having to re-invent the wheel
each time.

This is doubly important for AP modes where each node will hvae
different supported features and this needs to be used when
assembling transmit frames / configuring transmit parameters.

[adrian]

add url

[adrian]

add some more helper functions that I know are duplicated in the codebase

[adrian]

sys/net80211/ieee80211_ht.c

3722

Hm, should do the false check here and above too, will update with that

[bz]

sys/net80211/ieee80211_ht.c

3618

Can we move assignments away from declaration to before "first use"? (here and all the way down in these changes)

3625

return (peer_mpdudensity); (here and all the way down in the changes)

3701

Here you check for a NULL channel but in the two above you don't. Should we?
Could also fold that into a single check for all of them then?

if (ni->ni_chan == NULL ||
    ni->ni_chan == IEEE80211_CHAN_ANYC)
         return (false);

3707

you return bool; either it is 0 or not so no need for !! anymore, is there?

[emaste]

sys/net80211/ieee80211_ht.c

3618

style(9) has "Variables may be initialized where declared especially when they are constant for the rest of the scope." and shows as an example

struct foo one, *two;
struct baz *three = bar_get_baz(bar);
double four;
int *five, six;
char *seven, eight, nine, ten, eleven, twelve;

four = my_complicated_function(a1, f1, a4);

I prefer this style for this sort of initialization.

[bz]

sys/net80211/ieee80211_ht.c

3618

I don't in this case but maybe it is personal taste; for me it's because of the function argument; if you want to add debugging/tracing/KASSERT on the ni you have to possibly move the initialization.
It's probably fine for an lvalue but dereferencing a pointer before you can possibly check it (or even better as happens in some code -- not here -- check it afterwards) is questionable.

Adrian, up to you.

[emaste]

sys/net80211/ieee80211_ht.c

3707

Correct, with bool return type tricks like !! are not necessary.

[adrian]

hopefully update to address bz/emaste concerns; also constify things.

[adrian]

oops, missed these two

[adrian]

ok let's try this. I'm trying to be as exact as possible here. :-)

[adrian]

more updates

[bz]

sys/net80211/ieee80211_ht.c

3646

return ()

3663

style9 says " If you have to wrap a long statement, put the operator at the end of the line."

3675

and all here

3696

here as well (I'll sto pmarking them; but there are more down the file)

[adrian]

more updates

[bz]

sys/net80211/ieee80211_ht.c

3679

So here's to the too many layers and flags bits net80211 can be:

Given htcap_update_shortgi(), could the last two lines at least be folded into ni->ni_flags & IEEE80211_NODE_SGI20 or am I wrong?
I am *NOT* asking you to do that, Just for me to make sure I understand correctly.

That ni flag is otherwise confusingly single-use only for the ioctl (hmm and ifconfig and bsnmp use it for reporting -- and I wonder if they show what it says) and I wish we would not have 3+ options to express one thing...

I assume in times of bool a lot could be spelt more clearly, so these helpers here will do good - at least inside the kernel.

3708

Ignoring that ht_recv_action_ht_txchwidth() may have insufficient checking (*) is this not only possible if IEEE80211_IS_CHAN_HT40(ni->ni_chan), so a check is redundant here?
I'd leave it but just saying... for for the action frame handler I supposed.

(*) if we set it to 40 there but are not on a 40 channel (or more funny an AP not [announcing] supporting 40Mhz channels) we are partially doomed for TX?

3763

I keep wondering after all changes if this wants to call ieee80211_ht_check_tx_ht() and then only do the final check?

[adrian]

sys/net80211/ieee80211_ht.c

3679

Yup, part of why i'm trying to pull these out of drivers is they're all doing something mostly the same but subtly not consistently. That's mainly on me, trying to bring /up/ 11n support years ago with a lack of these checks existing.

It and the other questions below are good questions. I found way too many weird corner cases in the past (HT40 STA/AP, but the channel width command/notification? Some APs claiming HT but not announcing they receive any HT rates, so you couldn't ever transmit to them - confusing the snot out of AMRR and the iwn firmware? etc, etc) that I'd really like to get sorted out here.

(And there's more to come too after we sort through this stuff - sequence number assigning, A-MPDU BAR windows and QoS NULL frames. A hilarious combination.)

3763

Yeah that should simplify it quite a bit. Technically all of the tx_ht macros I wrote could just use tx_ht()... :p

[adrian]

refactor stuff!

[adrian]

sys/net80211/ieee80211_ht.c

3708

Ignoring that ht_recv_action_ht_txchwidth() may have insufficient checking (*) is this not only possible if IEEE80211_IS_CHAN_HT40(ni->ni_chan), so a check is redundant here?
I'd leave it but just saying... for for the action frame handler I supposed.

(*) if we set it to 40 there but are not on a 40 channel (or more funny an AP not [announcing] supporting 40Mhz channels) we are partially doomed for TX?

The drivers CAN choose to TX HT20 rates on a HT40 channel if available. I think they all support that.
I just don't think most do - I know ath(4) does due to hostap support.

At least with these functions we have a chance of cleaning the drivers up and making this work consistently, especially where people want to see hostap / ibss work.