rtwn: use ieee80211_ht_check_tx_shortgi_20() and ieee80211_ht_check_tx_shortgi_40()

[adrian]

Summary

Use the new net80211 routines rather than rolling our own.

[adrian]

add url

[adrian]

looking at these, i'm wondering whether the better move is to be doing the channel check inside the helper functions as well. i remember the node channel being .. weirdly sketchy thought. i'll go do some more deep diving on this before it lands.

[bz]

In D47751#1088951, @adrian wrote:

looking at these, i'm wondering whether the better move is to be doing the channel check inside the helper functions as well. i remember the node channel being .. weirdly sketchy thought. i'll go do some more deep diving on this before it lands.

Let us know when you come to the final conclusion and we can do the review then; I'd rather do it once than thrice.

[adrian]

Yeah, it shouldn't take long.

[adrian]

ah I remember it now. ni->ni_chw has to do with the currently configured channel width, which can be changed via a HT_TXCHWIDTH action frame. The STA can request that the AP switch to 20MHz for transmitting to that station for now, even if the station is associated in 40MHz.

So when it's associated, ni_chw == the width of ni->ni_chan.

I think I'm going to add another helper function that returns true if the channel is 40MHz, the peer is 40MHz /and/ the currently selected width is 40MHz, so if someone requests a channel width change, it'll correctly start transmitting at 20MHz even though it's associated at 40MHz. (which is perfectly legal.)

Ugh now I remember this mess in more detail. :-) Stay tuned.

[bz]

Staying tuned; please indicate when done.

[adrian]

Ok, this should be fine as-is just to use the new helpers.

I'd like to let that settle a bit whilst I go and update the rtwn selection logic a bit more in rtwn after these land. In particular I'll go and check the node channel width before transmitting on 20/40 MHz like I do with ath(4), and then we'll need to go over the rules for choosing a VHT rate / width when it's time to get VHT transmit rate handling into net80211.

[adrian]

done; and i cleaned up the 20/40MHz selection in a later diff in the stack.

(Although i think for RTL8192CU series chips HT40 is just going to be very broken until I fix firmware rate control, but that's a different story..)

[bz]

sys/dev/rtwn/rtl8192c/r92c_tx.c

180

Okay, I am totally confused by this. Then I noticed:
(a) you swapped the order of checks but the assignments stayed
(b) the assignments are the same for both but we have two different cases?
(c) based on the helper functions committed we don't need the IS_CHAN_HTxx check anymore as they do them already when calling into ieee80211_ht_check_tx_ht{,40}.

So effectively this now is ...?

if (ieee80211_ht_check_tx_shortgi_40(ni) ||
    ieee80211_ht_check_tx_shortgi_20(ni))
         txd->txdw5 |= htole32(R92C_TXDW5_SGI);

Or do I miss anything?

[adrian]

Yeah, thinking about it a bunch more this morning, I think the cleanest / correct-est way to do this is to do the "should I transmit ht40 ?" check first, and then pass in a value about the decided transmit width.

Otherwise this function will enable shortgi for 40MHz if the 20MHz checks pass, and that's no bueno.

[adrian]

be more intentional / specific about 40 vs 20MHz.

I don't entirely like this 100%, but it SHOULD be fine. I think eventually
the whole "do I do 80, 40, 20, HT, VHT, etc" decision should be made outside
of these functions and then they only enable it, but I'll do that in a later
diff.

[adrian]

sys/dev/rtwn/rtl8192c/r92c_tx.c

180

nope, it's still a mess; I hopefully clarified it enough this morning as an MVP for this clean-up.

[bz]

sys/dev/rtwn/rtl8192c/r92c_tx.c

180

@adrian: with c6b44f64c33010501aee2fd99016aeca54a09a1e the following is done at the beginning of ieee80211_ht_check_tx_shortgi_20():

+       if (! ieee80211_ht_check_tx_ht(ni))
+               return (false);

Similarly ieee80211_ht_check_tx_shortgi_40() does at the beginning:

+       if (! ieee80211_ht_check_tx_ht40(ni))
+               return (false);

So you do these checks twice now, right?

185

Sorry this makes no sense, there is no distinction between 40/20 SGI in the flag you enable -- or in other words: the driver cannot tell from what you are telling it if it's a SGI20 or SGI40 unless the flags are wrong .. and have been .. or I cannot spot the difference in these two lines:

txd->txdw5 |= htole32(R92C_TXDW5_SGI);
txd->txdw5 |= htole32(R92C_TXDW5_SGI);

[adrian]

sys/dev/rtwn/rtl8192c/r92c_tx.c

185

So two things!

• yes, when doing driver based TX rate selection, the hardware doesn't have a way to say SGI20 or SGI40 - you select the TX rate, the width, and then SGI. That's why they're the same.

• the difference between the net80211 call and how I'm using it is that the net80211 call only knows if you can do HT40, but it doesn't know what the driver has decided to do. Right now there's no top level "doing ht40 for sure" being selected first, only that every step in the path should be doing the same "should I do ht40?" check. So, if I just do a shortgi40 || shortgi20 net80211 call, the shortgi20 call will work because yes, the node can do both. :-)

There's a diff near the top of the stack where i also make the "should I enable ht40?" check to use the same net80211 call, so everything is consistent. Not pretty, but consistent.

The ideal solution I'll do after this stack has landed and has baked for a bit is to actually make a HT/VHT decision, then make a HT20/HT40/HT80 decision, then make a STBC/LDPC/Short-GI decision whilst passing in the HT20/HT40/HT80 decision flag(s), so that I'm not double/tripling up on the calls to can_tx_ht40().

[bz]

Of course. Sorry. Got you. We have nested if()s now and not && like in the original code for the checks.