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

Add support for port_id in object discovery for dpu #171

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AmithGspn
Copy link

Adding port_id support in objects_discovery

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this was removed?

Copy link
Author

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.

Copy link
Contributor

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.

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 ?

Copy link
Contributor

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

@mgheorghe
Copy link

lets set this as draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants