[PATCH v2 1/7] ie: fix ie_parse_data_rates to handle NULL

[PATCH v2 1/7] ie: fix ie_parse_data_rates to handle NULL

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

The code was partially there to handle NULL IEs but not
quite. If NULL IEs are passed in base the data rate totally
on the basic rate RSSI table.
---
 src/ie.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/src/ie.c b/src/ie.c
index 0f430b98..0b471f1b 100644
--- a/src/ie.c
+++ b/src/ie.c
@@ -1688,14 +1688,16 @@ static int ie_parse_supported_rates(struct ie_tlv_iter *supp_rates_iter,
 		max_rate = map->rate;
 	}
 
-	/* Find highest rate in Supported Rates IE */
-	rates = ie_tlv_iter_get_data(supp_rates_iter);
+	if (supp_rates_iter) {
+		/* Find highest rate in Supported Rates IE */
+		rates = ie_tlv_iter_get_data(supp_rates_iter);
 
-	for (i = 0; i < len; i++) {
-		uint8_t r = rates[i] & 0x7f;
+		for (i = 0; i < len; i++) {
+			uint8_t r = rates[i] & 0x7f;
 
-		if (r <= max_rate && r > highest)
-			highest = r;
+			if (r <= max_rate && r > highest)
+				highest = r;
+		}
 	}
 
 	/* Find highest rate in Extended Supported Rates IE */
@@ -1711,7 +1713,10 @@ static int ie_parse_supported_rates(struct ie_tlv_iter *supp_rates_iter,
 		}
 	}
 
-	*data_rate = (highest / 2) * 1000000;
+	if (highest)
+		*data_rate = (highest / 2) * 1000000;
+	else
+		*data_rate = (max_rate / 2) * 1000000;
 
 	return 0;
 }
-- 
2.31.1

[PATCH v2 2/7] scan: rework BSS ranking

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 4033 bytes --]

It was observed that IWD's ranking for BSS's did not always
end up with the fastest being chosen. This was due to IWD's
heavy weight on signal strength. This is a decent way of ranking
but even better is calculating a theoretical data rate which
was also done and factored in. The problem is the data rate
factor was always outdone by the signal strength.

Intead remove signal strength entirely as this is already taken
into account with the data rate calculation. This also removes
the check for rate IEs. If no IEs are found the parser will
base the data rate soley on RSSI.

There were a few other factors removed which will be added back
when ranking *networks* rather than BSS's. WPA version (or open)
was removed as well as the privacy capability. These values really
should not differ between BSS's in the same SSID and as such
should be used for network ranking instead.
---
 src/scan.c | 63 ++++++++++++++----------------------------------------
 1 file changed, 16 insertions(+), 47 deletions(-)

v2:
 * Added support for BSS's that did not have any rate IEs by
   using the same ie_parse_data_rates

