[PATCH 1/7] wiphy: fix runtime error from bit shift

[PATCH 1/7] wiphy: fix runtime error from bit shift

[James Prestwood]

The compiler treated the '1' as an int type which was not big enough
to hold a bit shift of 31. Change this to 1ULL to fix the error:

runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
---
 src/wiphy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wiphy.c b/src/wiphy.c
index 09b99fb2..4b9e130a 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -957,7 +957,7 @@ static void wiphy_print_he_capabilities(struct band *band,
 	uint8_t width_set = bit_field(he_cap->he_phy_capa[0], 1, 7);
 
 	for (i = 0; i < 32; i++) {
-		if (!(he_cap->iftypes & (1 << i)))
+		if (!(he_cap->iftypes & (1ULL << i)))
 			continue;
 
 		if (L_WARN_ON(s >= type_buf + sizeof(type_buf)))
-- 
2.34.1

[PATCH 2/7] scan: make scan_freq_set const in scan_passive

[James Prestwood]

The scan_passive API wasn't using a const struct scan_freq_set as it
should be since it's not modifying the contents. Changing this to
const did require some additional changes like making the scan_parameters
'freqs' member const as well.

After changing scan_parameters, p2p needed updating since it was using
scan_parameters.freqs directly. This was changed to using a separate
scan_freq_set pointer, then setting to scan_parameters.freqs when needed.
---
 src/p2p.c  | 21 +++++++++++++--------
 src/scan.c |  4 ++--
 src/scan.h |  4 ++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index a7207c30..ff3b8e45 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -1977,6 +1977,7 @@ static bool p2p_provision_scan_notify(int err, struct l_queue *bss_list,
 static void p2p_provision_scan_start(struct p2p_device *dev)
 {
 	struct scan_parameters params = {};
+	struct scan_freq_set *freqs = NULL;
 	uint8_t buf[256];
 
 	params.flush = true;
@@ -2005,16 +2006,17 @@ static void p2p_provision_scan_start(struct p2p_device *dev)
 	 * contain all of the 2.4 and 5G channels.
 	 */
 	if (dev->conn_go_scan_retry < 12) {
-		params.freqs = scan_freq_set_new();
-		scan_freq_set_add(params.freqs, dev->conn_go_oper_freq);
+		freqs = scan_freq_set_new();
+		scan_freq_set_add(freqs, dev->conn_go_oper_freq);
+		params.freqs = freqs;
 	}
 
 	dev->scan_id = scan_active_full(dev->wdev_id, &params, NULL,
 					p2p_provision_scan_notify, dev,
 					p2p_scan_destroy);
 
-	if (params.freqs)
-		scan_freq_set_free(params.freqs);
+	if (freqs)
+		scan_freq_set_free(freqs);
 }
 
 static void p2p_start_client_provision(struct p2p_device *dev)
@@ -3777,6 +3779,7 @@ schedule:
 static bool p2p_device_scan_start(struct p2p_device *dev)
 {
 	struct scan_parameters params = {};
+	struct scan_freq_set *freqs;
 	uint8_t buf[256];
 	unsigned int i;
 
@@ -3812,13 +3815,13 @@ static bool p2p_device_scan_start(struct p2p_device *dev)
 	 * Request frames intended for both P2P Devices and non-P2P Devices."
 	 */
 	params.no_cck_rates = true;
-	params.freqs = scan_freq_set_new();
+	freqs = scan_freq_set_new();
 
 	for (i = 0; i < L_ARRAY_SIZE(channels_social); i++) {
 		int chan = channels_social[i];
 		uint32_t freq = band_channel_to_freq(chan, BAND_FREQ_2_4_GHZ);
 
-		scan_freq_set_add(params.freqs, freq);
+		scan_freq_set_add(freqs, freq);
 	}
 
 	/*
@@ -3845,12 +3848,14 @@ static bool p2p_device_scan_start(struct p2p_device *dev)
 			dev->chans_per_scan = CHANS_PER_SCAN;
 		}
 
-		scan_freq_set_add(params.freqs, freq);
+		scan_freq_set_add(freqs, freq);
 	}
 
+	params.freqs = freqs;
+
 	dev->scan_id = scan_active_full(dev->wdev_id, &params, NULL,
 					p2p_scan_notify, dev, p2p_scan_destroy);
-	scan_freq_set_free(params.freqs);
+	scan_freq_set_free(freqs);
 
 	return dev->scan_id != 0;
 }
diff --git a/src/scan.c b/src/scan.c
index 39aef625..03e5b8d9 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -279,7 +279,7 @@ static void scan_freq_append(uint32_t freq, void *user_data)
 }
 
 static void scan_build_attr_scan_frequencies(struct l_genl_msg *msg,
-						struct scan_freq_set *freqs)
+					const struct scan_freq_set *freqs)
 {
 	struct scan_freq_append_data append_data = { msg, 0 };
 
@@ -654,7 +654,7 @@ static uint32_t scan_common(uint64_t wdev_id, bool passive,
 					priority, &work_ops);
 }
 
-uint32_t scan_passive(uint64_t wdev_id, struct scan_freq_set *freqs,
+uint32_t scan_passive(uint64_t wdev_id, const struct scan_freq_set *freqs,
 			scan_trigger_func_t trigger, scan_notify_func_t notify,
 			void *userdata, scan_destroy_func_t destroy)
 {
diff --git a/src/scan.h b/src/scan.h
index 79bec605..58b8332b 100644
--- a/src/scan.h
+++ b/src/scan.h
@@ -93,7 +93,7 @@ struct scan_bss {
 struct scan_parameters {
 	const uint8_t *extra_ie;
 	size_t extra_ie_size;
-	struct scan_freq_set *freqs;
+	const struct scan_freq_set *freqs;
 	uint16_t duration;
 	bool flush : 1;
 	bool randomize_mac_addr_hint : 1;
@@ -130,7 +130,7 @@ struct l_genl_msg *scan_build_trigger_scan_bss(uint32_t ifindex,
 						const uint8_t *ssid,
 						uint32_t ssid_len);
 
-uint32_t scan_passive(uint64_t wdev_id, struct scan_freq_set *freqs,
+uint32_t scan_passive(uint64_t wdev_id, const struct scan_freq_set *freqs,
 			scan_trigger_func_t trigger, scan_notify_func_t notify,
 			void *userdata, scan_destroy_func_t destroy);
 uint32_t scan_passive_full(uint64_t wdev_id,
-- 
2.34.1

[PATCH 3/7] util: add scan_freq_set_remove

[James Prestwood]

---
 src/util.c | 22 ++++++++++++++++++++++
 src/util.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/src/util.c b/src/util.c
index 2832486d..8fdb7f17 100644
--- a/src/util.c
+++ b/src/util.c
@@ -357,6 +357,28 @@ bool scan_freq_set_add(struct scan_freq_set *freqs, uint32_t freq)
 	return false;
 }
 
+bool scan_freq_set_remove(struct scan_freq_set *freqs, uint32_t freq)
+{
+	enum band_freq band;
+	uint8_t channel;
+
+	channel = band_freq_to_channel(freq, &band);
+	if (!channel)
+		return false;
+
+	switch (band) {
+	case BAND_FREQ_2_4_GHZ:
+		freqs->channels_2ghz &= ~(1 << (channel - 1));
+		return true;
+	case BAND_FREQ_5_GHZ:
+		return l_uintset_take(freqs->channels_5ghz, channel);
+	case BAND_FREQ_6_GHZ:
+		return l_uintset_take(freqs->channels_6ghz, channel);
+	}
+
+	return false;
+}
+
 bool scan_freq_set_contains(const struct scan_freq_set *freqs, uint32_t freq)
 {
 	enum band_freq band;
diff --git a/src/util.h b/src/util.h
index 75c29039..ff5211ed 100644
--- a/src/util.h
+++ b/src/util.h
@@ -110,6 +110,7 @@ typedef void (*scan_freq_set_func_t)(uint32_t freq, void *userdata);
 struct scan_freq_set *scan_freq_set_new(void);
 void scan_freq_set_free(struct scan_freq_set *freqs);
 bool scan_freq_set_add(struct scan_freq_set *freqs, uint32_t freq);
+bool scan_freq_set_remove(struct scan_freq_set *freqs, uint32_t freq);
 bool scan_freq_set_contains(const struct scan_freq_set *freqs, uint32_t freq);
 uint32_t scan_freq_set_get_bands(struct scan_freq_set *freqs);
 void scan_freq_set_foreach(const struct scan_freq_set *freqs,
-- 
2.34.1

[PATCH 4/7] util: add scan_freq_set_max

[James Prestwood]

Finds the maximum frequency in the set
---
 src/util.c | 19 +++++++++++++++++++
 src/util.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/src/util.c b/src/util.c
index 8fdb7f17..2563184e 100644
--- a/src/util.c
+++ b/src/util.c
@@ -379,6 +379,25 @@ bool scan_freq_set_remove(struct scan_freq_set *freqs, uint32_t freq)
 	return false;
 }
 
+uint32_t scan_freq_set_max(struct scan_freq_set *freqs)
+{
+	uint32_t channel;
+
+	if (!l_uintset_isempty(freqs->channels_6ghz)) {
+		channel = l_uintset_find_max(freqs->channels_6ghz);
+		return band_channel_to_freq(channel, BAND_FREQ_6_GHZ);
+	}
+
+	if (!l_uintset_isempty(freqs->channels_5ghz)) {
+		channel = l_uintset_find_max(freqs->channels_5ghz);
+		return band_channel_to_freq(channel, BAND_FREQ_5_GHZ);
+	}
+
+	channel = __builtin_popcount(freqs->channels_2ghz);
+
+	return band_channel_to_freq(channel, BAND_FREQ_2_4_GHZ);
+}
+
 bool scan_freq_set_contains(const struct scan_freq_set *freqs, uint32_t freq)
 {
 	enum band_freq band;
diff --git a/src/util.h b/src/util.h
index ff5211ed..964f6500 100644
--- a/src/util.h
+++ b/src/util.h
@@ -122,5 +122,6 @@ void scan_freq_set_constrain(struct scan_freq_set *set,
 bool scan_freq_set_isempty(const struct scan_freq_set *set);
 uint32_t *scan_freq_set_to_fixed_array(const struct scan_freq_set *set,
 					size_t *len_out);
+uint32_t scan_freq_set_max(struct scan_freq_set *freqs);
 
 #endif /* __UTIL_H */
-- 
2.34.1

[PATCH 5/7] manager: re-dump wiphy on regulatory change

[James Prestwood]

This is required due to the kernel not waiting for regulatory updates before
sending the wiphy dump. The initial dump usually contains all supported
frequencies, none of which are disabled. Later, the regulatory subsystem
populates the frequency list which can significantly limit allowed frequencies.

Even disregarding this problem, any regulatory update will modify the allowed
frequencies so re-dumping wiphy should still be done on a regulatory change.
---
 src/manager.c | 58 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/src/manager.c b/src/manager.c
index ca29e110..c8d56a99 100644
--- a/src/manager.c
+++ b/src/manager.c
@@ -666,6 +666,29 @@ static int manager_wiphy_filtered_dump(uint32_t wiphy_id,
 	return 0;
 }
 
+static unsigned int manager_dump_wiphy(uint32_t wiphy_id)
+{
+	struct l_genl_msg *msg;
+	unsigned int wiphy_dump;
+
+	msg = l_genl_msg_new_sized(NL80211_CMD_GET_WIPHY, 128);
+
+	if (wiphy_id)
+		l_genl_msg_append_attr(msg, NL80211_ATTR_WIPHY, 4, &wiphy_id);
+
+	l_genl_msg_append_attr(msg, NL80211_ATTR_SPLIT_WIPHY_DUMP, 0, NULL);
+	wiphy_dump = l_genl_family_dump(nl80211, msg,
+						manager_wiphy_dump_callback,
+						NULL,
+						manager_wiphy_dump_done);
+	if (!wiphy_dump) {
+		l_error("Wiphy information dump failed");
+		l_genl_msg_unref(msg);
+	}
+
+	return wiphy_dump;
+}
+
 static void manager_config_notify(struct l_genl_msg *msg, void *user_data)
 {
 	uint8_t cmd;
@@ -799,6 +822,25 @@ static void manager_config_notify(struct l_genl_msg *msg, void *user_data)
 	}
 }
 
+static void manager_reg_notify(struct l_genl_msg *msg, void *user_data)
+{
+	uint32_t wiphy_id;
+	uint8_t cmd = l_genl_msg_get_command(msg);
+
+	switch (cmd) {
+	case NL80211_CMD_REG_CHANGE:
+		manager_dump_wiphy(0);
+		break;
+	case NL80211_CMD_WIPHY_REG_CHANGE:
+		if (nl80211_parse_attrs(msg, NL80211_ATTR_WIPHY, &wiphy_id,
+					NL80211_ATTR_UNSPEC) < 0)
+			break;
+
+		manager_dump_wiphy(wiphy_id);
+		break;
+	}
+}
+
 static int manager_init(void)
 {
 	struct l_genl *genl = iwd_get_genl();
@@ -826,17 +868,13 @@ static int manager_init(void)
 		goto error;
 	}
 
-	msg = l_genl_msg_new_sized(NL80211_CMD_GET_WIPHY, 128);
-	l_genl_msg_append_attr(msg, NL80211_ATTR_SPLIT_WIPHY_DUMP, 0, NULL);
-	wiphy_dump = l_genl_family_dump(nl80211, msg,
-						manager_wiphy_dump_callback,
-						NULL,
-						manager_wiphy_dump_done);
-	if (!wiphy_dump) {
-		l_error("Initial wiphy information dump failed");
-		l_genl_msg_unref(msg);
+	if (!l_genl_family_register(nl80211, NL80211_MULTICAST_GROUP_REG,
+					manager_reg_notify, NULL, NULL))
+		l_error("Registering for regulatory notifications failed");
+
+	wiphy_dump = manager_dump_wiphy(0);
+	if (!wiphy_dump)
 		goto error;
-	}
 
 	msg = l_genl_msg_new(NL80211_CMD_GET_INTERFACE);
 	interface_dump = l_genl_family_dump(nl80211, msg,
-- 
2.34.1

[PATCH 6/7] wiphy: track disabled frequencies on multiple dumps

[James Prestwood]

If a frequency is disabled, it should not be added to the supported list which
it previously was. In this case its added to the (new) disabled_freqs list.

Now that wiphy's are dumped on regulatory changes frequencies may be added or
removed from either the supported or disabled frequency list. These changes
are now tracked to keep the lists accurate.
---
 src/wiphy.c | 35 ++++++++++++++++++++++++++++++++---
 src/wiphy.h |  1 +
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/src/wiphy.c b/src/wiphy.c
index 4b9e130a..7d123e0c 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -104,6 +104,7 @@ struct wiphy {
 	uint16_t supported_iftypes;
 	uint16_t supported_ciphers;
 	struct scan_freq_set *supported_freqs;
+	struct scan_freq_set *disabled_freqs;
 	struct band *band_2g;
 	struct band *band_5g;
 	struct band *band_6g;
@@ -316,6 +317,7 @@ static struct wiphy *wiphy_new(uint32_t id)
 
 	wiphy->id = id;
 	wiphy->supported_freqs = scan_freq_set_new();
+	wiphy->disabled_freqs = scan_freq_set_new();
 	watchlist_init(&wiphy->state_watches, NULL);
 	wiphy->extended_capabilities[0] = IE_TYPE_EXTENDED_CAPABILITIES;
 	wiphy->extended_capabilities[1] = EXT_CAP_LEN;
@@ -357,6 +359,7 @@ static void wiphy_free(void *data)
 	}
 
 	scan_freq_set_free(wiphy->supported_freqs);
+	scan_freq_set_free(wiphy->disabled_freqs);
 	watchlist_destroy(&wiphy->state_watches);
 	l_free(wiphy->model_str);
 	l_free(wiphy->vendor_str);
@@ -454,6 +457,11 @@ const struct scan_freq_set *wiphy_get_supported_freqs(
 	return wiphy->supported_freqs;
 }
 
+const struct scan_freq_set *wiphy_get_disabled_freqs(const struct wiphy *wiphy)
+{
+	return wiphy->disabled_freqs;
+}
+
 bool wiphy_can_transition_disable(struct wiphy *wiphy)
 {
 	/*
@@ -1208,19 +1216,40 @@ static void parse_supported_frequencies(struct wiphy *wiphy,
 	struct l_genl_attr attr;
 
 	while (l_genl_attr_next(freqs, NULL, NULL, NULL)) {
+		uint32_t freq = 0;
+		bool disabled = false;
+
 		if (!l_genl_attr_recurse(freqs, &attr))
 			continue;
 
 		while (l_genl_attr_next(&attr, &type, &len, &data)) {
-			uint32_t u32;
 
 			switch (type) {
 			case NL80211_FREQUENCY_ATTR_FREQ:
-				u32 = *((uint32_t *) data);
-				scan_freq_set_add(wiphy->supported_freqs, u32);
+				freq = *((uint32_t *) data);
+				break;
+			case NL80211_FREQUENCY_ATTR_DISABLED:
+				disabled = true;
 				break;
 			}
 		}
+
+		if (!freq)
+			continue;
+
+		/*
+		 * Wiphy is dumped again on every regulatory domain change which
+		 * may contain disabled frequencies which were previously
+		 * allowed. If disabled, remove from supported. If supported,
+		 * remove from disabled.
+		 */
+		if (disabled) {
+			scan_freq_set_add(wiphy->disabled_freqs, freq);
+			scan_freq_set_remove(wiphy->supported_freqs, freq);
+		} else {
+			scan_freq_set_add(wiphy->supported_freqs, freq);
+			scan_freq_set_remove(wiphy->disabled_freqs, freq);
+		}
 	}
 }
 
diff --git a/src/wiphy.h b/src/wiphy.h
index acc64193..9a3b96f9 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -93,6 +93,7 @@ const char *wiphy_get_path(struct wiphy *wiphy);
 uint32_t wiphy_get_supported_bands(struct wiphy *wiphy);
 const struct scan_freq_set *wiphy_get_supported_freqs(
 						const struct wiphy *wiphy);
+const struct scan_freq_set *wiphy_get_disabled_freqs(const struct wiphy *wiphy);
 bool wiphy_can_transition_disable(struct wiphy *wiphy);
 bool wiphy_can_offload(struct wiphy *wiphy);
 bool wiphy_supports_cmds_auth_assoc(struct wiphy *wiphy);
-- 
2.34.1

[PATCH 7/7] station: do full passive scan if 6GHz is supported but disabled.

[James Prestwood]

The kernel handles setting the regulatory domain by receiving beacons which
set the country IE. Presumably since most regulatory domains disallow 6GHz
the default (world) domain also disables it. This means until the country
is set, 6GHz is disabled.

This poses a problem for IWD's quick scanning since it only scans a few
frequencies and this likely isn't enough beacons for the firmware to update
the country, leaving 6Ghz inaccessable to the user without manual intervention
(e.g. iw scan passive, or periodic scans by IWD).

To try and work around this limitation the quick scan logic has been updated
to check if a 6GHz AP has been connected to before and if that frequency is
disabled (but supported). If this is the case IWD will opt for a full passive
scan rather than scanning a limited set of frequencies.
---
 src/station.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/station.c b/src/station.c
index 02f21c76..373bd1c3 100644
--- a/src/station.c
+++ b/src/station.c
@@ -1352,12 +1352,35 @@ static void station_quick_scan_destroy(void *userdata)
 
 static int station_quick_scan_trigger(struct station *station)
 {
+	uint32_t max_freq;
 	struct scan_freq_set *known_freq_set;
+	const struct scan_freq_set *supported = wiphy_get_supported_freqs(
+								station->wiphy);
+	const struct scan_freq_set *disabled = wiphy_get_disabled_freqs(
+								station->wiphy);
 
 	known_freq_set = known_networks_get_recent_frequencies(5);
 	if (!known_freq_set)
 		return -ENODATA;
 
+	/*
+	 * This means IWD has previously connected to a 6GHz AP before, but now
+	 * the regulatory domain disallows 6GHz likely caused by a reboot or
+	 * the firmware going down. The only way to re-enable 6GHz is to get
+	 * enough beacons via scanning for the firmware to set the regulatory
+	 * domain and open up the frequencies.
+	 */
+	max_freq = scan_freq_set_max(known_freq_set);
+	if (max_freq > 6000 && scan_freq_set_contains(disabled, max_freq)) {
+		l_debug("6GHz may be available, doing full passive scan");
+		station->quick_scan_id = scan_passive(
+					netdev_get_wdev_id(station->netdev),
+					supported, station_quick_scan_triggered,
+					station_quick_scan_results, station,
+					station_quick_scan_destroy);
+		goto done;
+	}
+
 	if (!wiphy_constrain_freq_set(station->wiphy, known_freq_set)) {
 		scan_freq_set_free(known_freq_set);
 		return -ENOTSUP;
@@ -1368,6 +1391,7 @@ static int station_quick_scan_trigger(struct station *station)
 						station_quick_scan_triggered,
 						station_quick_scan_results,
 						station_quick_scan_destroy);
+done:
 	scan_freq_set_free(known_freq_set);
 
 	if (!station->quick_scan_id)
-- 
2.34.1

Re: [PATCH 1/7] wiphy: fix runtime error from bit shift

[Denis Kenzior]

Hi James,

On 7/22/22 11:34, James Prestwood wrote:
> The compiler treated the '1' as an int type which was not big enough
> to hold a bit shift of 31. Change this to 1ULL to fix the error:
> 
> runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
> ---
>   src/wiphy.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/wiphy.c b/src/wiphy.c
> index 09b99fb2..4b9e130a 100644
> --- a/src/wiphy.c
> +++ b/src/wiphy.c
> @@ -957,7 +957,7 @@ static void wiphy_print_he_capabilities(struct band *band,
>   	uint8_t width_set = bit_field(he_cap->he_phy_capa[0], 1, 7);
>   
>   	for (i = 0; i < 32; i++) {
> -		if (!(he_cap->iftypes & (1 << i)))
> +		if (!(he_cap->iftypes & (1ULL << i)))

That seems overkill, wouldn't 1U be enough?  Or just make 'i' unsigned?

Have you looked into using wiphy_get_supported_iftypes() here instead?

>   			continue;
>   
>   		if (L_WARN_ON(s >= type_buf + sizeof(type_buf)))

Regards,
-Denis

Re: [PATCH 5/7] manager: re-dump wiphy on regulatory change

[Denis Kenzior]

Hi James,

On 7/22/22 11:34, James Prestwood wrote:
> This is required due to the kernel not waiting for regulatory updates before
> sending the wiphy dump. The initial dump usually contains all supported
> frequencies, none of which are disabled. Later, the regulatory subsystem
> populates the frequency list which can significantly limit allowed frequencies.
> 
> Even disregarding this problem, any regulatory update will modify the allowed
> frequencies so re-dumping wiphy should still be done on a regulatory change.

git likes it if the description lines are limited to 72 columns.

> ---
>   src/manager.c | 58 ++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/src/manager.c b/src/manager.c
> index ca29e110..c8d56a99 100644
> --- a/src/manager.c
> +++ b/src/manager.c
> @@ -666,6 +666,29 @@ static int manager_wiphy_filtered_dump(uint32_t wiphy_id,
>   	return 0;
>   }
>   
> +static unsigned int manager_dump_wiphy(uint32_t wiphy_id)
> +{
> +	struct l_genl_msg *msg;
> +	unsigned int wiphy_dump;
> +
> +	msg = l_genl_msg_new_sized(NL80211_CMD_GET_WIPHY, 128);
> +
> +	if (wiphy_id)

Do note that phys start numbering at 0.  So this is probably not going to work.

> +		l_genl_msg_append_attr(msg, NL80211_ATTR_WIPHY, 4, &wiphy_id);
> +
> +	l_genl_msg_append_attr(msg, NL80211_ATTR_SPLIT_WIPHY_DUMP, 0, NULL);
> +	wiphy_dump = l_genl_family_dump(nl80211, msg,
> +						manager_wiphy_dump_callback,

Are the parsers really setup to free-and-re-parse the wiphy dump information? 
Perhaps using a callback that would parse only a subset of the information would 
be better?

> +						NULL,
> +						manager_wiphy_dump_done);
> +	if (!wiphy_dump) {
> +		l_error("Wiphy information dump failed");
> +		l_genl_msg_unref(msg);
> +	}
> +
> +	return wiphy_dump;
> +}
> +
>   static void manager_config_notify(struct l_genl_msg *msg, void *user_data)
>   {
>   	uint8_t cmd;

Regards,
-Denis

Re: [PATCH 4/7] util: add scan_freq_set_max

[Denis Kenzior]

Hi James,

On 7/22/22 11:34, James Prestwood wrote:
> Finds the maximum frequency in the set
> ---
>   src/util.c | 19 +++++++++++++++++++
>   src/util.h |  1 +
>   2 files changed, 20 insertions(+)
> 

Do you really need this, or is scan_freq_set_get_bands() enough?

Regards,
-Denis

Re: [PATCH 6/7] wiphy: track disabled frequencies on multiple dumps

[Denis Kenzior]

Hi James,

On 7/22/22 11:34, James Prestwood wrote:
> If a frequency is disabled, it should not be added to the supported list which
> it previously was. In this case its added to the (new) disabled_freqs list.
> 
> Now that wiphy's are dumped on regulatory changes frequencies may be added or
> removed from either the supported or disabled frequency list. These changes
> are now tracked to keep the lists accurate.
> ---
>   src/wiphy.c | 35 ++++++++++++++++++++++++++++++++---
>   src/wiphy.h |  1 +
>   2 files changed, 33 insertions(+), 3 deletions(-)
> 

<snip>

> +		/*
> +		 * Wiphy is dumped again on every regulatory domain change which
> +		 * may contain disabled frequencies which were previously
> +		 * allowed. If disabled, remove from supported. If supported,
> +		 * remove from disabled.
> +		 */

I would do this differently and leave 'supported_freqs' alone and only track the 
disabled freqs.

> +		if (disabled) {
> +			scan_freq_set_add(wiphy->disabled_freqs, freq);
> +			scan_freq_set_remove(wiphy->supported_freqs, freq);
> +		} else {
> +			scan_freq_set_add(wiphy->supported_freqs, freq);
> +			scan_freq_set_remove(wiphy->disabled_freqs, freq);
> +		}

Also you can simply clear the 'disabled_freqs' set on reg change, before parsing 
the disabled flags.

Then make sure wiphy_constrain_freq_set() takes everything into account. 
Perhaps we need a new scan_freq_set_* API?


>   	}
>   }
>   

Regards,
-Denis

Re: [PATCH 5/7] manager: re-dump wiphy on regulatory change

[James Prestwood]

On Fri, 2022-07-22 at 12:38 -0500, Denis Kenzior wrote:
> Hi James,
> 
> On 7/22/22 11:34, James Prestwood wrote:
> > This is required due to the kernel not waiting for regulatory
> > updates before
> > sending the wiphy dump. The initial dump usually contains all
> > supported
> > frequencies, none of which are disabled. Later, the regulatory
> > subsystem
> > populates the frequency list which can significantly limit allowed
> > frequencies.
> > 
> > Even disregarding this problem, any regulatory update will modify
> > the allowed
> > frequencies so re-dumping wiphy should still be done on a
> > regulatory change.
> 
> git likes it if the description lines are limited to 72 columns.
> 
> > ---
> >   src/manager.c | 58 ++++++++++++++++++++++++++++++++++++++++++----
> > -----
> >   1 file changed, 48 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/manager.c b/src/manager.c
> > index ca29e110..c8d56a99 100644
> > --- a/src/manager.c
> > +++ b/src/manager.c
> > @@ -666,6 +666,29 @@ static int
> > manager_wiphy_filtered_dump(uint32_t wiphy_id,
> >         return 0;
> >   }
> >   
> > +static unsigned int manager_dump_wiphy(uint32_t wiphy_id)
> > +{
> > +       struct l_genl_msg *msg;
> > +       unsigned int wiphy_dump;
> > +
> > +       msg = l_genl_msg_new_sized(NL80211_CMD_GET_WIPHY, 128);
> > +
> > +       if (wiphy_id)
> 
> Do note that phys start numbering at 0.  So this is probably not
> going to work.

Whoops, I thought zero was reserved. I'll use -1 instead.

> 
> > +               l_genl_msg_append_attr(msg, NL80211_ATTR_WIPHY, 4,
> > &wiphy_id);
> > +
> > +       l_genl_msg_append_attr(msg, NL80211_ATTR_SPLIT_WIPHY_DUMP,
> > 0, NULL);
> > +       wiphy_dump = l_genl_family_dump(nl80211, msg,
> > +                                               manager_wiphy_dump_
> > callback,
> 
> Are the parsers really setup to free-and-re-parse the wiphy dump
> information? 
> Perhaps using a callback that would parse only a subset of the
> information would 
> be better?

I thought they were since the wiphy dump comes as tons of separate
messages. Once the wiphy object is created we just keep adding stuff to
it. But maybe I overlooked this.

> 
> > +                                               NULL,
> > +                                               manager_wiphy_dump_
> > done);
> > +       if (!wiphy_dump) {
> > +               l_error("Wiphy information dump failed");
> > +               l_genl_msg_unref(msg);
> > +       }
> > +
> > +       return wiphy_dump;
> > +}
> > +
> >   static void manager_config_notify(struct l_genl_msg *msg, void
> > *user_data)
> >   {
> >         uint8_t cmd;
> 
> Regards,
> -Denis

Re: [PATCH 4/7] util: add scan_freq_set_max

[James Prestwood]

On Fri, 2022-07-22 at 12:41 -0500, Denis Kenzior wrote:
> Hi James,
> 
> On 7/22/22 11:34, James Prestwood wrote:
> > Finds the maximum frequency in the set
> > ---
> >   src/util.c | 19 +++++++++++++++++++
> >   src/util.h |  1 +
> >   2 files changed, 20 insertions(+)
> > 
> 
> Do you really need this, or is scan_freq_set_get_bands() enough?

I think so, because if some known frequency is in the 6G band but not
even supported (e.g. if the card changes) we shouldn't bother doing the
full scan. Somewhat of a corner case though.

> 
> Regards,
> -Denis
> 

Re: [PATCH 1/7] wiphy: fix runtime error from bit shift

[James Prestwood]

On Fri, 2022-07-22 at 12:04 -0500, Denis Kenzior wrote:
> Hi James,
> 
> On 7/22/22 11:34, James Prestwood wrote:
> > The compiler treated the '1' as an int type which was not big
> > enough
> > to hold a bit shift of 31. Change this to 1ULL to fix the error:
> > 
> > runtime error: left shift of 1 by 31 places cannot be represented
> > in type 'int'
> > ---
> >   src/wiphy.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/wiphy.c b/src/wiphy.c
> > index 09b99fb2..4b9e130a 100644
> > --- a/src/wiphy.c
> > +++ b/src/wiphy.c
> > @@ -957,7 +957,7 @@ static void wiphy_print_he_capabilities(struct
> > band *band,
> >         uint8_t width_set = bit_field(he_cap->he_phy_capa[0], 1,
> > 7);
> >   
> >         for (i = 0; i < 32; i++) {
> > -               if (!(he_cap->iftypes & (1 << i)))
> > +               if (!(he_cap->iftypes & (1ULL << i)))
> 
> That seems overkill, wouldn't 1U be enough?  Or just make 'i'
> unsigned?
> 
> Have you looked into using wiphy_get_supported_iftypes() here
> instead?

Wasn't aware of that. Thats exactly what I need :)

> 
> >                         continue;
> >   
> >                 if (L_WARN_ON(s >= type_buf + sizeof(type_buf)))
> 
> Regards,
> -Denis