simplify flash area api#6492
Conversation
WalkthroughSecret storage now performs bulk flash writes via flash_area_write_data instead of per-quadword writes; results are checked and temporary data cleared. secret_bhk_regenerate writes eight words in a single bulk call and defers flash locking until after mpu_restore by calling flash_lock_write. STM32U5 flash layer added local quadword-size macros and made flash_write_quadword static. The quadword write API, its helpers, and related macros were removed from storage headers, low-level flash interface, and tests. USE_FLASH_BURST defines were consolidated into stm32u5_common.py and removed from several model-specific configs. Sequence Diagram(s)sequenceDiagram
participant Secret as Secret module
participant FlashArea as flash_area API
participant FlashLL as flash low-level
participant MPU as MPU/secure context
Note over Secret,FlashArea: secret_write / secret_bhk_regenerate flow
Secret->>FlashArea: flash_area_write_data(offset, data, length)
FlashArea->>FlashLL: choose write path (byte/word/burst)
FlashLL-->>FlashArea: write result
FlashArea-->>Secret: return result (success/failure)
Secret->>MPU: mpu_restore()
Secret->>FlashLL: flash_lock_write()
FlashLL-->>Secret: lock result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| model | device_test | click_test | persistence_test |
|---|---|---|---|
| T2T1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3B1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3T1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3W1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
Latest CI run: 22759619446
TychoVrahe
force-pushed
the
tychovrahe/u3/prep/flash_refactor
branch
3 times, most recently
from
5b28a31 to
e94a96a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/site_scons/models/stm32u5_common.py (1)
10-12: DeriveFLASH_BURST_SIZEfromFLASH_BURST_WORDSto avoid config drift.This keeps one source of truth if burst width changes later.
Proposed refactor
("USE_FLASH_BURST", "1"), ("FLASH_BURST_WORDS", "32"), - ("FLASH_BURST_SIZE", "128"), + ("FLASH_BURST_SIZE", "(FLASH_BURST_WORDS * sizeof(uint32_t))"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/site_scons/models/stm32u5_common.py` around lines 10 - 12, Replace the hardcoded ("FLASH_BURST_SIZE", "128") with a value computed from FLASH_BURST_WORDS so the size cannot drift; compute FLASH_BURST_SIZE as FLASH_BURST_WORDS * 4 (bytes per word) and store it as a string to match existing tuples (i.e. derive FLASH_BURST_SIZE from the FLASH_BURST_WORDS symbol rather than a separate literal), updating the tuple that contains ("USE_FLASH_BURST", "1"), ("FLASH_BURST_WORDS", "32"), ("FLASH_BURST_SIZE", ...) accordingly.core/embed/sec/secret/stm32u5/secret.c (1)
431-437: Replace magic8with aSECRET_BHK_LEN-derived word count.Line 431 hardcodes the BHK buffer length; deriving it from
SECRET_BHK_LENavoids silent drift.Proposed refactor
- uint32_t val[8] = {0}; + uint32_t val[SECRET_BHK_LEN / sizeof(uint32_t)] = {0};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/sec/secret/stm32u5/secret.c` around lines 431 - 437, The hardcoded array size "8" should be derived from SECRET_BHK_LEN so the BHK word count can't drift; change the static uint32_t val[8] and the loop bound to compute words = (SECRET_BHK_LEN + sizeof(uint32_t) - 1) / sizeof(uint32_t) (or SECRET_BHK_LEN / sizeof(uint32_t) if SECRET_BHK_LEN is guaranteed word-aligned), allocate uint32_t val[words] (or use a VLA/stack allocation), iterate for (int j = 0; j < words; j++) filling val[j] via rng_get(), and pass sizeof(val) (or words * sizeof(uint32_t)) to flash_area_write_data, memzero and ensure to keep existing behavior (reference symbols: val, rng_get, flash_area_write_data, memzero, ensure, SECRET_BHK_LEN).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/embed/sec/secret/stm32u5/secret.c`:
- Around line 147-150: secret_write currently forwards const uint8_t *data to
flash_area_write_data which expects flash-aligned data (and casts to const
uint32_t *), but secret_write neither enforces nor documents this alignment;
update secret_write to either validate that data is aligned to the flash block
size (e.g. check pointer and length against FLASH_WRITE_ALIGNMENT / flash block
size) or add clear documentation to its function signature/comments that callers
must pass an aligned buffer, and adjust callers if necessary; additionally
replace the hardcoded local array declaration uint32_t val[8] with a
maintainable sized declaration using SECRET_BHK_LEN (e.g. uint32_t
val[SECRET_BHK_LEN / sizeof(uint32_t)]) so the BHK buffer size derives from
SECRET_BHK_LEN and stays consistent with the regeneration loop that uses
ARRAY_LENGTH(val) and sizeof(val).
---
Nitpick comments:
In `@core/embed/sec/secret/stm32u5/secret.c`:
- Around line 431-437: The hardcoded array size "8" should be derived from
SECRET_BHK_LEN so the BHK word count can't drift; change the static uint32_t
val[8] and the loop bound to compute words = (SECRET_BHK_LEN + sizeof(uint32_t)
- 1) / sizeof(uint32_t) (or SECRET_BHK_LEN / sizeof(uint32_t) if SECRET_BHK_LEN
is guaranteed word-aligned), allocate uint32_t val[words] (or use a VLA/stack
allocation), iterate for (int j = 0; j < words; j++) filling val[j] via
rng_get(), and pass sizeof(val) (or words * sizeof(uint32_t)) to
flash_area_write_data, memzero and ensure to keep existing behavior (reference
symbols: val, rng_get, flash_area_write_data, memzero, ensure, SECRET_BHK_LEN).
In `@core/site_scons/models/stm32u5_common.py`:
- Around line 10-12: Replace the hardcoded ("FLASH_BURST_SIZE", "128") with a
value computed from FLASH_BURST_WORDS so the size cannot drift; compute
FLASH_BURST_SIZE as FLASH_BURST_WORDS * 4 (bytes per word) and store it as a
string to match existing tuples (i.e. derive FLASH_BURST_SIZE from the
FLASH_BURST_WORDS symbol rather than a separate literal), updating the tuple
that contains ("USE_FLASH_BURST", "1"), ("FLASH_BURST_WORDS", "32"),
("FLASH_BURST_SIZE", ...) accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/embed/sec/secret/stm32u5/secret.ccore/embed/sys/flash/stm32u5/flash.ccore/site_scons/models/D002/discovery2.pycore/site_scons/models/T3B1/trezor_t3b1_revB.pycore/site_scons/models/T3T1/trezor_t3t1_revE.pycore/site_scons/models/T3W1/trezor_t3w1_revA.pycore/site_scons/models/T3W1/trezor_t3w1_revB.pycore/site_scons/models/T3W1/trezor_t3w1_revC.pycore/site_scons/models/stm32u5_common.pystorage/flash_area.cstorage/flash_area.hstorage/flash_ll.hstorage/tests/c/flash.c
💤 Files with no reviewable changes (8)
- core/site_scons/models/T3T1/trezor_t3t1_revE.py
- storage/flash_area.h
- core/site_scons/models/T3W1/trezor_t3w1_revC.py
- core/site_scons/models/T3W1/trezor_t3w1_revB.py
- core/site_scons/models/T3B1/trezor_t3b1_revB.py
- core/site_scons/models/T3W1/trezor_t3w1_revA.py
- storage/tests/c/flash.c
- core/site_scons/models/D002/discovery2.py
| secbool result = flash_area_write_data(&SECRET_AREA, offset, data, len); | ||
| ensure(flash_lock_write(), "secret write"); | ||
| mpu_restore(mpu_mode); | ||
| return sectrue; | ||
| return result; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Inspect flash_area_write_data and its cast patterns ==="
fd -t f "flash_area.c" | head -5Repository: trezor/trezor-firmware
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 2) Look for uint32_t cast patterns in flash_area write path ==="
rg -n 'uint32_t.*\*data|data32|const uint32_t' storage/ core/ --max-count=20Repository: trezor/trezor-firmware
Length of output: 19458
🏁 Script executed:
#!/bin/bash
#!/bin/bash
set -euo pipefail
echo "=== 3) Examine secret_write implementation and callers ==="
fd -t f "secret.c" core/embed/sec/secret/ | xargs wc -lRepository: trezor/trezor-firmware
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 4) Check lines 147-150 context in secret.c ==="
fd -t f "secret.c" core/embed/sec/secret/stm32u5/ | xargs cat -n | sed -n '140,160p'Repository: trezor/trezor-firmware
Length of output: 892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 5) Check lines 431-437 context for BHK regeneration ==="
fd -t f "secret.c" core/embed/sec/secret/stm32u5/ | xargs cat -n | sed -n '425,445p'Repository: trezor/trezor-firmware
Length of output: 920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 6) Search for alignment/aliasing build flags ==="
rg -n 'strict-aliasing|UNALIGN|align' . -t c -t h --max-count=15Repository: trezor/trezor-firmware
Length of output: 26882
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Find SECRET_BHK_LEN or similar constant ==="
rg -n 'SECRET_BHK_LEN|BHK_LEN|#define.*8.*BHK' core/embed/sec/secret/ -A2 -B2Repository: trezor/trezor-firmware
Length of output: 2061
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 2) Check flash_area.h alignment requirements ==="
fd -t f "flash_area.h" | xargs sed -n '70,95p'Repository: trezor/trezor-firmware
Length of output: 1529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 3) Examine flash_area_write_data_padded signature and callers ==="
rg -n 'flash_area_write_data' core/embed/sec/secret/stm32u5/secret.c -B3 -A1Repository: trezor/trezor-firmware
Length of output: 653
Confirm alignment expectations in secret_write and improve BHK array size maintainability.
At line 147, secret_write passes const uint8_t *data directly to flash_area_write_data, which casts it to const uint32_t * (line 162 in flash_area.c) and dereferences it. The flash_area.h header documents that data must be aligned to flash block size, but secret_write does not enforce or document this contract to callers—add alignment documentation to its signature or ensure all callers provide properly aligned buffers.
For lines 431–437, the BHK regeneration loop correctly uses ARRAY_LENGTH(val) and sizeof(val), so that pattern is sound. However, val[8] is hardcoded while elsewhere the codebase uses SECRET_BHK_LEN / sizeof(uint32_t) (line 401). Consider defining the array size as uint32_t val[SECRET_BHK_LEN / sizeof(uint32_t)] for consistency and maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/embed/sec/secret/stm32u5/secret.c` around lines 147 - 150, secret_write
currently forwards const uint8_t *data to flash_area_write_data which expects
flash-aligned data (and casts to const uint32_t *), but secret_write neither
enforces nor documents this alignment; update secret_write to either validate
that data is aligned to the flash block size (e.g. check pointer and length
against FLASH_WRITE_ALIGNMENT / flash block size) or add clear documentation to
its function signature/comments that callers must pass an aligned buffer, and
adjust callers if necessary; additionally replace the hardcoded local array
declaration uint32_t val[8] with a maintainable sized declaration using
SECRET_BHK_LEN (e.g. uint32_t val[SECRET_BHK_LEN / sizeof(uint32_t)]) so the BHK
buffer size derives from SECRET_BHK_LEN and stays consistent with the
regeneration loop that uses ARRAY_LENGTH(val) and sizeof(val).
There was a problem hiding this comment.
coderabbit is right, strictly speaking we break some rules here.
HAL function requires that data address is aligned to 32bits, which we incorrectly translate to requirement for block alignment.
The 32 bit requirement is presumably because of unaligned access to the data, but we have that enabled so it works anyway.
As this is also not handled elsewhere in codebase, i suggest we dont deal with tis in this PR, but potentially fix this as separate issue.
what do you think, @cepetr ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yes, I agree. Unaligned arrays are handled correctly on cortex-m.
However, I think we should reconsider this practice we agreed on previously, as it could cause significant issues in the future on platforms that do not support it.
|
|
||
| #define FLASH_QUADWORD_WORDS (4) | ||
| #define FLASH_QUADWORD_SIZE (FLASH_QUADWORD_WORDS * sizeof(uint32_t)) | ||
|
|
There was a problem hiding this comment.
flash_write_quadword() at the end of file may be static now
| secbool result = flash_area_write_data(&SECRET_AREA, offset, data, len); | ||
| ensure(flash_lock_write(), "secret write"); | ||
| mpu_restore(mpu_mode); | ||
| return sectrue; | ||
| return result; |
There was a problem hiding this comment.
Yes, I agree. Unaligned arrays are handled correctly on cortex-m.
However, I think we should reconsider this practice we agreed on previously, as it could cause significant issues in the future on platforms that do not support it.
TychoVrahe
force-pushed
the
tychovrahe/u3/prep/flash_refactor
branch
from
e94a96a to
5477623
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/embed/sec/secret/stm32u5/secret.c (1)
439-441:⚠️ Potential issue | 🟠 MajorLock flash before restoring the MPU state.
mpu_restore()returns with interrupts enabled, so this ordering leaves flash unlocked for a window where other code can run. Please relock first, then restore, likesecret_write()does.Proposed fix
- mpu_restore(mpu_mode); - - ensure(flash_lock_write(), "Failed regenerating BHK"); + ensure(flash_lock_write(), "Failed regenerating BHK"); + mpu_restore(mpu_mode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/sec/secret/stm32u5/secret.c` around lines 439 - 441, The flash is being re-locked after calling mpu_restore(), which returns with interrupts enabled and leaves flash unlocked briefly; change the ordering to call flash_lock_write() (and ensure it succeeds) before calling mpu_restore(), mirroring the safe ordering used in secret_write() so flash is relocked while interrupts are still disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/embed/sec/secret/stm32u5/secret.c`:
- Around line 439-441: The flash is being re-locked after calling mpu_restore(),
which returns with interrupts enabled and leaves flash unlocked briefly; change
the ordering to call flash_lock_write() (and ensure it succeeds) before calling
mpu_restore(), mirroring the safe ordering used in secret_write() so flash is
relocked while interrupts are still disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd29b1ad-08dc-4a1a-8a39-1f2ed433133f
📒 Files selected for processing (13)
core/embed/sec/secret/stm32u5/secret.ccore/embed/sys/flash/stm32u5/flash.ccore/site_scons/models/D002/discovery2.pycore/site_scons/models/T3B1/trezor_t3b1_revB.pycore/site_scons/models/T3T1/trezor_t3t1_revE.pycore/site_scons/models/T3W1/trezor_t3w1_revA.pycore/site_scons/models/T3W1/trezor_t3w1_revB.pycore/site_scons/models/T3W1/trezor_t3w1_revC.pycore/site_scons/models/stm32u5_common.pystorage/flash_area.cstorage/flash_area.hstorage/flash_ll.hstorage/tests/c/flash.c
💤 Files with no reviewable changes (8)
- storage/tests/c/flash.c
- storage/flash_area.h
- core/site_scons/models/T3W1/trezor_t3w1_revA.py
- core/site_scons/models/T3W1/trezor_t3w1_revB.py
- core/site_scons/models/T3T1/trezor_t3t1_revE.py
- core/site_scons/models/T3W1/trezor_t3w1_revC.py
- core/site_scons/models/T3B1/trezor_t3b1_revB.py
- core/site_scons/models/D002/discovery2.py
🚧 Files skipped from review as they are similar to previous changes (2)
- core/site_scons/models/stm32u5_common.py
- core/embed/sys/flash/stm32u5/flash.c
[no changelog]
TychoVrahe
force-pushed
the
tychovrahe/u3/prep/flash_refactor
branch
from
5477623 to
998f4f2
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/flash_ll.h (1)
77-79:⚠️ Potential issue | 🟡 MinorThe burst API comment is now stale.
Burst size is no longer fixed in the driver, so “128-byte burst” is no longer always true. Please describe this as one flash burst, or otherwise reference the target-specific size.
📝 Suggested wording
-// Writes a 128-byte burst to specified 'offset' inside a flash 'sector' +// Writes one flash burst to specified 'offset' inside a flash 'sector'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/flash_ll.h` around lines 77 - 79, Update the stale comment for flash_write_burst: remove the hard-coded "128-byte burst" wording and instead describe the operation as writing "one flash burst" or note that the burst size is target-dependent (e.g., "writes a single flash burst to the specified 'offset' inside a flash 'sector'; burst size depends on target"). Ensure the documented symbol is flash_write_burst(uint16_t sector, uint32_t offset, const uint32_t *data) so readers know this applies to that API.
🧹 Nitpick comments (1)
core/embed/sys/flash/stm32u5/flash.c (1)
28-29: Add a compile-time guard betweenFLASH_BLOCK_WORDSand the new local quadword size.
flash_write_block()still takesflash_block_t, but these macros hardcode four words independently. If a future U5 config changesFLASH_BLOCK_WORDS, this driver will still read/program 16 bytes and can silently truncate or overrun the caller’s block.♻️ Proposed guard
`#define` FLASH_QUADWORD_WORDS (4) `#define` FLASH_QUADWORD_SIZE (FLASH_QUADWORD_WORDS * sizeof(uint32_t)) +#if FLASH_BLOCK_WORDS != FLASH_QUADWORD_WORDS +#error "STM32U5 flash driver expects FLASH_BLOCK_WORDS == FLASH_QUADWORD_WORDS." +#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/sys/flash/stm32u5/flash.c` around lines 28 - 29, Add a compile-time guard ensuring the driver’s hardcoded quadword size matches the block type: check that FLASH_BLOCK_WORDS (or sizeof(flash_block_t)) equals FLASH_QUADWORD_WORDS (or that sizeof(flash_block_t) == FLASH_QUADWORD_SIZE) and emit a preprocessor error/static_assert if not; update the top of flash.c near the FLASH_QUADWORD_WORDS/FLASH_QUADWORD_SIZE macros to add this check so flash_write_block() cannot silently truncate or overrun when FLASH_BLOCK_WORDS changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@storage/flash_ll.h`:
- Around line 77-79: Update the stale comment for flash_write_burst: remove the
hard-coded "128-byte burst" wording and instead describe the operation as
writing "one flash burst" or note that the burst size is target-dependent (e.g.,
"writes a single flash burst to the specified 'offset' inside a flash 'sector';
burst size depends on target"). Ensure the documented symbol is
flash_write_burst(uint16_t sector, uint32_t offset, const uint32_t *data) so
readers know this applies to that API.
---
Nitpick comments:
In `@core/embed/sys/flash/stm32u5/flash.c`:
- Around line 28-29: Add a compile-time guard ensuring the driver’s hardcoded
quadword size matches the block type: check that FLASH_BLOCK_WORDS (or
sizeof(flash_block_t)) equals FLASH_QUADWORD_WORDS (or that
sizeof(flash_block_t) == FLASH_QUADWORD_SIZE) and emit a preprocessor
error/static_assert if not; update the top of flash.c near the
FLASH_QUADWORD_WORDS/FLASH_QUADWORD_SIZE macros to add this check so
flash_write_block() cannot silently truncate or overrun when FLASH_BLOCK_WORDS
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a50d3ab-9105-4376-ae8c-fe961fa60379
📒 Files selected for processing (13)
core/embed/sec/secret/stm32u5/secret.ccore/embed/sys/flash/stm32u5/flash.ccore/site_scons/models/D002/discovery2.pycore/site_scons/models/T3B1/trezor_t3b1_revB.pycore/site_scons/models/T3T1/trezor_t3t1_revE.pycore/site_scons/models/T3W1/trezor_t3w1_revA.pycore/site_scons/models/T3W1/trezor_t3w1_revB.pycore/site_scons/models/T3W1/trezor_t3w1_revC.pycore/site_scons/models/stm32u5_common.pystorage/flash_area.cstorage/flash_area.hstorage/flash_ll.hstorage/tests/c/flash.c
💤 Files with no reviewable changes (8)
- core/site_scons/models/T3W1/trezor_t3w1_revA.py
- core/site_scons/models/T3B1/trezor_t3b1_revB.py
- core/site_scons/models/D002/discovery2.py
- core/site_scons/models/T3W1/trezor_t3w1_revC.py
- core/site_scons/models/T3T1/trezor_t3t1_revE.py
- core/site_scons/models/T3W1/trezor_t3w1_revB.py
- storage/tests/c/flash.c
- storage/flash_area.h
🚧 Files skipped from review as they are similar to previous changes (2)
- core/embed/sec/secret/stm32u5/secret.c
- core/site_scons/models/stm32u5_common.py
3 participants
Flash area API is simplified: namely the quadword function is removed from it and left internal to ll driver.
As a result, secret driver was refactored too and quadword usage was replace by writing data.
Flash burst size is moved to MCU sconscript instead of the driver as it can variate on different MCUs