sonic-sairedis icon indicating copy to clipboard operation
sonic-sairedis copied to clipboard

Manage LANES mapping on multi-ASIC platform

Open abohanyang opened this issue 3 years ago • 11 comments

This issue is related to the dynamic port breakout mode support on multi-ASIC platforms. When the port breakout mode changes, SWSS removes ports from LANES mapping and adds new ports to LANES mapping. This logic works properly on the single-chip platform (NPU switch type) but it does not work properly on the multi-ASIC platform (VOQ switch type) because LANES mapping is not created on the multi-ASIC platform.

To fix this issue, my proposal is to create LANES mapping on the multi-ASIC platform. Though the fix is straightforward, the comment in the existing codebase is a bit concerning to me. I am not sure if creating LANES mapping on the multi-ASIC platform could affect backward compatibility. @arlakshm

abohanyang avatar Sep 16 '22 07:09 abohanyang

Hi @kcudnik , Could you take another look at the latest commit and see if it looks good to you? The code analysis fails but I don't think it's related to my change.

abohanyang avatar Oct 03 '22 21:10 abohanyang

/Azp run Azure.sonic-sairedis

arlakshm avatar Oct 10 '22 20:10 arlakshm

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Oct 10 '22 20:10 azure-pipelines[bot]

/AzurePipelines run Azure.sonic-sairedis

abohanyang avatar Oct 24 '22 21:10 abohanyang

Commenter does not have sufficient privileges for PR 1127 in repo sonic-net/sonic-sairedis

azure-pipelines[bot] avatar Oct 24 '22 21:10 azure-pipelines[bot]

/azpw run Azure.sonic-sairedis

abohanyang avatar Oct 24 '22 22:10 abohanyang

/AzurePipelines run Azure.sonic-sairedis

mssonicbld avatar Oct 24 '22 22:10 mssonicbld

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Oct 24 '22 22:10 azure-pipelines[bot]

/azpw run LGTM analysis: C/C++

abohanyang avatar Oct 24 '22 23:10 abohanyang

/AzurePipelines run LGTM analysis: C/C++

mssonicbld avatar Oct 24 '22 23:10 mssonicbld

No pipelines are associated with this pull request.

azure-pipelines[bot] avatar Oct 24 '22 23:10 azure-pipelines[bot]

@shyam77git, @anamehra - would you like to review this as well? thanks

rlhui avatar Nov 19 '22 04:11 rlhui

Hi @abohanyang How is this change tested? Could you please add any details on the test and logs?

anamehra avatar Nov 19 '22 04:11 anamehra

@abohanyang Don't see mention of External-PHY/ gbsyncd in this. Did this change consider boards/LCs with External-PHY (gbsyncd vertical)? be it single-ASIC or multi-ASIC? If not, how to assess impact of this change to boards/LCs having External-PHY?

shyam77git avatar Nov 19 '22 05:11 shyam77git

@abohanyang Don't see mention of External-PHY/ gbsyncd in this. Did this change consider boards/LCs with External-PHY (gbsyncd vertical)? be it single-ASIC or multi-ASIC? If not, how to assess impact of this change to boards/LCs having External-PHY?

@shyam77git Actually, we just found a new issue last week when we test this change against a system with BCM54182 PHY (using broncos SAI). I submit the new change and you can see that helperCheckLaneMap is called only for NPU and VOQ. This change won't have any impact on SAI_SWITCH_TYPE_PHY.

abohanyang avatar Nov 21 '22 07:11 abohanyang

@anamehra @shyam77git , can you please review this PR and signoff if no more comments?

arlakshm avatar Dec 02 '22 05:12 arlakshm

@anamehra @shyam77git , can you please review this PR again and signoff if no more comments?

arlakshm avatar Dec 06 '22 20:12 arlakshm

LGTM! @abdosi , anything specific you want to check for chassis-packet?

anamehra avatar Dec 12 '22 20:12 anamehra

can we update PR title to Manage LANES mapping on VOQ system. VOQ can be single/multi-asic.

abdosi avatar Dec 14 '22 18:12 abdosi

@rlhui The change looks ok to me.

mlok-nokia avatar Dec 15 '22 00:12 mlok-nokia

can we update PR title to Manage LANES mapping on VOQ system. VOQ can be single/multi-asic.

@abohanyang , if you could please just update the title to reflect the exact scope of this, then we can merge this, thanks.

rlhui avatar Dec 16 '22 01:12 rlhui

can we update PR title to Manage LANES mapping on VOQ system. VOQ can be single/multi-asic.

@abohanyang , if you could please just update the title to reflect the exact scope of this, then we can merge this, thanks.

Done

abohanyang avatar Dec 16 '22 02:12 abohanyang