Skip to content
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

[orchagent]Modifying NPU_SI_SETTINGS_SYNC_STATUS based on NPU SI settings application status #2951

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[orchagent]Modifying NPU_SI_SETTINGS_SYNC_STATUS based on NPU SI settings application status #2951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd1d520
[orchagent] Modifying NPU_SI_SETTINGS_SYNC_STATUS based on NPU SI set…
mihirpat1 Nov 2, 2023
e6fae3e
Merge branch 'master' of https://github.com/sonic-net/sonic-swss into…
mihirpat1 Jan 5, 2024
d91a4f8
Replaced APPL_DB with STATE_DB for handling NPU_SI_SETTINGS_SYNC_STATUS
mihirpat1 Jan 5, 2024
92e944b
Added unit-test
mihirpat1 Jan 7, 2024
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
9 changes: 9 additions & 0 deletions orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4109,6 +4109,15 @@ void PortsOrch::doPortTask(Consumer &consumer)
SWSS_LOG_NOTICE("Set port %s SI settings is successful", p.m_alias.c_str());
p.m_preemphasis = serdes_attr;
m_portList[p.m_alias] = p;

mihirpat1 marked this conversation as resolved.
Show resolved Hide resolved
string value;
bool foundNPUSiSettingsSyncStatus = m_portStateTable.hget(p.m_alias, "NPU_SI_SETTINGS_SYNC_STATUS", value);
if (foundNPUSiSettingsSyncStatus) {
Copy link

Choose a reason for hiding this comment

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

Before bringing down port admin_status and applying serdes settings to SAI, Shall we also check if the currently programed serdes settings in SAI/SDK/NPU (via querying SAI or via maintaining a local cache for current serdes settings obtained from SAI) are the same with the ones we are going to apply?

This can serves as extra protection to eliminate unnecessary serdes setting re-applying (with interface shut/start), especially useful if the serdes attributes had already been set properly in SAI/SDK/NPU.

e.g. During sai_port_api->create_swtich call, some vendors creates ports inside create_switch phase with all the proper serdes settings, for static port creation case.

m_portStateTable.hset(p.m_alias, "NPU_SI_SETTINGS_SYNC_STATUS", "NPU_SI_SETTINGS_DONE");
Copy link

Choose a reason for hiding this comment

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

Shall we set DONE after port admin_status is restored?

Because according to CMIS spec, only after HOST TX is good, we can move to CMIS_DP_INIT.

SWSS_LOG_NOTICE("NPU_SI_SETTINGS_SYNC_STATUS modified to NPU_SI_SETTINGS_DONE for port %s", p.m_alias.c_str());
} else {
SWSS_LOG_ERROR("Unable to find NPU_SI_SETTINGS_SYNC_STATUS for port %s", p.m_alias.c_str());
}
}
else
{
Expand Down
25 changes: 25 additions & 0 deletions tests/mock_tests/mock_table.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,31 @@ namespace swss
table[key] = values;
}

void Table::hset(const std::string &key,
const std::string &field,
const std::string &value,
const std::string & /*op*/,
const std::string & /*prefix*/)
{
auto &table = gDB[m_pipe->getDbId()][getTableName()];
std::vector<FieldValueTuple> &fvs = table[key];

// Search for field in fvs and update it if exists, else add new field value tuple
for (auto &it : fvs)
{
if (it.first == field)
{
it.second = value;
return;
}
}

// Add new field value tuple
FieldValueTuple tuple(field, value);
fvs.push_back(tuple);
table[key] = fvs;
}

void Table::getKeys(std::vector<std::string> &keys)
{
keys.clear();
Expand Down
20 changes: 20 additions & 0 deletions tests/mock_tests/portsorch_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,12 @@ namespace portsorch_test
// Refill consumer
consumer->addToSync(kfvSerdes);

// Initialize NPU_SI_SETTINGS_SYNC_STATUS in STATE_DB
Table state_port_table(m_state_db.get(), STATE_PORT_TABLE_NAME);
state_port_table.set("Ethernet0", {
{"NPU_SI_SETTINGS_SYNC_STATUS", "NPU_SI_SETTINGS_NOTIFIED"}
});

_hook_sai_port_api();
uint32_t current_sai_api_call_count = _sai_set_admin_state_down_count;

Expand All @@ -878,6 +884,20 @@ namespace portsorch_test
ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", p));
ASSERT_TRUE(p.m_admin_state_up);

// Verify NPU_SI_SETTINGS_SYNC_STATUS is set by orchagent
std::vector<FieldValueTuple> values;
state_port_table.get("Ethernet0", values);
string npu_si_settings_sync_status_val = "INVALID";
for (auto &valueTuple : values)
{
if (fvField(valueTuple) == "NPU_SI_SETTINGS_SYNC_STATUS")
{
npu_si_settings_sync_status_val = fvValue(valueTuple);
break;
}
}
ASSERT_EQ(npu_si_settings_sync_status_val, "NPU_SI_SETTINGS_DONE");

// Verify idriver
std::vector<std::uint32_t> idriver = { 0x6, 0x6, 0x6, 0x6 };
ASSERT_EQ(p.m_preemphasis.at(SAI_PORT_SERDES_ATTR_IDRIVER), idriver);
Expand Down