| Message ID | 20240328181652.2956122-6-quic_adisi@quicinc.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [01/22] hostapd: MLO: fix for_each_mld_link macro | expand |
On Thu, Mar 28, 2024 at 11:46:35PM +0530, Aditya Kumar Singh wrote: > Modify authentication and association frames to be always sent with link > address as A1 and A3 for ease of Tx status handling. It would be nice to get a bit more complete commit message that explains why this can be done. Surely there was some reason for the previous more complex design.. > diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c > @@ -444,7 +437,7 @@ static int send_auth_reply(struct hostapd_data *hapd, struct sta_info *sta, > - os_memcpy(reply->bssid, bssid, ETH_ALEN); > + os_memcpy(reply->bssid, sa, ETH_ALEN); This removes the only use of bssid in send_auth_reply(). Is that really correct for all cases? It might be, but again, this needs more justification in the commit message. In addition, this should remove the bssid argument from this function and potentially in a number of calling functions as well since all the would now be unused.
On 4/16/24 13:35, Jouni Malinen wrote: > On Thu, Mar 28, 2024 at 11:46:35PM +0530, Aditya Kumar Singh wrote: >> Modify authentication and association frames to be always sent with link >> address as A1 and A3 for ease of Tx status handling. > > It would be nice to get a bit more complete commit message that explains > why this can be done. Surely there was some reason for the previous more > complex design.. > Okay. So, if we see mlme_event_mgmt_tx_status(), link_id will be only filled if the frame was the last transmitted on the whole drv level. For single MLD this approach is fine. But when considering multiple MLDs, there could be cases where multiple packets are sent out by various interfaces (BSS) under same drv. Now while handling the Tx status, only one interface will get the proper link_id. Rest all will get -1, and event will be routed to first BSS always. Now there is a possibility of frame getting dropped due to this since it is not routed to correct link hapd. Hence to make it convenient, it is better to pass A1, A3 as link address. Since via A3, in get_hapd_bssid(), your hapd->own_addr would match and proper link hapd will be selected even when link_id is not present. >> diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c > >> @@ -444,7 +437,7 @@ static int send_auth_reply(struct hostapd_data *hapd, struct sta_info *sta, >> - os_memcpy(reply->bssid, bssid, ETH_ALEN); >> + os_memcpy(reply->bssid, sa, ETH_ALEN); > > This removes the only use of bssid in send_auth_reply(). Is that really > correct for all cases? It might be, but again, this needs more > justification in the commit message. As said above, in order to ensure link_address only returns back in A3 during Tx status, we have to ensure we are filling link address in the packet being sent. Hence the above change. This was the place where issue was there when handling co-hosted MLDs hence made changes only here as of now. With more thorough testing, if needed will change as other places as well. > In addition, this should remove the > bssid argument from this function and potentially in a number of calling > functions as well since all the would now be unused. Yeah sure. So if you agree, I will send out a clean up patch separately?
diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c index 5a3132de45ef..b20300bab813 100644 --- a/src/ap/ieee802_11.c +++ b/src/ap/ieee802_11.c @@ -416,14 +416,7 @@ static int send_auth_reply(struct hostapd_data *hapd, struct sta_info *sta, struct wpabuf *ml_resp = NULL; #ifdef CONFIG_IEEE80211BE - /* - * Once a non-AP MLD is added to the driver, the addressing should use - * the MLD MAC address. Thus, use the MLD address instead of translating - * the addresses. - */ if (ap_sta_is_mld(hapd, sta)) { - sa = hapd->mld->mld_addr; - ml_resp = hostapd_ml_auth_resp(hapd); if (!ml_resp) return -1; @@ -444,7 +437,7 @@ static int send_auth_reply(struct hostapd_data *hapd, struct sta_info *sta, WLAN_FC_STYPE_AUTH); os_memcpy(reply->da, dst, ETH_ALEN); os_memcpy(reply->sa, sa, ETH_ALEN); - os_memcpy(reply->bssid, bssid, ETH_ALEN); + os_memcpy(reply->bssid, sa, ETH_ALEN); reply->u.auth.auth_alg = host_to_le16(auth_alg); reply->u.auth.auth_transaction = host_to_le16(auth_transaction); @@ -3265,14 +3258,9 @@ static void handle_auth(struct hostapd_data *hapd, bssid = mgmt->bssid; #ifdef CONFIG_IEEE80211BE - /* - * Once a non-AP MLD is added to the driver, the addressing should use - * the MLD MAC address. It is the responsibility of the driver to - * handle the translations. - */ if (ap_sta_is_mld(hapd, sta)) { dst = sta->addr; - bssid = hapd->mld->mld_addr; + bssid = hapd->own_addr; } #endif /* CONFIG_IEEE80211BE */ @@ -4823,15 +4811,6 @@ static u16 send_assoc_resp(struct hostapd_data *hapd, struct sta_info *sta, (reassoc ? WLAN_FC_STYPE_REASSOC_RESP : WLAN_FC_STYPE_ASSOC_RESP)); -#ifdef CONFIG_IEEE80211BE - /* - * Once a non-AP MLD is added to the driver, the addressing should use - * MLD MAC address. - */ - if (ap_sta_is_mld(hapd, sta) && allow_mld_addr_trans) - sa = hapd->mld->mld_addr; -#endif /* CONFIG_IEEE80211BE */ - os_memcpy(reply->da, addr, ETH_ALEN); os_memcpy(reply->sa, sa, ETH_ALEN); os_memcpy(reply->bssid, sa, ETH_ALEN);