Skip to content

[mstp] Fix for incorrectly passing mst internal index to stpsync #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vganesan-nokia wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[mstp] Fix for incorrectly passing mst internal index to stpsync #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 15 additions & 16 deletions mstp/mstp_mgr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ static void mstpmgr_free_mstp_port(PORT_ID port_number)
{
/* Delete CIST port from APP DB*/
stpsync_del_mst_port_info(ifname, MSTP_MSTID_CIST);
stpsync_del_port_state(ifname, MSTP_INDEX_CIST);
stpsync_del_port_state(ifname, MSTP_MSTID_CIST);
Copy link
Copy Markdown
Contributor

@divyachandralekha divyachandralekha Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vganesan-nokia
This call is intended for orchagent, and orchagent operates based on the MSTP index, not the MST ID.The MST ID is local to the STPd context and is used only for configuration and display purposes.

Therefore, for this call — as well as the other modified calls below — mstp_index is the correct parameter to use.

Copy link
Copy Markdown
Contributor Author

@vganesan-nokia vganesan-nokia Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@divyachandralekha, thanks for the review comment.

In orchangent there is mapping between the given number (either MST ID or MSTP index or any number) and the OID of the created STP instance m_stpInstOId (Refer src/sonic-swss/orchagent/stporch.cpp addStpInstance(), for example). So, functinoality-wise orchagent is indifferent to MST ID (which is user given) or any internal number (like MSTP index corrsponding to the user given MST ID). As long we choose one (either MST ID or internal MSTP index corresponding to the user given MST ID) and use it consistently the MSTP works.

However, based on the orchagent code, it seems the STP_PORT_STATE_TABLE records in APPL_DB use the key STP_PORT_STATE_TABLE:<interface name>:<instance id>. If we use internal MSTP index, we'll see the port state records of CIST with key STP_PORT_STATE_TABLE:<interface name>:64 and the port records for non CIST MST ID (say MST 1), we'll see those records with key STP_PORT_STATE_TABLE:<interface name>:0. This is bit confusing and also the apis related to stp instance in src/sonic-stp/stpsync/stp_sync.cpp and the orchagent functions for STP_PORT_STATE_TABLE indicate using instance.

If passing mstp_index is the preferred way, I'll close this PR. But, I would recommend to document the schema for APPL_DB STP_PORT_STATE_TABLE to indicate that the key has internal MSTP index but not the user given MST ID and add a fv pair in this records to give MST ID of the index in the key in addition to the state value.

I'll be happy to update the HLD: STP_PORT_STATE_TABLE. to provide the schema details.

}
mstpdata_free_port(port_number);
}
Expand Down Expand Up @@ -983,7 +983,7 @@ void mstpmgr_delete_member_port(MSTP_INDEX mstp_index, PORT_ID port_number)
{
if (!IS_MEMBER(cist_bridge->co.portmask, port_number))
return;
stpsync_del_port_state(ifname, mstp_index);
stpsync_del_port_state(ifname, mstputil_get_mstid(mstp_index));
clear_mask_bit(cist_bridge->co.portmask, port_number);
}
else
Expand All @@ -999,7 +999,7 @@ void mstpmgr_delete_member_port(MSTP_INDEX mstp_index, PORT_ID port_number)
{
/* Delete MSTI port from APP DB*/
stpsync_del_mst_port_info(ifname, mstputil_get_mstid(mstp_index));
stpsync_del_port_state(ifname, mstp_index);
stpsync_del_port_state(ifname, mstputil_get_mstid(mstp_index));
}
if (IS_MEMBER(cist_bridge->co.portmask, port_number))
return;
Expand Down Expand Up @@ -1608,7 +1608,7 @@ bool mstpmgr_config_instance_vlanmask(MSTP_MSTID mstid, VLAN_MASK *vlanmask)
vlanmask_set_bit(&cbridge->vlanmask, vlan_id);
if(mstpdata_is_vlan_present(vlan_id))
{
stpsync_add_vlan_to_instance(vlan_id, mstp_index);
stpsync_add_vlan_to_instance(vlan_id, mstid);
}
flag = true;
}
Expand All @@ -1631,7 +1631,7 @@ bool mstpmgr_config_instance_vlanmask(MSTP_MSTID mstid, VLAN_MASK *vlanmask)
vlanmask_clear_bit(&cbridge->vlanmask, vlan_id);
if(mstpdata_is_vlan_present(vlan_id))
{
stpsync_del_vlan_from_instance(vlan_id, mstp_index);
stpsync_del_vlan_from_instance(vlan_id, mstid);
}
flag = true;
}
Expand Down Expand Up @@ -2363,7 +2363,7 @@ void mstpmgr_process_vlan_mem_config_msg(void *msg)
mst_id = MSTP_GET_MSTID(mstp_bridge, pmsg->vlan_id);
mstp_index = MSTP_GET_INSTANCE_INDEX(mstp_bridge, mst_id);
if (mstp_index != MSTP_INDEX_INVALID)
stpsync_add_vlan_to_instance(pmsg->vlan_id, mstp_index);
stpsync_add_vlan_to_instance(pmsg->vlan_id, mst_id);
}
}

