Bluetooth: Mesh: adapting configuration parameters

[alxelax]

Commit adapts configuration ble mesh, trusted storage and mbedtls psa parameters to be able to run mesh samples and tests.

[omkar3141]

BT_MESH_NRF_SECURITY_ENABLER : Please use different name. It sounds as if, if this option is not there, mesh is not secure. Also seems to give some kind of implied meaning about security leading to misunderstanding. Suggestion BT_MESH_USE_NRF_SECURITY (soemthing like that)

Instead of using select, can we just use "imply" everywhere to avoid problems? That way all relavant stuff can go in prj.conf and it is easier to debug and understand. I think we also had discussions in past on imply vs select and I think it was concluded that imply is better.

I hope all of these options will come up with detailed help text to explain what they are doing. Common request we see several times from users about detailed explanation of KConfig options within the KConfig file itself.

[NordicBuilder]

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 3

Inputs:

Sources:

more details

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (0)

Outputs:

Toolchain

Version:
Build docker image:

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

• ❌ Toolchain
• ❌ Build twister
• ❌ Integration tests

Note: This message is automatically posted and updated by the CI

[NordicBuilder]

You can find the documentation preview for this PR at this link.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

[alxelax]

Just continue discussion here:
@PavelVPV 's feedback:

We should avoid any changes in sdk-zephyr as much as possible to reduce burden of doing upmerges. So to me sdk-nrf is the
right place for these options.

agree

Perhaps, BT_MESH_PSA_CRYPTO_STORAGE should be a choice if we want to give customers choice between trusted
storage and secure storage as I'm not sure imply vs select will work like this. Perhaps, same for other such Kconfigs?

All configuration options, those I added, are not public, and I didn't assume customer has ability to choose anything.
Should we allow selecting crypto backend at all?

Not sure why BT_LONG_WQ is changed there. If it is due to psa, then it should be upstreamed because IIRC no mesh code
runs from BT_LONG_WQ.

Upstream uses Secure Storage, not Trusted Storage. We see default value in upstream is sufficient to pass PTS tests and run samples (probably some configurations consume more memory, but we didn't meet them yet). NCS samples based on downstream and Trusted Storage falls into hard fault with this stack overflow immediately after start.

I doubt we need to fix it in upstream with such symptoms.

[alxelax]

BT_MESH_NRF_SECURITY_ENABLER : Please use different name. It sounds as if, if this option is not there, mesh is not secure. Also seems to give some kind of implied meaning about security leading to misunderstanding. Suggestion BT_MESH_USE_NRF_SECURITY (soemthing like that)

I just followed the name that is used already in upmerge branch: b16461d
The introduced solution in upmerge is temporary according to commit comment. I made similar in Mesh to have it as permanent.
Anycase it shouldn't be visible for customers since the option is not public.

Instead of using select, can we just use "imply" everywhere to avoid problems? That way all relavant stuff can go in prj.conf and it is easier to debug and understand. I think we also had discussions in past on imply vs select and I think it was concluded that imply is better.

I used imply everywhere except one case. Actually this option MBEDTLS_PSA_CRYPTO_STORAGE_C should be added automatically via upstream dependencies. However, it depends on Secure Storage. That is correct since Secure Storage is the only crypto backend in upstrea. In NCS we have to enable it manually. This option is internal mbedtls and must be. It enables mbedtls PSA API to persistent memory. It is not possible reassign in other files.

I hope all of these options will come up with detailed help text to explain what they are doing. Common request we see several times from users about detailed explanation of KConfig options within the KConfig file itself.

No. There will not be any text explanation since it makes option public and visible for customer. This was done intentionally.

[PavelVPV]

All configuration options, those I added, are not public, and I didn't assume customer has ability to choose anything. Should we allow selecting crypto backend at all?

Not sure, probably it should not be selectable. But I'm not sure if the overwritten Kconfig will also overwrite select SECURE_STORAGE from the original option definition. So just make sure the correct storage is picked up in NCS.

Upstream uses Secure Storage, not Trusted Storage. We see default value in upstream is sufficient to pass PTS tests and run samples (probably some configurations consume more memory, but we didn't meet them yet). NCS samples based on downstream and Trusted Storage falls into hard fault with this stack overflow immediately after start.

I doubt we need to fix it in upstream with such symptoms.

True, but then it is still not related to mesh, but to bt long wq with trusted storage, so should be outside of this Kconfig file.

[alxelax]

All configuration options, those I added, are not public, and I didn't assume customer has ability to choose anything. Should we allow selecting crypto backend at all?

Not sure, probably it should not be selectable. But I'm not sure if the overwritten Kconfig will also overwrite select SECURE_STORAGE from the original option definition. So just make sure the correct storage is picked up in NCS.

SECURE_STORAGE is not selected at the moment. There shouldn't be any problem with this.

Upstream uses Secure Storage, not Trusted Storage. We see default value in upstream is sufficient to pass PTS tests and run samples (probably some configurations consume more memory, but we didn't meet them yet). NCS samples based on downstream and Trusted Storage falls into hard fault with this stack overflow immediately after start.
I doubt we need to fix it in upstream with such symptoms.

True, but then it is still not related to mesh, but to bt long wq with trusted storage, so should be outside of this Kconfig file.

I have no idea where this option should be adapted for Mesh purpose. I created JIRA ticket and added reference to it here.
Let's keep it here now as temporary solution to pass ble mesh CI for upmerge.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[omkar3141]

Looks ok to me then.

[alxelax]

Commit from this PR has been taken into upmerge PR(#19720). Close it.