Skip to content

[framework] Remove MBEDTLS_USE_PSA_CRYPTO from PK module #167

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

Closed
wants to merge 1 commit into from

Conversation

valeriosetti
Copy link
Contributor

Description

This helps resolving Mbed-TLS/TF-PSA-Crypto#283

PR checklist

@valeriosetti valeriosetti self-assigned this May 7, 2025
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels May 7, 2025
@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label May 7, 2025
bjwtaylor
bjwtaylor previously approved these changes May 9, 2025
USE_PSA guard is not removed because the framework code can be used
also with the 3.6 LTS branch, but the guard is ignored when working
in tf-psa-crypto repo.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti valeriosetti force-pushed the issue283-framework branch from 4aaad73 to d1739b1 Compare May 9, 2025 15:35
@mpg mpg self-requested a review May 15, 2025 10:42
@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label May 15, 2025
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

I don't understand why this is needed now :)

/* In TF-PSA-Crypto USE_PSA is always enabled. However we cannot
* remove this guard because this test code can be used also with
* mbedtls-3.6 branch. */
#if defined(MBEDTLS_USE_PSA_CRYPTO) || defined(PSA_CRYPTO_API_VERSION_MAJOR)
Copy link
Contributor

@mpg mpg May 15, 2025

Choose a reason for hiding this comment

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

I don't think this is incorrect, but I don't understand why it would be needed now. Unless I'm missing something, MBEDTLS_USE_PSA_CRYPTO is still being defined in tf-psa-crypto for now (in config_adjust_legacy_from_psa.h) and still needs to be until we finish getting rid of it on the TLS side.

At some point in the future we'll stop defining it, and then I agree we'll need a change like this one, but in the meantime I don't understand why we'd need this. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point in the future we'll stop defining it

Arguably TF-PSA-Crypto should keep defining MBEDTLS_USE_PSA_CRYPTO and MBEDTLS_PSA_CRYPTO_CONFIG for at least as long as 3.6 is officially supported, to facilitate the life of third-party code that needs to work both with our LTS branch and our development branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point in the future we'll stop defining it, and then I agree we'll need a change like this one, but in the meantime I don't understand why we'd need this. Am I missing something?

No you are not. I was just thinking about this future case and I thought it could be nice to "prepare" framework for it. Since it's not needed I will remove it.

@valeriosetti
Copy link
Contributor Author

Closed as not necessary

@github-project-automation github-project-automation bot moved this from In Development to Done in Roadmap pull requests (new board) May 19, 2025
@valeriosetti valeriosetti deleted the issue283-framework branch May 19, 2025 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Development

Successfully merging this pull request may close these issues.

4 participants