Expand Down Expand Up @@ -2394,7 +2394,7 @@ void mstpmgr_process_vlan_mem_config_msg(void *msg)
{
mstputil_set_kernel_bridge_port_state(mstp_index, port_id, FORWARDING);
STP_LOG_INFO("[MST %d] Port %s FORWARDING", mst_id, port_list[port_count].intf_name);
stpsync_update_port_state(port_list[port_count].intf_name, mstp_index, FORWARDING);
stpsync_update_port_state(port_list[port_count].intf_name, mst_id, FORWARDING);
continue;
}
}
Expand Down Expand Up @@ -2429,7 +2429,7 @@ void mstpmgr_process_vlan_mem_config_msg(void *msg)
/* For CIST index h/w port state wouldnt have updated, when vlan mask is empty.
* So ensure to update the h/w port state*/
if (MSTP_IS_CIST_INDEX(mstp_index))
stpsync_update_port_state(stp_intf_get_port_name(port_id), mstp_index, state);
stpsync_update_port_state(stp_intf_get_port_name(port_id), mst_id, state);
}
port_id= port_mask_get_next_port(port_mask, port_id);
}
Expand Down Expand Up @@ -2460,8 +2460,7 @@ void mstpmgr_process_vlan_mem_config_msg(void *msg)
if(!mstpdata_is_vlan_present(pmsg->vlan_id))
{
mst_id = MSTP_GET_MSTID(mstp_bridge, pmsg->vlan_id);
mstp_index = MSTP_GET_INSTANCE_INDEX(mstp_bridge, mst_id);
stpsync_del_vlan_from_instance(pmsg->vlan_id, mstp_index);
stpsync_del_vlan_from_instance(pmsg->vlan_id, mst_id);
}
}
}
Expand Down Expand Up @@ -2709,7 +2708,7 @@ void mstpmgr_process_inst_vlan_config_msg(void *msg)
{
mstputil_set_kernel_bridge_port_state(MSTP_INDEX_CIST, port_number, FORWARDING);
/* stp disabled ports, make to forwarding */
stpsync_update_port_state(stp_intf_get_port_name(port_number), MSTP_INDEX_CIST, FORWARDING);
stpsync_update_port_state(stp_intf_get_port_name(port_number), MSTP_MSTID_CIST, FORWARDING);
port_number = port_mask_get_next_port(cist_stp_disabled_ports, port_number);
}

