Skip to content

Skip csr version check when cra_ring_root doesn't exist #195

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

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

zibaiwan
Copy link
Contributor

@zibaiwan zibaiwan commented Nov 3, 2022

In the system integrator, in the auto-discovery string, added a new field at the end of the device section to indicate whether the cra_ring_root exist or not. The runtime will parse the new field from the auto-discovery string and skip csr version check if cra_ring_root doesn't exit. Changed the csr_version to be optional.

Updated the forward-compatibility auto-discovery string test. Note: the previous test had an issue: The number of fields per device global is fixed (here to 1), not variable. I had to update that as well. Cherry picked from: 024154d

@zibaiwan zibaiwan force-pushed the remove-csr-version-check branch from 2ccac09 to 9966787 Compare November 3, 2022 17:05
@zibaiwan zibaiwan requested a review from pcolberg November 3, 2022 17:06
Copy link
Contributor

@pcolberg pcolberg left a comment

Choose a reason for hiding this comment

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

Thanks @zibaiwan! The logic looks fine, only some minor comments on the implementation.

@zibaiwan zibaiwan force-pushed the remove-csr-version-check branch from 9966787 to 4774009 Compare November 7, 2022 15:09
@zibaiwan zibaiwan self-assigned this Nov 7, 2022
@zibaiwan zibaiwan force-pushed the remove-csr-version-check branch 2 times, most recently from eb29930 to 348c8a6 Compare November 7, 2022 15:25
pcolberg
pcolberg previously approved these changes Nov 8, 2022
Copy link
Contributor

@pcolberg pcolberg left a comment

Choose a reason for hiding this comment

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

Thanks @zibaiwan, looks good!

pcolberg
pcolberg previously approved these changes Nov 8, 2022
@zibaiwan zibaiwan marked this pull request as draft November 9, 2022 14:52
@zibaiwan
Copy link
Contributor Author

zibaiwan commented Nov 9, 2022

Integration test failed due to the assertion in the acl_kernel_cra_read, the very first read will not have the kern->csr_version set. Fixing the code.

@zibaiwan zibaiwan force-pushed the remove-csr-version-check branch 2 times, most recently from bc237cb to b840804 Compare November 9, 2022 14:58
Copy link
Contributor

@pcolberg pcolberg left a comment

Choose a reason for hiding this comment

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

Thanks @zibaiwan for the testing!

@zibaiwan zibaiwan force-pushed the remove-csr-version-check branch from b840804 to 63d6a70 Compare November 9, 2022 22:09
@pcolberg pcolberg added the enhancement New feature or request label Nov 9, 2022
@pcolberg pcolberg added this to the 2023.1 milestone Nov 9, 2022
@zibaiwan zibaiwan force-pushed the remove-csr-version-check branch 2 times, most recently from 3007898 to ce1f640 Compare November 14, 2022 15:22
@zibaiwan zibaiwan marked this pull request as ready for review November 14, 2022 15:23
1. Parse the new field from the auto-discovery string
2. Changed csr_version to be optional.
3.Skip csr version check if cra_ring_root doesn't exit.
4.Update the forward-compatibility auto-discovery string test. Note:previous test had an issue: The number of fields per device global is fixed (here to 1), not variable. I had to update that as well.
5.Add new unit tests.
@zibaiwan zibaiwan force-pushed the remove-csr-version-check branch from ce1f640 to d431835 Compare November 14, 2022 15:26
Copy link
Contributor

@pcolberg pcolberg left a comment

Choose a reason for hiding this comment

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

Thanks @zibaiwan!

@zibaiwan
Copy link
Contributor Author

Test results look good. Merging into the main branch.

@zibaiwan zibaiwan merged commit 71ae4bb into intel:main Nov 18, 2022
@zibaiwan zibaiwan deleted the remove-csr-version-check branch November 18, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants