diff mbox series

[v2,08/20] BSS: Switch struct wpa_bss to use valid_links bitmask

Message ID 20240220131827.17766-9-benjamin@sipsolutions.net
State Accepted
Headers show
Series Various mostly WNM related fixes and cleanups | expand

Commit Message

Benjamin Berg Feb. 20, 2024, 1:18 p.m. UTC 
From: Benjamin Berg <benjamin.berg@intel.com>

This aligns both the supplicant and bss structures to use the same
pattern of a valid_links bitmask plus per-link entries.

Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
---
 wpa_supplicant/bss.c            | 28 ++++++++++++----------------
 wpa_supplicant/bss.h            |  5 +++--
 wpa_supplicant/events.c         |  5 ++---
 wpa_supplicant/sme.c            | 24 +++++++++++++-----------
 wpa_supplicant/wpa_supplicant.c | 12 ++++++------
 5 files changed, 36 insertions(+), 38 deletions(-)

Comments

Jouni Malinen March 2, 2024, 10:18 a.m. UTC | #1 
On Tue, Feb 20, 2024 at 02:18:15PM +0100, benjamin@sipsolutions.net wrote:
> This aligns both the supplicant and bss structures to use the same
> pattern of a valid_links bitmask plus per-link entries.

I can understand the other changes, but how is the following change
related to the rest of this patch and the commit message? And why should
that CTRL-EVENT-CONNECTED event message be changed from using the AP MLD
MAC address to the BSSID of the AP's affiliated link that was used for
association?

> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
> @@ -1006,13 +1006,13 @@ void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s,
>  	if (state == WPA_COMPLETED && wpa_s->new_connection) {
>  		struct wpa_ssid *ssid = wpa_s->current_ssid;
>  		int fils_hlp_sent = 0;
> -		char mld_addr[50];
> +		char assoc_link[50];
>  
> -		mld_addr[0] = '\0';
> +		assoc_link[0] = '\0';
>  		if (wpa_s->valid_links)
> -			os_snprintf(mld_addr, sizeof(mld_addr),
> -				    " ap_mld_addr=" MACSTR,
> -				    MAC2STR(wpa_s->ap_mld_addr));
> +			os_snprintf(assoc_link, sizeof(assoc_link),
> +				    " assoc_link=" MACSTR,
> +				    MAC2STR(wpa_s->links[wpa_s->mlo_assoc_link_id].bssid));
>  
>  #ifdef CONFIG_SME
>  		if ((wpa_s->drv_flags & WPA_DRIVER_FLAGS_SME) &&
> @@ -1029,7 +1029,7 @@ void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s,
>  			MAC2STR(wpa_s->bssid),
>  			ssid ? ssid->id : -1,
>  			ssid && ssid->id_str ? ssid->id_str : "",
> -			fils_hlp_sent ? " FILS_HLP_SENT" : "", mld_addr);
> +			fils_hlp_sent ? " FILS_HLP_SENT" : "", assoc_link);
>  #endif /* CONFIG_CTRL_IFACE || !CONFIG_NO_STDOUT_DEBUG */
>  		wpas_clear_temp_disabled(wpa_s, ssid, 1);
>  		wpa_s->consecutive_conn_failures = 0;
Benjamin Berg March 2, 2024, 10:31 a.m. UTC | #2 
On Sat, 2024-03-02 at 12:18 +0200, Jouni Malinen wrote:
> On Tue, Feb 20, 2024 at 02:18:15PM +0100,
> benjamin@sipsolutions.net wrote:
> > This aligns both the supplicant and bss structures to use the same
> > pattern of a valid_links bitmask plus per-link entries.
> 
> I can understand the other changes, but how is the following change
> related to the rest of this patch and the commit message? And why should
> that CTRL-EVENT-CONNECTED event message be changed from using the AP MLD
> MAC address to the BSSID of the AP's affiliated link that was used for
> association?

Hmm, you are right, that change really does not belong here.

I am not sure anymore. I guess I figured that bssid should be the MLD
address at this point, but I don't think I looked at it closer. And, if
that was the case, it should be changed.

Benjamin