diff --git a/src/scan.c b/src/scan.c
index a37e1714..a6c7473e 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -1326,39 +1326,31 @@ static struct scan_bss *scan_parse_result(struct l_genl_msg *msg,
 
 static void scan_bss_compute_rank(struct scan_bss *bss)
 {
-	static const double RANK_RSNE_FACTOR = 1.2;
-	static const double RANK_WPA_FACTOR = 1.0;
-	static const double RANK_OPEN_FACTOR = 0.5;
-	static const double RANK_NO_PRIVACY_FACTOR = 0.5;
 	static const double RANK_HIGH_UTILIZATION_FACTOR = 0.8;
 	static const double RANK_LOW_UTILIZATION_FACTOR = 1.2;
-	static const double RANK_MIN_SUPPORTED_RATE_FACTOR = 0.6;
-	static const double RANK_MAX_SUPPORTED_RATE_FACTOR = 1.3;
-	double rank;
+	float rank;
 	uint32_t irank;
-
+	uint64_t data_rate;
 	/*
-	 * Signal strength is in mBm (100 * dBm) and is negative.
-	 * WiFi range is -0 to -100 dBm
+	 * Maximum rate is 2340Mbps (VHT)
 	 */
-
-	/* Heavily slanted towards signal strength */
-	rank = 10000 + bss->signal_strength;
+	uint64_t max_rate = 2340000000U;
 
 	/*
-	 * Prefer RSNE first, WPA second.  Open networks are much less
-	 * desirable.
+	 * If parsing fails choose a very low data rate as its unknown what
+	 * this AP supports or why its IEs did not parse. Likely not an AP
+	 * we should prefer to connect to.
 	 */
-	if (bss->rsne)
-		rank *= RANK_RSNE_FACTOR;
-	else if (bss->wpa)
-		rank *= RANK_WPA_FACTOR;
-	else
-		rank *= RANK_OPEN_FACTOR;
+	if (ie_parse_data_rates(bss->has_sup_rates ?
+				bss->supp_rates_ie : NULL,
+				bss->ext_supp_rates_ie,
+				bss->ht_capable ? bss->ht_ie : NULL,
+				bss->vht_capable ? bss->vht_ie : NULL,
+				bss->signal_strength / 100,
+				&data_rate) != 0)
+		data_rate = 2000000;
 
-	/* We prefer networks with CAP PRIVACY */
-	if (!(bss->capability & IE_BSS_CAP_PRIVACY))
-		rank *= RANK_NO_PRIVACY_FACTOR;
+	rank = (float)data_rate / (float)max_rate * USHRT_MAX;
 
 	/* Prefer 5G networks over 2.4G */
 	if (bss->frequency > 4000)
@@ -1370,29 +1362,6 @@ static void scan_bss_compute_rank(struct scan_bss *bss)
 	else if (bss->utilization <= 63)
 		rank *= RANK_LOW_UTILIZATION_FACTOR;
 
-	if (bss->has_sup_rates || bss->ext_supp_rates_ie) {
-		uint64_t data_rate;
-
-		if (ie_parse_data_rates(bss->has_sup_rates ?
-					bss->supp_rates_ie : NULL,
-					bss->ext_supp_rates_ie,
-					bss->ht_capable ? bss->ht_ie : NULL,
-					bss->vht_capable ? bss->vht_ie : NULL,
-					bss->signal_strength / 100,
-					&data_rate) == 0) {
-			double factor = RANK_MAX_SUPPORTED_RATE_FACTOR -
-					RANK_MIN_SUPPORTED_RATE_FACTOR;
-
-			/*
-			 * Maximum rate is 2340Mbps (VHT)
-			 */
-			factor = factor * data_rate / 2340000000U +
-						RANK_MIN_SUPPORTED_RATE_FACTOR;
-			rank *= factor;
-		} else
-			rank *= RANK_MIN_SUPPORTED_RATE_FACTOR;
-	}
-
 	irank = rank;
 
 	if (irank > USHRT_MAX)
-- 
2.31.1

[PATCH v2 3/7] network: use WPA version and privacy for ranking

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

These ranking factors were moved out of scan.c and into
network.c as they are more relevant for network ranking
than BSS ranking.
---
 src/network.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/network.c b/src/network.c
index 5bd57777..feedf99f 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1446,6 +1446,10 @@ int network_rank_compare(const void *a, const void *b, void *user)
 
 void network_rank_update(struct network *network, bool connected)
 {
+	static const double RANK_RSNE_FACTOR = 1.2;
+	static const double RANK_WPA_FACTOR = 1.0;
+	static const double RANK_OPEN_FACTOR = 0.5;
+	static const double RANK_NO_PRIVACY_FACTOR = 0.5;
 	/*
 	 * Theoretically there may be difference between the BSS selection
 	 * here and in network_bss_select but those should be rare cases.
@@ -1485,6 +1489,21 @@ void network_rank_update(struct network *network, bool connected)
 		network->rank = rankmod_table[n] * best_bss->rank + USHRT_MAX;
 	} else
 		network->rank = best_bss->rank;
+
+	/*
+	 * Prefer RSNE first, WPA second.  Open networks are much less
+	 * desirable.
+	 */
+	if (best_bss->rsne)
+		network->rank *= RANK_RSNE_FACTOR;
+	else if (best_bss->wpa)
+		network->rank *= RANK_WPA_FACTOR;
+	else
+		network->rank *= RANK_OPEN_FACTOR;
+
+	/* We prefer networks with CAP PRIVACY */
+	if (!(best_bss->capability & IE_BSS_CAP_PRIVACY))
+		network->rank *= RANK_NO_PRIVACY_FACTOR;
 }
 
 static void network_unset_hotspot(struct network *network, void *user_data)
-- 
2.31.1

[PATCH v2 4/7] station: autoconnect based on network, not BSS

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 5082 bytes --]

Prior to the BSS blacklist a BSS based autoconnect list made
the most sense, but now station actually retries all BSS's upon
failure. This means that for each BSS in the autoconnect list
every other BSS under that SSID will be attempted to connect to
if there is a failure. Essentially this is a network based
autoconnect list, just an indirect way of doing it.

Intead the autoconnect list can be purely network based, using
the network rank for sorting. This avoids the need for a special
autoconnect_entry struct as well as ensures the last connected
network is chosen first (simply based on existing network ranking
logic).
---
 src/station.c | 66 +++++++++++----------------------------------------
 1 file changed, 14 insertions(+), 52 deletions(-)

diff --git a/src/station.c b/src/station.c
index 479f81f5..aa9a70aa 100644
--- a/src/station.c
+++ b/src/station.c
@@ -152,12 +152,6 @@ static bool station_is_autoconnecting(struct station *station)
 			station->state == STATION_STATE_AUTOCONNECT_QUICK;
 }
 
