Port serdes changes by dgsudharsan · Pull Request #1611 · sonic-net/sonic-swss

dgsudharsan · 2021-01-26T03:09:03Z

What I did
Migrating the port serdes programming from existing port attributes to port serdes object

Why I did it
This change is required for supporting 6 tap filters which cannot be supported through existing port attributes

How I verified it
Load the changes on existing platforms and verify the pre-emphasis settings are correctly programmed in the hardware
Load the changes on new platforms requiring 6tap filter and verified the settings are programmed correctly in the hardware

Details if related

dgsudharsan · 2021-01-26T17:43:36Z

retest this please

dgsudharsan · 2021-01-26T20:59:05Z

retest vs please

orchagent/portsorch.cpp

prsunny · 2021-01-27T18:00:51Z

orchagent/portsorch.cpp

-    attr.value.u32list.list = serdes_val.data();
+    if (port_attr.value.oid != SAI_NULL_OBJECT_ID)
+    {
+        status = sai_port_api->remove_port_serdes(port_attr.value.oid);


Can port_serdes be removed on the fly? Or do you require the port to be shutdown etc?

It can be removed on the fly. It just removes the software object. The logic is introduced because the port serdes object has all attributes as create only and since multiple attributes need to be set which the current set attribute doesn't support, we have this logic of deleting the existing object and recreating it.

prsunny · 2021-01-27T18:02:58Z

orchagent/portsorch.cpp

+    port_serdes_attr.id = SAI_PORT_SERDES_ATTR_PORT_ID;
+    port_serdes_attr.value.oid = port_id;
+    attr_list.emplace_back(port_serdes_attr);
+    SWSS_LOG_ERROR("Creating serdes for port 0x%" PRIx64, port_id);


This cannot be error -> please change to INFO as you already have one NOTICE below

prsunny · 2021-01-27T18:03:27Z

orchagent/portsorch.cpp

-        return false;
+        SWSS_LOG_ERROR("Failed to create port serdes for port 0x%" PRIx64,
+                       port_id);
+         return false;


Extra space

orchagent/portsorch.cpp

daall · 2021-01-28T02:08:39Z

It looks like the same set of tests (acl, CRM, mirror, etc.) are failing consistently between re-runs. @dgsudharsan can you take a look?

dgsudharsan · 2021-01-28T02:37:06Z

It looks like the same set of tests (acl, CRM, mirror, etc.) are failing consistently between re-runs. @dgsudharsan can you take a look?

Sure. Let me run the tests locally. Meanwhile let me also try to rebuild here again to see if it succeeds.

dgsudharsan · 2021-01-28T02:37:16Z

retest vs please

dgsudharsan · 2021-01-28T14:33:07Z

It looks like the same set of tests (acl, CRM, mirror, etc.) are failing consistently between re-runs. @dgsudharsan can you take a look?

Sure. Let me run the tests locally. Meanwhile let me also try to rebuild here again to see if it succeeds.

Currently one test failing. I don't think the failing tests are due to any of my changes as my changes come into play only when media_settings.json is defined which is not for a vs

dgsudharsan · 2021-01-28T14:33:33Z

retest vs please

dgsudharsan · 2021-01-28T17:20:20Z

retest vs please

prsunny

lgtm.. wait for other reviewers.

dgsudharsan · 2021-01-28T20:33:12Z

retest vs please

dgsudharsan · 2021-01-29T01:29:23Z

retest vs please

dgsudharsan · 2021-01-29T17:45:40Z

retest vs please

prsunny · 2021-01-30T01:55:48Z

/AzurePipeline run

azure-pipelines · 2021-01-30T01:55:56Z

Azure Pipelines successfully started running 1 pipeline(s).

lguohan · 2021-01-30T17:59:24Z

/AzurePipelines run

prsunny · 2021-02-02T05:47:27Z

/AzurePipelines run

azure-pipelines · 2021-02-02T05:47:36Z

Azure Pipelines successfully started running 1 pipeline(s).

jleveque · 2021-02-02T18:01:09Z

retest vs please

daall · 2021-02-02T23:48:54Z

orchagent/portsorch.cpp

+                }
+                else
+                {
+                    SWSS_LOG_ERROR("Unknown port field: %s", fvField(i).c_str());


The CRM tests are failing b/c the syslog is getting throttled due to this message:

Feb 2 23:46:10.029687 9a649eaefbe1 ERR #orchagent: message repeated 4793 times: [ :- doPortTask: Unknown port field: alias]

Do we need to add/skip "alias" here?

I can remove this as there might be other fields that could be added in the future which may not be significant for the SAI conversion.

dgsudharsan · 2021-02-04T15:14:06Z

retest this please

prsunny · 2021-02-04T16:49:22Z

/AzurePipelines run

azure-pipelines · 2021-02-04T16:49:31Z

Azure Pipelines successfully started running 1 pipeline(s).

dgsudharsan · 2021-02-06T03:59:07Z

retest this please

prsunny · 2021-02-08T16:04:34Z

/AzurePipelines run

azure-pipelines · 2021-02-08T16:04:39Z

Commenter does not have sufficient privileges for PR 1611 in repo Azure/sonic-swss

Co-authored-by: dgsudharsan <sudharsan_gopalarat@dell.com>

aravindmani-1 · 2021-03-04T05:08:24Z

@daall

Looks like this PR is not merged into Azure master image.
Can we merge this PR since we need it for DellEMC Z9332f platform?.

Thanks,
Aravind.

Co-authored-by: dgsudharsan <sudharsan_gopalarat@dell.com>

kcudnik · 2022-02-19T07:54:13Z

this pr is explicitly removing port serdes if it exists on a given port, tis rises a question whether this port serdes should be treated like vlan is (that needs to be explicitly removed before removing port) or whether it can be treated like a queue (that will be auto removed when port is removed)

when implementing portserdes in virtual switch on sairedis lib, first approach will break backward compatibility with previous unittets that are actually removing port, they would need to be updated to remove port serdes as well or remove for both scenarios could be implemented

…onic-net#1611) Fix sonic-net/sonic-buildimage#7433 Right now config reload -l is getting failed due to an error. I guess the problem is here in sonic-utilities repo. If user does not provide filename with config reload -l, command = "{} -j {} -v DEVICE_METADATA.localhost.hwsku".format(SONIC_CFGGEN_PATH, filename) will not provide cfg_hwsku i.e. hwsku parameter as it should which will later cause problem here command = "{} -H -k {} --write-to-db".format(SONIC_CFGGEN_PATH, cfg_hwsku) as hwsku is not available around that time. that's why we notice errors like No such file or directory: 'None' as pasted in this issue. - How I did it To Fix the issue, moved the part where the code gets cfg_hwsku command = "{} -j {} -v DEVICE_METADATA.localhost.hwsku".format(SONIC_CFGGEN_PATH, file) to the same location it needed as we get filename by default. - How to verify it 'sudo config reload -l' Added test cases. Signed-off-by: Sangita Maity <samaity@linkedin.com>

Co-authored-by: dgsudharsan <sudharsan_gopalarat@dell.com>

Port serdes changes

830409a

daall requested review from daall, jleveque, lguohan and prsunny January 26, 2021 23:03

daall added the Request for 202012 Branch label Jan 26, 2021

jleveque suggested changes Jan 27, 2021

View reviewed changes

orchagent/portsorch.cpp Outdated Show resolved Hide resolved

orchagent/portsorch.cpp Outdated Show resolved Hide resolved

orchagent/portsorch.cpp Outdated Show resolved Hide resolved

dgsudharsan added 2 commits January 27, 2021 07:00

Addressing code review comments

ce6b2d9

Fixing build due to errors in logs

1ff5675

prsunny reviewed Jan 27, 2021

View reviewed changes

daall reviewed Jan 27, 2021

View reviewed changes

orchagent/portsorch.cpp Outdated Show resolved Hide resolved

Addressing code review comments

6d9c8f4

Merge branch 'master' of https://github.com/Azure/sonic-swss into sixtap

db00b00

prsunny previously approved these changes Jan 28, 2021

View reviewed changes

Optimizing the parsing of fields

bfb04c1

dgsudharsan dismissed prsunny’s stale review via bfb04c1 January 30, 2021 01:46

Added comments

accc403

prsunny previously approved these changes Feb 2, 2021

View reviewed changes

daall reviewed Feb 2, 2021

View reviewed changes

Removing the error log as per comments

6cf96c5

dgsudharsan dismissed prsunny’s stale review via 6cf96c5 February 2, 2021 23:59

Merge branch 'master' of https://github.com/Azure/sonic-swss into sixtap

d14d9e0

prsunny approved these changes Feb 9, 2021

View reviewed changes

jleveque approved these changes Feb 9, 2021

View reviewed changes

jleveque requested a review from daall February 9, 2021 18:09

daall approved these changes Feb 9, 2021

View reviewed changes

daall merged commit b8193d1 into sonic-net:master Feb 9, 2021

daall pushed a commit that referenced this pull request Feb 16, 2021

Migrate serdes programming to port serdes object (#1611)

66e0016

Co-authored-by: dgsudharsan <sudharsan_gopalarat@dell.com>

daall added the Included in 202012 Branch label Feb 16, 2021

DavidZagury pushed a commit to DavidZagury/sonic-swss that referenced this pull request Mar 4, 2021

Migrate serdes programming to port serdes object (sonic-net#1611)

c8276f9

Co-authored-by: dgsudharsan <sudharsan_gopalarat@dell.com>

aravindmani-1 mentioned this pull request Mar 8, 2021

DellEMC Z9332f pre-emphasis settings sonic-net/sonic-buildimage#6984

Merged

4 tasks

raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-swss that referenced this pull request Oct 5, 2021

Migrate serdes programming to port serdes object (sonic-net#1611)

21fe0ca

Co-authored-by: dgsudharsan <sudharsan_gopalarat@dell.com>

dgsudharsan deleted the sixtap branch March 9, 2023 02:00

Janetxxx pushed a commit to Janetxxx/sonic-swss that referenced this pull request Nov 10, 2025

Migrate serdes programming to port serdes object (sonic-net#1611)

e710443

Co-authored-by: dgsudharsan <sudharsan_gopalarat@dell.com>

Conversation

dgsudharsan commented Jan 26, 2021

Uh oh!

dgsudharsan commented Jan 26, 2021

Uh oh!

dgsudharsan commented Jan 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prsunny Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

dgsudharsan Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

prsunny Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

prsunny Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daall commented Jan 28, 2021

Uh oh!

dgsudharsan commented Jan 28, 2021

Uh oh!

dgsudharsan commented Jan 28, 2021

Uh oh!

dgsudharsan commented Jan 28, 2021

Uh oh!

dgsudharsan commented Jan 28, 2021

Uh oh!

dgsudharsan commented Jan 28, 2021

Uh oh!

prsunny left a comment

Choose a reason for hiding this comment

Uh oh!

dgsudharsan commented Jan 28, 2021

Uh oh!

dgsudharsan commented Jan 29, 2021

Uh oh!

dgsudharsan commented Jan 29, 2021

Uh oh!

prsunny commented Jan 30, 2021

Uh oh!

azure-pipelines bot commented Jan 30, 2021

Uh oh!

lguohan commented Jan 30, 2021

Uh oh!

prsunny commented Feb 2, 2021

Uh oh!

azure-pipelines bot commented Feb 2, 2021

Uh oh!

jleveque commented Feb 2, 2021

Uh oh!

daall Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

dgsudharsan Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

dgsudharsan commented Feb 4, 2021

Uh oh!

prsunny commented Feb 4, 2021

Uh oh!

azure-pipelines bot commented Feb 4, 2021

Uh oh!

dgsudharsan commented Feb 6, 2021

Uh oh!

prsunny commented Feb 8, 2021

Uh oh!

azure-pipelines bot commented Feb 8, 2021

Uh oh!

aravindmani-1 commented Mar 4, 2021

Uh oh!

kcudnik commented Feb 19, 2022

Uh oh!

Reviewers

Assignees