RDKB-61540: MLO link reconfiguration for BPi-R4 by bmilcz-comcast · Pull Request #583 · rdkcentral/rdk-wifi-hal

bmilcz-comcast · 2026-02-23T13:53:24Z

Reason for change:Enable possibility of dynamically enabling/disabling MLO links for multiple MLD devices.
Test Procedure: Verify build, test MLO VAPs, enable/disable links in MLO VAPs, check if appropriate singular VAP gets reset properly, confirm connectivity with a client device.
Risks: None
Priority: P2

mateuszCieslak-GL · 2026-02-24T00:35:05Z

+                sizeof(vap->u.bss_info.mld_info.common_info.mld_addr));
+        }
+
+        if ((wifi_hal_is_mld_enabled(interface) == vap->u.bss_info.mld_info.common_info.mld_enable) &&


could it be rolled in to main 'if' perhaps ?

Suggested change

if ((wifi_hal_is_mld_enabled(interface) == vap->u.bss_info.mld_info.common_info.mld_enable) &&

if (vap->u.bss_info.mld_info.common_info.mld_enable) {

if ( vap->u.bss_info.enabled == interface->vap_info.u.bss_info.enabled) {

continue;

strncpy(interface->mld_name, mld_name, sizeof(interface->mld_name) - 1);

Not really - this is why I left this long TODO just above this. The problem is that normally, we receive config in a top-down fashion (OneWifi -> rdk-wifi-hal) and current DML is accustomed to this as well (setting the link IDs from OneWifi). On Banana Pi there was a requirement to not do that, as this was just an implementation done for a specific platform.

Because of that, to keep DML in sync I had to update in a reverse fashion (rdk-wifi-hal -> OneWifi), you can see this in wifi_hal.c

But unfortunately, even if I do this there is an issue where the DML cache that is used during setting of values is not getting that update. That results in any changes done to adjacent settings (say SSID for VAP) will cause to write the "old" link ID from the cache (unless one explicitly changes this via set) to try and override the values, which is why I have to then override this each time this function is called to prevent that.

mateuszCieslak-GL · 2026-02-24T00:39:49Z

+          }
+
+          if (nla_put_u32(msg, NL80211_ATTR_IFINDEX, interface->index) < 0) {
+              nlmsg_free(msg);


not needed, already checked and freed before return in line ~8259

Suggested change

nlmsg_free(msg);

mateuszCieslak-GL · 2026-02-24T00:40:03Z

+          frame_type = (WLAN_FC_TYPE_MGMT << 2) | (stypes[i] << 4);
+
+          if (nla_put_u16(msg, NL80211_ATTR_FRAME_TYPE, frame_type) < 0) {
+              nlmsg_free(msg);


as above
not needed, already checked and freed before return in line ~8259

mateuszCieslak-GL · 2026-02-24T00:40:14Z

+          }
+
+          if (nla_put(msg, NL80211_ATTR_FRAME_MATCH, 0, NULL) < 0) {
+              nlmsg_free(msg);


as above, not needed, already checked and freed before return in line ~8259

bmilcz-comcast · 2026-02-26T15:18:28Z

recheck

mateuszCieslak-GL · 2026-03-02T15:04:55Z

+                nl80211_interface_enable(interface->mld_name, false);
+            }
+
+            if (old_first_interface == interface) {


can this be moved under if above?
if (old_first_interface == interface) {
// First interface pointer could point to invalid data
// for shared resources.1
....
//unregister frame handlers..

I think it is possible, although that will cause the deinit_bss call to be duplicated in here (as we have to maintain order in what we do, as i.e. we cannot unregister netlink handlers before calling deinit_bss).
Still, your suggestion might be better style so I will attempt to do it.

mateuszCieslak-GL · 2026-03-02T15:05:58Z

+                    unregister_data_frame_socket(interface);
+                }
+
+                if (new_first_interface != NULL) {


is that dependent on old_new_interface being equal to interface? if not, move it outside the block
if yes - why? ;)

If we happen to remove a first link from the MLD group, we have to "move" the resources (as well as our handlers) into the next link and interface which will become the "new" first link for this MLD group in case that when removing the old one we still had at least 1 link left.

It happens only in this case, so this is dependant on old_first_interface.

bmilcz-comcast · 2026-03-03T12:16:42Z

Sign in to view

+        }
+
+        g_wifi_hal.mld_count--;
+        g_wifi_hal.mld_array = new_mld_array;


If the refcount (so the number of VAPs currently residing in MLD) reaches 0 we have to cleanup that hostapd_mld since it is no longer used.

MLD pointers are stored in g_wifi_hal.mld_array which can hold a pointer to more than one MLD - if that is the case (mld_count > 1 && refcount == 0), we remove the hostapd_mld to which this VAP refers to.

In the scenario you mentioned, and considering the place you commended on specifically, if the mld_count was == 1 and if this conditional was triggered (refcount == 0), then we are effectively removing the only MLD which was in the array, thus setting it to NULL.

Sign in to view

+                wifi_hal_info_print(
+                    "%s:%d: Mgmt frames already registered for %s and frame_type %d \n", __func__,
+                    __LINE__, wifi_hal_get_interface_name(interface), frame_type);
+                usleep(wait_usec);


mateuszCieslak-GL · 2026-03-02T15:34:11Z

+{
    if (interface->mgmt_frames_registered == 0) {
        wifi_hal_dbg_print("%s:%d: interface:%s mgmt frames not registered\n", __func__, __LINE__,
            wifi_hal_get_interface_name(interface));


Since you (rightfully) decided to use interface->name, it wouldnt hurt to change closest use of it too ;)

Actually, I'm thinking of doing it in opposite way and use wifi_hal_get_interface_name() instead :D as it takes into account if this should return the name of the MLD or interface. In general, it is hard to determine at times what is best to return in those helpers because of how the current implementation treats MLO (swapping the interface indexes and different behaviour for interface depending on mld_enable).

Let me know what you think on this.

Sign in to view

+{
+    if (interface->spurious_frames_registered == 0) {
+        wifi_hal_dbg_print("%s:%d: interface:%s spurious frames not registered\n", __func__,
+            __LINE__, wifi_hal_get_interface_name(interface));


mateuszCieslak-GL · 2026-03-02T15:39:27Z

on this occasion, this could probably moved above, to the beginning?

what do you think? i.e. have you ever seen this log - if so, putting it at the top will speed up execution. if not - none issue.

mateuszCieslak-GL · 2026-03-02T15:46:16Z

+    interface->in_reconf = true;
+
+    wifi_hal_info_print("%s:%d: interface:%s disable AP\n", __func__, __LINE__, interface_name);
+    nl80211_enable_ap(interface, false);


it would be nice to catch error here, even under (if (unlikely( ... < 0) )..

Sign in to view

+    // MLO links share some resources (i.e. RADIUS settings) of the first
+    // link - first take care of it, then attempt to set the rest
+    wifi_interface_info_t *first_interface = wifi_hal_get_first_mld_interface(interface);
+


mateuszCieslak-GL · 2026-03-10T01:30:26Z

    int ret = RETURN_ERR;



Suggested change

if (unlikely(interface == NULL)) {

return ret;

}

mateuszCieslak-GL · 2026-03-10T01:30:55Z

        wifi_interface_info_t *first_interface = wifi_hal_get_first_mld_interface(interface);

-        if (first_interface != interface) {
+        if (first_interface != interface && interface != NULL) {


Suggested change

if (first_interface != interface && interface != NULL) {

if (first_interface != interface) {

mateuszCieslak-GL · 2026-03-10T01:33:56Z

+#ifdef CONFIG_GENERIC_MLO
+        if (wifi_hal_is_mld_enabled(interface)) {
+            to_mac_str(mld_mac, mld_mac_str);
+        }


I'd suggest to move this at the start of previous ifdef, line 1915

#ifdef CONFIG_GENERIC_MLO
mld_mac = wifi_hal_get_mld_mac_address(interface);
to_mac_str ....

All mac_str conversions are done here - isn't it better to keep it this way ?

mateuszCieslak-GL · 2026-03-10T01:39:36Z

what do you think? i.e. have you ever seen this log - if so, putting it at the top will speed up execution. if not - none issue.

mateuszCieslak-GL · 2026-03-10T01:42:32Z

    }

-    if (interface->mld_name[0] == '\0') {
+    if ((interface->mld_name[0] == '\0') || (wifi_hal_is_mld_enabled(interface) == false) ||


evaluation is iirc done left to right, so in the interest of speed it might make sense to reverse the order of checks - if vap is disabled no use in further checks .

mateuszCieslak-GL · 2026-03-10T01:43:56Z

-int wifi_hal_set_mld_link_id(wifi_interface_info_t *interface, int link_id)
+int wifi_hal_get_mld_id(wifi_interface_info_t *interface)
+{
+    if (interface == NULL) {


at least log entry of hitting this function
wifi_hal_debug_print("... Entry") would be nice for easier debugging.

mateuszCieslak-GL · 2026-03-10T01:45:43Z

+        return NL80211_DRV_LINK_ID_NA;
+    }
+
+    if (interface->vap_info.vap_mode == wifi_vap_mode_ap) {


any thoughts of supporting sta vaps here? i.e. iadding this might break the mlo sta connection when its ready ;)
Perhaps check != wifi_vap_mode_monitor would be enough? or dropping this if altogether.

mateuszCieslak-GL · 2026-03-10T01:49:51Z

+    char *interface_name = wifi_hal_get_interface_name(interface);
+    wifi_radio_info_t *radio = get_radio_by_rdk_index(interface->rdk_radio_index);
+
+    if (radio == NULL) {


radio is not used in this function at all, if i see correctly.
Depending on scenario, might be worth checking if interface != NULL - but if its checked in caller of this function then this whole block might just be safely removed.

you are right - my bad, removing

mateuszCieslak-GL · 2026-03-10T01:51:33Z

+        return -1;
+    }
+
+    wifi_hal_info_print("%s:%d: interface:%s reload hostapd config\n", __func__, __LINE__,


info is enabled by default iitc, lets not overwhelm the buffer.
Debug level here should be enough

mateuszCieslak-GL · 2026-03-10T01:52:06Z

+    }
+    interface->bss_started = false;
+
+    wifi_hal_info_print("%s:%d: interface:%s free hostapd data\n", __func__, __LINE__,


switch to debug level?

mateuszCieslak-GL · 2026-03-10T01:52:20Z

+    char *interface_name = wifi_hal_get_interface_name(interface);
+    wifi_radio_info_t *radio = get_radio_by_rdk_index(interface->rdk_radio_index);
+
+    wifi_hal_info_print("%s:%d: interface:%s update hostapd params\n", __func__, __LINE__,


switch to debug level pls

mateuszCieslak-GL · 2026-03-10T01:53:37Z

+    if (interface->vap_info.u.bss_info.enabled && radio->configured && radio->oper_param.enable) {
+        wifi_hal_info_print("%s:%d: interface:%s enable ap\n", __func__, __LINE__, interface_name);
+        // only for single VAPs
+        interface->beacon_set = 0;


in reload_interface() above, line ~ 6022 this is under
#ifndef CONFIG_GENERIC_MLO

shouldnt that be the case here?

I can't tell what was the reason for putting that above in the #ifndef (this is how it was in original function), but normally this flag should be closely tied with start_bss as it influences how further nl80211 driver behaves (wifi_drv_set_ap) and what kind of command it sends. So current placement in my opinion is fine - that #ifndef though - no idea :)

mateuszCieslak-GL · 2026-03-10T01:55:58Z

+        return -1;
+    }
+
+    if (restart_interface(first_interface) < 0) {


one convention please.
by numbers looks like this one should be adjusted to pure
if (restart...) ;)

also,
why reload preceeds restart ? shouldnt this be either-or? or fail safe - if reload wasnt enough, restart whole darn thing ?

By convention I understand error checking - I made a mistake in reload, I want to be explicit about the values I expect, will fix

'reload' is supposed to, well - reload hostapd config :) and deinitialize some hostapd BSS data
'restart' is just restarting the BSS back into work.

I was naming that while doing merge on original reload_single_vap/reload_single_mlo functions while extracting common code and this is what I came up with. If you have a suggestion for different naming scheme

Copilot

Pull request overview

This PR adds support for dynamic enable/disable and reconfiguration of MLO links for multiple MLD devices, with Banana Pi R4–specific handling to ensure links and netlink handlers are correctly reset and restarted during MLO VAP changes.

Changes:

Adds MLD-ID APIs and expands exported nl80211 registration/unregistration helpers (mgmt/spurious/data frames).
Refactors VAP reload/restart logic into shared helpers, including an MLO-aware reload path.
Updates Banana Pi platform code to dynamically set up/tear down MLO links, manage hostapd MLD objects, and reload affected links.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`src/wifi_hal_priv.h`	Adds branch prediction macros and exports new/updated MLO + nl80211 helper prototypes.
`src/wifi_hal_nl80211_utils.c`	Updates MLD grouping logic and introduces new reload/restart helpers for VAP and MLO reconfiguration.
`src/wifi_hal_nl80211.c`	Improves (retry-based) registration of mgmt/spurious frames, adds unregister helpers, and exposes data frame socket registration APIs.
`src/wifi_hal_hostapd.c`	Adds a defensive NULL check in `update_hostap_interface_params()`.
`src/wifi_hal.c`	Hooks MLO reload behavior into VAP create/update path and updates cached link-id feedback.
`platform/banana-pi/platform.c`	Implements dynamic MLO setup/teardown for BPi-R4, including hostapd MLD allocation/deallocation and handler cleanup.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    for (int i = g_wifi_hal.num_radios - 1; i >= 0; i--) {
+        wifi_interface_info_t *interface_iter = NULL;
+        wifi_radio_info_t *radio = get_radio_by_rdk_index(i);
+
+        hash_map_foreach(radio->interface_map, interface_iter) {
+


+    wifi_hal_info_print("%s:%d: interface:%s ifindex:%d nl sock sta:%d, nl sock br_sock:%d\n",
+        __func__, __LINE__, interface->name, interface->index, interface->u.sta.sta_sock_fd,
+        interface->u.ap.br_sock_fd);
+
+    if (interface->u.sta.sta_sock_fd != 0) {
+        close(interface->u.sta.sta_sock_fd);
+        interface->u.sta.sta_sock_fd = -1;
+    }
+


+            if (wifi_hal_get_mac_address(mld_name, mld_mac) < 0) {
+                wifi_hal_error_print("%s:%d: Failed to get MAC address for interface %s\n",
+                    __func__, __LINE__, mld_name);
+                return -1;
+            }


+    if (reload_interface(interface) < 0 && restart_interface(interface) < 0) {
+        return -1;
+    }
+


+            ret = execute_send_and_recv(interface->nl_cb, interface->nl_event, msg,
+                mgmt_frame_register_handler, interface, NULL, NULL);



+        if (nla_put_u32(msg, NL80211_ATTR_IFINDEX, interface->index) < 0) {
+            wifi_hal_error_print("%s:%d: failed set interface index in message for %s interface\n",
+                __func__, __LINE__, wifi_hal_get_interface_name(interface));
+            goto error;
+        }


bmilcz-comcast · 2026-03-18T16:24:37Z

+        if (g_wifi_hal.mld_count > 1) {
+            // There are still other MLDs in the current config
+            new_mld_array = realloc(g_wifi_hal.mld_array,
+                (g_wifi_hal.mld_count - 1) * sizeof(struct hostapd_mld *));
+            if (new_mld_array == NULL) {
+                wifi_hal_error_print("%s:%d Failed to reallocate MLD array\n", __func__, __LINE__);
+                return -1;
+            }
+        } else {
+            new_mld_array = NULL;
+        }
+
+        g_wifi_hal.mld_count--;
+        g_wifi_hal.mld_array = new_mld_array;


This will be refactored in future to a linked list to simplify maintenance.

+#ifdef __GNUC__
+#define likely(x)       __builtin_expect(!!(x), 1)
+#define unlikely(x)     __builtin_expect(!!(x), 0)
+#else
+#define likely(x)       (x)
+#define unlikely(x)     (x)
+#endif


+    char *interface_name = wifi_hal_get_interface_name(interface);
+    wifi_radio_info_t *radio = get_radio_by_rdk_index(interface->rdk_radio_index);
+


Reason for change:Enable possibility of dynamically enabling/disabling MLO links for multiple MLD devices. Test Procedure: Verify build, test MLO VAPs, enable/disable links in MLO VAPs, check if appropriate singular VAP gets reset properly, confirm connectivity with a client device. Risks: None Priority: P2 Signed-off-by: Brayan Milczarek <brayan_milczarek@comcast.com>

bmilcz-comcast · 2026-03-18T16:22:03Z

just code-style changes

bmilcz-comcast · 2026-05-21T14:16:15Z

This PR was not merged due to ongoing issues introduced by other contributions.
This was long enough that the WAs that had to be implemented while working on link reconfig that actually this implementation took a slightly newer shape - due to this, I'm closing this PR and opening a new one for link reconfiguration.

New PR - rdkcentral/OneWifi#1133

bmilcz-comcast marked this pull request as draft February 23, 2026 13:53

mateuszCieslak-GL reviewed Feb 24, 2026

View reviewed changes

bmilcz-comcast force-pushed the bpi-link-reconf branch from ad003f9 to f1be62f Compare February 26, 2026 15:08

bmilcz-comcast marked this pull request as ready for review February 26, 2026 15:08

bmilcz-comcast force-pushed the bpi-link-reconf branch from f1be62f to 9c9798f Compare February 27, 2026 11:59

mateuszCieslak-GL reviewed Mar 2, 2026

View reviewed changes

Comment thread platform/banana-pi/platform.c

mateuszCieslak-GL reviewed Mar 2, 2026

View reviewed changes

Comment thread src/wifi_hal_nl80211.c

wifi_hal_info_print(

"%s:%d: Mgmt frames already registered for %s and frame_type %d \n", __func__,

__LINE__, wifi_hal_get_interface_name(interface), frame_type);

usleep(wait_usec);

This comment was marked as resolved.

Sign in to view

mateuszCieslak-GL reviewed Mar 2, 2026

View reviewed changes

Comment thread src/wifi_hal_nl80211.c

{

if (interface->spurious_frames_registered == 0) {

wifi_hal_dbg_print("%s:%d: interface:%s spurious frames not registered\n", __func__,

__LINE__, wifi_hal_get_interface_name(interface));

This comment was marked as duplicate.

Sign in to view

mateuszCieslak-GL reviewed Mar 2, 2026

View reviewed changes

bmilcz-comcast force-pushed the bpi-link-reconf branch 4 times, most recently from 133c355 to 78240ea Compare March 5, 2026 13:14

mateuszCieslak-GL reviewed Mar 10, 2026

View reviewed changes

bmilcz-comcast force-pushed the bpi-link-reconf branch 6 times, most recently from c17c9a0 to 515f57b Compare March 12, 2026 09:09

narendradandu requested a review from Copilot March 13, 2026 22:14

Copilot started reviewing on behalf of narendradandu March 13, 2026 22:14 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

bmilcz-comcast force-pushed the bpi-link-reconf branch 3 times, most recently from 106da04 to aa2ad4e Compare March 18, 2026 09:17

bmilcz-comcast force-pushed the bpi-link-reconf branch from 44d4b03 to 91bf630 Compare March 18, 2026 16:21

bmilcz-comcast mentioned this pull request May 21, 2026

RDKB-65811: MLO link reconfiguration for BPi-R4 #709

Merged

bmilcz-comcast closed this May 21, 2026

github-actions Bot locked and limited conversation to collaborators May 21, 2026

-        if ((wifi_hal_is_mld_enabled(interface) == vap->u.bss_info.mld_info.common_info.mld_enable) &&
+        if (vap->u.bss_info.mld_info.common_info.mld_enable) {
+           if ( vap->u.bss_info.enabled == interface->vap_info.u.bss_info.enabled) {
+                continue;
+                    strncpy(interface->mld_name, mld_name, sizeof(interface->mld_name) - 1);

+if (unlikely(interface == NULL)) {
+    return ret;
+}

	if (first_interface != interface && interface != NULL) {
	if (first_interface != interface) {

		ret = execute_send_and_recv(interface->nl_cb, interface->nl_event, msg,
		mgmt_frame_register_handler, interface, NULL, NULL);

		char *interface_name = wifi_hal_get_interface_name(interface);
		wifi_radio_info_t *radio = get_radio_by_rdk_index(interface->rdk_radio_index);

Conversation

bmilcz-comcast commented Feb 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmilcz-comcast commented Feb 26, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmilcz-comcast Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmilcz-comcast Mar 2, 2026 •

edited

Loading