drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

soburi · 2025-02-03T01:49:02Z

Add basic support for RaspberryPi Pico's SHA256 hardware accelerator.

soburi · 2025-02-14T22:22:08Z

Could you take a look if you have a bit of a while?

valeriosetti

The PR looks OK to me. I cannot test it locally because I don't have a board at hand, but at least the test that was extended builds correctly. I only left one minor non-blocking question/comment.

drivers/crypto/crypto_rpi_pico_sha256.c

ThreeEights · 2025-02-17T16:40:23Z

drivers/crypto/CMakeLists.txt

+zephyr_library_sources_ifdef(CONFIG_CRYPTO_SMARTBOND            crypto_smartbond.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_STM32                crypto_stm32.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_TINYCRYPT_SHIM       crypto_tc_shim.c)
+# zephyr-keep-sorted-stop


Sorting - good idea!

nit: I'd prefer it if the sorting is one commit, and the addition is a separate commit. The end result is the same, but it's easier to to see the two orthogonal things going on.

valeriosetti

Thanks for addressing previous comment. However reviewing the PR again I found a couple of new ones that I missed before. Sorry

drivers/crypto/crypto_rpi_pico_sha256.c

soburi · 2025-03-17T20:04:28Z

@ceolin @ThreeEights @ajf58

Could you take a look at it?

ajf58

The change looks functionally good, but I am concerned about the outright copy and paste of BSD code, then relabeling it as Apache 2.

I feel like someone else needs to comment on this.

ajf58 · 2025-03-17T21:23:15Z

drivers/crypto/CMakeLists.txt

+zephyr_library_sources_ifdef(CONFIG_CRYPTO_SMARTBOND            crypto_smartbond.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_STM32                crypto_stm32.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_TINYCRYPT_SHIM       crypto_tc_shim.c)
+# zephyr-keep-sorted-stop


nit: I'd prefer it if the sorting is one commit, and the addition is a separate commit. The end result is the same, but it's easier to to see the two orthogonal things going on.

ajf58 · 2025-03-17T21:53:37Z

drivers/crypto/crypto_rpi_pico_sha256.c

+LOG_MODULE_REGISTER(sha_rpi_pico_sha256, CONFIG_CRYPTO_LOG_LEVEL);
+
+/*
+ * This is based on the pico_sha256 implementation in the Pico-SDK.


Original code is a https://github.com/raspberrypi/pico-sdk/blob/2.1.0/src/rp2_common/pico_sha256/sha256.c , it's 3-Clause BSD (BSD-3-Clause).

I feel like this file has lifted-and-shifted too much for someone to ignore (many lines of code are the same, just formatted to meet the Zephyr Project's coding style).

I'd be happy if this file had a BSD-3-Clause on it. Zephyr must be compatible with this, we link against BSD-3-Clause all over the place. It's not easy t osee examples of that, however.

Alternatively, we could modify the version of the source code in the hal_rpi_pico. This is something I prefer to avoid, but that feels better at ensuring the software licence is upheld.

The codes derived from pico-sdk were separated into crypto_rpi_pico_sha256.h, and the license changed to BSD-3.

nit: I'd prefer it if the sorting is one commit, and the addition is a separate commit. The end result is the same, but it's easier to to see the two orthogonal things going on.

Addressed.

d3zd3z · 2025-05-20T22:38:23Z

To be clear here. Changing an existing license declaration on a file can only be done by permission of all of the contributing authors. Changing license declarations is much worse than bringing in the files with the licenses on them (and figuring out what needs to happen to use the code under the license the authors released the code under).

The license lines in the file are an indication, from the author, of the terms that they allow their code to be used under them. Removing those declarations is in bad-faith, and is something we have to be very careful never happens to code in this project.

nashif · 2025-05-21T13:50:48Z

This was discussed in the TSC and it was agreed that we will not accept non original, Apache licensed code for this type of driver. Please use another method of integration.

soburi · 2025-05-21T15:06:21Z

Thank you for your comment.

@d3zd3z @nashif

I am aware that this is not a good situation, and I am very sorry.
I also thank @ajf58 for pointing this out to me.

I think there is another way to deal with this, so I will carefully reconsider it.

ajf58 · 2025-05-22T20:42:07Z

I think there is another way to deal with this, so I will carefully reconsider it.

Agreed. There have been a few PRs recently where we need them to stay Apache 2.0. I think the best way to do this is in the HAL, which is where several of them originate from. I don't think there's much of a convention across the different vendor HALs here, but I'll take a look.

Add `zephyr-keep-sorted` and reorder file entries alphabetically. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

Update hal_rpi_pico to introduce the Zephyr adaptation for the sha256 driver. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

Add include paths and source files for the sha256 driver when enabled `CONFIG_PICOSDK_USE_SHA256` config. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

github-actions · 2025-06-15T23:01:36Z

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_rpi_pico	zephyrproject-rtos/hal_rpi_pico@`7b57b24`	zephyrproject-rtos/hal_rpi_pico#13	zephyrproject-rtos/hal_rpi_pico#13/files

⛔ DNM label due to: 1 project with PR revision

Note: This message is automatically posted and updated by the Manifest GitHub Action.

soburi · 2025-06-15T23:25:22Z

Add a public interface to the hal side and change the policy to use it.

valeriosetti · 2025-06-17T14:49:12Z

drivers/crypto/crypto_rpi_pico_sha256.c

+{
+	struct rpi_pico_sha256_data *data = dev->data;
+
+	assert(!data->state.locked);


Isn't assert() a bit too strong here? I suggest adding a mutex in struct rpi_pico_sha256_data and use it here:

lock the mutex

check data->state.locked and return if already set (releasing the mutex, of course before returning)

set data->state.locked (not on line 79 as it is done now)

unlock the mutex

remember to unset data->state.locked in rpi_pico_sha256_hash_session_free()

In this way you can have 2 threads willing to use the hash driver at the same time and then the one that comes later will just wait for the resource to be free.

The response has become slower.

It's true that assert is quite powerful, so I have modified it.
Your suggestion was to use a mutex, but the exclusive section completes in an extremely short time, so I implemented it using a spinlock.

Add basic support for RaspberryPi Pico's SHA256 hardware accelerator. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

Added sha256 accelerator to rpi_pico. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

Add rpi_pico2 to the crypto_hash test target. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

sonarqubecloud · 2025-06-27T05:58:28Z

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

soburi marked this pull request as draft February 3, 2025 01:49

zephyrbot added area: Crypto / RNG platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) labels Feb 3, 2025

zephyrbot requested review from ceolin, ThreeEights and yonsch February 3, 2025 01:49

zephyrbot assigned ceolin Feb 3, 2025

soburi mentioned this pull request Feb 3, 2025

TaskList for Supporting RaspberryPi Pico peripherals #53810

Open

55 tasks

soburi force-pushed the rpi_pico_sha256 branch from 0d6e6b8 to a2b3bb7 Compare February 3, 2025 02:32

soburi added the DNM This PR should not be merged (Do Not Merge) label Feb 3, 2025

soburi marked this pull request as ready for review February 3, 2025 04:02

valeriosetti self-requested a review February 10, 2025 08:31

soburi force-pushed the rpi_pico_sha256 branch from a2b3bb7 to 19c04b7 Compare February 11, 2025 20:31

soburi removed the DNM This PR should not be merged (Do Not Merge) label Feb 11, 2025

valeriosetti reviewed Feb 17, 2025

View reviewed changes

drivers/crypto/crypto_rpi_pico_sha256.c Outdated Show resolved Hide resolved

ThreeEights reviewed Feb 17, 2025

View reviewed changes

valeriosetti mentioned this pull request Feb 19, 2025

Add crypto support to TI cc23x0 SoC #84523

Merged

soburi force-pushed the rpi_pico_sha256 branch from 19c04b7 to 3f8ef55 Compare March 8, 2025 01:59

zephyrbot requested a review from ajf58 March 8, 2025 02:00

soburi requested review from valeriosetti and ThreeEights March 8, 2025 03:34

valeriosetti requested changes Mar 10, 2025

View reviewed changes

drivers/crypto/crypto_rpi_pico_sha256.c Outdated Show resolved Hide resolved

drivers/crypto/crypto_rpi_pico_sha256.c Outdated Show resolved Hide resolved

soburi force-pushed the rpi_pico_sha256 branch 2 times, most recently from a6aeaeb to f2ca146 Compare March 10, 2025 22:42

soburi requested a review from valeriosetti March 11, 2025 05:36

soburi force-pushed the rpi_pico_sha256 branch from f2ca146 to ee650de Compare March 14, 2025 13:15

valeriosetti previously approved these changes Mar 14, 2025

View reviewed changes

ajf58 reviewed Mar 17, 2025

View reviewed changes

soburi marked this pull request as draft March 22, 2025 15:27

soburi removed the TSC Topics that need TSC discussion label Mar 22, 2025

nashif added the Licensing The PR has licensing issues => licensing expert to review label Apr 16, 2025

nashif moved this from Todo to In Progress in TSC Attention Needed Apr 24, 2025

nashif moved this from In Progress to Done in TSC Attention Needed May 21, 2025

soburi added 3 commits June 16, 2025 07:47

drivers: crypto: sort file entries

1616dd5

Add `zephyr-keep-sorted` and reorder file entries alphabetically. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

manifest: Update hal_rpi_pico module

da0bf1a

Update hal_rpi_pico to introduce the Zephyr adaptation for the sha256 driver. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

modules: hal_rpi_pico: Add sha256 driver

8bb2bed

Add include paths and source files for the sha256 driver when enabled `CONFIG_PICOSDK_USE_SHA256` config. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

soburi force-pushed the rpi_pico_sha256 branch from 90f02be to da897ce Compare June 15, 2025 23:00

github-actions bot added manifest manifest-hal_rpi_pico DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Jun 15, 2025

soburi marked this pull request as ready for review June 15, 2025 23:40

soburi requested review from ajf58 and valeriosetti June 16, 2025 01:24

valeriosetti requested changes Jun 17, 2025

View reviewed changes

soburi force-pushed the rpi_pico_sha256 branch 2 times, most recently from ed7b1d2 to 43bbff2 Compare June 27, 2025 05:21

soburi added 3 commits June 27, 2025 14:32

drivers: crypto: Add initial support for rpi_pico sha256 accelerator

91f721b

Add basic support for RaspberryPi Pico's SHA256 hardware accelerator. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

boards: raspberrypi: rpi_pico2: Add sha256 accelerator

44108a5

Added sha256 accelerator to rpi_pico. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

tests: crypto: crypto_hash: Add rpi_pico2 to test target

236fd9d

Add rpi_pico2 to the crypto_hash test target. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

soburi force-pushed the rpi_pico_sha256 branch from 43bbff2 to 236fd9d Compare June 27, 2025 05:32

soburi requested a review from valeriosetti June 27, 2025 05:35

drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

Are you sure you want to change the base?

drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

Conversation

soburi commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soburi commented Feb 14, 2025

Uh oh!

valeriosetti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valeriosetti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

soburi commented Mar 17, 2025

Uh oh!

ajf58 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d3zd3z commented May 20, 2025

Uh oh!

nashif commented May 21, 2025

Uh oh!

soburi commented May 21, 2025

Uh oh!

ajf58 commented May 22, 2025

Uh oh!

github-actions bot commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soburi commented Jun 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 27, 2025

Quality Gate passed

Uh oh!

Uh oh!

soburi commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Jun 15, 2025 •

edited

Loading