Details

Reviewers

adrian
bz
nagymas_gmail.com

Group Reviewers

wireless

Summary

Reporting/debug path changed to display VHT80 and VHT160

Test Plan

Test iwx up to VHT80
Test iwlwifi up to VHT160

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

nagymas_gmail.com created this revision.Tue, Jun 23, 6:22 AM

Herald added subscribers: glebius, melifaro, imp. · View Herald TranscriptTue, Jun 23, 6:22 AM

nagymas_gmail.com requested review of this revision.Tue, Jun 23, 6:22 AM

adrian added a project: wireless.Tue, Jun 23, 6:24 AM

adrian added a reviewer: wireless.

adrian added a subscriber: wireless.

If we do it then let us do better than AI and also document 80P80.

sys/net80211/ieee80211_ht.c
1822	IEEE80211_IS_CHAN_VHT80P80(c) ? 160 :
sys/net80211/ieee80211_sta.c
1943	IEEE80211_IS_CHAN_VHT80P80(ni->ni_chan) ? ", VHT80P80" :

This revision now requires changes to proceed.Tue, Jun 23, 1:30 PM

Agreed but we could detail VHT40U and D and VHT20 too

Added every configuration currently available for more accurate debuging

Original reporting:
VHT80
VHT160
VHT (covers VHT20/VHT40)
Updated reporting:
VHT20
VHT40U
VHT40D
VHT80
VHT160
VHT80P80
Reason:
ni->ni_chan->ic_flags already contains the full channel-width information, so the patch makes the debug output reflect the actual negotiated channel state.

Debug string update to be able to display VHT40U and VHT40D

String format proved to be working
Jun 24 10:27:15 freebsd kernel: wlan0: notify scan done
Jun 24 10:27:15 freebsd kernel: wlan0: [] station assoc via MLME
Jun 24 10:27:15 freebsd kernel: wlan0: [] htinfo_update_chw: VHT; chanwidth=0x01; vhtflags=0x10020000
Jun 24 10:27:15 freebsd kernel: wlan0: [] htinfo_update_chw: VHT; trying lookup for 5180/0x10020142
Jun 24 10:27:15 freebsd kernel: wlan0: [] switch station to VHT160 channel 5180/0x100a0142
Jun 24 10:27:15 freebsd kernel: wlan0: ieee80211_sta_join1 0x<> SCAN -> AUTH, FC0_SUBTYPE_DEAUTH
Jun 24 10:27:15 freebsd kernel: wlan0: [] recv auth frame with algorithm 0 seq 2
Jun 24 10:27:15 freebsd kernel: wlan0: ieee80211_swscan_cancel_scan: called; F_SCAN=0, vap=match, signal=0
Jun 24 10:27:15 freebsd kernel: wlan0: [] assoc success at aid 2: short preamble, short slot time, QoS, VHT160 (+AMPDU) (+AMSDU)
Jun 24 10:27:15 freebsd kernel: wlan0: ieee80211_swscan_cancel_scan: called; F_SCAN=0, vap=match, signal=0
Jun 24 10:27:15 freebsd kernel: wlan0: [] ieee80211_scan_assoc_success
Jun 24 10:27:15 freebsd kernel: wlan0: link state changed to UP

Test HT20, HT40, VHT20, VHT40, VHT80, VHT160 not tested VHT80P80

Reverted to the original branching to keep the original structure. %s%d changed to %s%s to handle 80P80

I think we should just create a function to return a static channel description for this. something like

/*
 * Return a string like <MODE><WIDTH> - eg HT20, VHT80+80, etc.
 */
static const char * net80211_get_chanmode_descr(const struct ieee80211_channel *c)
{
  // do all the HT/VHT stuff currently done in the above log code
 // default
  return "<undef>";
}

What do you think?

In D57762#1327568, @adrian wrote:
I think we should just create a function to return a static channel description for this. something like
/*
 * Return a string like <MODE><WIDTH> - eg HT20, VHT80+80, etc.
 */
static const char * net80211_get_chanmode_descr(const struct ieee80211_channel *c)
{
  // do all the HT/VHT stuff currently done in the above log code
 // default
  return "<undef>";
}
What do you think?

Proposal:
This proposal follows the reviewer's suggestion of separating the mode and width handling. I think a simpler solution could achieve the same result, as the IEEE80211_IS_CHAN_VHTxxx macros already encode both the mode and the channel width. From my perspective, the more important improvement is correcting the original code's default behaviour, which could produce misleading diagnostic output.

static void
net80211_get_chanmode_descr(const struct ieee80211_channel *c,

const char **mode, const char **width)

