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

entropy: rpi_pico: implement entropy driver for RP2350 #83346

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

entropy: rpi_pico: implement entropy driver for RP2350 #83346

wants to merge 1 commit into from
+96 −0

Conversation

xudongzheng
Copy link
Contributor

Use get_rand_64() from Pico SDK for entropy.

This is somewhat based on #66764.

@xudongzheng xudongzheng force-pushed the rp2350-entropy-pr branch 2 times, most recently from df8de81 to be3cfa0 Compare December 24, 2024 03:59
soburi
soburi reviewed Dec 25, 2024
View reviewed changes
modules/hal_rpi_pico/CMakeLists.txt
@@ -151,6 +151,18 @@ if(CONFIG_HAS_RPI_PICO)
zephyr_include_directories_ifdef(CONFIG_PICOSDK_USE_CLAIM
${common_dir}/hardware_claim/include)

zephyr_library_sources_ifdef(CONFIG_PICOSDK_USE_RAND
${rp2_common_dir}/hardware_timer/timer.c
${rp2_common_dir}/pico_rand/rand.c
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code uses pico-sdk's spin-lock feature, not zephyr's one.
I think it can't use as is.
We need to implements zephyr's own TRNG driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to instances such as https://github.com/raspberrypi/pico-sdk/blob/95ea6acad131124694cda1c162c52cd30e0aece0/src/rp2_common/pico_rand/rand.c#L132-L133?

I'm not quite sure what the implications are if using the pico-sdk spinlock. Would it mess with interrupts?

Ideally https://github.com/raspberrypi/pico-sdk/blob/95ea6acad131124694cda1c162c52cd30e0aece0/src/rp2_common/pico_rand/rand.c#L134-L169 is code that I would like to avoid reproducing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it mess with interrupts?

See my comment above regarding wrapping the Pico-SDK call. If it gets wrapped, then there shouldn't be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable idea. I've wrapped entropy_rpi_pico_get_entropy() with Zephyr lock/unlock.

ceolin
ceolin reviewed Dec 29, 2024
View reviewed changes
drivers/entropy/entropy_rpi_pico.c Outdated
static int entropy_rpi_pico_get_entropy(const struct device *dev, uint8_t *buf, uint16_t len)
{
__ASSERT_NO_MSG(buf != NULL);
uint8_t *buf_bytes = (uint8_t *)buf;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this cast ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I took that part from an earlier version of another entropy driver. The cast has been removed.

tomi-font
tomi-font reviewed Dec 30, 2024
View reviewed changes
Copy link
Collaborator

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any answer to @yonsch's comment in your previous PR #66764 (comment)?

@xudongzheng
Copy link
Contributor Author

xudongzheng commented Dec 30, 2024 •
edited
Loading

Do you have any answer to @yonsch's comment in your previous PR #66764 (comment)?

@yonsch is referring to the fact that RP2040 lacks a hardware random number generator so it uses other sources as seed.

RP2350 has a hardware random number generator and uses that in pico-sdk: https://github.com/raspberrypi/pico-sdk/blob/95ea6acad131124694cda1c162c52cd30e0aece0/src/rp2_common/pico_rand/rand.c#L121

@xudongzheng xudongzheng force-pushed the rp2350-entropy-pr branch 2 times, most recently from 338dfec to f069b11 Compare December 30, 2024 15:49
ajf58
ajf58 suggested changes Dec 31, 2024
View reviewed changes
Copy link
Contributor

@ajf58 ajf58 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a few things need addressing here.

drivers/entropy/entropy_rpi_pico.c Outdated
uint8_t *buf_bytes = buf;

while (len > 0) {
uint64_t word = get_rand_64();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be wrapped in a Zephyr spinlock. There would be a slight performance hit because of the Zephyr spinlock + Pico SDK's hardware-based spin_lock (and we're holding a k_spinlock for longer than strictly required), but this would ensure that users get the Zephyr-provided guarantees about spinlocks.

nit: word has overloaded meaning, I'd avoid this variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable renamed to value.

modules/hal_rpi_pico/CMakeLists.txt
@@ -151,6 +151,18 @@ if(CONFIG_HAS_RPI_PICO)
zephyr_include_directories_ifdef(CONFIG_PICOSDK_USE_CLAIM
${common_dir}/hardware_claim/include)

zephyr_library_sources_ifdef(CONFIG_PICOSDK_USE_RAND
${rp2_common_dir}/hardware_timer/timer.c
${rp2_common_dir}/pico_rand/rand.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it mess with interrupts?

See my comment above regarding wrapping the Pico-SDK call. If it gets wrapped, then there shouldn't be a problem.

modules/hal_rpi_pico/CMakeLists.txt
@@ -151,6 +151,18 @@ if(CONFIG_HAS_RPI_PICO)
zephyr_include_directories_ifdef(CONFIG_PICOSDK_USE_CLAIM
${common_dir}/hardware_claim/include)

zephyr_library_sources_ifdef(CONFIG_PICOSDK_USE_RAND
${rp2_common_dir}/hardware_timer/timer.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs thinking about, in particular, how this interacts, by default, with Pico SDK's alarm pool. I think PICO_TIME_DEFAULT_ALARM_POOL_DISABLED will need to be set, at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the earlier PR #66764, PICO_TIME_DEFAULT_ALARM_POOL_DISABLED=1 was needed due to building ${common_dir}/pico_time/time.c. However PICO_TIME_DEFAULT_ALARM_POOL_DISABLED doesn't seem to be referenced in either of the two C files added in this PR:

  • ${rp2_common_dir}/hardware_timer/timer.c
  • ${rp2_common_dir}/pico_rand/rand.c

@xudongzheng
entropy: rpi_pico: implement entropy driver for RP2350
14d6fb1 
Use get_rand_64() from Pico SDK for entropy.

Signed-off-by: Xudong Zheng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomi-font tomi-font tomi-font left review comments

@ceolin ceolin ceolin left review comments

@soburi soburi soburi left review comments

@ajf58 ajf58 ajf58 requested changes

@yonsch yonsch Awaiting requested review from yonsch

At least 2 approving reviews are required to merge this pull request.

Assignees

@ceolin ceolin

Labels
area: Crypto / RNG platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@xudongzheng @soburi @ajf58 @ceolin @tomi-font @zephyrbot