Enable proto_mode handling code #3996

divyachandralekha · 2025-11-14T17:12:22Z

What I did
VLAN membership is now correctly populated in STPd when MSTP is enabled
VLAN member port updates handled dynamically after MSTP enable
PVST/MST mode switch logic corrected
Fixed structure alignment issues

How I verified it
Verified MSTP CIST instance convergence
Verified PVST convergence

mssonicbld · 2025-11-14T17:12:29Z

/azp run

azure-pipelines · 2025-11-14T17:12:40Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-11-19T08:00:07Z

/azp run

azure-pipelines · 2025-11-19T08:00:23Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-11-19T11:48:53Z

/azp run

azure-pipelines · 2025-11-19T11:49:03Z

Azure Pipelines successfully started running 1 pipeline(s).

divyachandralekha · 2025-11-19T11:49:12Z

@skulambi-cisco , Please review

divyachandralekha · 2025-11-19T11:54:37Z

@muhammadalihussnain
Please review

vganesan-nokia · 2025-11-20T22:36:43Z

cfgmgr/stpmgr.cpp

+
+            /* Send once when PVST/MSTP is enabled first time */
+            if ((l2ProtoEnabled == L2_MSTP) && (old_l2ProtoEnabled == L2_NONE))
+            {
+                SWSS_LOG_INFO("Populating VLANs for STP mode change: %d -> %d", old_l2ProtoEnabled, l2ProtoEnabled);
+
+                // Get all existing VLANs from state table instead of iterating 1-4095
+                vector<string> vlanKeys;
+                m_stateVlanTable.getKeys(vlanKeys);
+
+                SWSS_LOG_INFO("Found %zu configured VLANs to populate", vlanKeys.size());
+
+                for (auto vlanKey : vlanKeys)
+                {
+                    STP_MST_VLAN_PORT_MAP *vlan_msg = NULL;
+                    vector<PORT_ATTR> port_list;
+                    uint16_t portCnt = 0;
+                    int len;
+
+                    // Extract VLAN ID from key (e.g., "Vlan100" -> 100)
+                    string vlanIdStr = vlanKey.substr(4); // Remove Vlan prefix
+
+                    int vlan_id;
+                    try
+                    {
+                        vlan_id = stoi(vlanIdStr);
+                    }
+                    catch (const std::exception& e)
+                    {
+                        SWSS_LOG_ERROR("Invalid VLAN ID in key '%s': %s", vlanKey.c_str(), e.what());
+                        continue;
+                    }
+
+
+                    // Get all port members for this VLAN
+                    portCnt = (uint16_t)getAllVlanMem(vlanKey, port_list);
+                    if (!portCnt)
+                        continue;
+
+                    SWSS_LOG_DEBUG("VLAN %d has %d ports", vlan_id, portCnt);
+                    len = (int)(sizeof(STP_MST_VLAN_PORT_MAP) + (portCnt * sizeof(PORT_LIST)));
+
+                    vlan_msg = (STP_MST_VLAN_PORT_MAP *)calloc(1, len);
+                    if (!vlan_msg)
+                    {
+                        SWSS_LOG_ERROR("calloc failed for vlan %d", vlan_id);
+                        // Continue processing other VLANs instead of returning
+                        continue;
+                    }
+
+                    // Populate port list
+                    PORT_LIST *attr = vlan_msg->port_list;
+                    for (size_t i = 0; i < port_list.size(); i++)
+                    {
+                        snprintf(attr[i].intf_name, IFNAMSIZ, "%s", port_list[i].intf_name);
+                        attr[i].tagging_mode = port_list[i].mode;
+                        SWSS_LOG_DEBUG("  Port[%zu]: %s Mode %d", i, port_list[i].intf_name, port_list[i].mode);
+                    }
+
+                    // Fill in VLAN message
+                    vlan_msg->port_count = portCnt;
+                    vlan_msg->vlan_id = (uint16_t)vlan_id;
+                    vlan_msg->stp_mode = l2ProtoEnabled;
+                    vlan_msg->add = true;
+
+                    // Send message to STP daemon
+                    sendMsgStpd(STP_MST_VLAN_PORT_LIST_CONFIG, len, (void *)vlan_msg, (L2_PROTO_MODE)vlan_msg->stp_mode);
+
+                    free(vlan_msg);
+                }
+            }


This is good for populating the vlan member list when the MSTP is enabled after the state db vlan table update is complete. Please consider the scenarios of updating the vlan members after the MSTP is enabled or during boot up, the MSTP global enable may be done before all vlan members are updated fully in the state db.

