[PATCH 01/10] ap: make supported rates a common builder.

[PATCH 01/10] ap: make supported rates a common builder.

[James Prestwood]

The supported rates IE was being built in two places. This makes that
code common. Unfortunately it needs to support both an ie builder
and using a pointer directly which is why it only builds the contents
of the IE and the caller must set the type/length.
---
 src/ap.c | 68 +++++++++++++++++++++++++-------------------------------
 1 file changed, 30 insertions(+), 38 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index a9027f79..595c71c9 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -802,6 +802,29 @@ static size_t ap_write_wsc_ie(struct ap_state *ap,
 	return len;
 }
 
+static size_t ap_build_supported_rates(struct ap_state *ap,
+					uint8_t *rates)
+{
+	uint32_t minr, maxr, count, r;
+
+	minr = l_uintset_find_min(ap->rates);
+	maxr = l_uintset_find_max(ap->rates);
+	count = 0;
+	for (r = minr; r <= maxr && count < 8; r++)
+		if (l_uintset_contains(ap->rates, r)) {
+			uint8_t flag = 0;
+
+			/* Mark only the lowest rate as Basic Rate */
+			if (count == 0)
+				flag = 0x80;
+
+			*rates++ = r | flag;
+			count++;
+		}
+
+	return count;
+}
+
 static size_t ap_get_extra_ies_len(struct ap_state *ap,
 					enum mpdu_management_subtype type,
 					const struct mmpdu_header *client_frame,
@@ -858,8 +881,7 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
 	struct mmpdu_header *mpdu = (void *) out_buf;
 	uint16_t capability = IE_BSS_CAP_ESS | IE_BSS_CAP_PRIVACY;
 	const uint8_t *bssid = netdev_get_address(ap->netdev);
-	uint32_t minr, maxr, count, r;
-	uint8_t *rates;
+	size_t len;
 	struct ie_tlv_builder builder;
 
 	memset(mpdu, 0, 36); /* Zero out header + non-IE fields */
@@ -884,24 +906,8 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
 
 	/* Supported Rates IE */
 	ie_tlv_builder_next(&builder, IE_TYPE_SUPPORTED_RATES);
-	rates = ie_tlv_builder_get_data(&builder);
-
-	minr = l_uintset_find_min(ap->rates);
-	maxr = l_uintset_find_max(ap->rates);
-	count = 0;
-	for (r = minr; r <= maxr && count < 8; r++)
-		if (l_uintset_contains(ap->rates, r)) {
-			uint8_t flag = 0;
-
-			/* Mark only the lowest rate as Basic Rate */
-			if (count == 0)
-				flag = 0x80;
-
-			*rates++ = r | flag;
-			count++;
-		}
-
-	ie_tlv_builder_set_length(&builder, count);
+	len = ap_build_supported_rates(ap, ie_tlv_builder_get_data(&builder));
+	ie_tlv_builder_set_length(&builder, len);
 
 	/* DSSS Parameter Set IE for DSSS, HR, ERP and HT PHY rates */
 	ie_tlv_builder_next(&builder, IE_TYPE_DSSS_PARAMETER_SET);
@@ -1540,8 +1546,8 @@ static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
 	struct mmpdu_header *mpdu = (void *) mpdu_buf;
 	struct mmpdu_association_response *resp;
 	size_t ies_len = 0;
+	size_t len;
 	uint16_t capability = IE_BSS_CAP_ESS | IE_BSS_CAP_PRIVACY;
-	uint32_t r, minr, maxr, count;
 
 	memset(mpdu, 0, sizeof(*mpdu));
 
@@ -1561,23 +1567,9 @@ static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
 
 	/* Supported Rates IE */
 	resp->ies[ies_len++] = IE_TYPE_SUPPORTED_RATES;
-
-	minr = l_uintset_find_min(ap->rates);
-	maxr = l_uintset_find_max(ap->rates);
-	count = 0;
-	for (r = minr; r <= maxr && count < 8; r++)
-		if (l_uintset_contains(ap->rates, r)) {
-			uint8_t flag = 0;
-
-			/* Mark only the lowest rate as Basic Rate */
-			if (count == 0)
-				flag = 0x80;
-
-			resp->ies[ies_len + 1 + count++] = r | flag;
-		}
-
-	resp->ies[ies_len++] = count;
-	ies_len += count;
+	len = ap_build_supported_rates(ap, resp->ies + ies_len + 1);
+	resp->ies[ies_len++] = len;
+	ies_len += len;
 
 	ies_len += ap_write_extra_ies(ap, stype, req, req_len,
 					resp->ies + ies_len);
-- 
2.34.3

[PATCH 02/10] nl80211util: parse additional channel restriction flags

[James Prestwood]

---
 src/band.h        |  5 +++++
 src/nl80211util.c | 15 +++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/src/band.h b/src/band.h
index da4c0ae5..e196a416 100644
--- a/src/band.h
+++ b/src/band.h
@@ -59,6 +59,11 @@ struct band_freq_attrs {
 	bool supported : 1;
 	bool disabled : 1;
 	bool no_ir : 1;
+	bool no_ht40_plus : 1;
+	bool no_ht40_minus : 1;
+	bool no_80mhz : 1;
+	bool no_160mhz : 1;
+	bool no_he : 1;
 } __attribute__ ((packed));
 
 struct band {
diff --git a/src/nl80211util.c b/src/nl80211util.c
index 712cdec3..400871fd 100644
--- a/src/nl80211util.c
+++ b/src/nl80211util.c
@@ -533,6 +533,21 @@ int nl80211_parse_supported_frequencies(struct l_genl_attr *band_freqs,
 			case NL80211_FREQUENCY_ATTR_NO_IR:
 				freq_attr.no_ir = true;
 				break;
+			case NL80211_FREQUENCY_ATTR_NO_HT40_MINUS:
+				freq_attr.no_ht40_minus = true;
+				break;
+			case NL80211_FREQUENCY_ATTR_NO_HT40_PLUS:
+				freq_attr.no_ht40_plus = true;
+				break;
+			case NL80211_FREQUENCY_ATTR_NO_80MHZ:
+				freq_attr.no_80mhz = true;
+				break;
+			case NL80211_FREQUENCY_ATTR_NO_160MHZ:
+				freq_attr.no_160mhz = true;
+				break;
+			case NL80211_FREQUENCY_ATTR_NO_HE:
+				freq_attr.no_he = true;
+				break;
 			}
 		}
 
-- 
2.34.3

[PATCH 03/10] band: add ampdu_params value

[James Prestwood]

This is the last bit of information the kernel exposes about the
hardware's HT capabilities.
---
 src/band.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/band.h b/src/band.h
index e196a416..a4652fd8 100644
--- a/src/band.h
+++ b/src/band.h
@@ -77,6 +77,7 @@ struct band {
 	bool vht_supported : 1;
 	uint8_t ht_mcs_set[16];
 	uint8_t ht_capabilities[2];
+	uint8_t ht_ampdu_params;
 	bool ht_supported : 1;
 	uint16_t supported_rates_len;
 	uint8_t supported_rates[];
-- 
2.34.3

[PATCH 04/10] wiphy: add getter for HT capabilities

[James Prestwood]

This adds some additional parsing to obtain the AMPDU parameter
byte as well as wiphy_get_ht_capabilities() which returns the
complete IE (combining the 3 separate kernel attributes).
---
 src/wiphy.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/wiphy.h |  3 +++
 2 files changed, 56 insertions(+)

diff --git a/src/wiphy.c b/src/wiphy.c
index 4ea7c3f8..ab8aa6c0 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -910,6 +910,42 @@ bool wiphy_country_is_unknown(struct wiphy *wiphy)
 			(cc[0] == 'X' && cc[1] == 'X'));
 }
 
