XB10-2878: Reduce DB writes after MLO config check - #1297
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts MLO (MLD) update logic so the returned radio_bitmap only flags radios where mld_enable and/or mld_link_id actually changed, preventing unnecessary reconfiguration work that was causing a recurring ~25s delay on subsequent boots.
Changes:
- Update
update_mld_groups()to cache originalmld_enablestate per VAP and only setradio_bitmapwhen the final enable state differs from the baseline (and whenmld_link_idis overridden). - Centralize
MAX_VAP_INDEXinwifi_ctrl.h(removing the local define fromwifi_ctrl_queue_handlers.c). - Minor comment/docstring and formatting adjustments around MLD handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| source/core/wifi_ctrl.h | Introduces MAX_VAP_INDEX used for VAP indexing/bounds in core code. |
| source/core/wifi_ctrl.c | Refines update_mld_groups() radio bitmap calculation using baseline mld_enable caching and conditional bitmap updates. |
| source/core/wifi_ctrl_queue_handlers.c | Removes local MAX_VAP_INDEX define to rely on the shared header definition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5a0749c to
998a147
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
source/core/wifi_ctrl.c:3515
- This info log is very low-level (bound validation) and doesn’t identify which VAP/radio triggered it. Using dbg level and including vap_index makes the message actionable while reducing noise.
wifi_util_info_print(log_type, "%s:%d: mld_id=%u mld_link_id=%u\n",
__func__, __LINE__, mld_conf->mld_id, mld_conf->mld_link_id);
source/core/wifi_ctrl.c:3610
- Similar to the incompatible-case log, this is debug-level output (includes LINE) and should include which VAP was enabled. Switching to dbg level and adding vap_index/group id reduces info log noise and makes the log actionable.
wifi_util_info_print(log_type, "%s:%d: compatible, was false in mgr cache\n",
__func__, __LINE__);
source/core/wifi_ctrl.c:3505
- This log looks like debug-level diagnostic output (includes LINE and an internal condition) and the message lacks key context like vap_index/radio. Consider using wifi_util_dbg_print and include vap_index to avoid noisy info logs during normal operation.
This issue also appears in the following locations of the same file:
- line 3514
- line 3609
wifi_util_info_print(log_type, "%s:%d: target_vap->u.bss_info.enabled false\n",
__func__, __LINE__);
source/core/wifi_ctrl.c:3600
- This diagnostic message is emitted inside a tight loop and currently logs at info level without identifying which entry was incompatible. Consider switching to dbg level and including vap_index/group id to avoid noisy logs and improve debuggability.
wifi_util_info_print(log_type, "%s:%d: not compatible, but was true in mgr cache\n",
__func__, __LINE__);
998a147 to
4422a9b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
source/core/wifi_ctrl.c:3601
- This log uses
entry->vap_info->name, but other code in this file useswifi_vap_info_t::vap_name(e.g., mgr_vap->vap_name). Using the wrong field here can be a build break or log the wrong identifier. Also, the message contains a double newline.
wifi_util_info_print(log_type,
"%s:%d: MLO disabled for VAP %s - not compatible with MLO group %d\n\n",
__func__, __LINE__, entry->vap_info->name, entry->mld_conf->mld_id);
source/core/wifi_ctrl.c:3517
- The format string includes an embedded newline before "out of bounds" and is missing a trailing newline, which will split the log line and make it harder to read/grep.
This issue also appears on line 3599 of the same file.
wifi_util_dbg_print(log_type,
"%s:%d: mld_id=%u mld_link_id=%u\n out of bounds", __func__, __LINE__,
mld_conf->mld_id, mld_conf->mld_link_id);
4422a9b to
1564035
Compare
7098e59 to
831aa33
Compare
831aa33 to
507c500
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
source/core/wifi_ctrl.c:3517
- The out-of-bounds MLD config path is evaluated inside the outer
for (i = 0; i < MLD_UNIT_COUNT; i++)loop. For a VAP that was originally MLO-enabled, this will print the same debug line and set the sameradio_bitmapbit once per MLD unit (even though the state change happens only on thei == 0pass whenmld_enableis cleared). Gate the log/bit-update toi == 0to prevent duplicate logs and redundant work.
if (mld_conf->mld_id >= MLD_UNIT_COUNT || mld_conf->mld_link_id >= MAX_NUM_MLD_LINKS) {
if (was_enabled) {
wifi_util_dbg_print(log_type,
"%s:%d: mld_id=%u mld_link_id=%u out of bounds\n", __func__, __LINE__,
mld_conf->mld_id, mld_conf->mld_link_id);
source/core/wifi_ctrl.c:3507
update_mld_groups()iterates all VAPs for everyiin0..MLD_UNIT_COUNT-1. In the disabled-VAP early-continue path, a VAP that was originally MLO-enabled will emit the same debug message and set the sameradio_bitmapbit once per MLD unit (up to 8 times). Sincemld_enableis only cleared on thei == 0pass, gating this log/bit-update toi == 0avoids duplicate logs and redundant work without changing behavior.
This issue also appears on line 3513 of the same file.
if (target_vap->u.bss_info.enabled == false) {
if (was_enabled) {
wifi_util_dbg_print(log_type, "%s:%d: MLO-enabled VAP %d is disabled\n",
__func__, __LINE__, target_vap->vap_index);
radio_bitmap |= (1u << mgr_vap->radio_index);
507c500 to
81ac127
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
source/core/wifi_ctrl.c:3501
mgr_vap->vap_indexis used to indexoriginal_mld_enable[]without any bounds check. Other parts of the codebase treat valid VAP indices as0..MAX_VAP-1(e.g.,source/apps/whix/wifi_whix.c:1565checksvap_index >= MAX_VAP), so an out-of-rangevap_indexhere would cause an out-of-bounds write/read and potential crash/corruption.
was_enabled = original_mld_enable[mgr_vap->vap_index];
c1dbfaf to
b9f5cb0
Compare
b9f5cb0 to
245c5f3
Compare
| __func__, __LINE__, vap->vap_index, r_idx); | ||
| continue; | ||
| } | ||
| original_mld_enable[vap->vap_index] = vap->u.bss_info.mld_info.common_info.mld_enable; |
There was a problem hiding this comment.
vap->u.bss_info has only VAPs of type BSS. Some VAPs can be STA type which has vap->u.sta_info. instead of vap->u.bss_info
Non BSS VAPs need to be skipped
Similarly a few lines below
There was a problem hiding this comment.
Added isVapStaMesh checks
245c5f3 to
8253eec
Compare
Reason for change: update_mld_groups was improperly marking that a change was done during MLO compatibility checks, which caused increased boot time on each booting of the device. Test Procedure: Verify if boot time has increased due to DB writing, during subsequent reboots of the device. In order to see if write was done, check for "Updated MLD group" in logs. MLO link reconfiguration changes are expected to trigger writing to DB on next boot. Risks: None Priority: P2 Signed-off-by: Brayan Milczarek <brayan_milczarek@comcast.com>
8253eec to
73a12bf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
source/core/wifi_ctrl.c:1810
- The pre-update snapshot should also capture the original
mld_addrper VAP (at least for VAPs where MLO may be enabled), otherwise the later diff check can miss a realmld_addrchange and skip the DB update.
original_mld_enable[vap->vap_index] = vap->u.bss_info.mld_info.common_info.mld_enable;
original_link_id[vap->vap_index] = vap->u.bss_info.mld_info.common_info.mld_link_id;
source/core/wifi_ctrl.c:1841
- The DB update gating only checks
mld_enableandmld_link_id, butupdate_mld_groups()can also changemld_addr. Ifmld_addrchanges whilemld_enable/mld_link_idremain the same, the DB update will be skipped and persistence can drift from the mgr cache.
if (original_mld_enable[vap->vap_index] !=
vap->u.bss_info.mld_info.common_info.mld_enable ||
original_link_id[vap->vap_index] !=
vap->u.bss_info.mld_info.common_info.mld_link_id) {
source/core/wifi_ctrl.c:1787
init_wifi_mld_groups()decides whether to persist changes to DB, but it only snapshotsmld_enable/mld_link_id. Sinceupdate_mld_groups()can also changemld_addr, the function should snapshot the originalmld_addras well so it can detect/persist address-only changes when MLO is enabled.
This issue also appears in the following locations of the same file:
- line 1809
- line 1838
wifi_vap_info_map_t *mgr_vap_map = NULL;
bool original_mld_enable[MAX_VAP] = { false };
unsigned int original_link_id[MAX_VAP] = { 0 };
Reason for change: Due to always being set for all radios on all boots, it was causing constant 25s delay.
Test Procedure: Verify if the delay is present on subsequent boots without any changes to specific VAPs
Risks: None
Priority: P2