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

[macsec]: Set MTU for MACsec #2398

Merged
merged 5 commits into from
Aug 16, 2022
+110 −16
Merged

[macsec]: Set MTU for MACsec #2398

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
16cae21
Set mtu for MACsec
Pterosaur Jul 26, 2022
b8f618c
Support port mtu update w/o gearbox
jimmyzhai Jul 27, 2022
e55a065
Update fake_portorch.cpp
jimmyzhai Jul 27, 2022
e3ce7ca
Merge branch 'master' into fix_mtu_macsec
Pterosaur Jul 30, 2022
9ec471a
Fix issue if MTU wasn't set
Pterosaur Aug 3, 2022
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
Prev Previous commit
Fix issue if MTU wasn't set 
Signed-off-by: Ze Gan <ganze718@gmail.com>
@Pterosaur
Pterosaur committed Aug 4, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
commit 9ec471adc95b9cea1ffcdc6686a875f5ed70f9fe
40 changes: 38 additions & 2 deletions orchagent/portsorch.cpp
View file
Original file line number Diff line number Diff line change
@@ -1161,6 +1161,30 @@ bool PortsOrch::getPortAdminStatus(sai_object_id_t id, bool &up)
return true;
}

bool PortsOrch::getPortMtu(const Port& port, sai_uint32_t &mtu)
{
SWSS_LOG_ENTER();

sai_attribute_t attr;
attr.id = SAI_PORT_ATTR_MTU;

sai_status_t status = sai_port_api->get_port_attribute(port.m_port_id, 1, &attr);

if (status != SAI_STATUS_SUCCESS)
{
return false;
}

mtu = attr.value.u32 - (uint32_t)(sizeof(struct ether_header) + FCS_LEN + VLAN_TAG_LEN);

if (isMACsecPort(port.m_port_id))
{
mtu -= MAX_MACSEC_SECTAG_SIZE;
}

return true;
}

bool PortsOrch::setPortMtu(const Port& port, sai_uint32_t mtu)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we changing this API? Please have it as the original one and have the caller pass the extra value.

Copy link
Contributor Author

@Pterosaur Pterosaur Aug 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Due to the Gearbox architecture support, the original function cannot semantically support the functionality of set MTU. We just follow other existing functions, like set Speed, set Fec and so on, that are needed by Gearbox platform.

sonic-swss/orchagent/portsorch.cpp

Lines 2191 to 2209 in 9ec471a

task_process_status PortsOrch::setPortSpeed(Port &port, sai_uint32_t speed)
{
sai_attribute_t attr;
sai_status_t status;
SWSS_LOG_ENTER();
attr.id = SAI_PORT_ATTR_SPEED;
attr.value.u32 = speed;
status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
return handleSaiSetStatus(SAI_API_PORT, status);
}
setGearboxPortsAttr(port, SAI_PORT_ATTR_SPEED, &speed);
return task_success;
}

sonic-swss/orchagent/portsorch.cpp

Lines 1253 to 1277 in 9ec471a

bool PortsOrch::setPortFec(Port &port, sai_port_fec_mode_t mode)
{
SWSS_LOG_ENTER();
sai_attribute_t attr;
attr.id = SAI_PORT_ATTR_FEC_MODE;
attr.value.s32 = mode;
sai_status_t status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to set fec mode %d to port pid:%" PRIx64, mode, port.m_port_id);
task_process_status handle_status = handleSaiSetStatus(SAI_API_PORT, status);
if (handle_status != task_success)
{
return parseHandleSaiStatusFailure(handle_status);
}
}
SWSS_LOG_INFO("Set fec mode %d to port pid:%" PRIx64, mode, port.m_port_id);
setGearboxPortsAttr(port, SAI_PORT_ATTR_FEC_MODE, &mode);
return true;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, from the changes I still think you can reuse the existing function. But I see your point as making it consistent across other set functions. Approving from my side.

{
SWSS_LOG_ENTER();
@@ -1188,7 +1212,10 @@ bool PortsOrch::setPortMtu(const Port& port, sai_uint32_t mtu)
}
}

setGearboxPortsAttr(port, SAI_PORT_ATTR_MTU, &mtu);
if (m_gearboxEnabled)
{
setGearboxPortsAttr(port, SAI_PORT_ATTR_MTU, &mtu);
}
SWSS_LOG_INFO("Set MTU %u to port pid:%" PRIx64, attr.value.u32, port.m_port_id);
return true;
}
@@ -4551,6 +4578,12 @@ bool PortsOrch::initializePort(Port &port)
return false;
}

/* initialize port mtu */
if (!getPortMtu(port, port.m_mtu))
{
SWSS_LOG_ERROR("Failed to get initial port mtu %d", port.m_mtu);
}

/*
* always initialize Port SAI_HOSTIF_ATTR_OPER_STATUS based on oper_status value in appDB.
*/
@@ -7451,7 +7484,10 @@ void PortsOrch::setMACsecEnabledState(sai_object_id_t port_id, bool enabled)
m_macsecEnabledPorts.erase(port_id);
}

setPortMtu(p, p.m_mtu);
if (p.m_mtu)
{
setPortMtu(p, p.m_mtu);
}
}

bool PortsOrch::isMACsecPort(sai_object_id_t port_id) const
1 change: 1 addition & 0 deletions orchagent/portsorch.h
View file
Original file line number Diff line number Diff line change
@@ -311,6 +311,7 @@ class PortsOrch : public Orch, public Subject

bool setPortAdminStatus(Port &port, bool up);
bool getPortAdminStatus(sai_object_id_t id, bool& up);
bool getPortMtu(const Port& port, sai_uint32_t &mtu);
bool setPortMtu(const Port& port, sai_uint32_t mtu);
bool setPortTpid(sai_object_id_t id, sai_uint16_t tpid);
bool setPortPvid (Port &port, sai_uint32_t pvid);