> 
> > diff --git a/wpa_supplicant/wpa_supplicant.c
> > b/wpa_supplicant/wpa_supplicant.c
> > @@ -1006,13 +1006,13 @@ void wpa_supplicant_set_state(struct
> > wpa_supplicant *wpa_s,
> >  	if (state == WPA_COMPLETED && wpa_s->new_connection) {
> >  		struct wpa_ssid *ssid = wpa_s->current_ssid;
> >  		int fils_hlp_sent = 0;
> > -		char mld_addr[50];
> > +		char assoc_link[50];
> >  
> > -		mld_addr[0] = '\0';
> > +		assoc_link[0] = '\0';
> >  		if (wpa_s->valid_links)
> > -			os_snprintf(mld_addr, sizeof(mld_addr),
> > -				    " ap_mld_addr=" MACSTR,
> > -				    MAC2STR(wpa_s->ap_mld_addr));
> > +			os_snprintf(assoc_link,
> > sizeof(assoc_link),
> > +				    " assoc_link=" MACSTR,
> > +				    MAC2STR(wpa_s->links[wpa_s-
> > >mlo_assoc_link_id].bssid));
> >  
> >  #ifdef CONFIG_SME
> >  		if ((wpa_s->drv_flags & WPA_DRIVER_FLAGS_SME) &&
> > @@ -1029,7 +1029,7 @@ void wpa_supplicant_set_state(struct
> > wpa_supplicant *wpa_s,
> >  			MAC2STR(wpa_s->bssid),
> >  			ssid ? ssid->id : -1,
> >  			ssid && ssid->id_str ? ssid->id_str : "",
> > -			fils_hlp_sent ? " FILS_HLP_SENT" : "",
> > mld_addr);
> > +			fils_hlp_sent ? " FILS_HLP_SENT" : "",
> > assoc_link);
> >  #endif /* CONFIG_CTRL_IFACE || !CONFIG_NO_STDOUT_DEBUG */
> >  		wpas_clear_temp_disabled(wpa_s, ssid, 1);
> >  		wpa_s->consecutive_conn_failures = 0;
>
diff mbox series

Patch

diff --git a/wpa_supplicant/bss.c b/wpa_supplicant/bss.c
index 3a061ea60..44d9f8ff9 100644
--- a/wpa_supplicant/bss.c
+++ b/wpa_supplicant/bss.c
@@ -1510,9 +1510,6 @@  wpa_bss_parse_ml_rnr_ap_info(struct wpa_supplicant *wpa_s,
 	for (i = 0; i < count; i++) {
 		u8 bss_params;
 
-		if (bss->n_mld_links >= MAX_NUM_MLD_LINKS)
-			return;
-
 		if (end - pos < ap_info->tbtt_info_len)
 			break;
 
@@ -1543,13 +1540,12 @@  wpa_bss_parse_ml_rnr_ap_info(struct wpa_supplicant *wpa_s,
 					   wpa_s, neigh_bss->bssid)) {
 				struct mld_link *l;
 
-				l = &bss->mld_links[bss->n_mld_links];
-				l->link_id = link_id;
+				bss->valid_links |= BIT(link_id);
+				l = &bss->mld_links[link_id];
 				os_memcpy(l->bssid, pos + 1, ETH_ALEN);
 				l->freq = neigh_bss->freq;
 				l->disabled = mld_params[2] &
 					RNR_TBTT_INFO_MLD_PARAM2_LINK_DISABLED;
-				bss->n_mld_links++;
 			}
 		}
 
@@ -1675,15 +1671,16 @@  int wpa_bss_parse_basic_ml_element(struct wpa_supplicant *wpa_s,
 		os_memcpy(ap_mld_addr, ml_basic_common_info->mld_addr,
 			  ETH_ALEN);
 
-	bss->n_mld_links = 0;
-	l = &bss->mld_links[bss->n_mld_links];
 	link_id = ml_basic_common_info->variable[0] & EHT_ML_LINK_ID_MSK;
-	l->link_id = link_id;
+
+	bss->mld_link_id = link_id;
+	bss->valid_links = BIT(link_id);
+	seen = bss->valid_links;
+
+	l = &bss->mld_links[link_id];
 	os_memcpy(l->bssid, bss->bssid, ETH_ALEN);
 	l->freq = bss->freq;
 
-	seen = BIT(link_id);
-	bss->n_mld_links++;
 
 	/*
 	 * The AP MLD ID in the RNR corresponds to the MBSSID index, see
@@ -1733,13 +1730,12 @@  int wpa_bss_parse_basic_ml_element(struct wpa_supplicant *wpa_s,
 		}
 	}
 
-	wpa_printf(MSG_DEBUG, "MLD: n_mld_links=%u (unresolved: 0x%04hx)",
-		   bss->n_mld_links, missing);
+	wpa_printf(MSG_DEBUG, "MLD: valid_links=%04hx (unresolved: 0x%04hx)",
+		   bss->valid_links, missing);
 
-	for (i = 0; i < bss->n_mld_links; i++) {
+	for_each_link(bss->valid_links, i) {
 		wpa_printf(MSG_DEBUG, "MLD: link=%u, bssid=" MACSTR,
-			   bss->mld_links[i].link_id,
-			   MAC2STR(bss->mld_links[i].bssid));
+			   i, MAC2STR(bss->mld_links[i].bssid));
 	}
 
 	if (missing_links)
diff --git a/wpa_supplicant/bss.h b/wpa_supplicant/bss.h
index c06c20acf..7ec5a5111 100644
--- a/wpa_supplicant/bss.h
+++ b/wpa_supplicant/bss.h
@@ -126,11 +126,12 @@  struct wpa_bss {
 	size_t beacon_ie_len;
 	/** MLD address of the AP */
 	u8 mld_addr[ETH_ALEN];
+	/** MLD link of the AP */
+	u8 mld_link_id;
 
 	/** An array of MLD links */
-	u8 n_mld_links;
+	u16 valid_links;
 	struct mld_link {
-		u8 link_id;
 		u8 bssid[ETH_ALEN];
 		int freq;
 
diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
index cdc17b3c9..9346a4f61 100644
--- a/wpa_supplicant/events.c
+++ b/wpa_supplicant/events.c
@@ -1155,19 +1155,18 @@  static void owe_trans_ssid(struct wpa_supplicant *wpa_s, struct wpa_bss *bss,
 
 
 static bool wpas_valid_ml_bss(struct wpa_supplicant *wpa_s, struct wpa_bss *bss)
-
 {
 	u16 removed_links;
 
 	if (wpa_bss_parse_basic_ml_element(wpa_s, bss, NULL, NULL, NULL, NULL))
 		return true;
 
-	if (bss->n_mld_links == 0)
+	if (!bss->valid_links)
 		return true;
 
 	/* Check if the current BSS is going to be removed */
 	removed_links = wpa_bss_parse_reconf_ml_element(wpa_s, bss);
-	if (BIT(bss->mld_links[0].link_id) & removed_links)
+	if (BIT(bss->mld_link_id) & removed_links)
 		return false;
 
 	return true;
diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c
index d12ffb3ce..045ea6d62 100644
--- a/wpa_supplicant/sme.c
+++ b/wpa_supplicant/sme.c
@@ -517,21 +517,23 @@  out:
 static void wpas_sme_set_mlo_links(struct wpa_supplicant *wpa_s,
 				   struct wpa_bss *bss)
 {
-	int i;
+	u8 i;
 
 	wpa_s->valid_links = 0;
+	wpa_s->mlo_assoc_link_id = bss->mld_link_id;
 
-	for (i = 0; i < bss->n_mld_links; i++) {
-		u8 link_id = bss->mld_links[i].link_id;
+	for_each_link(bss->valid_links, i) {
 		const u8 *bssid = bss->mld_links[i].bssid;
 
-		if (i == 0)
-			wpa_s->mlo_assoc_link_id = link_id;
-		wpa_s->valid_links |= BIT(link_id);
-		os_memcpy(wpa_s->links[link_id].bssid, bssid, ETH_ALEN);
-		wpa_s->links[link_id].freq = bss->mld_links[i].freq;
-		wpa_s->links[link_id].bss = wpa_bss_get_bssid(wpa_s, bssid);
-		wpa_s->links[link_id].disabled = bss->mld_links[i].disabled;
+		wpa_s->valid_links |= BIT(i);
+		os_memcpy(wpa_s->links[i].bssid, bssid, ETH_ALEN);
+		wpa_s->links[i].freq = bss->mld_links[i].freq;
+		wpa_s->links[i].disabled = bss->mld_links[i].disabled;
+
+		if (bss->mld_link_id == i)
+			wpa_s->links[i].bss = bss;
+		else
+			wpa_s->links[i].bss = wpa_bss_get_bssid(wpa_s, bssid);
 	}
 }
 
@@ -572,7 +574,7 @@  static void sme_send_authentication(struct wpa_supplicant *wpa_s,
 	if ((wpa_s->drv_flags2 & WPA_DRIVER_FLAGS2_MLO) &&
 	    !wpa_bss_parse_basic_ml_element(wpa_s, bss, wpa_s->ap_mld_addr,
 					    NULL, ssid, NULL) &&
-	    bss->n_mld_links) {
+	    bss->valid_links) {
 		wpa_printf(MSG_DEBUG, "MLD: In authentication");
 		wpas_sme_set_mlo_links(wpa_s, bss);
 
diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
index c19ab810a..ca9ec3ed6 100644
--- a/wpa_supplicant/wpa_supplicant.c
+++ b/wpa_supplicant/wpa_supplicant.c
@@ -1006,13 +1006,13 @@  void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s,
 	if (state == WPA_COMPLETED && wpa_s->new_connection) {
 		struct wpa_ssid *ssid = wpa_s->current_ssid;
 		int fils_hlp_sent = 0;
-		char mld_addr[50];
+		char assoc_link[50];
 
-		mld_addr[0] = '\0';
+		assoc_link[0] = '\0';
 		if (wpa_s->valid_links)
-			os_snprintf(mld_addr, sizeof(mld_addr),
-				    " ap_mld_addr=" MACSTR,
-				    MAC2STR(wpa_s->ap_mld_addr));
+			os_snprintf(assoc_link, sizeof(assoc_link),
+				    " assoc_link=" MACSTR,
+				    MAC2STR(wpa_s->links[wpa_s->mlo_assoc_link_id].bssid));
 
 #ifdef CONFIG_SME
 		if ((wpa_s->drv_flags & WPA_DRIVER_FLAGS_SME) &&
@@ -1029,7 +1029,7 @@  void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s,
 			MAC2STR(wpa_s->bssid),
 			ssid ? ssid->id : -1,
 			ssid && ssid->id_str ? ssid->id_str : "",
-			fils_hlp_sent ? " FILS_HLP_SENT" : "", mld_addr);
+			fils_hlp_sent ? " FILS_HLP_SENT" : "", assoc_link);
 #endif /* CONFIG_CTRL_IFACE || !CONFIG_NO_STDOUT_DEBUG */
 		wpas_clear_temp_disabled(wpa_s, ssid, 1);
 		wpa_s->consecutive_conn_failures = 0;