Restructure cryptocell #13404

gpsimenos · 2020-08-10T09:13:39Z

Summary of changes

mbed-os/features/cryptocell/FEATURE_CRYPTOCELL310 directory moved to
mbed-os/connectivity/drivers/mbedtls/FEATURE_CRYPTOCELL310 and restructured according to the internal proposal.

This is one of a series of PRs aiming to clean up the mbed-os directory structure. The intention is to create a consistent tree among mbed components, following the below structure:

[component name]
├── mbed_lib.json                     // Each component comes with their own config that's appended to the App/System ├── config 
├── CMakeList.txt                     // This depends on the build system choices, part of another PREQ
├── README.md
├── include                           // The top-level include path
│   └── [component name]              // Public headers that can be accessed by the developers - `#include [component name]/*.h`
│       └── internal                  // Internal headers that implement or are included by a public header or are needed by other Mbed OS libraries - `#include [component name]/internal/*.h`
├── source                            // Source files and private headers, available only inside the component's source directory
└── tests
    ├── <framework>                   // <framework> test folder (for example CPPUTEST)
    │   └── <test suite one>          // Can be either source file or a directory
    ├── UNITTESTS                     // Unit tests (previously in UNITTESTS) folder using googletest framework (to be renamed to gtest at a later stage)
    │   └── <unit test suite one>     // Can be either source file or a directory
    └── TESTS                         // Greentea tests (previously in TESTS) folder (to be renamed to greentea at a later stage)
        └── <greentea test suite one> // Can be either source file or a directory
 
mbed-os
└── TESTS (to be renamed to greentea at a later stage)
    ├── integration   
    └── system

Impact of changes

None

Migration actions required

None

Documentation

None

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@evedon
@ashok-rao

ciarmcom · 2020-08-10T11:00:31Z

@gpsimenos, thank you for your changes.
@evedon @ashok-rao @ARMmbed/mbed-os-maintainers please review.

mergify · 2020-08-10T13:25:30Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

LDong-Arm

Question for @ARMmbed/mbed-os-connectivity:

Here we treat cryptocell as an mbedtls dependency, thus it's moved into connectivity/drivers/mbedtls. But it's also used by some PalCrypto* functions in the Nordic Cordio BLE driver (link) that aren't called anywhere.

Do we really want cryptocell in BLE? Going forward, if we apply dependency restriction, the Nordic Cordio driver may assume cryptocell is available (if FEATURE_CRYPTOCELL310 is still defined) but its path will not be actually accessible?

LDong-Arm · 2020-08-10T14:00:43Z

.astyleignore

@@ -3,7 +3,7 @@
 ^connectivity/libraries/mbed-coap
 ^connectivity/libraries/ppp
 ^connectivity/drivers/emac
-^features/cryptocell
+^connectivity/drivers/mbedtls/cryptocell


Suggested change

^connectivity/drivers/mbedtls/cryptocell

^connectivity/drivers/mbedtls/FEATURE_CRYPTOCELL310

LDong-Arm · 2020-08-10T14:40:05Z

@paul-szczepanek-arm suggests putting cryptocell in connectivity/drivers/cryptocell - being in drivers it'd still be vaguely an internal component. We need to revise the directory structure plan so everyone agrees I guess.

gpsimenos · 2020-08-11T08:30:47Z

Re-restructured to put cryptocell in connectivity/drivers. Rebased.

0xc0170

Move cryptocell

Please extend the commit message with a reason (just add details from the PR description)

LDong-Arm · 2020-08-11T09:53:20Z

@bulislaw Are you happy with cryptocell in connectivity/drivers/FEATURE_CRYPTOCELL310, one level above connectivity/drivers/mbedtls/FEATURE_CRYPTOCELL310 previously proposed?

gpsimenos · 2020-08-11T10:38:29Z

Rebased. Commit message extended.

Pull request has been modified.

LDong-Arm

LGTM if there's no objection from anyone towards the new path.

gpsimenos · 2020-08-12T09:25:56Z

Let's run CI @0xc0170

mergify · 2020-08-12T09:27:52Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

gpsimenos · 2020-08-12T09:34:38Z

Rebased.

0xc0170 · 2020-08-12T09:34:48Z

I'll start CI jobs on all PRs after one 5.15 job completes (should be within a hour or so)

0xc0170 · 2020-08-12T10:34:32Z

CI started

mbed-ci · 2020-08-12T11:29:36Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-ARM	✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	✔️

0xc0170 · 2020-08-12T19:38:03Z

@bulislaw Are you happy with cryptocell in connectivity/drivers/FEATURE_CRYPTOCELL310, one level above connectivity/drivers/mbedtls/FEATURE_CRYPTOCELL310 previously proposed?

I dont see this answered or anyone else. Fine with me

bulislaw · 2020-08-13T07:27:20Z

Sorry for being late to the part, like always. It's not really a connectivity driver... it's a specifically HW acceleration for TLS, I'd prefer to keep it with TLS so people can actually find it. I wouldn't think to look for it in connectivity drivers. Not sure why wouldn't we have a drivers in tls?

mbed-os/features/cryptocell/FEATURE_CRYPTOCELL310 directory moved to mbed-os/connectivity/drivers/cryptocell/FEATURE_CRYPTOCELL310 and restructured according to the internal proposal.

Pull request has been modified.

gpsimenos · 2020-08-13T08:27:52Z

Un-re-restructured to put cryptocell back in connectivity/drivers/mbedtls. Rebased.

LDong-Arm · 2020-08-13T08:32:38Z

