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

BLE - Nordic: Release crypto cell when not in use. #9372

Merged
merged 1 commit into from Feb 12, 2019

Conversation

Projects
None yet
9 participants
@pan-
Copy link
Member

pan- commented Jan 14, 2019

Description

Previously, the CryptoToolbox was allocated once as part of the security manager.
This was inneficient memory wise as it is only use to prepare key at initialization
and when we need to compute shared keys.
This was also inneficient power consumption wise as the Crypto cell was kept enabled even
when it wasn't used.

This fix creates a CryptoToolbox whenever it is needed and release it once it has fulfilled its
purpose. Note that CryptoToolbox allocation happens on the heap as mbed tls data structure are huge
and there's an high risk of crushing the stack.

Fixes: #9276

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

BLE - Nordic: Release crypto cell when not in use.
Previously, the CryptoToolbox was allocated once as part of the security manager.
This was inneficient memory wise as it is only use to prepare key at initialization
and when we need to compute shared keys.
This was also inneficient power consumption wise as the Crypto cell was kept enabled even
when it wasn't used.

This fix creates a CryptoToolbox whenever it is needed and release it once it has fulfilled its
purpose. Note that CryptoToolbox allocation happens on the heap as mbed tls data structure are huge
and there's an high risk of crushing the stack.
@pan-

This comment has been minimized.

Copy link
Member Author

pan- commented Jan 14, 2019

This fix is just a part of the solution. I still get consumption issues when the CC is actually used. It looks like SaSi_LibFini() doesn't release some resources.

It is similar to the issue described here.

@RonEld Any idea ?

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Jan 14, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Jan 14, 2019

@pan-, thank you for your changes.
@ARMmbed/mbed-os-pan @ARMmbed/mbed-os-maintainers please review.

@RonEld

This comment has been minimized.

Copy link
Contributor

RonEld commented Jan 14, 2019

@pan- SaSi_LibFini() should release any resource used by Cryptocell.
Is the issue a matter of power consumption as in the link you referenced, or a resource leak?

Is Sasi_LibFini() being called in your process? Do you call mbedtls_platform_teardown() when you finish you cryptographic operations? Is its reference counter set to zero, causing the Sasi_LibFini() to be called?

@pan-

This comment has been minimized.

Copy link
Member Author

pan- commented Jan 14, 2019

@RonEld The issue remaining is a power consumption one (just like the one I link). I call mbedtls_platform_teardown and Sasi_LibFini is called as well. I was wondering if the crypto cell lib in mbed was behind the crypto cell lib used by nordic in their 15th SDK ? Mbed use SDK 14.2 now.

@RonEld

This comment has been minimized.

Copy link
Contributor

RonEld commented Jan 14, 2019

@pan- Thanks for the confirmation
The cryptocell library should be the official release delivered to Nordic.
As mentioned in the link:

We have found some issues with the CC310 library and are performing fixes. The new CC310 library will be released as a part of the next SDK. The new CC310 library should solve the power consumption issues.

This makes me wonder whether they have made modifications to the code

@RonEld

This comment has been minimized.

Copy link
Contributor

RonEld commented Jan 14, 2019

In addition, perhaps if we upgrade to SDK 15, the power consumption issue will be fixed, in case the actual cause is in the SDK itself

@pan-

This comment has been minimized.

Copy link
Member Author

pan- commented Jan 14, 2019

@RonEld I can try that tomorrow, we have a PR open that updates the SDK to v15.

@TacoGrandeTX

This comment has been minimized.

Copy link
Contributor

TacoGrandeTX commented Jan 16, 2019

@RonEld I've made a capture with PPK on SDK15 using the HeartRateMonitor example (below) without this PR. This shows the time going from advertising to connected. The CC310 current bias is gone. @pan- Do you have a PPK to make a similar capture for comparison?

sdk15_connecting

@0xc0170

0xc0170 approved these changes Feb 5, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 5, 2019

@ARMmbed/mbed-os-pan What is the status for this PR?

@pan-

This comment has been minimized.

Copy link
Member Author

pan- commented Feb 5, 2019

Nothing changed, the PR is still good to go. However I do not have any update to provide on the issue exposed by Sasi_LibFini but that is out of the scope of this PR.

@RonEld

This comment has been minimized.

Copy link
Contributor

RonEld commented Feb 5, 2019

@pan- what issue exposed by `Sasi_LibFini``?

As you can see in #9372 (comment) , I believe that using SDK 15 fixes the power consumption issue.
What am I missing?

@pan-

This comment has been minimized.

Copy link
Member Author

pan- commented Feb 5, 2019

@RonEld As stated above:

It looks like SaSi_LibFini() doesn't release some resources.

I haven't tried with SDK15 and @TacoGrandeTX experiments are just an indication that the issue might have been fixed. It is not a proof (I really hope it is fixed!).
With SDK14.2, I observed that after computation and release of the CC, the power consumption remains as high as when the CC is enabled. The HeartRate monitor does not force any security and Android/iOS doesn't ask for pairing right after connection so the crypto is not use at all.

@RonEld

This comment has been minimized.

Copy link
Contributor

RonEld commented Feb 5, 2019

@pan- Thanks for the confirmation.
For the record, @TacoGrandeTX experiments on SDK 15 were only on updating the SDK, without updating CC library. IMHO, the issue is not in SaSi_LibFini(), but rather that NRF_CRYPTOCELL->ENABLE = 0; register in the board did not actually terminate the CC clock, which was fixed, I believe in SDK 15.
Have you managed to test on SDK 15 as well?

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 5, 2019

@ARMmbed/mbed-os-pan Please review to finalize the reviews

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 11, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 11, 2019

Test run: FAILED

Summary: 3 of 8 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
@alekla01

This comment has been minimized.

Copy link
Contributor

alekla01 commented Feb 11, 2019

Restarted CI

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 11, 2019

@alekla01 Why was this build aborted?

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 11, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 2
Build artifacts

@cmonr cmonr merged commit 58cba25 into ARMmbed:master Feb 12, 2019

22 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10245 cycles (+875 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.