{
if (IEEE80211_IS_CHAN_VHT(c)) {

		*mode = "VHT";

		if (IEEE80211_IS_CHAN_VHT80P80(c))
			*width = "80P80";
		else if (IEEE80211_IS_CHAN_VHT160(c))
			*width = "160";
		else if (IEEE80211_IS_CHAN_VHT80(c))
			*width = "80";
		else if (IEEE80211_IS_CHAN_VHT40(c))
			*width = "40";
		else if (IEEE80211_IS_CHAN_VHT20(c))
			*width = "20";
		else
			*width = "<undef>";

		return;

}

if (IEEE80211_IS_CHAN_HT(c)) {

		*mode = "HT";

		if (IEEE80211_IS_CHAN_HT40(c))
			*width = "40";
		else
			*width = "20";

		return;

}

*mode = "<undef>";
*width = "";
}

Call site
const char *mode;
const char *width;

net80211_get_chanmode_descr(c, &mode, &width);

printf("switch station to %s%s channel %u/0x%x",

mode, width,
ieee80211_chan2ieee(ic, c),
c->ic_flags);

New graph:

call helper

|
v

net80211_get_chanmode_descr(c, &mode, &width)

|
+-- VHT?
|     |
|     +-- mode = "VHT"
|     |
|     +-- VHT80P80? -> width = "80P80" -> return
|     +-- VHT160?   -> width = "160"   -> return
|     +-- VHT80?    -> width = "80"    -> return
|     +-- VHT40?    -> width = "40"    -> return
|     +-- VHT20?    -> width = "20"    -> return
|     +-- else      -> width = "<undef>" -> return
|
+-- HT?
|     |
|     +-- mode = "HT"
|     |
|     +-- HT40?     -> width = "40" -> return
|     +-- else      -> width = "20" -> return
|
+-- else
      |
      +-- mode = "<undef>"
      +-- width = ""
      +-- return

Old code:

Start

+-- first %s: mode

+-- IEEE80211_IS_CHAN_VHT(c)?

+-- yes -> "VHT"

+-- no -> "HT"

+-- second %s: width

|
+-- VHT80P80? -> "80P80"
+-- VHT160?   -> "160"
+-- VHT80?    -> "80"
+-- VHT40?    -> "40"
+-- VHT20?    -> "20"
+-- HT40?     -> "40"
+-- else      -> "20"

I personally I would go with the simpler solution calling IEEE80211_IS_CHAN_VHTxxx macros already combines mode and width.

static const char *
net80211_get_chanmode_descr(const struct ieee80211_channel *c)
{
if (IEEE80211_IS_CHAN_VHT(c)) {

		if (IEEE80211_IS_CHAN_VHT80P80(c))
			return ("VHT80P80");
		if (IEEE80211_IS_CHAN_VHT160(c))
			return ("VHT160");
		if (IEEE80211_IS_CHAN_VHT80(c))
			return ("VHT80");
		if (IEEE80211_IS_CHAN_VHT40(c))
			return ("VHT40");
		if (IEEE80211_IS_CHAN_VHT20(c))
			return ("VHT20");

		return ("<undef-vht-width>");

}

if (IEEE80211_IS_CHAN_HT(c)) {

		if (IEEE80211_IS_CHAN_HT40(c))
			return ("HT40");

		return ("HT20");

}

return ("<undef>");
}

In D57762#1327751, @nagymas_gmail.com wrote:

I personally I would go with the simpler solution calling IEEE80211_IS_CHAN_VHTxxx macros already combines mode and width.

static const char *
net80211_get_chanmode_descr(const struct ieee80211_channel *c)
{

...
<VHT*>

I wish we had an enum so it would be clear there is no "else". I think you can just remove that value because that's where your HT check comes in. See the original logic from the review.
...

}

return ("<undef>");
}

That <undef> isn't quite the best solution, is it?

nagymas_gmail.com added a comment.Sun, Jun 28, 1:18 AM

This comment was removed by nagymas_gmail.com.

nagymas_gmail.com added a comment.Sun, Jun 28, 2:01 AM

This comment was removed by nagymas_gmail.com.

nagymas_gmail.com added a comment.Sun, Jun 28, 11:49 AM

This comment was removed by nagymas_gmail.com.

For now I'd like to stay close to the original implementation and only extend the diagnostic where necessary.

Assumptions:

By the time the ieee80211_ht.c diagnostic is emitted, the channel has already been classified as at least HT, so preserving the original HT fallback is acceptable.
The HT width set is fixed in this context, so the existing HT40/HT20 logic is retained.
The VHT diagnostic is extended to report all supported VHT widths. If the VHT flag/path is selected but no VHT width predicate matches, report an explicit diagnostic instead of silently reporting VHT20.

