Re: [PATCH v3 07/10] station: roam blacklist BSS's, and consider when roaming

On 3/25/25 11:00 AM, James Prestwood wrote:
> If the BSS is requesting IWD roam elsewhere add this BSS to the
> blacklist using BLACKLIST_REASON_ROAM_REQUESTED. This will lower
> the chances of IWD roaming/connecting back to this BSS in the
> future.
>
> This then allows IWD to consider this blacklist state when picking
> a roam candidate. Its undesireable to fully ban a roam blacklisted
> BSS, so some additional sorting logic has been added. Prior to
> comparing based on rank, BSS's will be sorted into 3 groups:
>
> Optimal - Not roam blacklisted, and above the roaming threshold
> Above Threshold - May be blacklisted, and above the roaming threshold
> Below Threshold - BSS is below the roaming threshold
> ---
>   src/station.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 84 insertions(+), 4 deletions(-)
>
> diff --git a/src/station.c b/src/station.c
> index e2ed78f3..e3c7c189 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -155,6 +155,57 @@ struct anqp_entry {
>   	uint32_t pending;
>   };
>   
> +/*
> + * Rather than sorting BSS's purely based on ranking a higher level grouping
> + * is used. The purpose of this higher order grouping is the consider the BSS's
> + * roam blacklist status. The roam blacklist is a "soft" blacklist in that we
> + * still should connect to these BSS's if they are the only "good" option.
> + * The question here is: what makes a BSS "good" vs "bad".
> + *
> + * For an initial (probably nieve) approach here we can use the RoamThreshod[5G]
> + * and sort BSS's into "above" or "below" this threshold. Within each of these
> + * groups a BSS may be blacklisted, meaning it should get sorted lower on the
> + * list compared to others within the same group.
> + *
> + * Since we have several call sites needing to check/compare these groupings
> + * a bitmask can be used to describe the grouping:
> + *
> + * Each group will have 2 bits. The lower order bit signifies the BSS is in the
> + * group, but also blacklisted. The higher order bit signifies the BSS is in
> + * the group, but not blacklisted. The bitmask between two BSS's can then be
> + * compared directly similar to rank.
> + */
> +
> +#define UNDER_THRESHOLD_BIT 1
> +#define ABOVE_THRESHOLD_BIT 3
> +
> +static uint32_t evaluate_bss_group(const uint8_t *addr, uint32_t freq,
> +					int16_t signal_strength)
> +{
> +	int threshold;
> +	int signal = signal_strength / 100;
> +	bool roam_blacklist;
> +	uint32_t mask = 0;
> +
> +	if (blacklist_contains_bss(addr, BLACKLIST_REASON_CONNECT_FAILED))
> +		return mask;
> +
> +	roam_blacklist = blacklist_contains_bss(addr,
> +					BLACKLIST_REASON_ROAM_REQUESTED);
> +
> +	if (freq > 4000)
> +		netdev_get_low_signal_thresholds(NULL, &threshold);
> +	else
> +		netdev_get_low_signal_thresholds(&threshold, NULL);
> +
> +	if (signal >= threshold)
> +		set_bit(&mask, ABOVE_THRESHOLD_BIT - roam_blacklist);
> +	else
> +		set_bit(&mask, UNDER_THRESHOLD_BIT - roam_blacklist);
> +
> +	return mask;
> +}
> +
>   /*
>    * Used as entries for the roam list since holding scan_bss pointers directly
>    * from station->bss_list is not 100% safe due to the possibility of the
> @@ -164,11 +215,13 @@ struct roam_bss {
>   	uint8_t addr[6];
>   	uint16_t rank;
>   	int32_t signal_strength;
> +	uint32_t group;
>   	bool ft_failed: 1;
>   };
>   
>   static struct roam_bss *roam_bss_from_scan_bss(const struct scan_bss *bss,
> -						uint16_t rank)
> +						uint16_t rank,
> +						uint32_t group)
>   {
>   	struct roam_bss *rbss = l_new(struct roam_bss, 1);
>   
> @@ -176,6 +229,7 @@ static struct roam_bss *roam_bss_from_scan_bss(const struct scan_bss *bss,
>   	rbss->rank = rank;
>   	rbss->signal_strength = bss->signal_strength;
>   	rbss->ft_failed = false;
> +	rbss->group = group;
>   
>   	return rbss;
>   }
> @@ -184,6 +238,12 @@ static int roam_bss_rank_compare(const void *a, const void *b, void *user_data)
>   {
>   	const struct roam_bss *new_bss = a, *bss = b;
>   
> +	if (bss->group > new_bss->group)
> +		return 1;
> +	else if (bss->group < new_bss->group)
> +		return -1;
> +
> +	/* BSS's both have the same group, sort by rank */
>   	if (bss->rank == new_bss->rank)
>   		return (bss->signal_strength >
>   					new_bss->signal_strength) ? 1 : -1;
> @@ -2805,6 +2865,7 @@ static bool station_roam_scan_notify(int err, struct l_queue *bss_list,
>   	struct handshake_state *hs = netdev_get_handshake(station->netdev);
>   	struct scan_bss *current_bss = station->connected_bss;
>   	struct scan_bss *bss;
> +	uint32_t cur_bss_group = 0;
>   	double cur_bss_rank = 0.0;
>   	static const double RANK_FT_FACTOR = 1.3;
>   	uint16_t mdid;
> @@ -2835,6 +2896,9 @@ static bool station_roam_scan_notify(int err, struct l_queue *bss_list,
>   	bss = l_queue_find(bss_list, bss_match_bssid, current_bss->addr);
>   	if (bss && !station->ap_directed_roaming) {
>   		cur_bss_rank = bss->rank;
> +		cur_bss_group = evaluate_bss_group(current_bss->addr,
> +						current_bss->frequency,
> +						current_bss->signal_strength);
This is what caused the CI to fail on testPSK-roam. Instead I should 
have done the group evaluation on the "bss" pointer, not "current_bss". 
This will take into account the most recent scan, rather than the stale 
object we have from past scans.
>   
>   		if (hs->mde && bss->mde_present && l_get_le16(bss->mde) == mdid)
>   			cur_bss_rank *= RANK_FT_FACTOR;
> @@ -2859,6 +2923,9 @@ static bool station_roam_scan_notify(int err, struct l_queue *bss_list,
>   	while ((bss = l_queue_pop_head(bss_list))) {
>   		double rank;
>   		struct roam_bss *rbss;
> +		uint32_t group = evaluate_bss_group(bss->addr,
> +							bss->frequency,
> +							bss->signal_strength);
>   
>   		station_print_scan_bss(bss);
>   
> @@ -2889,7 +2956,15 @@ static bool station_roam_scan_notify(int err, struct l_queue *bss_list,
>   		if (hs->mde && bss->mde_present && l_get_le16(bss->mde) == mdid)
>   			rank *= RANK_FT_FACTOR;
>   
> -		if (rank <= cur_bss_rank)
> +		/*
> +		 * First check the group:
> +		 *   - If worse, disregard BSS candidate
> +		 *   - If better, keep BSS candidate
> +		 *   - If equal, compare based on rank
> +		 */
> +		if (group < cur_bss_group)
> +			goto next;
> +		else if (group == cur_bss_group && rank <= cur_bss_rank)
>   			goto next;
>   
>   		/*
> @@ -2898,7 +2973,7 @@ static bool station_roam_scan_notify(int err, struct l_queue *bss_list,
>   		 */
>   		station_update_roam_bss(station, bss);
>   
> -		rbss = roam_bss_from_scan_bss(bss, rank);
> +		rbss = roam_bss_from_scan_bss(bss, rank, group);
>   
>   		l_queue_insert(station->roam_bss_list, rbss,
>   				roam_bss_rank_compare, NULL);
> @@ -3268,6 +3343,10 @@ static void station_ap_directed_roam(struct station *station,
>   	l_timeout_remove(station->roam_trigger_timeout);
>   	station->roam_trigger_timeout = NULL;
>   
> +	blacklist_add_bss(station->connected_bss->addr,
> +				BLACKLIST_REASON_ROAM_REQUESTED);
> +	station_debug_event(station, "ap-roam-blacklist-added");
> +
>   	if (req_mode & WNM_REQUEST_MODE_PREFERRED_CANDIDATE_LIST) {
>   		l_debug("roam: AP sent a preferred candidate list");
>   		station_neighbor_report_cb(station->netdev, 0, body + pos,
> @@ -5344,7 +5423,8 @@ static bool station_force_roam_scan_notify(int err, struct l_queue *bss_list,
>   	/* The various roam routines expect this to be set from scanning */
>   	station->preparing_roam = true;
>   	l_queue_push_tail(station->roam_bss_list,
> -				roam_bss_from_scan_bss(target, target->rank));
> +				roam_bss_from_scan_bss(target, target->rank,
> +				0xffff));
>   
>   	station_update_roam_bss(station, target);
>