-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add support for port_id in object discovery for dpu #171
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: AmithGspn <[email protected]>
@@ -489,17 +489,6 @@ def objects_discovery(self): | |||
for idx, oid in enumerate(port_oids): | |||
self.create_alias(f"PORT_{idx}", 'SAI_OBJECT_TYPE_PORT', oid) | |||
|
|||
|
|||
status, data = self.get(dot1q_br_oid, ["SAI_BRIDGE_ATTR_PORT_LIST", "1:oid:0x0"], False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this was removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is removed since its not yet implemented for bmv2. This is the reason for the lines to be deleted. These were causing issues for PORT_ID object discovery. Once it is implemented in bmv2 The lines can be put back in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BMv2 is not the only target for SAI-C. It works for many other ASICs. Removing the code is not the right way to fix the issues. As an option, in case BMv2 returns SAI_STATUS_NOT_IMPLEMENTED or SAI_STATUS_NOT_SUPPORTED, some additional check can be added instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what will be a good way to make it work on BMv2 as well considering BMv2 does not support those SAI calls, should we add a if dpu dont execute the code else if npu execute it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgheorghe , in case BMv2 SAI implementation does not support these calls, it should return SAI_STATUS_NOT_SUPPORTED
or SAI_STATUS_NOT_IMPLEMENTED
status code and we should add appropriate status check here. In case for some reason it returns SAI_STATUS_SUCCESS
or just crashes (both options I'd consider as DASH/BMv2 bug), we can add a workaround-check on ASIC type which is initialized here:
https://github.com/opencomputeproject/SAI-Challenger/blob/40e09a6c17bad1bc35c57772cdf6bbebd3387a50/common/sai_dpu.py#L10C1-L10C1
lets set this as draft |
Adding port_id support in objects_discovery