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.

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

Remove lane map divide restriction #290

Merged
merged 2 commits into from
May 14, 2018
Merged

Conversation

kcudnik
Copy link
Collaborator

@kcudnik kcudnik commented Jan 30, 2018

No description provided.

*/

if (laneMap.size() < 32 || (laneMap.size() % 16 != 0))
if (laneMap.size() < 32)
Copy link
Contributor

Choose a reason for hiding this comment

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

it is interesting to have this check? any reason? if the switch has less than 32 ports, then we print error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean why do check the lanemap size at all? there could be switch with less than 32 ports.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i added this at first to rule out some errors and mistakes with configuration file since we didnt used any other configuration, but can there be a switch with sonic with 5 ports? :P

@stcheng
Copy link
Contributor

stcheng commented Apr 6, 2018

this is still not merged?

@pavel-shirshov
Copy link
Contributor

I think all of the checks should be removed.
But current brcm SAI gives us wrong number of lanes, which is an error.

@kcudnik
Copy link
Collaborator Author

kcudnik commented Apr 9, 2018

You can merge this any time, and those checks were put there initially to prevent bugs on SAI from vendos, and at the beginning we only dealed with 1 type of switch so there was no issue, also i think right now we will not have less than 32 ports in switch and i would leave this as a safety check and remove when switch will less than 32 ports come to play

@lguohan
Copy link
Contributor

lguohan commented Apr 9, 2018

ok. can you check the ERROR to NOTICE?

@kcudnik
Copy link
Collaborator Author

kcudnik commented Apr 9, 2018

I removed this restriction at all

@pavel-shirshov pavel-shirshov merged commit 75ed784 into sonic-net:master May 14, 2018
pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
* Remove lane map divide restriction

* Remove lane size restriction
jianyuewu pushed a commit to jianyuewu/sonic-sairedis that referenced this pull request Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants