| Message ID | DM4PR19MB6173508761EFFA0E7E366CD8A88E2@DM4PR19MB6173.namprd19.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Multi-AP: wpa_supplicant: added support for VLAN ID and Multi-AP profile parsing | expand |
Hi, Is there any feedback on this patch? Upper layers (e.g. Easymesh agent in a repeater) may need to know the VLAN ID of the backhaul AP, hence we would like to add this information to WPA_EVENT_CONNECTED. Best regards, Arnon -----Original Message----- From: Jurijs Soloveckis <jsoloveckis@maxlinear.com> Sent: Wednesday, 21 August 2024 10:35 To: hostap@lists.infradead.org Cc: Arnon Meydav <ameydav@maxlinear.com>; Sergejs Hatinecs <shatinecs@maxlinear.com> Subject: [PATCH] Multi-AP: wpa_supplicant: added support for VLAN ID and Multi-AP profile parsing For Multi-AP traffic separation requirement, wpa_supplicant parses 802.1Q Multi-AP sub-element and reports: - VLAN ID of the AP it connects to. - Multi-AP profile of the AP it connects to. Signed-off-by: Jurijs Soloveckis <jsoloveckis@maxlinear.com> --- wpa_supplicant/config_ssid.h | 7 +++++++ wpa_supplicant/events.c | 12 ++++++++++++ wpa_supplicant/wpa_supplicant.c | 7 +++++-- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/wpa_supplicant/config_ssid.h b/wpa_supplicant/config_ssid.h index d64c30508..48e4a1da0 100644 --- a/wpa_supplicant/config_ssid.h +++ b/wpa_supplicant/config_ssid.h @@ -1192,6 +1192,13 @@ struct wpa_ssid { */ int multi_ap_profile; + /** + * multi_ap_primary_vlanid - Multi-AP Primary VLAN ID (Multi-AP Specification v2.0) + * 0 = VLAN ID not set + * 1-4094 = VLAN ID + */ + u16 multi_ap_primary_vlanid; + /** * beacon_prot - Whether Beacon protection is enabled * diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c index 724f2413f..d80b65add 100644 --- a/wpa_supplicant/events.c +++ b/wpa_supplicant/events.c @@ -3093,6 +3093,7 @@ static void multi_ap_process_assoc_resp(struct wpa_supplicant *wpa_s, { struct ieee802_11_elems elems; struct multi_ap_params multi_ap; + struct wpa_ssid *ssid = wpa_s->current_ssid; u16 status; wpa_s->multi_ap_ie = 0; @@ -3112,6 +3113,17 @@ static void multi_ap_process_assoc_resp(struct wpa_supplicant *wpa_s, wpa_s->multi_ap_fronthaul = !!(multi_ap.capability & MULTI_AP_FRONTHAUL_BSS); wpa_s->multi_ap_ie = 1; + + if (!ssid) + return; + + ssid->multi_ap_primary_vlanid = 0; + + if (wpa_s->multi_ap_backhaul) { + ssid->multi_ap_profile = multi_ap.profile; + if (ssid->multi_ap_backhaul_sta) + ssid->multi_ap_primary_vlanid = multi_ap.vlanid; + } } diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c index 81858327b..c29fb562c 100644 --- a/wpa_supplicant/wpa_supplicant.c +++ b/wpa_supplicant/wpa_supplicant.c @@ -1114,11 +1114,14 @@ void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s, #if defined(CONFIG_CTRL_IFACE) || !defined(CONFIG_NO_STDOUT_DEBUG) wpa_msg(wpa_s, MSG_INFO, WPA_EVENT_CONNECTED "- Connection to " - MACSTR " completed [id=%d id_str=%s%s]%s", + MACSTR " completed [id=%d id_str=%s%s]%s" + " multi_ap_profile=%d multi_ap_primary_vlanid=%d", 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" : "", mld_addr, + ssid ? ssid->multi_ap_profile : 0, + ssid ? ssid->multi_ap_primary_vlanid : 0); #endif /* CONFIG_CTRL_IFACE || !CONFIG_NO_STDOUT_DEBUG */ wpas_clear_temp_disabled(wpa_s, ssid, 1); wpa_s->consecutive_conn_failures = 0;
On Wed, Nov 13, 2024 at 07:52:58AM +0000, Arnon Meydav wrote: > Is there any feedback on this patch? > Upper layers (e.g. Easymesh agent in a repeater) may need to know the VLAN ID of the backhaul AP, hence we would like to add this information to WPA_EVENT_CONNECTED. It would seem fine to add such information into the event or some other convenient location for fetching it (like STATUS command). However, this commit message and the proposed changed feel a bit confusing if this is just about reporting the value to upper layers. > For Multi-AP traffic separation requirement, wpa_supplicant parses 802.1Q Multi-AP sub-element and reports: > - VLAN ID of the AP it connects to. > - Multi-AP profile of the AP it connects to. It would be good to be clear here on why this is being added (i.e., what the information is used for) and that this is just for reporting and not changing some configuration parameters. > diff --git a/wpa_supplicant/config_ssid.h b/wpa_supplicant/config_ssid.h > @@ -1192,6 +1192,13 @@ struct wpa_ssid { > + /** > + * multi_ap_primary_vlanid - Multi-AP Primary VLAN ID (Multi-AP Specification v2.0) > + * 0 = VLAN ID not set > + * 1-4094 = VLAN ID > + */ > + u16 multi_ap_primary_vlanid; Why would this be added as a new configuration parameter for the network profile? This does not seem to have anything to do with local configuration, i.e., this is a value that shows up as a part of a backhaul association. > diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c > @@ -1114,11 +1114,14 @@ void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s, > > #if defined(CONFIG_CTRL_IFACE) || !defined(CONFIG_NO_STDOUT_DEBUG) > wpa_msg(wpa_s, MSG_INFO, WPA_EVENT_CONNECTED "- Connection to " > - MACSTR " completed [id=%d id_str=%s%s]%s", > + MACSTR " completed [id=%d id_str=%s%s]%s" > + " multi_ap_profile=%d multi_ap_primary_vlanid=%d", > 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" : "", mld_addr, > + ssid ? ssid->multi_ap_profile : 0, > + ssid ? ssid->multi_ap_primary_vlanid : 0); I would prefer to extend the CTRL-EVENT-CONNECTED event to include these new values only if the association is for Wi-Fi EasyMesh backhaul connection. This event is used by many external programs and it is better to try to minimize risk of confusing them in cases where the association has nothing to do with Wi-Fi EasyMesh.
> From: Jouni Malinen <mailto:j@w1.fi> > It would seem fine to add such information into the event or some other > convenient location for fetching it (like STATUS command). However, this > commit message and the proposed changed feel a bit confusing if this is > just about reporting the value to upper layers. I prefer to keep the information in the event and not in the status, so that the upper layers can assign the appropriate VLAN immediately when the STA connects. > > For Multi-AP traffic separation requirement, wpa_supplicant parses 802.1Q Multi-AP sub-element and reports: > > - VLAN ID of the AP it connects to. > > - Multi-AP profile of the AP it connects to. > > It would be good to be clear here on why this is being added (i.e., what > the information is used for) and that this is just for reporting and not > changing some configuration parameters. I will ask our team to improve the commit message when submitting v2. > > diff --git a/wpa_supplicant/config_ssid.h b/wpa_supplicant/config_ssid.h > > @@ -1192,6 +1192,13 @@ struct wpa_ssid { > > + /** > > + * multi_ap_primary_vlanid - Multi-AP Primary VLAN ID (Multi-AP Specification v2.0) > > + * 0 = VLAN ID not set > > + * 1-4094 = VLAN ID > > + */ > > + u16 multi_ap_primary_vlanid; > > Why would this be added as a new configuration parameter for the network > profile? This does not seem to have anything to do with local > configuration, i.e., this is a value that shows up as a part of a > backhaul association. We wanted to keep the multi_ap information adjacent to the network information, since this is part of the link definition. However, as you rightly pointed out, this is retrieved dynamically from the AP, and not configured in the wpa_supplicant configuration. Reviewing the code again, perhaps this should be added directly in struct wpa_supplicant? Do you agree, or is there a more suitable location for such information to remain persistent throughout the connection? > I would prefer to extend the CTRL-EVENT-CONNECTED event to include > these new values only if the association is for Wi-Fi EasyMesh backhaul > connection. This event is used by many external programs and it is > better to try to minimize risk of confusing them in cases where the > association has nothing to do with Wi-Fi EasyMesh. Noted. I will ask that this change is applied to v2 of the commit.
On Mon, Dec 23, 2024 at 04:55:42PM +0000, Arnon Meydav wrote: > > > diff --git a/wpa_supplicant/config_ssid.h b/wpa_supplicant/config_ssid.h > > > @@ -1192,6 +1192,13 @@ struct wpa_ssid { > > > + u16 multi_ap_primary_vlanid; > > > > Why would this be added as a new configuration parameter for the network > > profile? This does not seem to have anything to do with local > > configuration, i.e., this is a value that shows up as a part of a > > backhaul association. > > We wanted to keep the multi_ap information adjacent to the network information, since this is part of the link definition. > However, as you rightly pointed out, this is retrieved dynamically from the AP, and not configured in the wpa_supplicant configuration. > Reviewing the code again, perhaps this should be added directly in struct wpa_supplicant? > Do you agree, or is there a more suitable location for such information to remain persistent throughout the connection? Yes, struct wpa_supplicant would be a better place for this type of information that applies for the current association and is learned during the association and needed a bit later (i.e., cannot be sent out in an event directly at the time of reception and instead, needs to be kept somewhere in memory between that point and the time it is used).
diff --git a/wpa_supplicant/config_ssid.h b/wpa_supplicant/config_ssid.h index d64c30508..48e4a1da0 100644 --- a/wpa_supplicant/config_ssid.h +++ b/wpa_supplicant/config_ssid.h @@ -1192,6 +1192,13 @@ struct wpa_ssid { */ int multi_ap_profile; + /** + * multi_ap_primary_vlanid - Multi-AP Primary VLAN ID (Multi-AP Specification v2.0) + * 0 = VLAN ID not set + * 1-4094 = VLAN ID + */ + u16 multi_ap_primary_vlanid; + /** * beacon_prot - Whether Beacon protection is enabled * diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c index 724f2413f..d80b65add 100644 --- a/wpa_supplicant/events.c +++ b/wpa_supplicant/events.c @@ -3093,6 +3093,7 @@ static void multi_ap_process_assoc_resp(struct wpa_supplicant *wpa_s, { struct ieee802_11_elems elems; struct multi_ap_params multi_ap; + struct wpa_ssid *ssid = wpa_s->current_ssid; u16 status; wpa_s->multi_ap_ie = 0; @@ -3112,6 +3113,17 @@ static void multi_ap_process_assoc_resp(struct wpa_supplicant *wpa_s, wpa_s->multi_ap_fronthaul = !!(multi_ap.capability & MULTI_AP_FRONTHAUL_BSS); wpa_s->multi_ap_ie = 1; + + if (!ssid) + return; + + ssid->multi_ap_primary_vlanid = 0; + + if (wpa_s->multi_ap_backhaul) { + ssid->multi_ap_profile = multi_ap.profile; + if (ssid->multi_ap_backhaul_sta) + ssid->multi_ap_primary_vlanid = multi_ap.vlanid; + } } diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c index 81858327b..c29fb562c 100644 --- a/wpa_supplicant/wpa_supplicant.c +++ b/wpa_supplicant/wpa_supplicant.c @@ -1114,11 +1114,14 @@ void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s, #if defined(CONFIG_CTRL_IFACE) || !defined(CONFIG_NO_STDOUT_DEBUG) wpa_msg(wpa_s, MSG_INFO, WPA_EVENT_CONNECTED "- Connection to " - MACSTR " completed [id=%d id_str=%s%s]%s", + MACSTR " completed [id=%d id_str=%s%s]%s" + " multi_ap_profile=%d multi_ap_primary_vlanid=%d", 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" : "", mld_addr, + ssid ? ssid->multi_ap_profile : 0, + ssid ? ssid->multi_ap_primary_vlanid : 0); #endif /* CONFIG_CTRL_IFACE || !CONFIG_NO_STDOUT_DEBUG */ wpas_clear_temp_disabled(wpa_s, ssid, 1); wpa_s->consecutive_conn_failures = 0;
For Multi-AP traffic separation requirement, wpa_supplicant parses 802.1Q Multi-AP sub-element and reports: - VLAN ID of the AP it connects to. - Multi-AP profile of the AP it connects to. Signed-off-by: Jurijs Soloveckis <jsoloveckis@maxlinear.com> --- wpa_supplicant/config_ssid.h | 7 +++++++ wpa_supplicant/events.c | 12 ++++++++++++ wpa_supplicant/wpa_supplicant.c | 7 +++++-- 3 files changed, 24 insertions(+), 2 deletions(-)