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

Add support for configuring loopback mode (PHY and MAC) #3095

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

Add support for configuring loopback mode (PHY and MAC) #3095

wants to merge 1 commit into from

Conversation

royyi8
Copy link
Contributor

@royyi8 royyi8 commented Mar 29, 2024 •
edited
Loading

What I did:
Add a new port attribute loopback_mode to PORT_TABLE in CONFIG_DB. Portsorch will use the enum value to set the sai attribute SAI_PORT_ATTR_LOOPBACK_MODE.

Why I did it:
This PR adds support for setting loopback mode for an interface in CONFIG DB. It can be set at runtime or during initialization in config_db.json.

How I verified it:
Run component tests to set loopback mode in CONFIG DB PORT TABLE and check the corresponding loopback attribute is set in ASIC DB.

Details if related:
There is no impact to warmboot since the PR adds support for new port loopback config.

@royyi8 royyi8 requested a review from prsunny as a code owner March 29, 2024 00:17
@royyi8 royyi8 force-pushed the loopback_mode branch 3 times, most recently from d596a97 to d0349c4 Compare April 1, 2024 18:33
@prsunny
Copy link
Collaborator

prsunny commented Apr 10, 2024

Is there an HLD for this PR? If so, please link it. Modify the PR description to follow the template and give details of the changes. Please mention any impact on Warmboot.

@prsunny prsunny requested a review from prgeor April 10, 2024 22:44
@prsunny
Copy link
Collaborator

prsunny commented Apr 10, 2024

Nit: Update the title to reflect it is phy loopback

@royyi8 royyi8 force-pushed the loopback_mode branch 4 times, most recently from 93a0467 to bca089e Compare April 11, 2024 22:54
@royyi8
Copy link
Contributor Author

royyi8 commented Apr 13, 2024

Hi Prince, thanks for your review. I included more details for the feature below. I have not created an HLD yet, but please let me know if you would like me to open an HLD PR.

Overview:
Interfaces on a switch can be configured into loopback mode, which is useful for port management and diagnostic purposes. This PR allows port loopback mode to be configured directly from CONFIG DB, and supports PHY and MAC (local/remote) loopback modes.

Design:
This PR introduces a new port attribute (loopback_mode) that SWSS will use to configure loopback mode. The attribute can be specified in PORT TABLE in config_db.json to be processed during initialization, or set in PORT_TABLE CONFIG DB during runtime. Portsorch will set the attribute SAI_PORT_ATTR_LOOPBACK_MODE through sai_port_api->set_port_attribute according to the enum defined in the CONFIG DB port table.

CONFIG DB Schema:

PORT_TABLE:Ethernet0
"loopback_mode": "<enum_value>"

_sai_port_loopback_mode_t enum:

DB Schema SAI Attribute
none SAI_PORT_LOOPBACK_MODE_NONE
phy_local SAI_PORT_LOOPBACK_MODE_PHY
mac_local SAI_PORT_LOOPBACK_MODE_MAC
phy_remote SAI_PORT_LOOPBACK_MODE_PHY_REMOTE
mac_remote SAI_PORT_LOOPBACK_MODE_MAC_REMOTE

Warmboot:
This PR has no impact on warmboot, since it adds support for new loopback port config.

Testing:
The component tests verify that setting loopback mode in CONFIG DB port table results in the corresponding SAI attribute set in the ASIC DB.

