sys/fido2: improve & simplify flash handling#21110
sys/fido2: improve & simplify flash handling#21110Ollrogge wants to merge 5 commits intoRIOT-OS:masterfrom
Conversation
Ollrogge
requested review from
MichelRottleuthner,
aabadie and
leandrolanzieri
as code owners
github-actions
bot
added
Area: tests
Ollrogge
force-pushed
the
fido2_flash_handling_improvements
branch
from
771229a to
8fdb7a6
Compare
mguetschow
added
the
CI: ready for build
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
Thanks, some comments below. I'm not very familiar with the mtd API, maybe @benpicco could have a look as well?
sys/include/fido2/ctap/ctap_mem.h
Outdated
| * @param[in] key pointer to authenticator state | ||
| * @param[in] rp_id_hash pointer to hash of rp domain string | ||
| * @param[in] addr pointer to address where to read from | ||
| * @param[in] absolute_offset pointer to a variable holding the total offset from the |
There was a problem hiding this comment.
the parameter is called off in the implementation. Would be nice to align both.
From an API perspective, I would be nicer to have some iterator struct, that hides the offset information from the user.
There was a problem hiding this comment.
Sounds good, do you have an example for an iterator struct somewhere in RIOT ?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the recommendation :) Updated the code to use an iterator-like struct
| #define ENABLE_DEBUG 0 | ||
| #include "debug.h" | ||
|
|
||
| static mtd_dev_t *_mtd_dev; |
There was a problem hiding this comment.
| static mtd_dev_t *_mtd_dev; | |
| static mtd_dev_t *_mtd_dev = CONFIG_FIDO2_MTD_DEV; |
We should make the application select this
There was a problem hiding this comment.
Doing this I get:
/home/h0ps/Programming/RIOT/tests/sys/fido2_ctap_hid/bin/nrf52840dongle/riotbuild/riotbuild.h:21:30: error: initializer element is not constant
21 | #define CONFIG_FIDO2_MTD_DEV mtd_aux
| ^~~~~~~Therefore I am setting _mtd_dev inside fido2_ctap_mem_init
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
Had another look, still no in-depth review I'm afraid. But at least some more comments below.
| FEATURES_REQUIRED += periph_flashpage_aux | ||
| CFLAGS += -DCONFIG_FIDO2_MTD_DEV=mtd_aux |
There was a problem hiding this comment.
Is this different to tests/sys/fido2_ctap/Makefile on purpose?
There was a problem hiding this comment.
Nope it wasn't. Differentiating between native and any board in the Makefile now
There was a problem hiding this comment.
I guess you forgot a git push?
There was a problem hiding this comment.
No, the change is in the fido2_ctap Makefile since it has support for the native target
Ollrogge
force-pushed
the
fido2_flash_handling_improvements
branch
from
790087f to
1d6c51f
Compare
4 participants
Contribution description
This PR tries to improve the flash handling done by the FIDO2 implementation. Specifically this entails:
mtd_auxdevice to reserve flash memory for the FIDO2 data_flash_is_erasedfunction)mtd_flashpageapiSince the AUX slot approach is integrated into the
mtdAPI, which is the recommended method for working with flash, I wonder if theperiph_flashpage_in_address_spacefeature I added to reserve flash memory for FIDO2 can now be deprecated ?@benpicco, with the
mtd_auxapproach, how can two different applications reserve flash memory without risking corruption of each other's data? Is it possible to divide the AUX slot into multiple smaller slots, that can be allocated for use by different applications?Testing procedure
/tests/sys/fido2_ctapwith bothnativeandnrf52840dkas target/tests/sys/fido2_ctap_hid