portsorch: initial support for link-training

[ds952811]

Signed-off-by: Dante Su dante.su@broadcom.com

What I did

1. Add Link-Training support to portsorch, while Gearbox is not in the scope
2. Add support for remote speed advertisement
3. Add support for capability checker

Why I did it

1. In the case of DAC, static pre-calibrated pre-emphasis is rarely available on SONIC, as most of the ODM are expecting this to be done dynamically at runtime via link-training, hence we'll need this feature to improve the link quality
2. Help users identify the cause of autoneg link failure
3. Avoid un-necessary SAI calls for autoneg and link-training controls if this feature is not supported

How I verified it

1. Manual test
2. Ran the Unit-tests to the corresponding changes

Details if related
This is a subsequent PR of #2149
HLD: sonic-net/SONiC#924, sonic-net/SONiC#925

[ds952811]

DVS tarball:

https://sonic-build.azurewebsites.net/api/sonic/artifacts?branchName=master&platform=vs&buildId=115121&target=target%2Fdocker-sonic-vs.gz

Test result

ds952811@qemu-ub20:dvs_tests$ ./test.sh
+ '[' '!' -e /var/run/redis/redis.sock ']'
+ docker exec -it vs bash -c 'echo '\''/tmp/core.%e.%p'\'' | tee /proc/sys/kernel/core_pattern'
/tmp/core.%e.%p
+ docker exec -it vs cat /proc/sys/kernel/core_pattern
/tmp/core.%e.%p
+ docker exec -it vs md5sum /usr/bin/orchagent
3622aba59d7164249d1dbdbb9dd74927  /usr/bin/orchagent
+ docker cp ./sonic-swss/orchagent/orchagent vs:/usr/bin/
+ docker exec -it vs md5sum /usr/bin/orchagent
98cbb1956d3dfdd10f235647f15e0c24  /usr/bin/orchagent
+ cd ./sonic-swss/tests
+ sudo python3 -m pytest --dvsname=vs test_port_an.py test_port_lt.py test_route.py test_storm_control.py test_sub_port_intf.py
============================= test session starts ==============================
platform linux -- Python 3.8.10, pytest-7.1.1, pluggy-1.0.0
rootdir: /projects/fastpath/ds952811/community/sonic-swss/tests
plugins: flaky-3.7.0
collected 39 items

test_port_an.py .....                                                    [ 12%]
test_port_lt.py ....                                                     [ 23%]
test_route.py ......sss....                                              [ 56%]
test_storm_control.py .......                                            [ 74%]
test_sub_port_intf.py ..........                                         [100%]
...... omitted ......
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========== 36 passed, 3 skipped, 13 warnings in 1361.03s (0:22:41) ============

[prsunny]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[prgeor]

can we avoid TODO?

[ds952811]

It's about the SAI commit associated with sonic-sairedis, hence there is not much we can do right now, this will need to be revisited once the SAI commit is updated

[ds952811]

Just for a record, this port attribute is already merged into the master branch of SAI repo. a few months ago.

[prgeor]

@ds952811 please resolve merge conflict

[ds952811]

@ds952811 please resolve merge conflict

Done, please check it out

[prgeor]

please add your changes after this check.

[ds952811]

Done

[prgeor]

is this check better? because -1 means not initialized?

if (p.m_cap_an == 0)

[ds952811]

Although it's okay to use (p.m_cap_an == 0) right here, but please note we don't want to process the AN request if it's neither initialized nor supported. Hence I prefer keeping 'if (p.m_cap_an < 1)' right here.

[prgeor]

can you add the p.m_cap_an value in the log?

[ds952811]

Done

[prgeor]

there will be too many doTask, for redability, could rename to doRefreshPortAnLtStateTask()

[ds952811]

In that case, this will become an infra. change to the orchagent, and is outside the scope of this PR, it's better to revisit the timer support in the orchagent if this is a concern

[prgeor]

m_port_state_poll[] is getting updated in two different contexts?

[ds952811]

No, it's a selectable timer built on top of file descriptors, and it's handled in the same context of other tasks.
i.e.
https://github.com/Azure/sonic-swss/blob/eb79ca4a524899f57995abd5ae5ec5f773ec85d0/orchagent/orchdaemon.cpp#L620

[prgeor]

can you use PORT_STATE_POLL_NONE instead of 0 ?

[ds952811]

Done

[prgeor]

why assume LT is enabled? it should be "unknown"?

[ds952811]

The rx status is either 'trained' or 'not trained', and we already have a checker in line 7337 to only process this timer task when LT is enabled, and the status is supposed to be 'on'
i.e. 'on' means LT is enabled, while none of further failure/status is available.

[ds952811]

i.e.
If both SAI_PORT_ATTR_LINK_TRAINING_RX_STATUS and SAI_PORT_ATTR_LINK_TRAINING_FAILURE_STATUS are not supported in the SAI, the possible LT status is as follows:

• off
• on

If both SAI_PORT_ATTR_LINK_TRAINING_RX_STATUS is supported while SAI_PORT_ATTR_LINK_TRAINING_FAILURE_STATUS is not, the possible LT status is as follows:

