From: Peter Hessler <phessler@theapt.org>
Subject: Re: replace ieee80211_chan2mode()
To: tech@openbsd.org
Date: Fri, 1 Aug 2025 16:27:24 +0200

On 2025 Aug 01 (Fri) at 13:16:19 +0200 (+0200), Stefan Sperling wrote:
:ieee80211_chan2mode() is broken in several ways.
:
:chan2mode does not detect 11g APs properly. This can be fixed now that
:we are setting the ERP flag not only in hostap mode but also in client
:mode.
:
:If chan2mode is called while our phy mode is fixed, then it always
:returns our fixed phy mode. This means it wrongly selects 11g rates if
:our phy mode is fixed to 11g even if we are talking to an 11b-only peer
:which cannot receive 11g rates.
:
:chan2mode tries to handle a NULL channel which is not relevant to any
:of the callers.
:
:I want to replace chan2mode with a new ieee80211_node_abg_mode() function
:which serves the same purpose but does not have any of the above issues.
:
:Tested with qwx(4), which can now automatically associate with an AP which
:denies clients that use 11b data rates. Previously, 'ifconfig qwx0 mode 11g'
:was required to associate with such APs.
:
:ok?
:

OK, two optional comments inline


:blob - 2e02e1fff93930e65fc955599b4f7f1d85843259
:blob + a9abfb9366f3bc80f5abaaabceb9480bd06d9a5f
:--- sys/net80211/ieee80211_node.c
:+++ sys/net80211/ieee80211_node.c
:@@ -931,8 +931,28 @@ ieee80211_create_ibss(struct ieee80211com* ic, struct 
: 		mode = IEEE80211_MODE_11AC;
: 	else if (ic->ic_flags & IEEE80211_F_HTON)
: 		mode = IEEE80211_MODE_11N;
:-	else
:-		mode = ieee80211_chan2mode(ic, ni->ni_chan);
:+	else {

As I mentioned on icb, can you add the comment from the other place you
have this switch?


:+		switch (IFM_MODE(ic->ic_media.ifm_cur->ifm_media)) {
:+		case IFM_IEEE80211_11A:
:+			mode = IEEE80211_MODE_11A;
:+			break;
:+		case IFM_IEEE80211_11G:
:+			mode = IEEE80211_MODE_11G;
:+			break;
:+		case IFM_IEEE80211_11B:
:+			mode = IEEE80211_MODE_11B;
:+			break;
:+		default:
:+			if (IEEE80211_IS_CHAN_5GHZ(ni->ni_chan))
:+				mode = IEEE80211_MODE_11A;
:+			else if ((ni->ni_chan->ic_flags &
:+			    (IEEE80211_CHAN_OFDM | IEEE80211_CHAN_DYN)) != 0)
:+				mode = IEEE80211_MODE_11G;
:+			else
:+				mode = IEEE80211_MODE_11B;
:+			break;
:+		}
:+	}
: 	ieee80211_setmode(ic, mode);
: 	/* Pick an appropriate mode for supported legacy rates. */
: 	if (ic->ic_curmode == IEEE80211_MODE_11AC) {



:blob - 3d8d649c8f0ff5e79c98fc9b6ec95d0c4d68b7f2
:blob + 2463eec03cbbb297d18a40f1016e9c8800859f7f
:--- sys/net80211/ieee80211_proto.c
:+++ sys/net80211/ieee80211_proto.c
:@@ -221,7 +221,7 @@ ieee80211_fix_rate(struct ieee80211com *ic, struct iee
: 
: 	error = 0;
: 	okrate = badrate = fixedrate = 0;
:-	srs = &ic->ic_sup_rates[ieee80211_chan2mode(ic, ni->ni_chan)];
:+	srs = &ic->ic_sup_rates[ieee80211_node_abg_mode(ic, ni)];
: 	nrs = &ni->ni_rates;
: 	for (i = 0; i < nrs->rs_nrates; ) {
: 		ignore = 0;
:@@ -1153,8 +1153,8 @@ justcleanup:
: 		/* initialize bss for probe request */
: 		IEEE80211_ADDR_COPY(ni->ni_macaddr, etherbroadcastaddr);
: 		IEEE80211_ADDR_COPY(ni->ni_bssid, etherbroadcastaddr);
:-		ni->ni_rates = ic->ic_sup_rates[
:-			ieee80211_chan2mode(ic, ni->ni_chan)];
:+		ni->ni_rates = ic->ic_sup_rates[ieee80211_node_abg_mode(ic,
:+		    ni)];

Purely a bikeshed, but the line-wrapping here makes my eye twitch.  Can
you wrap it in the same style as the - lines?


: 		ni->ni_associd = 0;
: 		ni->ni_rstamp = 0;
: 		ni->ni_rsn_supp_state = RSNA_SUPP_INITIALIZE;

-- 
Two can Live as Cheaply as One for Half as Long.
		-- Howard Kandel