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

[hmac, sw] Revert hash stop hang workaround #25594

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

andrea-caforio
Copy link
Contributor

@andrea-caforio andrea-caforio commented Dec 11, 2024

In line with the RTL fix in #24771 for the hash stop hang bug described in #24767, this commit reverts four commits that implemented a software workaround for the issue and only keeps the idle polling timeout implementation from #24944. This workaround and its corresponding documentation is now obsolete.

Reverted commits:

In line with the RTL fix in lowRISC#24771 for the hash stop hang bug
described in lowRISC#24767, this commit reverts four commits that implemented
a software workaround for the issue. This workaround and its
corresponding documentation is now obsolete.

Reverted commits:
  - e869152 lowRISC#25070
  - 2b072c4 lowRISC#25012
  - 7f32449 lowRISC#24966
  - f591bea lowRISC#24944

Signed-off-by: Andrea Caforio <[email protected]>
Co-authored-by: Martin Velay <[email protected]>
@andrea-caforio andrea-caforio self-assigned this Dec 11, 2024
@andrea-caforio andrea-caforio added the CI:Rerun Rerun failed CI jobs label Dec 12, 2024
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Dec 12, 2024
@andrea-caforio andrea-caforio marked this pull request as ready for review December 12, 2024 09:53
@andrea-caforio andrea-caforio requested a review from a team as a code owner December 12, 2024 09:53
@andrea-caforio andrea-caforio requested review from jon-flatley and removed request for a team December 12, 2024 09:53
Comment on lines 78 to 79
* TODO(#23191): It might be beneficial to have a timeout value for the polling.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing a plain revert of the other commits is fine and the approach is good, it's easier to review this.

What is not ideal is that it means to re-introduce this TODO here which got resolved by the workaround. Would it be possible to do a separate commit or even PR (whatever you prefer @andrea-caforio ) to resolve this? You can take inspiration from the reverted commits on how to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @vogelpi for the review and catching this issue. I reinstantiated the polling timeout implementation in a separate commit as to not spam the PR list with code that has already been review before.

The previously reverted commit f591bea (later refined in e869152)
contained a desirable timeout implementation for the HMAC idle
polling (see lowRISC#23191). This commit reinstantiates it.

Signed-off-by: Andrea Caforio <[email protected]>
Co-authored-by: Martin Velay <[email protected]>
Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @andrea-caforio !

@vogelpi vogelpi requested review from jadephilipoom and martin-velay and removed request for jon-flatley December 13, 2024 14:53
@vogelpi vogelpi mentioned this pull request Oct 30, 2024
13 tasks
Copy link
Contributor

@martin-velay martin-velay left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks @andrea-caforio for taking care of it !

@vogelpi vogelpi merged commit a0304b2 into lowRISC:master Dec 16, 2024
38 checks passed
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.

4 participants