So in addition to this, we may also need to handle vlan member update to send add/remove port info in STP_MST_VLAN_PORT_LIST_CONFIG message for single port updates (add or remove). i.e., we may have to do something like enhancing doVlanMemUpdateTask() to send this message for MSTP (if the protocol mode is MSTP).

Done. Please check doVlanMemUpdateTask()

Done. Please check doVlanMemUpdateTask()

Thanks @divyachandralekha. The logic looks good. I have some minor code comments for this.

vganesan-nokia · 2025-11-20T22:59:04Z

cfgmgr/stpmgr.cpp

+        // Key is just the instance ID (e.g., "0", "1", "2")
+        uint16_t instance_id;
+        try
+        {
+            instance_id = static_cast<uint16_t>(stoi(key.c_str()));
+        }
+        catch (const std::exception& e)
+        {
+            SWSS_LOG_ERROR("Invalid instance ID in key '%s': %s", key.c_str(), e.what());
+            it = consumer.m_toSync.erase(it);
+            continue;
+        }


This may need changes in the sonic-utilities/config/stp.py to avoid using "MST_INSTANCE" in all places where STP_MST_INST table entries are accessed (add/remove/update or retrieve). For example adjustment is needed where, when mstp is enabled, config db STP_MST_INST is populated for instace 0 with MST_INSTANCE in the key string.

taken care in stp.py

vganesan-nokia · 2025-11-20T23:13:42Z

cfgmgr/stpmgr.cpp

        }

-        sendMsgStpd(STP_MST_INST_CONFIG, len, (void *)msg);
+        sendMsgStpd(STP_MST_INST_CONFIG, len, (void *)msg, l2ProtoEnabled);


The message construction here does not appear to be conforming to the expectation of the processing in sonic-stp/mstp/mstp_mgr.c: mstpmgr_process_inst_vlan_config_msg()).

As per the expectation of the processing of this STP_MST_INST_CONFIG messge (in sonic-stp/mstp/mstp_mgr.c: mstpmgr_process_inst_vlan_config_msg()), the STP_MST_INST_CONFIG message is supposed to carry a list of instances. The instance information is to be filled as an element (of type MST_INST_CONFIG_MSG) of array mst_list[] - a filed in STP_MST_INSTANCE_CONFIG_MSG and msg->mst_count should be set accordingly. (This STP_MST_INSTANCE_CONFIG_MSG used (from stp_ipc.h) in the above mentioned function (from sonic-stp) is different, which uses the mst_count and mst_list).

As of now I have verified CIST instance alone. I will do the MST verification and related fixes in the next PR.

cfgmgr/stpmgr.cpp

vganesan-nokia · 2025-11-20T23:34:27Z

cfgmgr/stpmgr.cpp

        }

-        sendMsgStpd(STP_MST_GLOBAL_CONFIG, sizeof(msg), (void *)&msg);
+        sendMsgStpd(STP_MST_GLOBAL_CONFIG, sizeof(msg), (void *)&msg, l2ProtoEnabled);


Please check. In the message construction above, there may be type casting problem for msg.revision_number. In STP_MST_GLOBAL_CONFIG_MSG, the field revision_number is declared as uint16_t. But we are type casting as uint32_t. Also, casting stoi() output which is int to uint8_t may give unexpected results (reducing from 4 bytes to 1 byte).

I do not see the changes for the type casting. Hope this will be taken care in the final merge version. Thanks for addressing the comment

prsunny · 2025-11-24T22:46:49Z

@divyachandralekha , please follow template to provide title description etc. The PR is lacking all details

mssonicbld · 2025-11-25T17:35:26Z

/azp run

azure-pipelines · 2025-11-25T17:35:41Z

Azure Pipelines successfully started running 1 pipeline(s).

vganesan-nokia · 2025-11-25T20:57:39Z

cfgmgr/stpmgr.cpp

+            vlan_msg->add = (op == SET_COMMAND) ? true : false;
+
+            sendMsgStpd(STP_MST_VLAN_PORT_LIST_CONFIG, len, (void *)vlan_msg, (L2_PROTO_MODE)vlan_msg->stp_mode);
+            if (vlan_msg)


Should we reset vlan_msg to null after freeing memory since this is used in a loop and we may get here without any memory allocation if isLagEmpty() is true?
OR
If we put the entire processing under the if(!isLagEmpty()) block, we may not need this check at all.