Expand All @@ -2735,7 +2734,7 @@ void mstpmgr_process_inst_vlan_config_msg(void *msg)
{
mstputil_set_kernel_bridge_port_state(mstp_index, port_number, FORWARDING);
/* stp disabled ports, make to forwarding */
stpsync_update_port_state(stp_intf_get_port_name(port_number), mstp_index, FORWARDING);
stpsync_update_port_state(stp_intf_get_port_name(port_number), mst_id, FORWARDING);
STP_LOG_INFO("[MST %d] Port %d FORWARDING", mst_id , port_number);
port_number = port_mask_get_next_port(msti_stp_disabled_ports, port_number);
}
Expand Down Expand Up @@ -2792,7 +2791,7 @@ void mstpmgr_process_inst_vlan_config_msg(void *msg)
mstputil_set_kernel_bridge_port_state(MSTP_INDEX_CIST, port_number, FORWARDING);
/* stp disabled ports, make to forwarding */
STP_LOG_INFO("[MST %d] Port %d FORWARDING", MSTP_MSTID_CIST, port_number);
stpsync_update_port_state(stp_intf_get_port_name(port_number), MSTP_INDEX_CIST, FORWARDING);
stpsync_update_port_state(stp_intf_get_port_name(port_number), MSTP_MSTID_CIST, FORWARDING);
port_number = port_mask_get_next_port(cist_stp_disabled_ports, port_number);
}

Expand Down Expand Up @@ -3023,7 +3022,7 @@ bool mstpmgr_delete_control_port(PORT_ID port_id, bool delete_port)
mstputil_set_kernel_bridge_port_state(mstp_index,port_id,FORWARDING);
flag = mstpmgr_set_control_mask(mstp_index);
if(!delete_port)
stpsync_update_port_state(stp_intf_get_port_name(port_id), mstp_index, FORWARDING);
stpsync_update_port_state(stp_intf_get_port_name(port_id), mstputil_get_mstid(mstp_index), FORWARDING);
}
}

Expand All @@ -3034,7 +3033,7 @@ bool mstpmgr_delete_control_port(PORT_ID port_id, bool delete_port)
mstputil_set_kernel_bridge_port_state(MSTP_INDEX_CIST, port_id,FORWARDING);
flag = mstpmgr_set_control_mask(MSTP_INDEX_CIST);
if(!delete_port)
stpsync_update_port_state(stp_intf_get_port_name(port_id), MSTP_INDEX_CIST, FORWARDING);
stpsync_update_port_state(stp_intf_get_port_name(port_id), MSTP_MSTID_CIST, FORWARDING);
}

if(flag)
Expand Down Expand Up @@ -3249,4 +3248,4 @@ void mstpmgr_clear_statistics_all()
mstpmgr_clear_port_statistics(port_number);
port_number = port_mask_get_next_port(mstp_bridge->control_mask, port_number);
}
}
}
8 changes: 4 additions & 4 deletions mstp/mstp_util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,14 +1090,14 @@ bool mstputil_set_port_state(MSTP_INDEX mstp_index, PORT_ID port_number, enum L2

if(ifname)
{
stpsync_update_port_state(ifname, mstp_index, state);
stpsync_update_port_state(ifname, mst_id, state);
}
return true;
}

/*****************************************************************************/
/* mstputil_flush: sets the port state for all the vlans associated */
/* with the input mst instance to the input state */
/* mstputil_flush: Flushes the FDB for a given port of a specific MST */
/* insance if the fdb flush is pending after topology change */
/*****************************************************************************/
bool mstputil_flush(MSTP_INDEX mstp_index, PORT_ID port_number)
{
Expand All @@ -1110,7 +1110,7 @@ bool mstputil_flush(MSTP_INDEX mstp_index, PORT_ID port_number)
ifname = stp_intf_get_port_name(port_number);
if(!ifname)
return false;
stpsync_flush_instance_port(ifname, mstp_index);
stpsync_flush_instance_port(ifname, mst_id);
STP_LOG_INFO("[MST %d] %s flush", mst_id, ifname);


Expand Down