The nrf52840dongle bootloader mechanism has an option PARTICLE_MONOFIRMWARE_CHECKSUMLIMIT that makes it checksum just the header, so this is not strictly necessary.

It may well make sense to do this, especially if we decide that it's legitimate for bootloader to verify the image's integrity, and conversely necessary for any flash writer to declare the flash region.

I suggest that app_data be renamed to reflect these semantics: That it's flash that there is an intention to write to. (So maybe flash_writable?). With PRs like #16730 I expect that some RIOT modules would like to declare such pages as well (even though the resource friendly usage pattern would be for all users to gang up to minimize flash writes), and then the name app_data would be ill-advised.

As we define that region we may also want to consider whether in that region it's legitimate to execute code from. I don't know if any of the MPUs around support that, but setting that section non-executable may be a good idea. (That should not be acted on in this PR, but a note on the documentation of that new region could say something like

While currently not enforced on any system, that memory region should be regarded as non-executable; systems with memory protection may start enforcing this as the use of MPUs in RIOT progresses.

).

pages as well (even though the resource friendly usage pattern would be for all users to gang up to minimize flash writes), and then the

Are you sure that the Open USB bootloader of the nrf52840dongle supports this option ? I can't find it anywhere in the SDK.
Also I am not building the bootloader so I can't supply any CFLAGS.

I will rename the section as you suggested.

@chrysn I updated the section name to be flash_writable. Anything else missing now?

from a quick look (currently on the road): any idea where this section could be documented? if not, i'd like to instigate some documentation on the linker scripts and sections (which would then take over documenting this).

No me neither. I think documenting RIOT specific linker scripts and sections is a good idea. This would have helped me on multiple occasions.

i can do a full implementation review later (incl tests), but would appreciate a high-level review ("is having a dedicated section for any writable parts of flash the way to go?") by someone who knows which trickery is hidden in our linker scripts.

That would be great ! Do you know anyone I could ping for the high-level review ?

Since the section is not part of the firmware I can not statically initialize the memory.
Is it not? What makes it not? AFAIK, none of the bootloaders or programmers look at "end_fw" or other markers, they just put everything that is in ROM onto the chip. (Statics without initial value are probably zero-initialized.)

This is due to the section being defined as NOLOAD. Removing this directive will result in the problem with the nrf52840dongle I described in the FIDO2 follow up PR. Setting PARTICLE_MONOFIRMWARE_CHECKSUMLIMIT does not fix this issue.

Many use cases can benefit from this data section. We saw the need in previous flashage discussions (related to FIDO2). More generically, all sorts of non-volatile key- or configuration storages would make use of it.

if we decide that it's legitimate for bootloader to verify the image's integrity

To me, that is a totally legitimate assumption (what does RIOTBOOT do?). The ARM PSA specification, for example, requires a root of trust which includes the trusted system startup.

documentation on the linker scripts and sections

+1

Should this FEATURE depend on PERIPH_FLASHPAGE_PAGEWISE ? I am wondering if we should encourage every module using the flash_writable section to use buffers aligned to flash pages or not. If we don't encourage this multiple modules could share a flashpage but the chances that a module accidentally corrupts data by using the page num instead of the address is higher.

What do you think ?

Frankly I don't see what periph_flashpage_pagewise actually describes. Either way, yes I think we should encourage using this mechanism over plain flash page numbers. There is no portable way to know what a flashpage number means, and whether it conflicts with anything.