vganesan-nokia · 2025-11-25T21:02:01Z

cfgmgr/stpmgr.cpp

+        {
+            STP_MST_VLAN_PORT_MAP *vlan_msg;
+            int len;
+            PORT_LIST *attr;


This local variable may be unnecessary. We canc access the port_list directly like vlan_msg->port_list[0].tagging_mode.

vganesan-nokia · 2025-11-25T21:05:43Z

cfgmgr/stpmgr.cpp

+                    return;
+            }
+
+            attr = vlan_msg->port_list;


If isLagEmpty() is true, there is no memory allocation and vlan_msg is un-initialized. So we may access uninitialized vlan_msg. Reccommend to put the vlan_msg encoding and sendMsgStpd also (not just memory allocation) inside if(!isLagEmpty(intfName)) block.

mssonicbld · 2025-11-26T08:09:41Z

/azp run

azure-pipelines · 2025-11-26T08:09:51Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-11-26T08:14:21Z

/azp run

azure-pipelines · 2025-11-26T08:14:32Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-11-26T11:10:46Z

/azp run

azure-pipelines · 2025-11-26T11:10:56Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-11-26T15:42:49Z

/azp run

azure-pipelines · 2025-11-26T15:43:00Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-11-26T16:57:25Z

/azp run

azure-pipelines · 2025-11-26T16:57:36Z

Azure Pipelines successfully started running 1 pipeline(s).

vganesan-nokia

Thanks for responding to the comments. Looking forward to the next PR

mssonicbld · 2025-11-27T06:54:24Z

/azp run

azure-pipelines · 2025-11-27T06:54:33Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-11-27T07:32:01Z

/azp run

azure-pipelines · 2025-11-27T07:32:12Z

Azure Pipelines successfully started running 1 pipeline(s).

Enable proto_mode handling code

b977282

divyachandralekha force-pushed the test_pvst branch from cc87267 to b977282 Compare November 19, 2025 08:00

Merge branch 'master' into test_pvst

ca2473d

divyachandralekha marked this pull request as ready for review November 19, 2025 11:48

divyachandralekha requested a review from prsunny as a code owner November 19, 2025 11:48

vganesan-nokia reviewed Nov 20, 2025

View reviewed changes

Enable proto_mode handling code

b935a74

vganesan-nokia reviewed Nov 25, 2025

View reviewed changes

Enable proto_mode handling code

cfdcbc0

Merge branch 'master' into test_pvst

33bbb6c

divyachandralekha requested a review from vganesan-nokia November 26, 2025 09:17

Enable proto_mode handling code

fd6a67d

Enable proto_mode handling code

0729e82

Enable proto_mode handling code

74a7090

vganesan-nokia approved these changes Nov 26, 2025

View reviewed changes

Enable proto_mode handling code

9d49282

Enable proto_mode handling code

133f8da

Enable proto_mode handling code #3996

Are you sure you want to change the base?

Enable proto_mode handling code #3996

Conversation

divyachandralekha commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Nov 14, 2025

Uh oh!

azure-pipelines bot commented Nov 14, 2025

Uh oh!

mssonicbld commented Nov 19, 2025

Uh oh!

azure-pipelines bot commented Nov 19, 2025

Uh oh!

mssonicbld commented Nov 19, 2025

Uh oh!

azure-pipelines bot commented Nov 19, 2025

Uh oh!

divyachandralekha commented Nov 19, 2025

Uh oh!

divyachandralekha commented Nov 19, 2025

Uh oh!

vganesan-nokia Nov 20, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vganesan-nokia Nov 20, 2025 • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vganesan-nokia Nov 26, 2025 • 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!

prsunny commented Nov 24, 2025

Uh oh!

mssonicbld commented Nov 25, 2025

Uh oh!

azure-pipelines bot commented Nov 25, 2025

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!

mssonicbld commented Nov 26, 2025

Uh oh!

azure-pipelines bot commented Nov 26, 2025

Uh oh!

mssonicbld commented Nov 26, 2025

Uh oh!

azure-pipelines bot commented Nov 26, 2025

Uh oh!

mssonicbld commented Nov 26, 2025

Uh oh!

azure-pipelines bot commented Nov 26, 2025

divyachandralekha commented Nov 14, 2025 •

edited

Loading

vganesan-nokia Nov 20, 2025 •

edited

Loading

vganesan-nokia Nov 20, 2025 •

edited

Loading

vganesan-nokia Nov 26, 2025 •

edited

Loading