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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Feb 12, 2019
Merged

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

merged 1 commit into from
Feb 12, 2019

Conversation

pan-
Copy link
Member

@pan- 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

@pan-
BLE - Nordic: Release crypto cell when not in use.
a10a10a 
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-
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 a team January 14, 2019 18:00
@ciarmcom
Copy link
Member

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

@RonEld
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-
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
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
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-
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
Copy link
Contributor

@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
Copy link
Contributor

0xc0170 commented Feb 5, 2019

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

@pan-
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
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-
Copy link
Member Author

pan- commented Feb 5, 2019 •
edited
Loading

@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
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
Copy link
Contributor

0xc0170 commented Feb 5, 2019

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

@RonEld RonEld mentioned this pull request Feb 10, 2019
Merged
@bearsh bearsh mentioned this pull request Feb 11, 2019
Closed
3 tasks
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 11, 2019

CI started

@mbed-ci
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
Copy link
Contributor

Restarted CI

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 11, 2019

@alekla01 Why was this build aborted?

@mbed-ci
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paul-szczepanek-arm paul-szczepanek-arm paul-szczepanek-arm approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
release-version: 5.11.5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nrf52: after disconnect, device doesn't go into sleep again
9 participants
@pan- @ciarmcom @RonEld @TacoGrandeTX @0xc0170 @mbed-ci @alekla01 @paul-szczepanek-arm @cmonr