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

Jump to bottom

[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG #1747

Merged
merged 10 commits into from
Jun 1, 2021
Merged

[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG #1747

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7c2768c
[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG
gechiang May 17, 2021
3522e81
Changed per review comment to move TPID capability query under switch…
gechiang May 19, 2021
37285d1
Resolved merge conflict
gechiang May 26, 2021
9bc5391
Removed unnecessary application DB check before the TPID test begins
gechiang May 26, 2021
88619b0
Fix test case issue
gechiang May 29, 2021
a4855a7
Fix test case issue
gechiang May 29, 2021
88eea45
remove unnecessary check
gechiang May 31, 2021
ef088bc
Merge remote-tracking branch 'upstream/master'
gechiang Jun 1, 2021
090a2b7
Re-enable Portchannel TPID testing after merged latest from master in…
gechiang Jun 1, 2021
67b0f87
Fix missing Producer DB that accidentally deleted on previous merge
gechiang Jun 1, 2021
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
27 changes: 26 additions & 1 deletion cfgmgr/portmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,21 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu)
return true;
}

bool PortMgr::setPortTpid(const string &alias, const string &tpid)
{
stringstream cmd;
string res;

// Set the port TPID in application database to update port TPID
vector<FieldValueTuple> fvs;
FieldValueTuple fv("tpid", tpid);
fvs.push_back(fv);
m_appPortTable.set(alias, fvs);

return true;
}


bool PortMgr::setPortAdminStatus(const string &alias, const bool up)
{
stringstream cmd;
Expand Down Expand Up @@ -102,7 +117,7 @@ void PortMgr::doTask(Consumer &consumer)
continue;
}

string admin_status, mtu, learn_mode;
string admin_status, mtu, learn_mode, tpid;

bool configured = (m_portList.find(alias) != m_portList.end());

Expand Down Expand Up @@ -131,6 +146,10 @@ void PortMgr::doTask(Consumer &consumer)
{
learn_mode = fvValue(i);
}
else if (fvField(i) == "tpid")
{
tpid = fvValue(i);
}
}

if (!mtu.empty())
Expand All @@ -150,6 +169,12 @@ void PortMgr::doTask(Consumer &consumer)
setPortLearnMode(alias, learn_mode);
SWSS_LOG_NOTICE("Configure %s MAC learn mode to %s", alias.c_str(), learn_mode.c_str());
}

if (!tpid.empty())
{
setPortTpid(alias, tpid);
SWSS_LOG_NOTICE("Configure %s TPID to %s", alias.c_str(), tpid.c_str());
}
}
else if (op == DEL_COMMAND)
{
Expand Down
1 change: 1 addition & 0 deletions cfgmgr/portmgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class PortMgr : public Orch

void doTask(Consumer &consumer);
bool setPortMtu(const std::string &alias, const std::string &mtu);
bool setPortTpid(const std::string &alias, const std::string &tpid);
bool setPortAdminStatus(const std::string &alias, const bool up);
bool setPortLearnMode(const std::string &alias, const std::string &learn_mode);
bool isPortStateOk(const std::string &alias);
Expand Down
26 changes: 26 additions & 0 deletions cfgmgr/teammgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ void TeamMgr::doLagTask(Consumer &consumer)
string admin_status = DEFAULT_ADMIN_STATUS_STR;
string mtu = DEFAULT_MTU_STR;
string learn_mode;
string tpid;

for (auto i : kfvFieldsValues(t))
{
Expand Down Expand Up @@ -178,6 +179,11 @@ void TeamMgr::doLagTask(Consumer &consumer)
SWSS_LOG_INFO("Get learn_mode %s",
learn_mode.c_str());
}
else if (fvField(i) == "tpid")
{
tpid = fvValue(i);
SWSS_LOG_INFO("Get TPID %s", tpid.c_str());
}
}