• off
• on (When the poller is not yet executed)
• not_trained
• trained

If both SAI_PORT_ATTR_LINK_TRAINING_RX_STATUS and SAI_PORT_ATTR_LINK_TRAINING_FAILURE_STATUS are supported, the possible LT status is as follows:

• off
• on (When the poller is not yet executed)
• trained
• frame_lock (i.e. not_trained)
• snr_low (i.e. not_trained)
• timeout (i.e. not_trained)

[prgeor]

@ds952811 could you please rebase with latest master?

[ds952811]

@ds952811 could you please rebase with latest master?

Done, please check it out, thanks

[ds952811]

/azp run

[azure-pipelines]

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

[ds952811]

The sanity check failed as follows, which has nothing to do with this PR

fake_portorch.cpp:521:6: error: no declaration matches 'bool PortsOrch::setPortFec(swss::Port&, sai_port_fec_mode_t)'
 bool PortsOrch::setPortFec(Port &port, sai_port_fec_mode_t mode)
      ^~~~~~~~~
In file included from fake_portorch.cpp:9:
../../../orchagent/portsorch.h:317:10: note: candidate is: 'bool PortsOrch::setPortFec(swss::Port&, std::__cxx11::string&)'
     bool setPortFec(Port &port, std::string &mode);
          ^~~~~~~~~~
../../../orchagent/portsorch.h:76:7: note: 'class PortsOrch' defined here
 class PortsOrch : public Orch, public Subject
       ^~~~~~~~~
make[4]: *** [Makefile:1568: p4orch_tests-fake_portorch.o] Error 1

[ds952811]

/azpw run

[mssonicbld]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ds952811]

It failed as follows, which has nothing to do with this PR

==================================== ERRORS ====================================
_______________ ERROR at setup of test_mclagFdb_basic_config_add _______________

tp = <class 'AssertionError'>, value = None, tb = None

    def reraise(tp, value, tb=None):
        try:
            if value is None:
                value = tp()
            if value.__traceback__ is not tb:
>               raise value.with_traceback(tb)

/usr/lib/python3/dist-packages/six.py:702: 

[adyeung]

@prsunny please help merge

[vboykox]

@ds952811 is it possible to monitor ilnk-training status in case setting LT on/off is not supported?

[ds952811]

It's done by dropping the checker in the status poller, but why do we need this? If the LT on/off is not supported (i.e. standalone link-training), it's most unlikely the LT status get operations are implemented. Best Wishes, Dante (Kuo-Jung) Su

…

On Fri, Aug 5, 2022 at 1:56 AM Volodymyr Boiko ***@***.***> wrote: @ds952811 <https://github.com/ds952811> is it possible to monitor ilnk-training status in case setting LT on/off is not supported? — Reply to this email directly, view it on GitHub <#2359 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMLISKNUAJMIA7B675PZEL3VXP74TANCNFSM52BMTXOQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.

[vboykox]

It's done by dropping the checker in the status poller, but why do we need this? If the LT on/off is not supported (i.e. standalone link-training), it's most unlikely the LT status get operations are implemented. Best Wishes, Dante (Kuo-Jung) Su
…
On Fri, Aug 5, 2022 at 1:56 AM Volodymyr Boiko @.> wrote: @ds952811 https://github.com/ds952811 is it possible to monitor ilnk-training status in case setting LT on/off is not supported? — Reply to this email directly, view it on GitHub <#2359 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMLISKNUAJMIA7B675PZEL3VXP74TANCNFSM52BMTXOQ . You are receiving this because you were mentioned.Message ID: @.>
-- This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.

Even if standalone is not supported we might want to provide oper status after running AN - if AN is completed we might want to show the link is trained.
Or I miss something?

[ds952811]

Yes, this is a valid use case, we could activate the LT status poller when either AN or LT is enabled.
But how do we deal with the RFE in the community? i.e. Should I raise a new PR for this?

[vboykox]

Yes, this is a valid use case, we could activate the LT status poller when either AN or LT is enabled. But how do we deal with the RFE in the community? i.e. Should I raise a new PR for this?

@ds952811 do you mean PR for documentation? I think yes, better to have the idea reviewed in community and documented first before chaning the behavior.

[ds952811]

While the requested feature is working on the BRCM chips, I do not know what will be the case for the non-BRCM chips, and the poller will generate a series of error logs if the LT status retrieval is not supported by the chip. (Either PHY or MAC).
And the link-training status for AutoNeg is mainly for debugging purposes. Hence it's better to stick with the current implementation as of now, and we could circle back.

Note: In the case of BRCM private image, we do have LT status poller enabled for both AN and LT, and it's for debugging the link failure for AN. e.g. LT is likely to fail on the optics, hence having AN enabled on a optic is most likely fail at this case.

[vboykox]

@ds952811 the question why the poller should generate the error logs if status "not supported" is returned from SAI call?
If the not_supported is returned, we can poll silently/stop polling the port at all for LT status.

[ds952811]

Yep, you're absolutely right about it, we could suppress the error logs if NOT_SUPPORTED is returned from the SAI library. Let's raise a new PR for this enhancement.