@bulislaw Cryptocell is used in Cordio, so Mbed TLS is not the only component that uses it. The Cordio functions that use it are not used by Mbed OS BLE, but still if we switch to a new build system and Cryptocell becomes unavailable to Cordio, we can get build errors.
So my question for the connectivity team is whether we can get rid of those usages in Cordio.

paul-szczepanek-arm · 2020-08-13T08:43:53Z

crytpocell wasn't in mbedtls before and I don't think it's an element of mbedtls, it's just used by it but there's ~~nothing mbedtls specific in it~~ there's quite a lot in fact

bulislaw · 2020-08-13T09:02:48Z

In a nutshell it's a crypto accelerator, it's not meant to be directly exposed to the users, but used via MbedTLS/Crypto. If it's used in BLE I'm ok to put it under connectivity/drivers/crypto or mbedtls.

donatieng · 2020-08-13T09:29:24Z

Agreed with Bartek, ultimately we'll have to move it out of connectivity - as it's crypto related.
For now the proposed connectivity/drivers/crypto sounds good to me.

rajkan01 · 2020-08-14T11:41:05Z

@0xc0170 Could you check, why the continuous-integration/jenkins/pr-head was not reported the status

LDong-Arm · 2020-08-14T11:43:50Z

@0xc0170 Could you check, why the continuous-integration/jenkins/pr-head was not reported the status

It shows up like this when CI hasn't been run since the last update.

0xc0170 · 2020-08-14T11:49:58Z

CI started

rajkan01 · 2020-08-14T12:01:33Z

@0xc0170 Could you check, why the continuous-integration/jenkins/pr-head was not reported the status

It shows up like this when CI hasn't been run since the last update.

Thanks for checking

mbed-ci · 2020-08-14T12:46:25Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-ARM	✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	✔️

rajkan01 · 2020-08-14T13:06:17Z

@0xc0170 CI passed for this PR, please merge

mergify bot added the needs: work label Aug 10, 2020

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Aug 10, 2020

ciarmcom requested review from ashok-rao, evedon and a team August 10, 2020 11:00

ciarmcom added the needs: review label Aug 10, 2020

mergify bot removed the needs: review label Aug 10, 2020

LDong-Arm reviewed Aug 10, 2020

View reviewed changes

gpsimenos force-pushed the gp-cryptocell-restruct branch from 1076cab to 2e1d0eb Compare August 11, 2020 08:29

paul-szczepanek-arm approved these changes Aug 11, 2020

View reviewed changes

0xc0170 previously requested changes Aug 11, 2020

View reviewed changes

gpsimenos force-pushed the gp-cryptocell-restruct branch from 2e1d0eb to a28bc09 Compare August 11, 2020 10:37

LDong-Arm approved these changes Aug 11, 2020

View reviewed changes

gpsimenos force-pushed the gp-cryptocell-restruct branch from a28bc09 to 6598e54 Compare August 12, 2020 09:31

0xc0170 added needs: CI and removed needs: work labels Aug 12, 2020

0xc0170 previously approved these changes Aug 12, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Aug 12, 2020

0xc0170 added needs: review and removed ready for merge labels Aug 13, 2020

Move cryptocell

a4fc83d

mbed-os/features/cryptocell/FEATURE_CRYPTOCELL310 directory moved to mbed-os/connectivity/drivers/cryptocell/FEATURE_CRYPTOCELL310 and restructured according to the internal proposal.

gpsimenos force-pushed the gp-cryptocell-restruct branch from 6598e54 to a4fc83d Compare August 13, 2020 08:26

0xc0170 approved these changes Aug 14, 2020

View reviewed changes

0xc0170 merged commit 94c62f2 into ARMmbed:master Aug 14, 2020

0xc0170 removed the needs: review label Aug 14, 2020

mbedmain added release-version: 6.2.1 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Aug 16, 2020

	^connectivity/drivers/mbedtls/cryptocell
	^connectivity/drivers/mbedtls/FEATURE_CRYPTOCELL310

Restructure cryptocell #13404

Restructure cryptocell #13404

Uh oh!

Conversation

gpsimenos commented Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Aug 10, 2020

Uh oh!

mergify bot commented Aug 10, 2020

Uh oh!

LDong-Arm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LDong-Arm Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

LDong-Arm commented Aug 10, 2020

Uh oh!

gpsimenos commented Aug 11, 2020

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

LDong-Arm commented Aug 11, 2020

Uh oh!

gpsimenos commented Aug 11, 2020

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

gpsimenos commented Aug 12, 2020

Uh oh!

mergify bot commented Aug 12, 2020

Uh oh!

gpsimenos commented Aug 12, 2020

Uh oh!

0xc0170 commented Aug 12, 2020

Uh oh!

0xc0170 commented Aug 12, 2020

Uh oh!

mbed-ci commented Aug 12, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

0xc0170 commented Aug 12, 2020

Uh oh!

bulislaw commented Aug 13, 2020

Uh oh!

gpsimenos commented Aug 13, 2020

Uh oh!

LDong-Arm commented Aug 13, 2020

Uh oh!

paul-szczepanek-arm commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bulislaw commented Aug 13, 2020

Uh oh!

donatieng commented Aug 13, 2020

Uh oh!

rajkan01 commented Aug 14, 2020

Uh oh!

LDong-Arm commented Aug 14, 2020

Uh oh!

0xc0170 commented Aug 14, 2020

Uh oh!

rajkan01 commented Aug 14, 2020

Uh oh!

mbed-ci commented Aug 14, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

gpsimenos commented Aug 10, 2020 •

edited

Loading

LDong-Arm left a comment •

edited

Loading

paul-szczepanek-arm commented Aug 13, 2020 •

edited

Loading