This preserves the original control flow, adds the missing VHT width reporting, and only introduces error reporting where the current implementation could produce misleading diagnostic output. What do you think?

ht.c:

mode is selected from channel object c

sta.c:

highest node family is selected first:
    VHT before HT

---------------------------------------------------------ieee80211_ht.c

 if (c != NULL && c != ni->ni_chan) {
	IEEE80211_NOTE(ni->ni_vap,
	    IEEE80211_MSG_ASSOC | IEEE80211_MSG_11N, ni,
 	    "switch station to %s%s channel %u/0x%x",
	    IEEE80211_IS_CHAN_VHT(c) ? "VHT" : "HT",
	    IEEE80211_IS_CHAN_VHT(c) ?
	        (IEEE80211_IS_CHAN_VHT80P80(c) ? "80P80" :
	         IEEE80211_IS_CHAN_VHT160(c)   ? "160" :
	         IEEE80211_IS_CHAN_VHT80(c)    ? "80" :
	         IEEE80211_IS_CHAN_VHT40(c)    ? "40" :
	         IEEE80211_IS_CHAN_VHT20(c)    ? "20" :
	         "<unknown VHT channel width>") :
	        (IEEE80211_IS_CHAN_HT40(c) ? "40" : "20"),
	    c->ic_freq, c->ic_flags);

	ni->ni_chan = c;
	ret = 1;
 }

START

|
+-- channel changed?
|     |
|     +-- no  -> no diagnostic
|     |
|     +-- yes
|           |
|           +-- mode text:
|           |     |
|           |     +-- VHT? -> "VHT"
|           |     +-- else -> "HT"
|           |
|           +-- width text:
|                 |
|                 +-- VHT?
|                 |     |
|                 |     +-- VHT80P80 -> "80P80"
|                 |     +-- VHT160   -> "160"
|                 |     +-- VHT80    -> "80"
|                 |     +-- VHT40    -> "40"
|                 |     +-- VHT20    -> "20"
|                 |     +-- no match -> "<unknown VHT channel width>"
|                 |
|                 +-- else / HT path
|                       |
|                       +-- HT40 -> "40"
|                       +-- else -> "20"
|
+-- print:
      "switch station to <mode><width> channel ..."

---------------------------------------------------------ieee80211_sta.c

  vap->iv_flags & IEEE80211_F_SHSLOT ? "short" : "long",
  vap->iv_flags & IEEE80211_F_USEPROT ? ", protection" : "",
  ni->ni_flags & IEEE80211_NODE_QOS ? ", QoS" : "",
  ni->ni_flags & IEEE80211_NODE_VHT ?
  	(IEEE80211_IS_CHAN_VHT160(ni->ni_chan) ? ", VHT160" :
	 IEEE80211_IS_CHAN_VHT80P80(ni->ni_chan) ? ", VHT80P80" :
	 IEEE80211_IS_CHAN_VHT80(ni->ni_chan) ? ", VHT80" :
	 IEEE80211_IS_CHAN_VHT40(ni->ni_chan) ? ", VHT40" :
	 IEEE80211_IS_CHAN_VHT20(ni->ni_chan) ? ", VHT20" :
	 ", <unknown VHT channel width>") :
  ni->ni_flags & IEEE80211_NODE_HT ?
  	(ni->ni_chw == NET80211_STA_RX_BW_40 ? ", HT40" : ", HT20") : "",
  ni->ni_flags & IEEE80211_NODE_AMPDU ? " (+AMPDU)" : "",

START

+-- assoc success diagnostic

|
+-- QoS?
|     |
|     +-- yes -> ", QoS"
|     +-- no  -> ""
|
+-- mode text:
      |
      +-- NODE_VHT?
      |     |
      |     +-- VHT160   -> ", VHT160"
      |     +-- VHT80P80 -> ", VHT80P80"
      |     +-- VHT80    -> ", VHT80"
      |     +-- VHT40    -> ", VHT40"
      |     +-- VHT20    -> ", VHT20"
      |     +-- no match -> ", <unknown VHT channel width>"
      |
      +-- else NODE_HT?
      |     |
      |     +-- HT40 -> ", HT40"
      |     +-- else -> ", HT20"
      |
      +-- else -> ""
|
+-- AMPDU / AMSDU / SMPS / RIFS / etc.

Reporting/debug path changed to display VHT80 and VHT160
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 180490

sys/net80211/ieee80211_ht.c

sys/net80211/ieee80211_sta.c

Reporting/debug path changed to display VHT80 and VHT160Needs ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 180490

sys/net80211/ieee80211_ht.c

sys/net80211/ieee80211_sta.c

Reporting/debug path changed to display VHT80 and VHT160
Needs ReviewPublic
Actions

Revision Contents
Changeset List