+const uint8_t *wiphy_get_ht_capabilities(const struct wiphy *wiphy,
+						enum band_freq band,
+						size_t *size)
+{
+	static uint8_t ht_capa[26];
+	const struct band *bandp = wiphy_get_band(wiphy, band);
+
+	if (!bandp)
+		return NULL;
+
+	if (!bandp->ht_supported)
+		return NULL;
+
+	memset(ht_capa, 0, sizeof(ht_capa));
+
+	/*
+	 * The kernel segments the HT capabilities element into multiple
+	 * attributes. For convenience on the caller just combine them and
+	 * return the full IE rather than adding 3 separate getters. This also
+	 * provides a way to check if HT is supported.
+	 */
+	memcpy(ht_capa, bandp->ht_capabilities, 2);
+	ht_capa[2] = bandp->ht_ampdu_params;
+	memcpy(ht_capa + 3, bandp->ht_mcs_set, 16);
+
+	/*
+	 * TODO: HT Extended capabilities, beamforming, and ASEL capabilities
+	 * are not available to get from the kernel, leave as zero.
+	 */
+
+	if (size)
+		*size = sizeof(ht_capa);
+
+	return ht_capa;
+}
+
 int wiphy_estimate_data_rate(struct wiphy *wiphy,
 				const void *ies, uint16_t ies_len,
 				const struct scan_bss *bss,
@@ -1617,6 +1653,23 @@ static void parse_supported_bands(struct wiphy *wiphy,
 				memcpy(band->ht_capabilities, data, len);
 				band->ht_supported = true;
 				break;
+			/*
+			 * AMPDU factor/density are part of A-MPDU Parameters,
+			 * 802.11-2020 Section 9.4.2.55.3.
+			 */
+			case NL80211_BAND_ATTR_HT_AMPDU_FACTOR:
+				if (L_WARN_ON(len != 1))
+					continue;
+
+				band->ht_ampdu_params |= l_get_u8(data) & 0x3;
+				break;
+			case NL80211_BAND_ATTR_HT_AMPDU_DENSITY:
+				if (L_WARN_ON(len != 1))
+					continue;
+
+				band->ht_ampdu_params |=
+						(l_get_u8(data) & 0x7) >> 2;
+				break;
 			case NL80211_BAND_ATTR_IFTYPE_DATA:
 				if (!l_genl_attr_recurse(&attr, &nested))
 					continue;
diff --git a/src/wiphy.h b/src/wiphy.h
index 6616da61..5cf22537 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -138,6 +138,9 @@ bool wiphy_get_rsnxe(const struct wiphy *wiphy, uint8_t *buf, size_t len);
 void wiphy_get_reg_domain_country(struct wiphy *wiphy, char *out);
 bool wiphy_country_is_unknown(struct wiphy *wiphy);
 
+const uint8_t *wiphy_get_ht_capabilities(const struct wiphy *wiphy,
+						enum band_freq band,
+						size_t *size);
 void wiphy_generate_random_address(struct wiphy *wiphy, uint8_t addr[static 6]);
 void wiphy_generate_address_from_ssid(struct wiphy *wiphy, const char *ssid,
 					uint8_t addr[static 6]);
-- 
2.34.3

[PATCH 05/10] band: add APIs to generate a chandef from frequency

[James Prestwood]

For AP mode its convenient for IWD to choose an appropriate
channel definition rather than require the user provide very
low level parameters such as channel width, center1 frequency
etc. This adds two new APIs to generate a channel definition, one
for legacy and one for HT.

The legacy API is very simple and chooses the first matching
operating class using 20Mhz channel widths.

The HT API is a bit more complex since it has to take into account
40MHz and check any channel restrictions. If an operating class is
found that is supported/not restricted it is marked as 'best' until
a better one is found. In this case 'better' is a larger channel
width. Since this is HT only 20mhz and 40mhz widths are checked.
---
 src/band.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/band.h |   4 ++
 2 files changed, 163 insertions(+)

diff --git a/src/band.c b/src/band.c
index d89b2a90..b1e319d7 100644
--- a/src/band.c
+++ b/src/band.c
@@ -1194,6 +1194,165 @@ int oci_from_chandef(const struct band_chandef *own, uint8_t oci[static 3])
 	return -ENOENT;
 }
 
+static bool freq_in_oper_class(uint32_t freq,
+				const struct operating_class_info *info,
+				enum band_freq *out_band)
+{
+	uint8_t channel;
+	enum band_freq band;
+
+	channel = band_freq_to_channel(freq, &band);
+	if (!channel)
+		return false;
+
+	switch (band) {
+	case BAND_FREQ_2_4_GHZ:
+		if (info->starting_frequency > 5000)
+			return false;
+		break;
+	case BAND_FREQ_5_GHZ:
+		if (info->starting_frequency < 5000 ||
+				info->starting_frequency > 5950)
+			return false;
+		break;
+	case BAND_FREQ_6_GHZ:
+		if (info->starting_frequency < 5900)
+			return false;
+		break;
+	}
+
+	if (e4_channel_to_frequency(info, channel) < 0)
+		return false;
+
+	if (out_band)
+		*out_band = band;
+
+	return true;
+}
+
+/* Find a legacy chandef for the frequency */
+int band_freq_to_chandef(uint32_t freq, const struct band_freq_attrs *attr,
+				struct band_chandef *chandef)
+{
+	enum band_freq band;
+	uint8_t channel;
+	unsigned int i;
+	const struct operating_class_info *best = NULL;
+
+	channel = band_freq_to_channel(freq, &band);
+	if (!channel)
+		return -EINVAL;
+
+	if (attr->disabled || !attr->supported)
+		return -EINVAL;
+
+	for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) {
+		const struct operating_class_info *info =
+						&e4_operating_classes[i];
+
+		if (!freq_in_oper_class(freq, info, NULL))
+			continue;
+
+		if (info->channel_spacing != 20)
+			continue;
+
+		best = info;
+		break;
+	}
+
+	if (!best)
+		return -ENOENT;
+
+	chandef->frequency = freq;
+	chandef->channel_width = BAND_CHANDEF_WIDTH_20NOHT;
+
+	return 0;
+}
+
+/* Find an HT chandef for the frequency */
+int band_freq_to_ht_chandef(uint32_t freq, const struct band_freq_attrs *attr,
+				struct band_chandef *chandef)
+{
+	enum band_freq band;
+	enum band_chandef_width width;
+	unsigned int i;
+	const struct operating_class_info *best = NULL;
+
+	if (attr->disabled || !attr->supported)
+		return -EINVAL;
+
+	for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) {
+		const struct operating_class_info *info =
+						&e4_operating_classes[i];
+		enum band_chandef_width w;
+
+		if (!freq_in_oper_class(freq, info, &band))
+			continue;
+
+		/* Any restrictions for this channel width? */
+		switch (info->channel_spacing) {
+		case 20:
+			w = BAND_CHANDEF_WIDTH_20;
+			break;
+		case 40:
+			w = BAND_CHANDEF_WIDTH_40;
+
+			/* 6GHz remove the upper/lower 40mhz channel concept */
+			if (band == BAND_FREQ_6_GHZ)
+				break;
+
+			if (info->flags & PRIMARY_CHANNEL_UPPER &&
+						attr->no_ht40_plus)
+				continue;
+
+			if (info->flags & PRIMARY_CHANNEL_LOWER &&
+						attr->no_ht40_minus)
+				continue;
+
+			break;
+		default:
+			continue;
+		}
+
+		if (!best || best->channel_spacing < info->channel_spacing) {
+			best = info;
+			width = w;
+		}
+	}
+
+	if (!best)
+		return -ENOENT;
+
+	chandef->frequency = freq;
+	chandef->channel_width = width;
+
+	/*
+	 * Choose a secondary channel frequency:
+	 * - 20mhz no secondary
+	 * - 40mhz we can base the selection off the channel flags, either
+	 *   higher or lower.
+	 */
+	switch (width) {
+	case BAND_CHANDEF_WIDTH_20:
+		return 0;
+	case BAND_CHANDEF_WIDTH_40:
+		if (band == BAND_FREQ_6_GHZ)
+			return 0;
+
+		if (best->flags & PRIMARY_CHANNEL_UPPER)
+			chandef->center1_frequency = freq - 10;
+		else
+			chandef->center1_frequency = freq + 10;
+
+		return 0;
+	default:
+		/* Should never happen */
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 uint8_t band_freq_to_channel(uint32_t freq, enum band_freq *out_band)
 {
 	uint32_t channel = 0;
diff --git a/src/band.h b/src/band.h
index a4652fd8..f7f0c318 100644
--- a/src/band.h
+++ b/src/band.h
@@ -101,6 +101,10 @@ int band_estimate_nonht_rate(const struct band *band,
 				const uint8_t *supported_rates,
 				const uint8_t *ext_supported_rates,
 				int32_t rssi, uint64_t *out_data_rate);
+int band_freq_to_chandef(uint32_t freq, const struct band_freq_attrs *attr,
+				struct band_chandef *chandef);
+int band_freq_to_ht_chandef(uint32_t freq, const struct band_freq_attrs *attr,
+				struct band_chandef *chandef);
 
 int oci_to_frequency(uint32_t operating_class, uint32_t channel);
 
-- 
2.34.3

[PATCH 06/10] band: add band_chandef_width_to_string

[James Prestwood]

---
 src/band.c | 20 ++++++++++++++++++++
 src/band.h |  1 +
 2 files changed, 21 insertions(+)

diff --git a/src/band.c b/src/band.c
index b1e319d7..73a63c79 100644
--- a/src/band.c
+++ b/src/band.c
@@ -1581,3 +1581,23 @@ enum band_freq band_oper_class_to_band(const uint8_t *country,
 	else
 		return 0;
 }
+
+const char *band_chandef_width_to_string(enum band_chandef_width width)
+{
+	switch (width) {
+	case BAND_CHANDEF_WIDTH_20NOHT:
+		return "20MHz (no-HT)";
+	case BAND_CHANDEF_WIDTH_20:
+		return "20MHz";
+	case BAND_CHANDEF_WIDTH_40:
+		return "40MHz";
+	case BAND_CHANDEF_WIDTH_80:
+		return "80MHz";
+	case BAND_CHANDEF_WIDTH_80P80:
+		return "80+80MHz";
+	case BAND_CHANDEF_WIDTH_160:
+		return "160MHz";
+	}
+
+	return NULL;
+}
diff --git a/src/band.h b/src/band.h
index f7f0c318..1b3f1669 100644
--- a/src/band.h
+++ b/src/band.h
@@ -115,3 +115,4 @@ uint8_t band_freq_to_channel(uint32_t freq, enum band_freq *out_band);
 uint32_t band_channel_to_freq(uint8_t channel, enum band_freq band);
 enum band_freq band_oper_class_to_band(const uint8_t *country,
 					uint8_t oper_class);
+const char *band_chandef_width_to_string(enum band_chandef_width width);
-- 
2.34.3

[PATCH 07/10] wiphy: add wiphy_supports_uapsd

[James Prestwood]

---
 src/wiphy.c | 9 +++++++++
 src/wiphy.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/src/wiphy.c b/src/wiphy.c
index ab8aa6c0..021b5e38 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -137,6 +137,7 @@ struct wiphy {
 	bool registered : 1;
 	bool self_managed : 1;
 	bool ap_probe_resp_offload : 1;
+	bool supports_uapsd : 1;
 };
 
 static struct l_queue *wiphy_list = NULL;
@@ -910,6 +911,11 @@ bool wiphy_country_is_unknown(struct wiphy *wiphy)
 			(cc[0] == 'X' && cc[1] == 'X'));
 }
 