if (m_lagList.find(alias) == m_lagList.end())
Expand All @@ -198,6 +204,11 @@ void TeamMgr::doLagTask(Consumer &consumer)
setLagLearnMode(alias, learn_mode);
SWSS_LOG_NOTICE("Configure %s MAC learn mode to %s", alias.c_str(), learn_mode.c_str());
}
if (!tpid.empty())
{
setLagTpid(alias, tpid);
SWSS_LOG_NOTICE("Configure %s TPID to %s", alias.c_str(), tpid.c_str());
}
}
else if (op == DEL_COMMAND)
{
Expand Down Expand Up @@ -395,6 +406,21 @@ bool TeamMgr::setLagMtu(const string &alias, const string &mtu)
return true;
}

bool TeamMgr::setLagTpid(const string &alias, const string &tpid)
{
SWSS_LOG_ENTER();

vector<FieldValueTuple> fvs;
FieldValueTuple fv("tpid", tpid);
fvs.push_back(fv);
m_appLagTable.set(alias, fvs);

SWSS_LOG_NOTICE("Set port channel %s TPID to %s", alias.c_str(), tpid.c_str());

return true;
}


bool TeamMgr::setLagLearnMode(const string &alias, const string &learn_mode)
{
// Set the port MAC learn mode in application database
Expand Down
1 change: 1 addition & 0 deletions cfgmgr/teammgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class TeamMgr : public Orch
bool setLagAdminStatus(const std::string &alias, const std::string &admin_status);
bool setLagMtu(const std::string &alias, const std::string &mtu);
bool setLagLearnMode(const std::string &alias, const std::string &learn_mode);
bool setLagTpid(const std::string &alias, const std::string &tpid);


bool isPortEnslaved(const std::string &);
Expand Down
7 changes: 7 additions & 0 deletions orchagent/port.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ extern "C" {
*/
#define DEFAULT_MTU 1492

/*
* Default TPID is 8100
* User can configure other values such as 9100, 9200, or 88A8
*/
#define DEFAULT_TPID 0x8100

#define VNID_NONE 0xFFFFFFFF

namespace swss {
Expand Down Expand Up @@ -128,6 +134,7 @@ class Port
std::vector<sai_object_id_t> m_priority_group_ids;
sai_port_priority_flow_control_mode_t m_pfc_asym = SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_COMBINED;
uint8_t m_pfc_bitmask = 0;
uint16_t m_tpid = DEFAULT_TPID;
uint32_t m_nat_zone_id = 0;
uint32_t m_vnid = VNID_NONE;
uint32_t m_fdb_count = 0;
Expand Down
113 changes: 113 additions & 0 deletions orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,6 +917,36 @@ bool PortsOrch::setPortMtu(sai_object_id_t id, sai_uint32_t mtu)
return true;
}


bool PortsOrch::setPortTpid(sai_object_id_t id, sai_uint16_t tpid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we combine this with Lag to have single function and pass is_lag as boolean since only two lines are different and rest are the same? Just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a chance that we may end up having to implement the LAG handling by having Orchagent apply the LAG TPID config to all its LAG members. This is because currently only one SAI vendor supports TPID setting on Port only and not yet for LAG. For our fanout switch usage, this is already good enough. But if we ever want to apply it to LAG while SAI vendor not supporting it, it will be much easier to have the port/LAG handling having its own function to do the handling. So for now, let's keep the way it is just in case the enhancement is needed in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok

{
SWSS_LOG_ENTER();
sai_status_t status = SAI_STATUS_SUCCESS;
sai_attribute_t attr;

attr.id = SAI_PORT_ATTR_TPID;

attr.value.u16 = (uint16_t)tpid;

status = sai_port_api->set_port_attribute(id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to set TPID 0x%x to port pid:%" PRIx64 ", rv:%d",
attr.value.u16, id, status);
task_process_status handle_status = handleSaiSetStatus(SAI_API_PORT, status);
if (handle_status != task_success)
{
return parseHandleSaiStatusFailure(handle_status);
}
}
else
{
SWSS_LOG_NOTICE("Set TPID 0x%x to port pid:%" PRIx64, attr.value.u16, id);
}
return true;
}


bool PortsOrch::setPortFec(Port &port, sai_port_fec_mode_t mode)
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -2353,6 +2383,8 @@ void PortsOrch::doPortTask(Consumer &consumer)
string learn_mode;
int an = -1;
int index = -1;
string tpid_string;
uint16_t tpid = 0;

for (auto i : kfvFieldsValues(t))
{
Expand Down Expand Up @@ -2384,6 +2416,15 @@ void PortsOrch::doPortTask(Consumer &consumer)
{
mtu = (uint32_t)stoul(fvValue(i));
}
/* Set port TPID */
if (fvField(i) == "tpid")
{
tpid_string = fvValue(i);
// Need to get rid of the leading 0x
tpid_string.erase(0,2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it better to use str.find and then str.erase(0, pos)? Do we always expect a 0x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CLI enforces that the TPID values must be one of the following: "0x8100", "0x9100", "0x9200", or "0x88A8".
So yes it will always have the "0x" which we can get rid of.

tpid = (uint16_t)stoi(tpid_string, 0, 16);
SWSS_LOG_DEBUG("Handling TPID to 0x%x, string value:%s", tpid, tpid_string.c_str());
}
/* Set port speed */
else if (fvField(i) == "speed")
{
Expand Down Expand Up @@ -2699,6 +2740,22 @@ void PortsOrch::doPortTask(Consumer &consumer)
}
}

if (tpid != 0 && tpid != p.m_tpid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we reset? I think the check should be just tpid != p.m_tpid, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case "tpid !=0" check serves the purpose of identifying whether it is handling non-TPID operations in which case variable tpid is initialized to "0" and there is no need to perform TPID operation. This is inline with how MTU and speed setting is being handled. Hope this explains why this check is required.

{
SWSS_LOG_DEBUG("Set port %s TPID to 0x%x", alias.c_str(), tpid);
if (setPortTpid(p.m_port_id, tpid))
{
p.m_tpid = tpid;
m_portList[alias] = p;
}
else
{
SWSS_LOG_ERROR("Failed to set port %s TPID to 0x%x", alias.c_str(), tpid);
it++;
continue;
}
}

if (!fec_mode.empty())
{
if (fec_mode_map.find(fec_mode) != fec_mode_map.end())
Expand Down Expand Up @@ -3151,6 +3208,8 @@ void PortsOrch::doLagTask(Consumer &consumer)
string operation_status;
uint32_t lag_id = 0;
int32_t switch_id = -1;
string tpid_string;
uint16_t tpid = 0;

for (auto i : kfvFieldsValues(t))
{
Expand Down Expand Up @@ -3180,6 +3239,14 @@ void PortsOrch::doLagTask(Consumer &consumer)
{
switch_id = stoi(fvValue(i));
}
else if (fvField(i) == "tpid")
{
tpid_string = fvValue(i);
// Need to get rid of the leading 0x
tpid_string.erase(0,2);
tpid = (uint16_t)stoi(tpid_string, 0, 16);
SWSS_LOG_DEBUG("reading TPID string:%s to uint16: 0x%x", tpid_string.c_str(), tpid);
}
}

if (table_name == CHASSIS_APP_LAG_TABLE_NAME)
Expand Down Expand Up @@ -3236,6 +3303,23 @@ void PortsOrch::doLagTask(Consumer &consumer)
updateChildPortsMtu(l, mtu);
}

if (tpid != 0)
{
if (tpid != l.m_tpid)
{
if(!setLagTpid(l.m_lag_id, tpid))
{
SWSS_LOG_ERROR("Failed to set LAG %s TPID 0x%x", alias.c_str(), tpid);
}
else
{
SWSS_LOG_DEBUG("Set LAG %s TPID to 0x%x", alias.c_str(), tpid);
l.m_tpid = tpid;
m_portList[alias] = l;
}
}
}

if (!learn_mode.empty() && (l.m_learn_mode != learn_mode))
{
if (l.m_bridge_port_id != SAI_NULL_OBJECT_ID)
Expand Down Expand Up @@ -4441,6 +4525,35 @@ bool PortsOrch::removeLagMember(Port &lag, Port &port)
return true;
}

bool PortsOrch::setLagTpid(sai_object_id_t id, sai_uint16_t tpid)
{
SWSS_LOG_ENTER();
sai_status_t status = SAI_STATUS_SUCCESS;
sai_attribute_t attr;

attr.id = SAI_LAG_ATTR_TPID;

attr.value.u16 = (uint16_t)tpid;

status = sai_lag_api->set_lag_attribute(id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to set TPID 0x%x to LAG pid:%" PRIx64 ", rv:%d",
attr.value.u16, id, status);
task_process_status handle_status = handleSaiSetStatus(SAI_API_LAG, status);
if (handle_status != task_success)
{
return parseHandleSaiStatusFailure(handle_status);
}
}
else
{
SWSS_LOG_NOTICE("Set TPID 0x%x to LAG pid:%" PRIx64 , attr.value.u16, id);
}
return true;
}


bool PortsOrch::setCollectionOnLagMember(Port &lagMember, bool enableCollection)
{
/* Port must be LAG member */
Expand Down
2 changes: 2 additions & 0 deletions orchagent/portsorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ class PortsOrch : public Orch, public Subject

bool addLag(string lag, uint32_t spa_id, int32_t switch_id);
bool removeLag(Port lag);
bool setLagTpid(sai_object_id_t id, sai_uint16_t tpid);
bool addLagMember(Port &lag, Port &port, bool enableForwarding);
bool removeLagMember(Port &lag, Port &port);
bool setCollectionOnLagMember(Port &lagMember, bool enableCollection);
Expand All @@ -259,6 +260,7 @@ class PortsOrch : public Orch, public Subject
bool setPortAdminStatus(Port &port, bool up);
bool getPortAdminStatus(sai_object_id_t id, bool& up);
bool setPortMtu(sai_object_id_t id, sai_uint32_t mtu);
bool setPortTpid(sai_object_id_t id, sai_uint16_t tpid);
bool setPortPvid (Port &port, sai_uint32_t pvid);
bool getPortPvid(Port &port, sai_uint32_t &pvid);
bool setPortFec(Port &port, sai_port_fec_mode_t mode);
Expand Down
54 changes: 54 additions & 0 deletions orchagent/switchorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ SwitchOrch::SwitchOrch(DBConnector *db, vector<TableConnector>& connectors, Tabl
Orch::addExecutor(restartCheckNotifier);

initSensorsTable();
querySwitchTpidCapability();
auto executorT = new ExecutableTimer(m_sensorsPollerTimer, this, "ASIC_SENSORS_POLL_TIMER");
Orch::addExecutor(executorT);
}
Expand Down Expand Up @@ -485,3 +486,56 @@ void SwitchOrch::set_switch_capability(const std::vector<FieldValueTuple>& value
{
m_switchTable.set("switch", values);
}

void SwitchOrch::querySwitchTpidCapability()
{
SWSS_LOG_ENTER();
// Check if SAI is capable of handling TPID config and store result in StateDB switch capability table
{
vector<FieldValueTuple> fvVector;
sai_status_t status = SAI_STATUS_SUCCESS;
sai_attr_capability_t capability;

// Check if SAI is capable of handling TPID for Port
status = sai_query_attribute_capability(gSwitchId, SAI_OBJECT_TYPE_PORT, SAI_PORT_ATTR_TPID, &capability);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_WARN("Could not query port TPID capability %d", status);
// Since pre-req of TPID support requires querry capability failed, it means TPID not supported
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_PORT_TPID_CAPABLE, "false");
}
else
{
if (capability.set_implemented)
{
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_PORT_TPID_CAPABLE, "true");
}
else
{
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_PORT_TPID_CAPABLE, "false");
}
SWSS_LOG_NOTICE("port TPID capability %d", capability.set_implemented);
}
// Check if SAI is capable of handling TPID for LAG
status = sai_query_attribute_capability(gSwitchId, SAI_OBJECT_TYPE_LAG, SAI_LAG_ATTR_TPID, &capability);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_WARN("Could not query LAG TPID capability %d", status);
// Since pre-req of TPID support requires querry capability failed, it means TPID not supported
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_LAG_TPID_CAPABLE, "false");
}
else
{
if (capability.set_implemented)
{
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_LAG_TPID_CAPABLE, "true");
}
else
{
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_LAG_TPID_CAPABLE, "false");
}
SWSS_LOG_NOTICE("LAG TPID capability %d", capability.set_implemented);
}
set_switch_capability(fvVector);
}
}
Loading