| Message ID | 20240306173947.2611965-2-quic_adisi@quicinc.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hostapd: scale up MLO flows | expand |
Hi, On Wed, 2024-03-06 at 23:09 +0530, Aditya Kumar Singh wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > Currently mld_id is provided as a user configuration to > identify partner bss belonging to the same MLD. The same > id is used at protocol level also to indicate the MLD ID > of the MLD. > > But, in general mld_id is a relative reference of the MLD > where 0 is used as the mld_id to represent the self MLD and > in case of MLO MBSSID mld_id of a non transmitted BSS affiliated > to a MLD is based on the relative bss index of the non transmitted > BSS from the transmitted BSS. Hence mld_id need not be fetched > from users, rather it can be identified wherever required. > > To verify if the partners are belonging to the same MLD the > interface name can be checked, since all link BSS partners > of the same MLD belongs to the same interface. > > Hence, remove usage of mld_id user config and instead introduce two > functions hostapd_is_ml_partner() and hostapd_get_mld_id(). The > former is used to verfiy if partneres belong to same MLD or not and > the later is used the get the MLD ID of the bss. Yeah, that was horribly broken, and this patch fixes it up a lot. Thanks for that! That said, I am wondering if we everything is right yet after the patch, see inline comments. > Signed-off-by: Sriram R <quic_srirrama@quicinc.com> > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > --- > hostapd/config_file.c | 2 -- > hostapd/ctrl_iface.c | 6 +++--- > hostapd/hostapd.conf | 3 --- > hostapd/main.c | 10 ++++++++-- > src/ap/beacon.c | 5 ++--- > src/ap/ctrl_iface_ap.c | 2 +- > src/ap/drv_callbacks.c | 2 +- > src/ap/hostapd.c | 24 +++++++++++++++++++++--- > src/ap/hostapd.h | 6 +++++- > src/ap/ieee802_11.c | 16 +++++++--------- > src/ap/ieee802_11_eht.c | 6 +++--- > src/ap/ieee802_1x.c | 2 +- > src/ap/sta_info.c | 4 ++-- > src/ap/wpa_auth_glue.c | 4 ++-- > 14 files changed, 56 insertions(+), 36 deletions(-) > > diff --git a/hostapd/config_file.c b/hostapd/config_file.c > index d30f2fe9f272..55ec0b2bcb81 100644 > --- a/hostapd/config_file.c > +++ b/hostapd/config_file.c > @@ -5014,8 +5014,6 @@ static int hostapd_config_fill(struct > hostapd_config *conf, > conf->punct_acs_threshold = val; > } else if (os_strcmp(buf, "mld_ap") == 0) { > bss->mld_ap = !!atoi(pos); > - } else if (os_strcmp(buf, "mld_id") == 0) { > - bss->mld_id = atoi(pos); > } else if (os_strcmp(buf, "mld_addr") == 0) { > if (hwaddr_aton(pos, bss->mld_addr)) { > wpa_printf(MSG_ERROR, "Line %d: Invalid > mld_addr", > diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c > index 8a12fee886db..6f2b31eaf6be 100644 > --- a/hostapd/ctrl_iface.c > +++ b/hostapd/ctrl_iface.c > @@ -3479,7 +3479,7 @@ static int hostapd_ctrl_iface_enable_mld(struct > hostapd_iface *iface) > struct hostapd_bss_config *h_conf = h_hapd->conf; > > if (!h_conf->mld_ap || > - h_conf->mld_id != iface->bss[0]->conf->mld_id) > + !hostapd_is_ml_partner(h_hapd, iface->bss[0])) > continue; > > if (hostapd_enable_iface(h_iface)) { > @@ -3520,7 +3520,7 @@ static int > hostapd_ctrl_iface_disable_mld(struct hostapd_iface *iface) > struct hostapd_bss_config *h_conf = h_hapd->conf; > > if (!h_conf->mld_ap || > - h_conf->mld_id != iface->bss[0]->conf->mld_id) > + !hostapd_is_ml_partner(h_hapd, iface->bss[0])) > continue; > > if (!h_hapd->mld_first_bss) { > @@ -3541,7 +3541,7 @@ static int > hostapd_ctrl_iface_disable_mld(struct hostapd_iface *iface) > struct hostapd_bss_config *h_conf = h_hapd->conf; > > if (!h_conf->mld_ap || > - h_conf->mld_id != iface->bss[0]->conf->mld_id || > + !hostapd_is_ml_partner(h_hapd, iface->bss[0]) || > !h_hapd->mld_first_bss) > continue; > > diff --git a/hostapd/hostapd.conf b/hostapd/hostapd.conf > index f8b4f9c65957..e855ca45792c 100644 > --- a/hostapd/hostapd.conf > +++ b/hostapd/hostapd.conf > @@ -1071,9 +1071,6 @@ wmm_ac_vo_acm=0 > # 1 = yes (MLO) > #mld_ap=0 > > -# MLD ID - Affiliated MLD ID > -#mld_id=1 > - > # AP MLD MAC address > # The configured address will be set as the interface hardware > address and used > # as the AP MLD MAC address. If not set, the current interface > hardware address > diff --git a/hostapd/main.c b/hostapd/main.c > index c95cf8a71e3d..a7610b8a54f5 100644 > --- a/hostapd/main.c > +++ b/hostapd/main.c > @@ -175,9 +175,15 @@ static int hostapd_driver_init(struct > hostapd_iface *iface) > continue; > } > > - if (!hconf->mld_ap || hconf->mld_id != conf->mld_id) > { > + if (!hconf->mld_ap) { > wpa_printf(MSG_DEBUG, > - "MLD: Skip non matching mld_id"); > + "MLD: Skip non MLD"); > + continue; > + } > + > + if (!hostapd_is_ml_partner(hapd, h_hapd)) { > + wpa_printf(MSG_DEBUG, > + "MLD: Skip non matching mld vif > name"); > continue; > } > > diff --git a/src/ap/beacon.c b/src/ap/beacon.c > index e50f0a0c976e..9c028b5b72d6 100644 > --- a/src/ap/beacon.c > +++ b/src/ap/beacon.c > @@ -960,7 +960,7 @@ static void > hostapd_fill_probe_resp_ml_params(struct hostapd_data *hapd, > * We want to include the AP MLD ID in the response if it > was > * included in the request. > */ > - probed_mld_id = mld_id != -1 ? mld_id : hapd->conf->mld_id; > + probed_mld_id = mld_id != -1 ? mld_id : > hostapd_get_mld_id(hapd); We are not (yet) hitting these code paths. But, I think conceptually it would make more sense to get the probed hapd from the MLD ID. i.e. something like: probed_hapd = mld_id == 0 ? hapd : hostapd_get_mbssid_hapd(hapd, mld_id); > for_each_mld_link(link, i, j, hapd->iface->interfaces, > probed_mld_id) { Then we need to change the iteration, but I think that makes sense anyway, see below. > @@ -2675,8 +2675,7 @@ int ieee802_11_set_beacon(struct hostapd_data > *hapd) > continue; > > #ifdef CONFIG_IEEE80211BE > - if (hapd->conf->mld_ap && other->bss[0]->conf- > >mld_ap && > - hapd->conf->mld_id == other->bss[0]->conf- > >mld_id) > + if (hostapd_is_ml_partner(hapd, other->bss[0])) > mld_ap = true; > #endif /* CONFIG_IEEE80211BE */ > > diff --git a/src/ap/ctrl_iface_ap.c b/src/ap/ctrl_iface_ap.c > index e65c3c4b2749..cdd777287361 100644 > --- a/src/ap/ctrl_iface_ap.c > +++ b/src/ap/ctrl_iface_ap.c > @@ -1013,7 +1013,7 @@ int hostapd_ctrl_iface_status(struct > hostapd_data *hapd, char *buf, > "mld_id[%d]=%d\n" > "mld_link_id[%d]=%d\n", > (int) i, MAC2STR(bss- > >mld_addr), > - (int) i, bss->conf- > >mld_id, > + (int) i, > hostapd_get_mld_id(bss), > (int) i, bss- > >mld_link_id); > if (os_snprintf_error(buflen - len, ret)) > return len; > diff --git a/src/ap/drv_callbacks.c b/src/ap/drv_callbacks.c > index 814901977057..3b89c700d33f 100644 > --- a/src/ap/drv_callbacks.c > +++ b/src/ap/drv_callbacks.c > @@ -1965,7 +1965,7 @@ static bool search_mld_sta(struct hostapd_data > **p_hapd, const u8 *src) > struct hostapd_bss_config *hconf = h_hapd->conf; > > if (!hconf->mld_ap || > - hconf->mld_id != hapd->conf->mld_id) > + !hostapd_is_ml_partner(h_hapd, hapd)) > continue; > > h_hapd = hostapd_find_by_sta(h, src, false); > diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c > index 0def1b918a01..adb5b001a784 100644 > --- a/src/ap/hostapd.c > +++ b/src/ap/hostapd.c > @@ -3220,8 +3220,7 @@ int hostapd_disable_iface(struct hostapd_iface > *hapd_iface) > struct hostapd_bss_config *h_conf = h_hapd- > >conf; > > if (!h_conf->mld_ap || > - h_conf->mld_id != > - hapd_iface->bss[0]->conf->mld_id || > + !hostapd_is_ml_partner(h_hapd, > hapd_iface->bss[0]) || > h_iface == hapd_iface) > continue; > > @@ -4415,7 +4414,7 @@ struct hostapd_data * > hostapd_mld_get_link_bss(struct hostapd_data *hapd, > struct hostapd_data *h_hapd = h->bss[0]; > struct hostapd_bss_config *hconf = h_hapd->conf; > > - if (!hconf->mld_ap || hconf->mld_id != hapd->conf- > >mld_id) > + if (!hconf->mld_ap || !hostapd_is_ml_partner(hapd, > h_hapd)) > continue; > > if (h_hapd->mld_link_id == link_id) > @@ -4424,4 +4423,23 @@ struct hostapd_data * > hostapd_mld_get_link_bss(struct hostapd_data *hapd, > > return NULL; > } > + > +bool hostapd_is_ml_partner(struct hostapd_data *hapd1, struct > hostapd_data *hapd2) > +{ > + if (!hapd1->conf->mld_ap || !hapd2->conf->mld_ap) > + return false; > + > + return !(os_strcmp(hapd1->conf->iface, hapd2->conf->iface)); > +} > + > +u8 hostapd_get_mld_id(struct hostapd_data *hapd) > +{ > + if (!hapd->conf->mld_ap) > + return 255; > + > + /* MLD ID 0 represents self */ > + return 0; > + > + /* TODO MLD ID for MBSS cases */ > +} > #endif /* CONFIG_IEEE80211BE */ > diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h > index 22540bb371c1..4c84fb8f56d9 100644 > --- a/src/ap/hostapd.h > +++ b/src/ap/hostapd.h > @@ -783,6 +783,10 @@ struct hostapd_data * > hostapd_mld_get_link_bss(struct hostapd_data *hapd, > int hostapd_link_remove(struct hostapd_data *hapd, u32 count); > > #ifdef CONFIG_IEEE80211BE > +u8 hostapd_get_mld_id(struct hostapd_data *hapd); > +bool hostapd_is_ml_partner(struct hostapd_data *hapd1, > + struct hostapd_data *hapd2); > + > #define for_each_mld_link(_link, _bss_idx, _iface_idx, _ifaces, > _mld_id) \ > for (_iface_idx = > 0; \ > _iface_idx < (_ifaces)- > >count; \ > @@ -794,7 +798,7 @@ int hostapd_link_remove(struct hostapd_data > *hapd, u32 count); > for (_link > = \ > (_ifaces)->iface[_iface_idx]- > >bss[_bss_idx]; \ > _link && _link->conf->mld_ap > && \ > - _link->conf->mld_id == > _mld_id; \ > + hostapd_get_mld_id(_link) == > _mld_id; \ > _link = NULL) I think this part is slightly wrong, as the mld_id here would be local to the link, but it should to be local to the hapd from which we started the iteration. Maybe we can update the macro and simplify it a bit at the same time. We always have an hapd instance anyway, so passing the interfaces explicitly does not really make sense. So, we can just start with an hapd instance, and iterate all links (including itself). Something like: #define for_each_mld_link(_hapd, _link, _bss_idx, _iface_idx) \ for (_iface_idx = 0; \ _iface_idx < (_hapd)->iface->interfaces)->count; \ _iface_idx++) \ for (_bss_idx = 0; \ _bss_idx < \ (_ifaces)->iface[_iface_idx]->num_bss; \ _bss_idx++) \ for (_link = \ (_hapd)->iface->interfaces-> \ iface[_iface_idx]->bss[_bss_idx]; \ _link && \ hostapd_is_ml_partner(_link, _hapd); \ _link = NULL) Benjamin > #else /* CONFIG_IEEE80211BE */ > #define for_each_mld_link(_link, _bss_idx, _iface_idx, _ifaces, > _mld_id) \ > diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c > index 41508ff509c0..22f3841752e1 100644 > --- a/src/ap/ieee802_11.c > +++ b/src/ap/ieee802_11.c > @@ -4565,7 +4565,7 @@ int hostapd_process_assoc_ml_info(struct > hostapd_data *hapd, > continue; > > if (iface->bss[0]->conf->mld_ap && > - hapd->conf->mld_id == iface->bss[0]- > >conf->mld_id && > + hostapd_is_ml_partner(hapd, iface- > >bss[0]) && > i == iface->bss[0]->mld_link_id) > break; > } > @@ -5781,7 +5781,7 @@ static bool hostapd_ml_handle_disconnect(struct > hostapd_data *hapd, > assoc_hapd->iface->interfaces- > >iface[i]->bss[0]; > > if (!tmp_hapd->conf->mld_ap || > - assoc_hapd->conf->mld_id != tmp_hapd- > >conf->mld_id) > + !hostapd_is_ml_partner(assoc_hapd, > tmp_hapd)) > continue; > > for (tmp_sta = tmp_hapd->sta_list; tmp_sta; > @@ -6449,7 +6449,7 @@ static void hostapd_ml_handle_assoc_cb(struct > hostapd_data *hapd, > hapd->iface->interfaces->iface[i]- > >bss[0]; > > if (!tmp_hapd->conf->mld_ap || > - hapd->conf->mld_id != tmp_hapd->conf- > >mld_id) > + !hostapd_is_ml_partner(tmp_hapd, hapd)) > continue; > > for (tmp_sta = tmp_hapd->sta_list; tmp_sta; > @@ -7412,8 +7412,7 @@ static size_t > hostapd_eid_rnr_multi_iface_len(struct hostapd_data *hapd, > bool ap_mld = false; > > #ifdef CONFIG_IEEE80211BE > - if (hapd->conf->mld_ap && iface->bss[0]->conf- > >mld_ap && > - hapd->conf->mld_id == iface->bss[0]->conf- > >mld_id) > + if (hostapd_is_ml_partner(hapd, iface->bss[0])) > ap_mld = true; > #endif /* CONFIG_IEEE80211BE */ > > @@ -7586,10 +7585,10 @@ static bool hostapd_eid_rnr_bss(struct > hostapd_data *hapd, > u8 param_ch = hapd->eht_mld_bss_param_change; > > if (reporting_hapd->conf->mld_ap && > - bss->conf->mld_id == reporting_hapd->conf- > >mld_id) > + hostapd_is_ml_partner(bss, reporting_hapd)) > *eid++ = 0; > else > - *eid++ = hapd->conf->mld_id; > + *eid++ = hostapd_get_mld_id(hapd); > > *eid++ = hapd->mld_link_id | ((param_ch & 0xF) << > 4); > *eid = (param_ch >> 4) & 0xF; > @@ -7687,8 +7686,7 @@ static u8 * hostapd_eid_rnr_multi_iface(struct > hostapd_data *hapd, u8 *eid, > bool ap_mld = false; > > #ifdef CONFIG_IEEE80211BE > - if (hapd->conf->mld_ap && iface->bss[0]->conf- > >mld_ap && > - hapd->conf->mld_id == iface->bss[0]->conf- > >mld_id) > + if (hostapd_is_ml_partner(hapd, iface->bss[0])) > ap_mld = true; > #endif /* CONFIG_IEEE80211BE */ > > diff --git a/src/ap/ieee802_11_eht.c b/src/ap/ieee802_11_eht.c > index b599e10433ff..de5855ebe352 100644 > --- a/src/ap/ieee802_11_eht.c > +++ b/src/ap/ieee802_11_eht.c > @@ -513,8 +513,8 @@ static u8 * > hostapd_eid_eht_basic_ml_common(struct hostapd_data *hapd, > > if (include_mld_id) { > wpa_printf(MSG_DEBUG, "MLD: AP MLD ID=0x%x", > - hapd->conf->mld_id); > - wpabuf_put_u8(buf, hapd->conf->mld_id); > + hostapd_get_mld_id(hapd)); > + wpabuf_put_u8(buf, hostapd_get_mld_id(hapd)); > } > > if (!mld_info) > @@ -1048,7 +1048,7 @@ static int > hostapd_mld_validate_assoc_info(struct hostapd_data *hapd, > continue; > > if (other_hapd->conf->mld_ap && > - other_hapd->conf->mld_id == hapd->conf- > >mld_id && > + hostapd_is_ml_partner(hapd, other_hapd) > && > link_id == other_hapd->mld_link_id) > break; > } > diff --git a/src/ap/ieee802_1x.c b/src/ap/ieee802_1x.c > index f13c60a9ee1a..42eb5a173e45 100644 > --- a/src/ap/ieee802_1x.c > +++ b/src/ap/ieee802_1x.c > @@ -173,7 +173,7 @@ static void > ieee802_1x_ml_set_sta_authorized(struct hostapd_data *hapd, > hapd->iface->interfaces->iface[i]- > >bss[0]; > > if (!tmp_hapd->conf->mld_ap || > - hapd->conf->mld_id != tmp_hapd->conf- > >mld_id) > + !hostapd_is_ml_partner(hapd, tmp_hapd)) > continue; > > for (tmp_sta = tmp_hapd->sta_list; tmp_sta; > diff --git a/src/ap/sta_info.c b/src/ap/sta_info.c > index dc5e3b419034..97ed805ca5c2 100644 > --- a/src/ap/sta_info.c > +++ b/src/ap/sta_info.c > @@ -979,7 +979,7 @@ static bool ap_sta_ml_disconnect(struct > hostapd_data *hapd, > tmp_hapd = interfaces->iface[i]->bss[0]; > > if (!tmp_hapd->conf->mld_ap || > - assoc_hapd->conf->mld_id != tmp_hapd- > >conf->mld_id) > + !hostapd_is_ml_partner(tmp_hapd, > assoc_hapd)) > continue; > > for (tmp_sta = tmp_hapd->sta_list; tmp_sta; > @@ -1731,7 +1731,7 @@ static void ap_sta_remove_link_sta(struct > hostapd_data *hapd, > unsigned int i, j; > > for_each_mld_link(tmp_hapd, i, j, hapd->iface->interfaces, > - hapd->conf->mld_id) { > + hostapd_get_mld_id(hapd)) { > struct sta_info *tmp_sta; > > if (hapd == tmp_hapd) > diff --git a/src/ap/wpa_auth_glue.c b/src/ap/wpa_auth_glue.c > index 9ce58f24611a..d23f218351a6 100644 > --- a/src/ap/wpa_auth_glue.c > +++ b/src/ap/wpa_auth_glue.c > @@ -1557,7 +1557,7 @@ static int > hostapd_wpa_auth_get_ml_rsn_info(void *ctx, > hapd->iface->interfaces->iface[j]; > > if (!iface->bss[0]->conf->mld_ap || > - hapd->conf->mld_id != iface->bss[0]- > >conf->mld_id || > + !hostapd_is_ml_partner(hapd, iface- > >bss[0]) || > link_id != iface->bss[0]->mld_link_id || > !iface->bss[0]->wpa_auth) > continue; > @@ -1600,7 +1600,7 @@ static int > hostapd_wpa_auth_get_ml_key_info(void *ctx, > hapd->iface->interfaces->iface[j]; > > if (!iface->bss[0]->conf->mld_ap || > - hapd->conf->mld_id != iface->bss[0]- > >conf->mld_id || > + !hostapd_is_ml_partner(hapd, iface- > >bss[0]) || > link_id != iface->bss[0]->mld_link_id || > !iface->bss[0]->wpa_auth) > continue;
On 3/18/24 16:23, Benjamin Berg wrote: ... >> diff --git a/src/ap/beacon.c b/src/ap/beacon.c >> index e50f0a0c976e..9c028b5b72d6 100644 >> --- a/src/ap/beacon.c >> +++ b/src/ap/beacon.c >> @@ -960,7 +960,7 @@ static void >> hostapd_fill_probe_resp_ml_params(struct hostapd_data *hapd, >> * We want to include the AP MLD ID in the response if it >> was >> * included in the request. >> */ >> - probed_mld_id = mld_id != -1 ? mld_id : hapd->conf->mld_id; >> + probed_mld_id = mld_id != -1 ? mld_id : >> hostapd_get_mld_id(hapd); > > We are not (yet) hitting these code paths. But, I think conceptually it > would make more sense to get the probed hapd from the MLD ID. i.e. > something like: > probed_hapd = mld_id == 0 ? hapd : hostapd_get_mbssid_hapd(hapd, mld_id); > Yes correct. But fixes regarding MLO+MBSSID is in later part of our work. As you rightly said, this we are not hitting as of today. Also, this patch series tries to just support 1 MLD interface (as it was doing already before this) and at the same time just scale up certain structures/handling so that it can be leveraged when support for multiple MLD interfaces (co-hosted) is added later (this I'm working on right now and will probably send out series soon :)). So at that time this would be fixed properly. >> for_each_mld_link(link, i, j, hapd->iface->interfaces, >> probed_mld_id) { > > Then we need to change the iteration, but I think that makes sense > anyway, see below. > ... \ >> @@ -794,7 +798,7 @@ int hostapd_link_remove(struct hostapd_data >> *hapd, u32 count); >> for (_link >> = \ >> (_ifaces)->iface[_iface_idx]- >>> bss[_bss_idx]; \ >> _link && _link->conf->mld_ap >> && \ >> - _link->conf->mld_id == >> _mld_id; \ >> + hostapd_get_mld_id(_link) == >> _mld_id; \ >> _link = NULL) > > I think this part is slightly wrong, as the mld_id here would be local > to the link, but it should to be local to the hapd from which we > started the iteration. > > Maybe we can update the macro and simplify it a bit at the same time. > We always have an hapd instance anyway, so passing the interfaces > explicitly does not really make sense. So, we can just start with an > hapd instance, and iterate all links (including itself). Something > like: > > #define for_each_mld_link(_hapd, _link, _bss_idx, _iface_idx) \ > for (_iface_idx = 0; \ > _iface_idx < (_hapd)->iface->interfaces)->count; \ > _iface_idx++) \ > for (_bss_idx = 0; \ > _bss_idx < \ > (_ifaces)->iface[_iface_idx]->num_bss; \ > _bss_idx++) \ > for (_link = \ > (_hapd)->iface->interfaces-> \ > iface[_iface_idx]->bss[_bss_idx]; \ > _link && \ > hostapd_is_ml_partner(_link, _hapd); \ > _link = NULL) > > Benjamin Yeah that makes sense. Thanks for the input! But as I said above, in the next series, actually this logic will be replaced with another one. Hence did not change much here. Now (after this series) since we have all affiliated links tied up to a common MLD structure via a list member, there is no need of these many arguments for this macro. We can just use dl_list_for_each() and iterate over all affiliated links.
Hi, On Mon, 2024-03-18 at 17:49 +0530, Aditya Kumar Singh wrote: > On 3/18/24 16:23, Benjamin Berg wrote: > ... > > > diff --git a/src/ap/beacon.c b/src/ap/beacon.c > > > index e50f0a0c976e..9c028b5b72d6 100644 > > > --- a/src/ap/beacon.c > > > +++ b/src/ap/beacon.c > > > @@ -960,7 +960,7 @@ static void > > > hostapd_fill_probe_resp_ml_params(struct hostapd_data *hapd, > > > * We want to include the AP MLD ID in the response if > > > it > > > was > > > * included in the request. > > > */ > > > - probed_mld_id = mld_id != -1 ? mld_id : hapd->conf- > > > >mld_id; > > > + probed_mld_id = mld_id != -1 ? mld_id : > > > hostapd_get_mld_id(hapd); > > > > We are not (yet) hitting these code paths. But, I think > > conceptually it > > would make more sense to get the probed hapd from the MLD ID. i.e. > > something like: > > probed_hapd = mld_id == 0 ? hapd : > > hostapd_get_mbssid_hapd(hapd, mld_id); > > > > Yes correct. But fixes regarding MLO+MBSSID is in later part of our > work. As you rightly said, this we are not hitting as of today. Also, > this patch series tries to just support 1 MLD interface (as it was doing > already before this) and at the same time just scale up certain > structures/handling so that it can be leveraged when support for > multiple MLD interfaces (co-hosted) is added later (this I'm working on > right now and will probably send out series soon :)). So at that time > this would be fixed properly. > > > > for_each_mld_link(link, i, j, hapd->iface->interfaces, > > > probed_mld_id) { > > > > Then we need to change the iteration, but I think that makes sense > > anyway, see below. > > > > ... \ > > > @@ -794,7 +798,7 @@ int hostapd_link_remove(struct hostapd_data > > > *hapd, u32 count); > > > for (_link > > > = \ > > > (_ifaces)->iface[_iface_idx]- > > > > bss[_bss_idx]; \ > > > _link && _link->conf->mld_ap > > > && \ > > > - _link->conf->mld_id == > > > _mld_id; \ > > > + hostapd_get_mld_id(_link) == > > > _mld_id; \ > > > _link = NULL) > > > > I think this part is slightly wrong, as the mld_id here would be > > local > > to the link, but it should to be local to the hapd from which we > > started the iteration. > > > > Maybe we can update the macro and simplify it a bit at the same > > time. > > We always have an hapd instance anyway, so passing the interfaces > > explicitly does not really make sense. So, we can just start with > > an > > hapd instance, and iterate all links (including itself). Something > > like: > > > > #define for_each_mld_link(_hapd, _link, _bss_idx, > > _iface_idx) \ > > for (_iface_idx = > > 0; \ > > _iface_idx < (_hapd)->iface->interfaces)- > > >count; \ > > > > _iface_idx++) \ > > for (_bss_idx = > > 0; \ > > _bss_idx > > < \ > > (_ifaces)->iface[_iface_idx]- > > >num_bss; \ > > > > _bss_idx++) \ > > for (_link > > = \ > > (_hapd)->iface->interfaces- > > > \ > > iface[_iface_idx]- > > >bss[_bss_idx]; \ > > _link > > && \ > > hostapd_is_ml_partner(_link, > > _hapd); \ > > _link = NULL) > > > > Benjamin > > Yeah that makes sense. Thanks for the input! But as I said above, in the > next series, actually this logic will be replaced with another one. > Hence did not change much here. > > Now (after this series) since we have all affiliated links tied up to a > common MLD structure via a list member, there is no need of these many > arguments for this macro. We can just use dl_list_for_each() and iterate > over all affiliated links. Yeah, it'll become much nicer. I had not realized you added struct hostapd_mld when I wrote the mail. So, with that, I am happy with it as-is :-) Benjamin
diff --git a/hostapd/config_file.c b/hostapd/config_file.c index d30f2fe9f272..55ec0b2bcb81 100644 --- a/hostapd/config_file.c +++ b/hostapd/config_file.c @@ -5014,8 +5014,6 @@ static int hostapd_config_fill(struct hostapd_config *conf, conf->punct_acs_threshold = val; } else if (os_strcmp(buf, "mld_ap") == 0) { bss->mld_ap = !!atoi(pos); - } else if (os_strcmp(buf, "mld_id") == 0) { - bss->mld_id = atoi(pos); } else if (os_strcmp(buf, "mld_addr") == 0) { if (hwaddr_aton(pos, bss->mld_addr)) { wpa_printf(MSG_ERROR, "Line %d: Invalid mld_addr", diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c index 8a12fee886db..6f2b31eaf6be 100644 --- a/hostapd/ctrl_iface.c +++ b/hostapd/ctrl_iface.c @@ -3479,7 +3479,7 @@ static int hostapd_ctrl_iface_enable_mld(struct hostapd_iface *iface) struct hostapd_bss_config *h_conf = h_hapd->conf; if (!h_conf->mld_ap || - h_conf->mld_id != iface->bss[0]->conf->mld_id) + !hostapd_is_ml_partner(h_hapd, iface->bss[0])) continue; if (hostapd_enable_iface(h_iface)) { @@ -3520,7 +3520,7 @@ static int hostapd_ctrl_iface_disable_mld(struct hostapd_iface *iface) struct hostapd_bss_config *h_conf = h_hapd->conf; if (!h_conf->mld_ap || - h_conf->mld_id != iface->bss[0]->conf->mld_id) + !hostapd_is_ml_partner(h_hapd, iface->bss[0])) continue; if (!h_hapd->mld_first_bss) { @@ -3541,7 +3541,7 @@ static int hostapd_ctrl_iface_disable_mld(struct hostapd_iface *iface) struct hostapd_bss_config *h_conf = h_hapd->conf; if (!h_conf->mld_ap || - h_conf->mld_id != iface->bss[0]->conf->mld_id || + !hostapd_is_ml_partner(h_hapd, iface->bss[0]) || !h_hapd->mld_first_bss) continue; diff --git a/hostapd/hostapd.conf b/hostapd/hostapd.conf index f8b4f9c65957..e855ca45792c 100644 --- a/hostapd/hostapd.conf +++ b/hostapd/hostapd.conf @@ -1071,9 +1071,6 @@ wmm_ac_vo_acm=0 # 1 = yes (MLO) #mld_ap=0 -# MLD ID - Affiliated MLD ID -#mld_id=1 - # AP MLD MAC address # The configured address will be set as the interface hardware address and used # as the AP MLD MAC address. If not set, the current interface hardware address diff --git a/hostapd/main.c b/hostapd/main.c index c95cf8a71e3d..a7610b8a54f5 100644 --- a/hostapd/main.c +++ b/hostapd/main.c @@ -175,9 +175,15 @@ static int hostapd_driver_init(struct hostapd_iface *iface) continue; } - if (!hconf->mld_ap || hconf->mld_id != conf->mld_id) { + if (!hconf->mld_ap) { wpa_printf(MSG_DEBUG, - "MLD: Skip non matching mld_id"); + "MLD: Skip non MLD"); + continue; + } + + if (!hostapd_is_ml_partner(hapd, h_hapd)) { + wpa_printf(MSG_DEBUG, + "MLD: Skip non matching mld vif name"); continue; } diff --git a/src/ap/beacon.c b/src/ap/beacon.c index e50f0a0c976e..9c028b5b72d6 100644 --- a/src/ap/beacon.c +++ b/src/ap/beacon.c @@ -960,7 +960,7 @@ static void hostapd_fill_probe_resp_ml_params(struct hostapd_data *hapd, * We want to include the AP MLD ID in the response if it was * included in the request. */ - probed_mld_id = mld_id != -1 ? mld_id : hapd->conf->mld_id; + probed_mld_id = mld_id != -1 ? mld_id : hostapd_get_mld_id(hapd); for_each_mld_link(link, i, j, hapd->iface->interfaces, probed_mld_id) { @@ -2675,8 +2675,7 @@ int ieee802_11_set_beacon(struct hostapd_data *hapd) continue; #ifdef CONFIG_IEEE80211BE - if (hapd->conf->mld_ap && other->bss[0]->conf->mld_ap && - hapd->conf->mld_id == other->bss[0]->conf->mld_id) + if (hostapd_is_ml_partner(hapd, other->bss[0])) mld_ap = true; #endif /* CONFIG_IEEE80211BE */ diff --git a/src/ap/ctrl_iface_ap.c b/src/ap/ctrl_iface_ap.c index e65c3c4b2749..cdd777287361 100644 --- a/src/ap/ctrl_iface_ap.c +++ b/src/ap/ctrl_iface_ap.c @@ -1013,7 +1013,7 @@ int hostapd_ctrl_iface_status(struct hostapd_data *hapd, char *buf, "mld_id[%d]=%d\n" "mld_link_id[%d]=%d\n", (int) i, MAC2STR(bss->mld_addr), - (int) i, bss->conf->mld_id, + (int) i, hostapd_get_mld_id(bss), (int) i, bss->mld_link_id); if (os_snprintf_error(buflen - len, ret)) return len; diff --git a/src/ap/drv_callbacks.c b/src/ap/drv_callbacks.c index 814901977057..3b89c700d33f 100644 --- a/src/ap/drv_callbacks.c +++ b/src/ap/drv_callbacks.c @@ -1965,7 +1965,7 @@ static bool search_mld_sta(struct hostapd_data **p_hapd, const u8 *src) struct hostapd_bss_config *hconf = h_hapd->conf; if (!hconf->mld_ap || - hconf->mld_id != hapd->conf->mld_id) + !hostapd_is_ml_partner(h_hapd, hapd)) continue; h_hapd = hostapd_find_by_sta(h, src, false); diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c index 0def1b918a01..adb5b001a784 100644 --- a/src/ap/hostapd.c +++ b/src/ap/hostapd.c @@ -3220,8 +3220,7 @@ int hostapd_disable_iface(struct hostapd_iface *hapd_iface) struct hostapd_bss_config *h_conf = h_hapd->conf; if (!h_conf->mld_ap || - h_conf->mld_id != - hapd_iface->bss[0]->conf->mld_id || + !hostapd_is_ml_partner(h_hapd, hapd_iface->bss[0]) || h_iface == hapd_iface) continue; @@ -4415,7 +4414,7 @@ struct hostapd_data * hostapd_mld_get_link_bss(struct hostapd_data *hapd, struct hostapd_data *h_hapd = h->bss[0]; struct hostapd_bss_config *hconf = h_hapd->conf; - if (!hconf->mld_ap || hconf->mld_id != hapd->conf->mld_id) + if (!hconf->mld_ap || !hostapd_is_ml_partner(hapd, h_hapd)) continue; if (h_hapd->mld_link_id == link_id) @@ -4424,4 +4423,23 @@ struct hostapd_data * hostapd_mld_get_link_bss(struct hostapd_data *hapd, return NULL; } + +bool hostapd_is_ml_partner(struct hostapd_data *hapd1, struct hostapd_data *hapd2) +{ + if (!hapd1->conf->mld_ap || !hapd2->conf->mld_ap) + return false; + + return !(os_strcmp(hapd1->conf->iface, hapd2->conf->iface)); +} + +u8 hostapd_get_mld_id(struct hostapd_data *hapd) +{ + if (!hapd->conf->mld_ap) + return 255; + + /* MLD ID 0 represents self */ + return 0; + + /* TODO MLD ID for MBSS cases */ +} #endif /* CONFIG_IEEE80211BE */ diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h index 22540bb371c1..4c84fb8f56d9 100644 --- a/src/ap/hostapd.h +++ b/src/ap/hostapd.h @@ -783,6 +783,10 @@ struct hostapd_data * hostapd_mld_get_link_bss(struct hostapd_data *hapd, int hostapd_link_remove(struct hostapd_data *hapd, u32 count); #ifdef CONFIG_IEEE80211BE +u8 hostapd_get_mld_id(struct hostapd_data *hapd); +bool hostapd_is_ml_partner(struct hostapd_data *hapd1, + struct hostapd_data *hapd2); + #define for_each_mld_link(_link, _bss_idx, _iface_idx, _ifaces, _mld_id) \ for (_iface_idx = 0; \ _iface_idx < (_ifaces)->count; \ @@ -794,7 +798,7 @@ int hostapd_link_remove(struct hostapd_data *hapd, u32 count); for (_link = \ (_ifaces)->iface[_iface_idx]->bss[_bss_idx]; \ _link && _link->conf->mld_ap && \ - _link->conf->mld_id == _mld_id; \ + hostapd_get_mld_id(_link) == _mld_id; \ _link = NULL) #else /* CONFIG_IEEE80211BE */ #define for_each_mld_link(_link, _bss_idx, _iface_idx, _ifaces, _mld_id) \ diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c index 41508ff509c0..22f3841752e1 100644 --- a/src/ap/ieee802_11.c +++ b/src/ap/ieee802_11.c @@ -4565,7 +4565,7 @@ int hostapd_process_assoc_ml_info(struct hostapd_data *hapd, continue; if (iface->bss[0]->conf->mld_ap && - hapd->conf->mld_id == iface->bss[0]->conf->mld_id && + hostapd_is_ml_partner(hapd, iface->bss[0]) && i == iface->bss[0]->mld_link_id) break; } @@ -5781,7 +5781,7 @@ static bool hostapd_ml_handle_disconnect(struct hostapd_data *hapd, assoc_hapd->iface->interfaces->iface[i]->bss[0]; if (!tmp_hapd->conf->mld_ap || - assoc_hapd->conf->mld_id != tmp_hapd->conf->mld_id) + !hostapd_is_ml_partner(assoc_hapd, tmp_hapd)) continue; for (tmp_sta = tmp_hapd->sta_list; tmp_sta; @@ -6449,7 +6449,7 @@ static void hostapd_ml_handle_assoc_cb(struct hostapd_data *hapd, hapd->iface->interfaces->iface[i]->bss[0]; if (!tmp_hapd->conf->mld_ap || - hapd->conf->mld_id != tmp_hapd->conf->mld_id) + !hostapd_is_ml_partner(tmp_hapd, hapd)) continue; for (tmp_sta = tmp_hapd->sta_list; tmp_sta; @@ -7412,8 +7412,7 @@ static size_t hostapd_eid_rnr_multi_iface_len(struct hostapd_data *hapd, bool ap_mld = false; #ifdef CONFIG_IEEE80211BE - if (hapd->conf->mld_ap && iface->bss[0]->conf->mld_ap && - hapd->conf->mld_id == iface->bss[0]->conf->mld_id) + if (hostapd_is_ml_partner(hapd, iface->bss[0])) ap_mld = true; #endif /* CONFIG_IEEE80211BE */ @@ -7586,10 +7585,10 @@ static bool hostapd_eid_rnr_bss(struct hostapd_data *hapd, u8 param_ch = hapd->eht_mld_bss_param_change; if (reporting_hapd->conf->mld_ap && - bss->conf->mld_id == reporting_hapd->conf->mld_id) + hostapd_is_ml_partner(bss, reporting_hapd)) *eid++ = 0; else - *eid++ = hapd->conf->mld_id; + *eid++ = hostapd_get_mld_id(hapd); *eid++ = hapd->mld_link_id | ((param_ch & 0xF) << 4); *eid = (param_ch >> 4) & 0xF; @@ -7687,8 +7686,7 @@ static u8 * hostapd_eid_rnr_multi_iface(struct hostapd_data *hapd, u8 *eid, bool ap_mld = false; #ifdef CONFIG_IEEE80211BE - if (hapd->conf->mld_ap && iface->bss[0]->conf->mld_ap && - hapd->conf->mld_id == iface->bss[0]->conf->mld_id) + if (hostapd_is_ml_partner(hapd, iface->bss[0])) ap_mld = true; #endif /* CONFIG_IEEE80211BE */ diff --git a/src/ap/ieee802_11_eht.c b/src/ap/ieee802_11_eht.c index b599e10433ff..de5855ebe352 100644 --- a/src/ap/ieee802_11_eht.c +++ b/src/ap/ieee802_11_eht.c @@ -513,8 +513,8 @@ static u8 * hostapd_eid_eht_basic_ml_common(struct hostapd_data *hapd, if (include_mld_id) { wpa_printf(MSG_DEBUG, "MLD: AP MLD ID=0x%x", - hapd->conf->mld_id); - wpabuf_put_u8(buf, hapd->conf->mld_id); + hostapd_get_mld_id(hapd)); + wpabuf_put_u8(buf, hostapd_get_mld_id(hapd)); } if (!mld_info) @@ -1048,7 +1048,7 @@ static int hostapd_mld_validate_assoc_info(struct hostapd_data *hapd, continue; if (other_hapd->conf->mld_ap && - other_hapd->conf->mld_id == hapd->conf->mld_id && + hostapd_is_ml_partner(hapd, other_hapd) && link_id == other_hapd->mld_link_id) break; } diff --git a/src/ap/ieee802_1x.c b/src/ap/ieee802_1x.c index f13c60a9ee1a..42eb5a173e45 100644 --- a/src/ap/ieee802_1x.c +++ b/src/ap/ieee802_1x.c @@ -173,7 +173,7 @@ static void ieee802_1x_ml_set_sta_authorized(struct hostapd_data *hapd, hapd->iface->interfaces->iface[i]->bss[0]; if (!tmp_hapd->conf->mld_ap || - hapd->conf->mld_id != tmp_hapd->conf->mld_id) + !hostapd_is_ml_partner(hapd, tmp_hapd)) continue; for (tmp_sta = tmp_hapd->sta_list; tmp_sta; diff --git a/src/ap/sta_info.c b/src/ap/sta_info.c index dc5e3b419034..97ed805ca5c2 100644 --- a/src/ap/sta_info.c +++ b/src/ap/sta_info.c @@ -979,7 +979,7 @@ static bool ap_sta_ml_disconnect(struct hostapd_data *hapd, tmp_hapd = interfaces->iface[i]->bss[0]; if (!tmp_hapd->conf->mld_ap || - assoc_hapd->conf->mld_id != tmp_hapd->conf->mld_id) + !hostapd_is_ml_partner(tmp_hapd, assoc_hapd)) continue; for (tmp_sta = tmp_hapd->sta_list; tmp_sta; @@ -1731,7 +1731,7 @@ static void ap_sta_remove_link_sta(struct hostapd_data *hapd, unsigned int i, j; for_each_mld_link(tmp_hapd, i, j, hapd->iface->interfaces, - hapd->conf->mld_id) { + hostapd_get_mld_id(hapd)) { struct sta_info *tmp_sta; if (hapd == tmp_hapd) diff --git a/src/ap/wpa_auth_glue.c b/src/ap/wpa_auth_glue.c index 9ce58f24611a..d23f218351a6 100644 --- a/src/ap/wpa_auth_glue.c +++ b/src/ap/wpa_auth_glue.c @@ -1557,7 +1557,7 @@ static int hostapd_wpa_auth_get_ml_rsn_info(void *ctx, hapd->iface->interfaces->iface[j]; if (!iface->bss[0]->conf->mld_ap || - hapd->conf->mld_id != iface->bss[0]->conf->mld_id || + !hostapd_is_ml_partner(hapd, iface->bss[0]) || link_id != iface->bss[0]->mld_link_id || !iface->bss[0]->wpa_auth) continue; @@ -1600,7 +1600,7 @@ static int hostapd_wpa_auth_get_ml_key_info(void *ctx, hapd->iface->interfaces->iface[j]; if (!iface->bss[0]->conf->mld_ap || - hapd->conf->mld_id != iface->bss[0]->conf->mld_id || + !hostapd_is_ml_partner(hapd, iface->bss[0]) || link_id != iface->bss[0]->mld_link_id || !iface->bss[0]->wpa_auth) continue;