-struct autoconnect_entry {
-	uint16_t rank;
-	struct network *network;
-	struct scan_bss *bss;
-};
-
 static void station_property_set_scanning(struct station *station,
 								bool scanning)
 {
@@ -176,25 +170,23 @@ static void station_enter_state(struct station *station,
 
 static void station_autoconnect_next(struct station *station)
 {
-	struct autoconnect_entry *entry;
+	struct network *network;
 	int r;
 
-	while ((entry = l_queue_pop_head(station->autoconnect_list))) {
-		const char *ssid = network_get_ssid(entry->network);
+	while ((network = l_queue_pop_head(station->autoconnect_list))) {
+		const char *ssid = network_get_ssid(network);
+		struct scan_bss *bss = network_bss_select(network, false);
 
 		l_debug("Considering autoconnecting to BSS '%s' with SSID: %s,"
 			" freq: %u, rank: %u, strength: %i",
-			util_address_to_string(entry->bss->addr), ssid,
-			entry->bss->frequency, entry->rank,
-			entry->bss->signal_strength);
+			util_address_to_string(bss->addr), ssid,
+			bss->frequency, bss->rank,
+			bss->signal_strength);
 
-		if (blacklist_contains_bss(entry->bss->addr)) {
-			l_free(entry);
+		if (blacklist_contains_bss(bss->addr))
 			continue;
-		}
 
-		r = network_autoconnect(entry->network, entry->bss);
-		l_free(entry);
+		r = network_autoconnect(network, bss);
 
 		if (!r) {
 			station_enter_state(station, STATION_STATE_CONNECTING);
@@ -212,33 +204,6 @@ static void station_autoconnect_next(struct station *station)
 	}
 }
 
-static int autoconnect_rank_compare(const void *a, const void *b, void *user)
-{
-	const struct autoconnect_entry *new_ae = a;
-	const struct autoconnect_entry *ae = b;
-
-	return (ae->rank > new_ae->rank) ? 1 : -1;
-}
-
-static void station_add_autoconnect_bss(struct station *station,
-					struct network *network,
-					struct scan_bss *bss)
-{
-	double rankmod;
-	struct autoconnect_entry *entry;
-
-	/* See if network is autoconnectable (is a known network) */
-	if (!network_rankmod(network, &rankmod))
-		return;
-
-	entry = l_new(struct autoconnect_entry, 1);
-	entry->network = network;
-	entry->bss = bss;
-	entry->rank = bss->rank * rankmod;
-	l_queue_insert(station->autoconnect_list, entry,
-				autoconnect_rank_compare, NULL);
-}
-
 static void bss_free(void *data)
 {
 	struct scan_bss *bss = data;
@@ -457,12 +422,9 @@ static bool match_nai_realms(const struct network_info *info, void *user_data)
 static void network_add_foreach(struct network *network, void *user_data)
 {
 	struct station *station = user_data;
-	struct scan_bss *bss = network_bss_select(network, false);
-
-	if (!bss)
-		return;
 
-	station_add_autoconnect_bss(station, network, bss);
+	l_queue_insert(station->autoconnect_list, network,
+				network_rank_compare, NULL);
 }
 
 static bool match_pending(const void *a, const void *b)
@@ -553,7 +515,7 @@ request_done:
 	/* Notify all watchers now that every ANQP request has finished */
 	l_queue_foreach_remove(station->anqp_pending, anqp_entry_foreach, NULL);
 
-	l_queue_destroy(station->autoconnect_list, l_free);
+	l_queue_destroy(station->autoconnect_list, NULL);
 	station->autoconnect_list = l_queue_new();
 
 	if (station_is_autoconnecting(station)) {
@@ -662,7 +624,7 @@ void station_set_scan_results(struct station *station,
 
 	l_queue_clear(station->hidden_bss_list_sorted, NULL);
 
-	l_queue_destroy(station->autoconnect_list, l_free);
+	l_queue_destroy(station->autoconnect_list, NULL);
 	station->autoconnect_list = l_queue_new();
 
 	station_bss_list_remove_expired_bsses(station, freqs);
@@ -3600,7 +3562,7 @@ static void station_free(struct station *station)
 	l_hashmap_destroy(station->networks, network_free);
 	l_queue_destroy(station->bss_list, bss_free);
 	l_queue_destroy(station->hidden_bss_list_sorted, NULL);
-	l_queue_destroy(station->autoconnect_list, l_free);
+	l_queue_destroy(station->autoconnect_list, NULL);
 
 	watchlist_destroy(&station->state_watches);
 
-- 
2.31.1

[PATCH v2 5/7] netdev: introduce [General].RoamThreshold5G

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3006 bytes --]

This value sets the roaming threshold on 5GHz networks. The
threshold has been separated from 2.4GHz because in many cases
5GHz can perform much better at low RSSI than 2.4GHz.

In addition the BSS ranking logic was re-worked and now 5GHz is
much more preferred, even at low RSSI. This means we need a
lower floor for RSSI before roaming, otherwise IWD would end
up roaming immediately after connecting due to low RSSI CQM
events.
---
 src/netdev.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 4fbe813a..9e7fc821 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -891,6 +891,7 @@ struct netdev *netdev_find(int ifindex)
 
 /* Threshold RSSI for roaming to trigger, configurable in main.conf */
 static int LOW_SIGNAL_THRESHOLD;
+static int LOW_SIGNAL_THRESHOLD_5GHZ;
 
 static void netdev_cqm_event_rssi_threshold(struct netdev *netdev,
 						uint32_t rssi_event)
@@ -925,6 +926,8 @@ static void netdev_cqm_event_rssi_value(struct netdev *netdev, int rssi_val)
 {
 	bool new_rssi_low;
 	uint8_t prev_rssi_level_idx = netdev->cur_rssi_level_idx;
+	int threshold = netdev->frequency > 4000 ? LOW_SIGNAL_THRESHOLD_5GHZ :
+						LOW_SIGNAL_THRESHOLD;
 
 	if (!netdev->connected)
 		return;
@@ -939,7 +942,7 @@ static void netdev_cqm_event_rssi_value(struct netdev *netdev, int rssi_val)
 	if (!netdev->event_filter)
 		return;
 
-	new_rssi_low = rssi_val < LOW_SIGNAL_THRESHOLD;
+	new_rssi_low = rssi_val < threshold;
 	if (netdev->cur_rssi_low != new_rssi_low) {
 		int event = new_rssi_low ?
 			NETDEV_EVENT_RSSI_THRESHOLD_LOW :
@@ -4806,9 +4809,11 @@ static struct l_genl_msg *netdev_build_cmd_cqm_rssi_update(
 	uint32_t hyst = 5;
 	int thold_count;
 	int32_t thold_list[levels_num + 2];
+	int threshold = netdev->frequency > 4000 ? LOW_SIGNAL_THRESHOLD_5GHZ :
+						LOW_SIGNAL_THRESHOLD;
 
 	if (levels_num == 0) {
-		thold_list[0] = LOW_SIGNAL_THRESHOLD;
+		thold_list[0] = threshold;
 		thold_count = 1;
 	} else {
 		/*
@@ -4827,13 +4832,12 @@ static struct l_genl_msg *netdev_build_cmd_cqm_rssi_update(
 			if (i && thold_list[thold_count - 1] >= val)
 				return NULL;
 
-			if (val >= LOW_SIGNAL_THRESHOLD && !low_sig_added) {
-				thold_list[thold_count++] =
-					LOW_SIGNAL_THRESHOLD;
+			if (val >= threshold && !low_sig_added) {
+				thold_list[thold_count++] = threshold;
 				low_sig_added = true;
 
 				/* Duplicate values are not allowed */
-				if (val == LOW_SIGNAL_THRESHOLD)
+				if (val == threshold)
 					continue;
 			}
 
@@ -5833,6 +5837,10 @@ static int netdev_init(void)
 					&LOW_SIGNAL_THRESHOLD))
 		LOW_SIGNAL_THRESHOLD = -70;
 
+	if (!l_settings_get_int(settings, "General", "RoamThreshold5G",
+					&LOW_SIGNAL_THRESHOLD_5GHZ))
+		LOW_SIGNAL_THRESHOLD_5GHZ = -76;
+
 	if (!l_settings_get_bool(settings, "General", "ControlPortOverNL80211",
 					&pae_over_nl80211))
 		pae_over_nl80211 = true;
-- 
2.31.1

[PATCH v2 6/7] doc: document [General].RoamThreshold5G

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

---
 src/iwd.config.rst | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/iwd.config.rst b/src/iwd.config.rst
index 478bd616..8e3662d4 100644
--- a/src/iwd.config.rst
+++ b/src/iwd.config.rst
@@ -131,7 +131,14 @@ The group ``[General]`` contains general settings.
    * - RoamThreshold
      - Value: rssi dBm value, from -100 to 1, default: **-70**
 
-       This can be used to control how aggressively **iwd** roams.
+       This can be used to control how aggressively **iwd** roams from 2.4Ghz
+       networks.
+
+   * - RoamThreshold5G
+     - Value: rssi dBm value, from -100 to 1, default: **-76**
+
+       This value can be used to control how aggressively **iwd** roams from
+       5GHz networks.
 
    * - RoamRetryInterval
      - Value: unsigned int value in seconds (default: **60**)
-- 
2.31.1

[PATCH v2 7/7] test-runner: remove stale file after test

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

test-runner will print out if files were left behind after a
test which lets the developer know something was not cleaned
up. But in this case test-runner should also remove these files
so they are not left, and printed, for each subsequent test.
---
 tools/test-runner | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/test-runner b/tools/test-runner
index dd4cbf82..892e63ff 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -1186,6 +1186,7 @@ def post_test(ctx, to_copy):
 	allowed = ['phonesim.conf', 'certs', 'secrets', 'iwd']
 	for f in [f for f in os.listdir('/tmp') if f not in allowed]:
 		dbg("File %s was not cleaned up!" % f)
+		os.remove('/tmp/' + f)
 
 	if ctx.args.valgrind:
 		with open('/tmp/valgrind.log', 'r') as f:
-- 
2.31.1

Re: [PATCH v2 1/7] ie: fix ie_parse_data_rates to handle NULL

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]

Hi James,

On 5/7/21 3:26 PM, James Prestwood wrote:
> The code was partially there to handle NULL IEs but not
> quite. If NULL IEs are passed in base the data rate totally
> on the basic rate RSSI table.
> ---
>   src/ie.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 

I went ahead and applied patches 1, 5, 6 (see reply separately) and 7.  I'll 
leave the rest up for a few days to see if there's any additional comments.

Regards,
-Denis

Re: [PATCH v2 6/7] doc: document [General].RoamThreshold5G

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

Hi James,

On 5/7/21 3:26 PM, James Prestwood wrote:
> ---
>   src/iwd.config.rst | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/iwd.config.rst b/src/iwd.config.rst
> index 478bd616..8e3662d4 100644
> --- a/src/iwd.config.rst
> +++ b/src/iwd.config.rst
> @@ -131,7 +131,14 @@ The group ``[General]`` contains general settings.
>      * - RoamThreshold
>        - Value: rssi dBm value, from -100 to 1, default: **-70**
>   
> -       This can be used to control how aggressively **iwd** roams.
> +       This can be used to control how aggressively **iwd** roams from 2.4Ghz

I reworded this slightly to read:
This value can be used to control how aggressively **iwd** roams when connected 
to a 2.4Ghz access point.

> +       networks.
> +
> +   * - RoamThreshold5G
> +     - Value: rssi dBm value, from -100 to 1, default: **-76**
> +
> +       This value can be used to control how aggressively **iwd** roams from
> +       5GHz networks.

ditto.

>   
>      * - RoamRetryInterval
>        - Value: unsigned int value in seconds (default: **60**)
> 

applied, thanks.

Regards,
-Denis

Re: [PATCH v2 2/7] scan: rework BSS ranking

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

Hi James,

On 5/7/21 3:26 PM, James Prestwood wrote:
> It was observed that IWD's ranking for BSS's did not always
> end up with the fastest being chosen. This was due to IWD's
> heavy weight on signal strength. This is a decent way of ranking
> but even better is calculating a theoretical data rate which
> was also done and factored in. The problem is the data rate
> factor was always outdone by the signal strength.
> 
> Intead remove signal strength entirely as this is already taken
> into account with the data rate calculation. This also removes
> the check for rate IEs. If no IEs are found the parser will
> base the data rate soley on RSSI.
> 
> There were a few other factors removed which will be added back
> when ranking *networks* rather than BSS's. WPA version (or open)
> was removed as well as the privacy capability. These values really
> should not differ between BSS's in the same SSID and as such
> should be used for network ranking instead.
> ---
>   src/scan.c | 63 ++++++++++++++----------------------------------------
>   1 file changed, 16 insertions(+), 47 deletions(-)
> 
> v2:
>   * Added support for BSS's that did not have any rate IEs by
>     using the same ie_parse_data_rates
> 

I went ahead and applied the rest of this series (2-4).

Regards,
-Denis