+bool wiphy_supports_uapsd(const struct wiphy *wiphy)
+{
+	return wiphy->supports_uapsd;
+}
+
 const uint8_t *wiphy_get_ht_capabilities(const struct wiphy *wiphy,
 						enum band_freq band,
 						size_t *size)
@@ -1830,6 +1836,9 @@ static void wiphy_parse_attributes(struct wiphy *wiphy,
 		case NL80211_ATTR_PROBE_RESP_OFFLOAD:
 			wiphy->ap_probe_resp_offload = true;
 			break;
+		case NL80211_ATTR_SUPPORT_AP_UAPSD:
+			wiphy->supports_uapsd = true;
+			break;
 		}
 	}
 }
diff --git a/src/wiphy.h b/src/wiphy.h
index 5cf22537..1056ac0c 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -137,6 +137,7 @@ const uint8_t *wiphy_get_rm_enabled_capabilities(struct wiphy *wiphy);
 bool wiphy_get_rsnxe(const struct wiphy *wiphy, uint8_t *buf, size_t len);
 void wiphy_get_reg_domain_country(struct wiphy *wiphy, char *out);
 bool wiphy_country_is_unknown(struct wiphy *wiphy);
+bool wiphy_supports_uapsd(const struct wiphy *wiphy);
 
 const uint8_t *wiphy_get_ht_capabilities(const struct wiphy *wiphy,
 						enum band_freq band,
-- 
2.34.3

[PATCH 08/10] ap: include WMM parameter IE

[James Prestwood]

The WMM parameter IE is expected by the linux kernel for any AP
supporting HT/VHT etc. IWD won't actually use WMM and its not
clear exactly why the kernel uses this restriction, but regardless
it must be included to support HT.
---
 src/ap.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/src/ap.c b/src/ap.c
index 595c71c9..d3188d1d 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -111,6 +111,7 @@ struct ap_state {
 	bool in_event : 1;
 	bool free_pending : 1;
 	bool scanning : 1;
+	bool supports_ht : 1;
 };
 
 struct sta_state {
@@ -834,6 +835,9 @@ static size_t ap_get_extra_ies_len(struct ap_state *ap,
 
 	len += ap_get_wsc_ie_len(ap, type, client_frame, client_frame_len);
 
+	if (ap->supports_ht)
+		len += 26;
+
 	if (ap->ops->get_extra_ies_len)
 		len += ap->ops->get_extra_ies_len(type, client_frame,
 							client_frame_len,
@@ -842,6 +846,56 @@ static size_t ap_get_extra_ies_len(struct ap_state *ap,
 	return len;
 }
 
+/* WMM Specification 2.2.2 WMM Parameter Element */
+struct ap_wmm_ac_record {
+	uint8_t aifsn : 4;
+	uint8_t acm : 1;
+	uint8_t aci : 2;
+	uint8_t reserved : 1;
+	uint8_t ecw_min : 4;
+	uint8_t ecw_max : 4;
+	__le16 txop_limit;
+} __attribute__((packed));
+
+static size_t ap_write_wmm_ies(struct ap_state *ap, uint8_t *out_buf)
+{
+	unsigned int i;
+	struct wiphy *wiphy = netdev_get_wiphy(ap->netdev);
+
+	/*
+	 * Linux kernel requires APs include WMM Information element if
+	 * supporting HT/VHT/etc.
+	 *
+	 * The only value we can actually get from the kernel is UAPSD. The
+	 * remaining values (AC parameter records) are made up or defaults
+	 * defined in the WMM spec are used.
+	 */
+	*out_buf++ = IE_TYPE_VENDOR_SPECIFIC;
+	*out_buf++ = 24;
+	memcpy(out_buf, microsoft_oui, sizeof(microsoft_oui));
+	out_buf += sizeof(microsoft_oui);
+	*out_buf++ = 2; /* WMM OUI Type */
+	*out_buf++ = 1; /* WMM Parameter subtype */
+	*out_buf++ = 1; /* WMM Version */
+	*out_buf++ = wiphy_supports_uapsd(wiphy) ? 1 << 7 : 0;
+	*out_buf++ = 0; /* reserved */
+
+	for (i = 0; i < 4; i++) {
+		struct ap_wmm_ac_record ac = { 0 };
+
+		ac.aifsn = 2;
+		ac.acm = 0;
+		ac.aci = i;
+		ac.ecw_min = 1;
+		ac.ecw_max = 15;
+		l_put_le16(0, &ac.txop_limit);
+
+		memcpy(out_buf + (i * 4), &ac, sizeof(struct ap_wmm_ac_record));
+	}
+
+	return 26;
+}
+
 static size_t ap_write_extra_ies(struct ap_state *ap,
 					enum mpdu_management_subtype type,
 					const struct mmpdu_header *client_frame,
@@ -853,6 +907,9 @@ static size_t ap_write_extra_ies(struct ap_state *ap,
 	len += ap_write_wsc_ie(ap, type, client_frame, client_frame_len,
 				out_buf + len);
 
+	if (ap->supports_ht)
+		len += ap_write_wmm_ies(ap, out_buf + len);
+
 	if (ap->ops->write_extra_ies)
 		len += ap->ops->write_extra_ies(type,
 						client_frame, client_frame_len,
@@ -3255,6 +3312,9 @@ static int ap_load_config(struct ap_state *ap, const struct l_settings *config,
 		ap->band = BAND_FREQ_2_4_GHZ;
 	}
 
+	ap->supports_ht = wiphy_get_ht_capabilities(wiphy, ap->band,
+							NULL) != NULL;
+
 	if (!ap_validate_band_channel(ap)) {
 		l_error("AP Band and Channel combination invalid");
 		return -EINVAL;
-- 
2.34.3

[PATCH 09/10] ap: build HT Capabilities/Operation elements

[James Prestwood]

If supported this will include the HT capabilities and HT
operations elements in beacons/probes. Some shortcuts were taken
here since not all the information is currently parsed from the
hardware. Namely the HT operation element does not include the
basic MCS set. Still, this will at least show stations that the
AP is capable of more than just basic rates.

The builders themselves are structured similar to the basic rates
builder where they build only the contents and return the length.
The caller must set the type/length manually. This is to support
the two use cases of using with an IE builder vs direct pointer.
---
 src/ap.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/src/ap.c b/src/ap.c
index d3188d1d..58fb4a6b 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -136,6 +136,9 @@ struct sta_state {
 	bool wsc_v2;
 	struct l_dhcp_lease *ip_alloc_lease;
 	bool ip_alloc_sent;
+
+	bool ht_support : 1;
+	bool ht_greenfield : 1;
 };
 
 struct ap_wsc_pbc_probe_record {
@@ -918,6 +921,66 @@ static size_t ap_write_extra_ies(struct ap_state *ap,
 	return len;
 }
 
+static size_t ap_build_ht_capability(struct ap_state *ap, uint8_t *buf)
+{
+	struct wiphy *wiphy = netdev_get_wiphy(ap->netdev);
+	size_t ht_capa_len;
+	const uint8_t *ht_capa = wiphy_get_ht_capabilities(wiphy, ap->band,
+								&ht_capa_len);
+
+	memcpy(buf, ht_capa, ht_capa_len);
+
+	return ht_capa_len;
+}
+
+static size_t ap_build_ht_operation(struct ap_state *ap, uint8_t *buf)
+{
+	const struct l_queue_entry *e;
+	unsigned int non_ht = false;
+	unsigned int non_greenfield = false;
+
+	memset(buf, 0, 22);
+	*buf++ = ap->channel;
+
+	/*
+	 * If 40MHz set 'Secondary Channel Offset' (bits 0-1) to above/below
+	 * and set 'STA Channel Width' (bit 2) to indicate non-20Mhz.
+	 */
+	if (ap->chandef.channel_width == BAND_CHANDEF_WIDTH_20)
+		goto check_stas;
+	else if (ap->chandef.frequency < ap->chandef.center1_frequency)
+		*buf |= 1 & 0x3;
+	else
+		*buf |= 3 & 0x3;
+
+	*buf |= 1 << 2;
+
+check_stas:
+	for (e = l_queue_get_entries(ap->sta_states); e; e = e->next) {
+		struct sta_state *sta = e->data;
+
+		if (!sta->associated)
+			continue;
+
+		if (!sta->ht_support)
+			non_ht = true;
+		else if (!sta->ht_greenfield)
+			non_greenfield = true;
+	}
+
+	if (non_greenfield)
+		set_bit(buf, 10);
+
+	if (non_ht)
+		set_bit(buf, 12);
+
+	/*
+	 * TODO: Basic MCS set for all associated STAs
+	 */
+
+	return 22;
+}
+
 /*
  * Build a Beacon frame or a Probe Response frame's header and body until
  * the TIM IE.  Except for the optional TIM IE which is inserted by the
@@ -970,6 +1033,18 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
 	ie_tlv_builder_next(&builder, IE_TYPE_DSSS_PARAMETER_SET);
 	ie_tlv_builder_set_data(&builder, &ap->channel, 1);
 
+	if (ap->supports_ht) {
+		ie_tlv_builder_next(&builder, IE_TYPE_HT_CAPABILITIES);
+		len = ap_build_ht_capability(ap,
+					ie_tlv_builder_get_data(&builder));
+		ie_tlv_builder_set_length(&builder, len);
+
+		ie_tlv_builder_next(&builder, IE_TYPE_HT_OPERATION);
+		len = ap_build_ht_operation(ap,
+					ie_tlv_builder_get_data(&builder));
+		ie_tlv_builder_set_length(&builder, len);
+	}
+
 	ie_tlv_builder_finalize(&builder, &out_len);
 	return 36 + out_len;
 }
@@ -1628,6 +1703,18 @@ static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
 	resp->ies[ies_len++] = len;
 	ies_len += len;
 
+	if (ap->supports_ht) {
+		resp->ies[ies_len++] = IE_TYPE_HT_CAPABILITIES;
+		len = ap_build_ht_capability(ap, resp->ies + ies_len + 1);
+		resp->ies[ies_len++] = len;
+		ies_len += len;
+
+		resp->ies[ies_len++] = IE_TYPE_HT_OPERATION;
+		len = ap_build_ht_operation(ap, resp->ies + ies_len + 1);
+		resp->ies[ies_len++] = len;
+		ies_len += len;
+	}
+
 	ies_len += ap_write_extra_ies(ap, stype, req, req_len,
 					resp->ies + ies_len);
 
@@ -1833,6 +1920,17 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 
 			fils_ip_req = true;
 			break;
+		case IE_TYPE_HT_CAPABILITIES:
+			if (ie_tlv_iter_get_length(&iter) != 26) {
+				err = MMPDU_REASON_CODE_INVALID_IE;
+				goto bad_frame;
+			}
+
+			if (test_bit(ie_tlv_iter_get_data(&iter), 4))
+				sta->ht_greenfield = true;
+
+			sta->ht_support = true;
+			break;
 		}
 
 	if (!rates || !ssid || (!wsc_data && !rsn) ||
@@ -2676,6 +2774,9 @@ static void ap_handle_new_station(struct ap_state *ap, struct l_genl_msg *msg)
 
 	l_queue_push_tail(ap->sta_states, sta);
 
+	if (ap->supports_ht)
+		ap_update_beacon(ap);
+
 	msg = nl80211_build_set_station_unauthorized(
 					netdev_get_ifindex(ap->netdev), mac);
 
@@ -3712,6 +3813,9 @@ bool ap_station_disconnect(struct ap_state *ap, const uint8_t *mac,
 	if (!sta)
 		return false;
 
+	if (ap->supports_ht)
+		ap_update_beacon(ap);
+
 	ap_del_station(sta, reason, false);
 	ap_sta_free(sta);
 	return true;
-- 
2.34.3

[PATCH 10/10] ap: generate chandef for starting AP

[James Prestwood]

To include HT support a chandef needs to be created for whatever
frequency is being used. This allows IWD to provide a secondary
channel to the kernel in the case of 40MHz operation. Now the AP
will generate a chandef when starting based on the channel set
in the user profile (or default).
---
 src/ap.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 58fb4a6b..2d859538 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -72,6 +72,7 @@ struct ap_state {
 	uint8_t psk[32];
 	enum band_freq band;
 	uint8_t channel;
+	struct band_chandef chandef;
 	uint8_t *authorized_macs;
 	unsigned int authorized_macs_num;
 	char wsc_name[33];
@@ -2556,8 +2557,6 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 	uint32_t nl_akm = CRYPTO_AKM_PSK;
 	uint32_t wpa_version = NL80211_WPA_VERSION_2;
 	uint32_t auth_type = NL80211_AUTHTYPE_OPEN_SYSTEM;
-	uint32_t ch_freq = band_channel_to_freq(ap->channel, ap->band);
-	uint32_t ch_width = NL80211_CHAN_WIDTH_20;
 	unsigned int i;
 
 	static const uint8_t bcast_addr[6] = {
@@ -2605,8 +2604,13 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_WPA_VERSIONS, 4, &wpa_version);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_AKM_SUITES, 4, &nl_akm);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_AUTH_TYPE, 4, &auth_type);
-	l_genl_msg_append_attr(cmd, NL80211_ATTR_WIPHY_FREQ, 4, &ch_freq);
-	l_genl_msg_append_attr(cmd, NL80211_ATTR_CHANNEL_WIDTH, 4, &ch_width);
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_WIPHY_FREQ, 4,
+				&ap->chandef.frequency);
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_CHANNEL_WIDTH, 4,
+				&ap->chandef.channel_width);
+	if (ap->chandef.center1_frequency)
+		l_genl_msg_append_attr(cmd, NL80211_ATTR_CENTER_FREQ1, 4,
+					&ap->chandef.center1_frequency);
 
 	if (wiphy_supports_probe_resp_offload(wiphy)) {
 		uint8_t probe_resp[head_len + tail_len];
@@ -3326,6 +3330,7 @@ static bool ap_validate_band_channel(struct ap_state *ap)
 	struct wiphy *wiphy = netdev_get_wiphy(ap->netdev);
 	uint32_t freq;
 	const struct band_freq_attrs *attr;
+	int ret;
 
 	if (!(wiphy_get_supported_bands(wiphy) & ap->band)) {
 		l_error("AP hardware does not support band");
@@ -3345,6 +3350,22 @@ static bool ap_validate_band_channel(struct ap_state *ap)
 		l_error("AP frequency %u disabled or unsupported", freq);
 		return false;
 	}
+
+	if (!ap->supports_ht)
+		ret = band_freq_to_chandef(freq, attr, &ap->chandef);
+	else
+		ret = band_freq_to_ht_chandef(freq, attr, &ap->chandef);
+
+	if (ret < 0) {
+		l_error("AP unable to find chandef for freq %u", freq);
+		return false;
+	}
+
+	l_debug("AP using frequency %u and channel width %s",
+			ap->chandef.frequency,
+			band_chandef_width_to_string(
+				ap->chandef.channel_width));
+
 	return true;
 }
 
-- 
2.34.3

Re: [PATCH 01/10] ap: make supported rates a common builder.

[Denis Kenzior]

Hi James,

On 12/20/22 15:43, James Prestwood wrote:
> The supported rates IE was being built in two places. This makes that
> code common. Unfortunately it needs to support both an ie builder
> and using a pointer directly which is why it only builds the contents
> of the IE and the caller must set the type/length.
> ---
>   src/ap.c | 68 +++++++++++++++++++++++++-------------------------------
>   1 file changed, 30 insertions(+), 38 deletions(-)
> 

Patch 1-3 applied, thanks.

Regards,
-Denis

Re: [PATCH 04/10] wiphy: add getter for HT capabilities

[Denis Kenzior]

Hi James,

On 12/20/22 15:43, James Prestwood wrote:
> This adds some additional parsing to obtain the AMPDU parameter
> byte as well as wiphy_get_ht_capabilities() which returns the
> complete IE (combining the 3 separate kernel attributes).
> ---
>   src/wiphy.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/wiphy.h |  3 +++
>   2 files changed, 56 insertions(+)
> 

<snip>

> @@ -1617,6 +1653,23 @@ static void parse_supported_bands(struct wiphy *wiphy,
>   				memcpy(band->ht_capabilities, data, len);
>   				band->ht_supported = true;
>   				break;
> +			/*
> +			 * AMPDU factor/density are part of A-MPDU Parameters,
> +			 * 802.11-2020 Section 9.4.2.55.3.
> +			 */
> +			case NL80211_BAND_ATTR_HT_AMPDU_FACTOR:
> +				if (L_WARN_ON(len != 1))
> +					continue;
> +
> +				band->ht_ampdu_params |= l_get_u8(data) & 0x3;
> +				break;
> +			case NL80211_BAND_ATTR_HT_AMPDU_DENSITY:
> +				if (L_WARN_ON(len != 1))
> +					continue;
> +
> +				band->ht_ampdu_params |=
> +						(l_get_u8(data) & 0x7) >> 2;

So I think you meant to shift left here, i.e. ' << 2'.  I made amended this 
patch to reflect that.  Let me know if I'm incorrect about this.

> +				break;
>   			case NL80211_BAND_ATTR_IFTYPE_DATA:
>   				if (!l_genl_attr_recurse(&attr, &nested))
>   					continue;

Applied, thanks.

Regards,
-Denis

Re: [PATCH 06/10] band: add band_chandef_width_to_string

[Denis Kenzior]

Hi James,

On 12/20/22 15:43, James Prestwood wrote:
> ---
>   src/band.c | 20 ++++++++++++++++++++
>   src/band.h |  1 +
>   2 files changed, 21 insertions(+)
> 

Patch 6 & 7 applied, thanks.

Regards,
-Denis

Re: [PATCH 05/10] band: add APIs to generate a chandef from frequency

[Denis Kenzior]

Hi James,

On 12/20/22 15:43, James Prestwood wrote:
> For AP mode its convenient for IWD to choose an appropriate
> channel definition rather than require the user provide very
> low level parameters such as channel width, center1 frequency
> etc. This adds two new APIs to generate a channel definition, one
> for legacy and one for HT.
> 
> The legacy API is very simple and chooses the first matching
> operating class using 20Mhz channel widths.
> 
> The HT API is a bit more complex since it has to take into account
> 40MHz and check any channel restrictions. If an operating class is
> found that is supported/not restricted it is marked as 'best' until
> a better one is found. In this case 'better' is a larger channel
> width. Since this is HT only 20mhz and 40mhz widths are checked.

I wonder if the HT one can be implemented using the noHT one?

> ---
>   src/band.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/band.h |   4 ++
>   2 files changed, 163 insertions(+)
> 
> diff --git a/src/band.c b/src/band.c
> index d89b2a90..b1e319d7 100644
> --- a/src/band.c
> +++ b/src/band.c
> @@ -1194,6 +1194,165 @@ int oci_from_chandef(const struct band_chandef *own, uint8_t oci[static 3])
>   	return -ENOENT;
>   }
>   
> +static bool freq_in_oper_class(uint32_t freq,
> +				const struct operating_class_info *info,
> +				enum band_freq *out_band)
> +{
> +	uint8_t channel;
> +	enum band_freq band;
> +
> +	channel = band_freq_to_channel(freq, &band);
> +	if (!channel)
> +		return false;

You already call band_freq_to_channel right at the top of band_freq_to_chandef 
(but not band_freq_to_ht_chandef?).  Not really a big deal, but why continue 
calling it inside this function which is being called in a loop?  You already 
know the channel and the band...

> +
> +	switch (band) {
> +	case BAND_FREQ_2_4_GHZ:
> +		if (info->starting_frequency > 5000)
> +			return false;
> +		break;
> +	case BAND_FREQ_5_GHZ:
> +		if (info->starting_frequency < 5000 ||
> +				info->starting_frequency > 5950)
> +			return false;
> +		break;
> +	case BAND_FREQ_6_GHZ:
> +		if (info->starting_frequency < 5900)
> +			return false;
> +		break;
> +	}

So you don't trust band_freq_to_channel?

Also, I'd really like to isolate 'magic frequency numbers' use as much as 
possible.  Ideally contain this in band_freq_to_channel and band_channel_to_freq 
only.  And even then maybe we should look into whether we can rework 
band_channel_to_freq to use the e4 table.

If there's a chance that band_freq_to_channel can produce a channel/band 
combination that isn't in e4, then perhaps we should simply add a 'band' member 
to 'struct operating_class_info'.  That way this could be sanity checked directly.

> +
> +	if (e4_channel_to_frequency(info, channel) < 0)
> +		return false;
> +
> +	if (out_band)
> +		*out_band = band;
> +
> +	return true;
> +}
> +
> +/* Find a legacy chandef for the frequency */
> +int band_freq_to_chandef(uint32_t freq, const struct band_freq_attrs *attr,
> +				struct band_chandef *chandef)
> +{
> +	enum band_freq band;
> +	uint8_t channel;
> +	unsigned int i;
> +	const struct operating_class_info *best = NULL;
> +
> +	channel = band_freq_to_channel(freq, &band);
> +	if (!channel)
> +		return -EINVAL;
> +
> +	if (attr->disabled || !attr->supported)
> +		return -EINVAL;
> +
> +	for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) {
> +		const struct operating_class_info *info =
> +						&e4_operating_classes[i];
> +
> +		if (!freq_in_oper_class(freq, info, NULL))
> +			continue;
> +
> +		if (info->channel_spacing != 20)
> +			continue;

nit, but you might want to check this first.

> +
> +		best = info;
> +		break;
> +	}
> +
> +	if (!best)
> +		return -ENOENT;
> +
> +	chandef->frequency = freq;
> +	chandef->channel_width = BAND_CHANDEF_WIDTH_20NOHT;
> +
> +	return 0;
> +}
> +
> +/* Find an HT chandef for the frequency */
> +int band_freq_to_ht_chandef(uint32_t freq, const struct band_freq_attrs *attr,
> +				struct band_chandef *chandef)
> +{
> +	enum band_freq band;
> +	enum band_chandef_width width;
> +	unsigned int i;
> +	const struct operating_class_info *best = NULL;
> +
> +	if (attr->disabled || !attr->supported)
> +		return -EINVAL;
> +
> +	for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) {
> +		const struct operating_class_info *info =
> +						&e4_operating_classes[i];
> +		enum band_chandef_width w;
> +
> +		if (!freq_in_oper_class(freq, info, &band))
> +			continue;
> +
> +		/* Any restrictions for this channel width? */
> +		switch (info->channel_spacing) {
> +		case 20:
> +			w = BAND_CHANDEF_WIDTH_20;
> +			break;
> +		case 40:
> +			w = BAND_CHANDEF_WIDTH_40;
> +
> +			/* 6GHz remove the upper/lower 40mhz channel concept */
> +			if (band == BAND_FREQ_6_GHZ)
> +				break;
> +
> +			if (info->flags & PRIMARY_CHANNEL_UPPER &&
> +						attr->no_ht40_plus)
> +				continue;
> +
> +			if (info->flags & PRIMARY_CHANNEL_LOWER &&
> +						attr->no_ht40_minus)
> +				continue;
> +
> +			break;
> +		default:
> +			continue;
> +		}
> +
> +		if (!best || best->channel_spacing < info->channel_spacing) {
> +			best = info;
> +			width = w;
> +		}
> +	}
> + > +	if (!best)
> +		return -ENOENT;

So why not limit the above loop to 40 mhz channel spacing, and if nothing is 
found, do something like

if (!best)
	return band_freq_to_chandef();

Or alternatively, pass in a parameter for maximum desired width.

> +
> +	chandef->frequency = freq;
> +	chandef->channel_width = width;
> +
> +	/*
> +	 * Choose a secondary channel frequency:
> +	 * - 20mhz no secondary
> +	 * - 40mhz we can base the selection off the channel flags, either
> +	 *   higher or lower.
> +	 */
> +	switch (width) {
> +	case BAND_CHANDEF_WIDTH_20:
> +		return 0;
> +	case BAND_CHANDEF_WIDTH_40:
> +		if (band == BAND_FREQ_6_GHZ)
> +			return 0;
> +
> +		if (best->flags & PRIMARY_CHANNEL_UPPER)
> +			chandef->center1_frequency = freq - 10;
> +		else
> +			chandef->center1_frequency = freq + 10;
> +
> +		return 0;
> +	default:
> +		/* Should never happen */
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   uint8_t band_freq_to_channel(uint32_t freq, enum band_freq *out_band)
>   {
>   	uint32_t channel = 0;

Regards,
-Denis

Re: [PATCH 08/10] ap: include WMM parameter IE

[Denis Kenzior]

Hi James,

On 12/20/22 15:43, James Prestwood wrote:
> The WMM parameter IE is expected by the linux kernel for any AP
> supporting HT/VHT etc. IWD won't actually use WMM and its not
> clear exactly why the kernel uses this restriction, but regardless
> it must be included to support HT.
> ---
>   src/ap.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 60 insertions(+)
> 

<snip>

> @@ -842,6 +846,56 @@ static size_t ap_get_extra_ies_len(struct ap_state *ap,
>   	return len;
>   }
>   
> +/* WMM Specification 2.2.2 WMM Parameter Element */
> +struct ap_wmm_ac_record {
> +	uint8_t aifsn : 4;
> +	uint8_t acm : 1;
> +	uint8_t aci : 2;
> +	uint8_t reserved : 1;
> +	uint8_t ecw_min : 4;
> +	uint8_t ecw_max : 4;

Since this bitfield is multiple bytes, you have to be careful about endianness. 
  Compiler will store these bits differently depending on native endian order. 
See src/mpdu.h (mmpdu_field_capability definition) for an example of how we have 
solved this in the past.

> +	__le16 txop_limit;
> +} __attribute__((packed));
> +

Regards,
-Denis

Re: [PATCH 05/10] band: add APIs to generate a chandef from frequency

[James Prestwood]

On Tue, 2022-12-27 at 12:07 -0600, Denis Kenzior wrote:
> Hi James,
> 
> On 12/20/22 15:43, James Prestwood wrote:
> > For AP mode its convenient for IWD to choose an appropriate
> > channel definition rather than require the user provide very
> > low level parameters such as channel width, center1 frequency
> > etc. This adds two new APIs to generate a channel definition, one
> > for legacy and one for HT.
> > 
> > The legacy API is very simple and chooses the first matching
> > operating class using 20Mhz channel widths.
> > 
> > The HT API is a bit more complex since it has to take into account
> > 40MHz and check any channel restrictions. If an operating class is
> > found that is supported/not restricted it is marked as 'best' until
> > a better one is found. In this case 'better' is a larger channel
> > width. Since this is HT only 20mhz and 40mhz widths are checked.
> 
> I wonder if the HT one can be implemented using the noHT one?
> 
> > ---
> >   src/band.c | 159
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   src/band.h |   4 ++
> >   2 files changed, 163 insertions(+)
> > 
> > diff --git a/src/band.c b/src/band.c
> > index d89b2a90..b1e319d7 100644
> > --- a/src/band.c
> > +++ b/src/band.c
> > @@ -1194,6 +1194,165 @@ int oci_from_chandef(const struct
> > band_chandef *own, uint8_t oci[static 3])
> >         return -ENOENT;
> >   }
> >   
> > +static bool freq_in_oper_class(uint32_t freq,
> > +                               const struct operating_class_info
> > *info,
> > +                               enum band_freq *out_band)
> > +{
> > +       uint8_t channel;
> > +       enum band_freq band;
> > +
> > +       channel = band_freq_to_channel(freq, &band);
> > +       if (!channel)
> > +               return false;
> 
> You already call band_freq_to_channel right at the top of
> band_freq_to_chandef 
> (but not band_freq_to_ht_chandef?).  Not really a big deal, but why
> continue 
> calling it inside this function which is being called in a loop?  You
> already 
> know the channel and the band...
> 
> > +
> > +       switch (band) {
> > +       case BAND_FREQ_2_4_GHZ:
> > +               if (info->starting_frequency > 5000)
> > +                       return false;
> > +               break;
> > +       case BAND_FREQ_5_GHZ:
> > +               if (info->starting_frequency < 5000 ||
> > +                               info->starting_frequency > 5950)
> > +                       return false;
> > +               break;
> > +       case BAND_FREQ_6_GHZ:
> > +               if (info->starting_frequency < 5900)
> > +                       return false;
> > +               break;
> > +       }
> 
> So you don't trust band_freq_to_channel?
> 
> Also, I'd really like to isolate 'magic frequency numbers' use as
> much as 
> possible.  Ideally contain this in band_freq_to_channel and
> band_channel_to_freq 
> only.  And even then maybe we should look into whether we can rework 
> band_channel_to_freq to use the e4 table.

Yeah this was dumb, I removed this completely and just use
e4_has_frequency() in the loop for both functions.

> 
> If there's a chance that band_freq_to_channel can produce a
> channel/band 
> combination that isn't in e4, then perhaps we should simply add a
> 'band' member 
> to 'struct operating_class_info'.  That way this could be sanity
> checked directly.
> 
> > +
> > +       if (e4_channel_to_frequency(info, channel) < 0)
> > +               return false;
> > +
> > +       if (out_band)
> > +               *out_band = band;
> > +
> > +       return true;
> > +}
> > +
> > +/* Find a legacy chandef for the frequency */
> > +int band_freq_to_chandef(uint32_t freq, const struct
> > band_freq_attrs *attr,
> > +                               struct band_chandef *chandef)
> > +{
> > +       enum band_freq band;
> > +       uint8_t channel;
> > +       unsigned int i;
> > +       const struct operating_class_info *best = NULL;
> > +
> > +       channel = band_freq_to_channel(freq, &band);
> > +       if (!channel)
> > +               return -EINVAL;
> > +
> > +       if (attr->disabled || !attr->supported)
> > +               return -EINVAL;
> > +
> > +       for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) {
> > +               const struct operating_class_info *info =
> > +                                               &e4_operating_class
> > es[i];
> > +
> > +               if (!freq_in_oper_class(freq, info, NULL))
> > +                       continue;
> > +
> > +               if (info->channel_spacing != 20)
> > +                       continue;
> 
> nit, but you might want to check this first.

Looking at this function again after a break I don't even think we need
it. Since the only width for non-HT is 20MHz and we just set the
frequency...

The caller could just validate the frequency (which it already does)
and set into the chandef if HT is not supported.

> 
> > +
> > +               best = info;
> > +               break;
> > +       }
> > +
> > +       if (!best)
> > +               return -ENOENT;
> > +
> > +       chandef->frequency = freq;
> > +       chandef->channel_width = BAND_CHANDEF_WIDTH_20NOHT;
> > +
> > +       return 0;
> > +}
> > +
> > +/* Find an HT chandef for the frequency */
> > +int band_freq_to_ht_chandef(uint32_t freq, const struct
> > band_freq_attrs *attr,
> > +                               struct band_chandef *chandef)
> > +{
> > +       enum band_freq band;
> > +       enum band_chandef_width width;
> > +       unsigned int i;
> > +       const struct operating_class_info *best = NULL;
> > +
> > +       if (attr->disabled || !attr->supported)
> > +               return -EINVAL;
> > +
> > +       for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) {
> > +               const struct operating_class_info *info =
> > +                                               &e4_operating_class
> > es[i];
> > +               enum band_chandef_width w;
> > +
> > +               if (!freq_in_oper_class(freq, info, &band))
> > +                       continue;
> > +
> > +               /* Any restrictions for this channel width? */
> > +               switch (info->channel_spacing) {
> > +               case 20:
> > +                       w = BAND_CHANDEF_WIDTH_20;
> > +                       break;
> > +               case 40:
> > +                       w = BAND_CHANDEF_WIDTH_40;
> > +
> > +                       /* 6GHz remove the upper/lower 40mhz
> > channel concept */
> > +                       if (band == BAND_FREQ_6_GHZ)
> > +                               break;
> > +
> > +                       if (info->flags & PRIMARY_CHANNEL_UPPER &&
> > +                                               attr->no_ht40_plus)
> > +                               continue;
> > +
> > +                       if (info->flags & PRIMARY_CHANNEL_LOWER &&
> > +                                               attr-
> > >no_ht40_minus)
> > +                               continue;
> > +
> > +                       break;
> > +               default:
> > +                       continue;
> > +               }
> > +
> > +               if (!best || best->channel_spacing < info-
> > >channel_spacing) {
> > +                       best = info;
> > +                       width = w;
> > +               }
> > +       }
> > + > +   if (!best)
> > +               return -ENOENT;
> 
> So why not limit the above loop to 40 mhz channel spacing, and if
> nothing is 
> found, do something like

We still need to support 20Mhz since there is this distinction between
20MHz HT and 20Mhz no-HT. If only 20MHz HT is supported we should pick
that rather than the non-HT width.

> 
> if (!best)
>         return band_freq_to_chandef();

So ap.c just tries to pick the best chandef so in theory we could fall
back to non-HT and this wouldn't be totally unexpected (from the users
perspective). The problem is we would then need to disable HT in ap.c
if we fell back to non-HT.

Since I may just remove band_freq_to_chandef() entirely I might instead
keep the -ENOENT return and fall back inside ap.c itself. Then I don't
have to check chandef->channel_width after a successful return and we
know this will always return an HT chandef, or fail.

> 
> Or alternatively, pass in a parameter for maximum desired width.
> 
> > +
> > +       chandef->frequency = freq;
> > +       chandef->channel_width = width;
> > +
> > +       /*
> > +        * Choose a secondary channel frequency:
> > +        * - 20mhz no secondary
> > +        * - 40mhz we can base the selection off the channel flags,
> > either
> > +        *   higher or lower.
> > +        */
> > +       switch (width) {
> > +       case BAND_CHANDEF_WIDTH_20:
> > +               return 0;
> > +       case BAND_CHANDEF_WIDTH_40:
> > +               if (band == BAND_FREQ_6_GHZ)
> > +                       return 0;
> > +
> > +               if (best->flags & PRIMARY_CHANNEL_UPPER)
> > +                       chandef->center1_frequency = freq - 10;
> > +               else
> > +                       chandef->center1_frequency = freq + 10;
> > +
> > +               return 0;
> > +       default:
> > +               /* Should never happen */
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >   uint8_t band_freq_to_channel(uint32_t freq, enum band_freq
> > *out_band)
> >   {
> >         uint32_t channel = 0;
> 
> Regards,
> -Denis

Re: [PATCH 05/10] band: add APIs to generate a chandef from frequency

[Denis Kenzior]

Hi James,

> We still need to support 20Mhz since there is this distinction between
> 20MHz HT and 20Mhz no-HT. If only 20MHz HT is supported we should pick
> that rather than the non-HT width.

Hmm, but there's no difference in operating class between 20HT and 20NoHT.  So 
this could be done post-factum, no?

> 
> So ap.c just tries to pick the best chandef so in theory we could fall
> back to non-HT and this wouldn't be totally unexpected (from the users
> perspective). The problem is we would then need to disable HT in ap.c
> if we fell back to non-HT.

Something like if ap->chandef.channel_width == 20HT && !ap->supports_ht -> set 
channel_width appropriately.

> 
> Since I may just remove band_freq_to_chandef() entirely I might instead
> keep the -ENOENT return and fall back inside ap.c itself. Then I don't
> have to check chandef->channel_width after a successful return and we
> know this will always return an HT chandef, or fail.
> 

Or do that.  Or pass in the ht capability to band_freq_to_chandef() and let it 
figure it out.

Regards,
-Denis