On platforms that do have (some of their, at least the part in flash_writable) flash memory address-mapped, this should be the preferred mechanism. On others, users will need to make usable page numbers appear out of thin air, but then these platforms (and I think it's just native) hopefully have a means to do that.

(One might even consider playing with the linker script of native, and make the flashpage implementation of native map the file in with mmap, strongly asserting (backed by the linker script's configuration) that the kernel will indeed map the backing file into the right location), [edit] but that's out of scope for here.

Frankly I don't see what periph_flashpage_pagewise actually describes. Either way, yes I think we should encourage using this mechanism over plain flash page numbers. There is no portable way to know what a flashpage number means, and whether it conflicts with anything.

So, I had a chat with @bergzand about the periph_flashpage_pagewise feature.
He added this feature when making the flashpage API optional.
This feature describes if a board supports the pagewise API. The API is supportd
when a device has a single compile-time page size.

Based on this I think that the feature introduced with this PR should be dependent
on the pagewise API feature since the reserved flash regions need to be
aligned to FLASHPAGE_SIZE to not corrupt each other. Otherwise
writing to a buffer that lays in the middle of a flash sector will corrupt
other adjacent buffers since the sector needs to be erased first and no function
of the current flash API's implements read-modify-write-operations
when using addresses instead of page numbers.

This will exclude platforms that don't have a constant page size from this
feature. I think this is fine for now because I see no other convenient way
to use the .flash_writable section without having the reserved regions
be aligned to FLASHPAGE_SIZE.

@chrysn I would really like to move on with this. Can you please look at the changes and tell me what is missing ?

I can't do a comprehensive review right now, but I can add a few questions:

• The sections are all ALIGN(4). Why? The actual relevant alignments come in through the FLASH_WRITABLE_INIT anyway.
• In terms of commit history cleanliness, I'd prefer the last rename to be applied back to commits that originally introduced the names, this would ease final review. (There are no unresolved threads open, so please force push once that is applied).

I'll give this a final review once those two are answered and/or addressed [edit: but don't see anything showstopper-wise left].

I can't do a comprehensive review right now, but I can add a few questions:

• The sections are all ALIGN(4). Why? The actual relevant alignments come in through the FLASH_WRITABLE_INIT anyway.

Good question. I think I added this ALIGN before adding the FLASH_WRITABLE_INIT macro to make sure that the section is properly aligned in order to align flash writes. This however wouldn't even have worked in all cases since FLASHPAGE_WRITE_BLOCK_ALIGNMENT can be 8 as well.

Anyways, I removed the usage of ALIGN.

• In terms of commit history cleanliness, I'd prefer the last rename to be applied back to commits that originally introduced the names, this would ease final review. (There are no unresolved threads open, so please force push once that is applied).

Done !

@chrysn could you please look at the latest changes and make a final review if possible ?

OK, so for tests I've performed:

• The new test runs through with plausible output on native and particle-xenon (with riotboot bootloader).

$ FEATURES_REQUIRED+=riotboot USEMODULE+=usbus_dfu make BOARD=particle-xenon all riotboot/flash-slot1 test PROGRAMMER=dfu-util

Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
rt addr:                0x00000000
Page size:                      4096
Number of pages:                256
Number of first free page:      140 
Number of last free page:       255 
> 

> 
> test_last_raw

> test_last_raw
wrote raw short buffer to last flash page
> help
help
...
> test_last_pagewise
wrote local page buffer to last flash page
> help
help
...
> test_reserved_pagewise
Reserved page num: 138 
Since the last firmware update this test has been run 3 times 
wrote local page buffer to reserved flash page

When running on a bootloader, as an extra check, try restarting the board and check whether this application still comes up.
> help
help
...
> 

• The test behaves well when the sizes (both _abacking_memory and _backing_memory have to be set in lockstep because of the way they're compared) are set large enough to fill up the remaining memory (increasing them so that "number of first free page" becomes "number of pages").

  In particular, increasing the memory further does trigger the size checks (as it should -- this checked that the memory is properly accounted for even though it is not flashed).

diff --git a/tests/periph_flashpage/main.c b/tests/periph_flashpage/main.c
index cff25750ab..5bfbd16280 100644
--- a/tests/periph_flashpage/main.c
+++ b/tests/periph_flashpage/main.c
@@ -64 +64 @@ static uint8_t page_mem[FLASHPAGE_SIZE] ALIGNMENT_ATTR;
-FLASH_WRITABLE_INIT(_backing_memory, 0x1);
+FLASH_WRITABLE_INIT(_backing_memory, 59);
@@ -69 +69 @@ FLASH_WRITABLE_INIT(_backing_memory, 0x1);
-FLASH_WRITABLE_INIT(_abacking_memory, 0x1);
+FLASH_WRITABLE_INIT(_abacking_memory, 59);

Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
rt addr:                0x00000000
Page size:                      4096
Number of pages:                256
Number of first free page:      256 
Number of last free page:       255 
> 

> 
> test_last_raw

> test_last_raw
wrote raw short buffer to last flash page
> help
help
...
> test_last_pagewise
wrote local page buffer to last flash page
> help
help
...
> test_reserved_pagewise
Reserved page num: 196 
Since the last firmware update this test has been run 2 times 
wrote local page buffer to reserved flash page

When running on a bootloader, as an extra check, try restarting the board and check whether this application still comes up.
> help
help
...
> 

By the way, this shows a flaw in the last_pagewise test: It writes to the "last free page" (255) even though the first free page (256) indicates that there are simply no free pages at all -- thus destroying "data" in the area set aside by one of the new reserved pages. Given that that API is becoming deprecated and it's just a flaw in the test (albeit one that a user might create themselves as well), that's likely not too bad -- just please be sure to follow up with that deprecation.

• The two last commits could need a bit of restructuring:

  • The dependency should be set right where it is defined. (ie. the last commit applied to where the one MODULE_PERIPH_FLASHPAGE_IN_ADDRESS_SPACE is defined).
  • The commit could be named "periph/flashpage: Add _in_address_space and FLASH_WRITABLE_INIT" and spell the feature name out in the second line. (CODING_CONVENTION.md is a bit terse on the topic; the commit message length limit is enforced by CI).
  • As that is an awkward workaround for the name being too long, and because the commit does three things in one go (introduce a feature that is (by no fault of its own) spread out over dozens of files, introduce a FLASH_WRITABLE_INIT macro, and extend a test), please consider splitting it up.

So content-wise I'm happy; please adjust the commit and message, and then this can pass.

(Also, with my recent forays through MTD, I think this might be great to combine with an add-on routine that sets up the mtd_flashpage and mtd_mapper to make the flash-writable area available to flash file systems, but that's a different PR.)

Thanks for testing !

By the way, this shows a flaw in the last_pagewise test: It writes to the "last free page" (255) even though the first free page (256) indicates that there are simply no free pages at all -- thus destroying "data" in the area set aside by one of the new reserved pages. Given that that API is becoming deprecated and it's just a flaw in the test (albeit one that a user might create themselves as well), that's likely not too bad -- just please be sure to follow up with that deprecation.

Yes I am planning to deprecate the flashpage_*_free functions once this PR is done.

• The two last commits could need a bit of restructuring:

• The dependency should be set right where it is defined. (ie. the last commit applied to where the one MODULE_PERIPH_FLASHPAGE_IN_ADDRESS_SPACE is defined).

• The commit could be named "periph/flashpage: Add _in_address_space and FLASH_WRITABLE_INIT" and spell the feature name out in the second line. (CODING_CONVENTION.md is a bit terse on the topic; the commit message length limit is enforced by CI).

• As that is an awkward workaround for the name being too long, and because the commit does three things in one go (introduce a feature that is (by no fault of its own) spread out over dozens of files, introduce a FLASH_WRITABLE_INIT macro, and extend a test), please consider splitting it up.

Done :) I split up the commits into three commits. One for the feature, one for the macro and one for the changed tests.

CI is complaining about discrepancies between feature names. Looks like an easy name mismatch (one name has an _init in it even though it shouldn't), please squash right away when you find it.

Hi, I think this re-introduced the linker warning I fixed in #17581 because there is no memory region named rom in the RISC-V linker script. Did you mean to use flash instead? (:

... Sorry for that. Yes I meant to use flash. Do you want to fix this quickly or should I ?