net80211: clean up / add more macros to check the frame types

[adrian]

Summary

• Add new macros to check the version, version+type and version+type+subtype of a frame.
• Use these for existing frame type checks
• Add new frame type checks
• Convert the use of these flag checks to use the macros, not direct header poking.

Notably I'm calling out things like QOS any versus QOS data, the
kind of NULL frames, etc. Eg, in the TKIP code it's checking whether
a frame is ANY kind of QOS frame, not just QOS data.

These macros should hopefully make the header checks clearer and less
error prone. They're also useful in drivers that are doing their own
header parsing.

Locally:

• ath(4) - AP, STA, AP+STA modes
• local ath10k/athp - AP, STA modes

[bz]

I'll review next week when back in the office.

[bz]

sys/net80211/ieee80211.h

209

"ext" missing in the comment.

221

I like the above. I was talking about WiFi version checks with someone the other day :)

222

If this is now no longer needed we should remove it.

224

This will need a de-dup with a few drivers. Are you planning on doing that?

226

/* This returns true if it is any QOS frame. */

shorter, less confusing, and fits in a single line; but then this is what the macro name indicates and makes one wonder if the comment is needed at all?

230

I don't see this possibly used anywhere in our current code base. Do you need it?

234

No "/" markup in comments. "only" already emphasize the point.
Also below.

238

This is unused? Do we then need it or are we just adding it for the sake of having more stuff at this point?

248

All three are unused and we seem to do checks on the subtype in switch/case statements. Do you need them anywhere at all?

I see on possible use case for IS_ACTION_ACK() and that is called "IS_ACTION" everywhere else (incl. the case in ath). I'll find this confusing.
I don't see any use cases for the latter two. If you need any, please specify?

258

Unused? We seem to use IEEE80211_FC0_SUBTYPE_DISASSOC basically in switch/case statements for checks. Do you plan to use it as well? Do we need this?

sys/net80211/ieee80211_crypto_tkip.c

863–864

superfluous comment (that's what the macro is) and we don't do random markup in kernel comments usually.

sys/net80211/ieee80211_proto.h

332 ↗

(On Diff #110698)

Where do you need that? I don't like adding unused inline unctions for the sake of being there.
And even if we'd either need to comment the hard coded values or use offsetof so make it clear.

[adrian]

They're all used in the ath10k driver port I'm working on. They're available in the linuxkpi for liinux drivers too :-)

[adrian]

sys/net80211/ieee80211.h

222

i'll go sweep the drivers and see what else can be done to remove this. I didn't want to break the rest of the system in the first pass.

[adrian]

sys/net80211/ieee80211_proto.h

332 ↗

(On Diff #110698)

it's actually exposed in mac80211, and it's used by my net80211 driver. there's a bunch of qos frame encap/decap being done in the driver because of what the hardware/firmware is doing to frames.

(The TL;DR is the microsoft wifi format was non-QoS, so to natively support microsoft Wifi frame formats, the hardware would decap the QoS section for you and then stuff the contents in the receive descriptor so you could re-build it for the stack.)

[adrian]

sys/net80211/ieee80211.h

222

hi! one driver uses this, so guess I'll turn this into a bit of a commit stack. stay tuned.

[bz]

sys/net80211/ieee80211.h

222

Must be out-of-tree as a grep -r through the entire tree only showed net80211. I'll stay tuned.

sys/net80211/ieee80211_proto.h

332 ↗

(On Diff #110698)

The only thing I am aware is ieee80211_get_qos_ctl() and that does return the pointer not the offset very much like we do for ieee80211_getqos() or ieee80211_gettid() in net80211.

[adrian]

sys/net80211/ieee80211.h

222

It's included in the iwl linux driver from the linux include stuff. Guess you're currently using it in the compat code?

sys/net80211/ieee80211_proto.h

332 ↗

(On Diff #110698)

Lemme see; this i can do away with for now cause it's only used by the ath10k port that's not yet landed.

[adrian]

Update this old diff to the subset of stuff bz@ called out;
the rest of the macros are useful but yes there aren't yet
any in-tree users.

[adrian]

The comments belong to the previous commit; I've marked em as done.

@bz if you have any updates or wanna re-do the comments on right lines then please let me know!

[bz]

You need to properly rebase your changes or diff to the right branch. The SPDX Tag or the $FreeBSD$ removal should not be part of a review change here.

sys/net80211/ieee80211.h

2

(*) General comment.

[adrian]

In D36615#1083293, @bz wrote:

You need to properly rebase your changes or diff to the right branch. The SPDX Tag or the $FreeBSD$ removal should not be part of a review change here.

weird, i did this specifically against -head as of yesterday. i wonder what's up with that.

[adrian]

In D36615#1083293, @bz wrote:

You need to properly rebase your changes or diff to the right branch. The SPDX Tag or the $FreeBSD$ removal should not be part of a review change here.

I checked, what's wrong exactly? The diff itself doesn't show any changes, and it has the correct SPDX tag in the file and not in the actual diff being looked at.

[bz]

Sorry. My fault. It seems I was stuck on the change from diff1 to diff2. I'll try to have a look during Sunday. It's after 1AM here.

[bz]

If you want you can remove the (!<space>IEEE..) spaces but otherwise I am fine by reading through it again.
Like the QOS_MASK_ANY change introduced.

I haven't build/tested it.

[adrian]

comments from bz

[bz]

If you want me to build/test the stack of changes let me know; otherwise this one is good to go from me.