@royyi8 royyi8 changed the title Add support for configuring loopback mode Add support for configuring loopback mode (PHY and MAC) Apr 13, 2024
prgeor
prgeor reviewed Apr 14, 2024
View reviewed changes
orchagent/portsorch.cpp
sai_status_t status = sai_port_api->set_port_attribute(id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
LOG_ERROR_AND_RETURN(ReturnCode(status)
Copy link
Contributor

Choose a reason for hiding this comment

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

@royyi8 this won't crash OA right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct. It logs the error and returns from the current function.

https://github.com/sonic-net/sonic-swss/blob/master/orchagent/return_code.h#L50

Copy link
Contributor

Choose a reason for hiding this comment

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

@royyi8 can we avoid crash. Why crash OA if one port fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prgeor To clarify, the code will not crash OA. It will simply log an error and return status failed in the setPortLoopbackMode function. OA will proceed in the else block on line 4319.

prgeor
prgeor reviewed Apr 14, 2024
View reviewed changes
orchagent/port/porthlpr.cpp
static const std::unordered_map<std::string, sai_port_loopback_mode_t> portLoopbackModeMap = {
{"none", SAI_PORT_LOOPBACK_MODE_NONE},
{"phy_local", SAI_PORT_LOOPBACK_MODE_PHY},
{"mac_local", SAI_PORT_LOOPBACK_MODE_MAC},
Copy link
Contributor

Choose a reason for hiding this comment

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

@royyi8 how is it different from SAI_PORT_INTERNAL_LOOPBACK_MODE_MAC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SAI_PORT_LOOPBACK_MODE_MAC is the corresponding attribute for SAI_PORT_INTERNAL_LOOPBACK_MODE_MAC. We are using sai_port_loopback_mode_t enum instead of _sai_port_internal_loopback_mode_t since the later is to be deprecated in SAI.

https://github.com/opencomputeproject/SAI/blob/master/inc/saiport.h#L114

prgeor
prgeor reviewed Apr 14, 2024
View reviewed changes
orchagent/port/porthlpr.cpp
@@ -118,6 +118,14 @@ static const std::unordered_map<std::string, Port::Role> portRoleMap =
{ PORT_ROLE_DPC, Port::Role::Dpc }
};

static const std::unordered_map<std::string, sai_port_loopback_mode_t> portLoopbackModeMap = {
{"none", SAI_PORT_LOOPBACK_MODE_NONE},
{"phy_local", SAI_PORT_LOOPBACK_MODE_PHY},
Copy link
Contributor

Choose a reason for hiding this comment

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

@royyi8 How is it different from SAI_PORT_INTERNAL_LOOPBACK_MODE_PHY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason as above. SAI_PORT_LOOPBACK_MODE_PHY is the corresponding attribute for SAI_PORT_INTERNAL_LOOPBACK_MODE_PHY in the sai_port_loopback_mode_t enum.

@royyi8 royyi8 requested a review from prgeor April 17, 2024 17:16
@royyi8
Copy link
Contributor Author

royyi8 commented Apr 24, 2024

Hi @prgeor @prsunny could you please review the PR again? Thanks!

@royyi8
Copy link
Contributor Author

royyi8 commented May 1, 2024

Friendly ping for review @prgeor @prsunny

1 similar comment
@royyi8
Copy link
Contributor Author

royyi8 commented May 6, 2024

Friendly ping for review @prgeor @prsunny

@royyi8
Copy link
Contributor Author

royyi8 commented May 17, 2024

@prgeor @prsunny Could you please let me know if you have more comments/suggestions? Thanks!

prgeor
prgeor previously approved these changes May 20, 2024
View reviewed changes
@prsunny
Copy link
Collaborator

prsunny commented May 23, 2024

@royyi8 , can you please resolve conflict and rebase?

@royyi8 royyi8 force-pushed the loopback_mode branch 2 times, most recently from 4193ab3 to b703499 Compare May 29, 2024 00:56
@royyi8 @prgeor
Add support for configuring loopback mode
b810c51 
Added SWSS support for configuring loopback modes through CONFIG DB.
@prsunny
Copy link
Collaborator

prsunny commented Jun 5, 2024

@royyi8 , @prgeor , can you confirm there is a yang model defined for this new attribute?

@dgsudharsan for viz

@royyi8
Copy link
Contributor Author

royyi8 commented Jun 5, 2024

@royyi8 , @prgeor , can you confirm there is a yang model defined for this new attribute?

@dgsudharsan for viz

@prsunny, I have not made changes to the sonic yang model. Could you please let me know if the new attribute needs to be added in sonic-buildimage sonic-port.yang?

@prsunny
Copy link
Collaborator

prsunny commented Jun 6, 2024 •
edited
Loading

@royyi8 , @prgeor , can you confirm there is a yang model defined for this new attribute?
@dgsudharsan for viz

@prsunny, I have not made changes to the sonic yang model. Could you please let me know if the new attribute needs to be added in sonic-buildimage sonic-port.yang?

Yes, also we need for the link-damping feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prgeor prgeor prgeor approved these changes

@prsunny prsunny Awaiting requested review from prsunny prsunny is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@royyi8 @prsunny @prgeor