drivers/mtd_flashpage: allow to define AUX slot on flash

[benpicco]

Contribution description

The issue for needing a config area on the MCU flash has been brought up multiple times now. So far the 'solution' has been to arbitrarily define a flash page and using the periph_flashpage API to write to it.

This has several issues, the most obvious being non-portability and silent firmware corruption if the firmware reaches into the chosen flash page region.

To fix this, this defines a new AUX Slot (similar to SLOT0/SLOT1 with SUIT) that occupies a linker section. This way, if things do not fit, it will fail at link-time and not at run-time.

The mtd_flashpage driver has been extended to implement the pagewise API. (#19258) This makes use of it more straightforward (tests in tests/mtd_raw now pass) and allows the user to interact with this through the MTD API, allowing to move the config area to e.g. an external EEPROM if needed without changing the application code.

As the riotboot slot mechanism is currently only implemented for cpu/cortexm_common, this PR also only implements the additional AUX slot only for cpu/cortexm_common as it modifies those variables. There is nothing in principle that ties this to Cortex-M, but moving this to a common place (along with riotboot slot) is left to be done in a follow-up PR.

Testing procedure

A board just has to define

SLOT_AUX_LEN := 0x8000

to create a 32 kiB AUX slot. (Must align with flash page size).

A mtd_aux/mtd_flash_aux_slot device will be available that can be used just like any other MTD device, you can even create a filesystem on it

#include "mtd_flashpage.h"
VFS_AUTO_MOUNT(littlefs2, VFS_MTD(mtd_flash_aux_slot), VFS_DEFAULT_NVM(0), 0);

Issues/PRs references

#5773

[chrysn]

This value would need to be agreed between software to be upgradable. riotboot does not concern itself with this, but SUIT does. Should some form of this value (say, -aux32768) become part of the SUIT_CLASS_ID?

This PR describes no common format for the data in there; that is fine with me. But can we assist in getting only compatible users to be upgradable to each other, eg. by placing not only the size but also an identifier of the downstream format into SUIT_CLASS_ID? The alternative to doing something like that here is probably to recommend that users place some own identifier into the data, so they can tell after the fact that they've just upgraded into something with incompatible aux data; users who rely on aux data being present and in the right format would then probably modify their board identifications themselves.

[benpicco]

I think you are mixing something up. This PR has nothing to do with SUIT / works independent of it. It is just about allocating a portion of the internal flash to be used as a MTD device.

[chrysn]

It adds a parametrization to boards that was previously dependent only on RIOTBOOT_LEN / NUM_SLOTS [edit: (or more precisely, (flash - RIOTBOOT_LEN) / NUM_SLOTS)]. Before this PR, one could use mechanisms building on riotboot (such as SUIT) to update images and rely on things to work (i.e. if you tested the image by direct flashing and the linker doesn't complain when making it into a riotboot image B, it'd work). After this, if your new application sets a different AUX size (and I understand this to be an application and not board property, the way it is presented), all of a sudden the application would be broken.

[benpicco]

It should be a board property, but to test it on CI I have to define it in the test application if I don't want to add an AUX slot to all boards by default.

[chrysn]

That sounds good to me -- a good default might be "two erasure units". Do you plan to add the AUX in as this PR matures? (I'd welcome it -- it'd avoid a vacuum time of chaos).

Setting this will be a breaking change to bootloaders unless the users opt out by setting SLOT_AUX_LEN = 0. Given all our bootloading is rather young that's probably OK, but do we have a story how this should work on the long run, or how to handle things if a user needs a larger AUX_LEN?

[benpicco]

I think this is a fixed value. It's more like an EEPROM or the bootloader sector - we can't change that retroactively either.

Do you plan to add the AUX in as this PR matures? (I'd welcome it -- it'd avoid a vacuum time of chaos).

I'm not sure what you mean - specify it for a select few boards?

[chrysn]

I'm not sure what you mean - specify it for a select few boards?

Yes, or even all boards that have no EEPROM or other dedicated storage. As this is a partitioning-breaking change, I'd prefer to go through it once, and not have a "this board changed its bootloader partitioning" note on some boards throughout the next ten releases.

[benpicco]

Hm I'm not so sure about that. Pretty much all of our boards are dev boards, so they don't serve a single purpose and will be re-flashed with different applications. Allocating a chunk of flash for something that might be never used by most applications seems rather wasteful.

[chrysn]

But this will only cut down sizes when one is already using a bootloader, and in that case the usable space is halved already (and often needlessly -- once a non-bare riotboot is present, the a/b partitioning is to some extent superfluous).

I think we should make an effort to make the default boards (the "dev boards" include arduino style devices, which people often use quite long) usable with OTA just as easily as they are usable from a debugger. This works only with a stable partition layout.

[benpicco]

There is no connection to the bootloader with this feature. If you set SLOT_AUX_LEN that space will always be reserved for auxiliary data.

[chrysn]

OK, then I'm seriously misunderstanding this.

For storing data within a single firmware version, FLASH_WRITABLE_INIT works well already -- only in connection with bootloaders do I see the need a memory region that has a stable position.

What's this good for, them?

[benpicco]

If we have firmware updates, storing the AUX section at a fixed location (end of the flash) is a hard requirement, but that doesn't mean we can't use the same mechanism without firmware updates too.

This does exactly that - chop off a section of flash at the end that can then be used to store user data. If this is then used with a bootloader or with a fixed application is entirely orthogonal.

The main objective is to give a linker error if the firmware grows into the AUX section.

[chrysn]

The main objective is to give a linker error if the firmware grows into the AUX section.

That's also provided by FLASH_WRITABLE_INIT (just with less the hassle of components needing to fight over who gets to use it) -- with the added benefit of taking precisely the size the application needs.

I wouldn't use this "just because it's a unified API" with the cross-firmware use case either -- these cases have different requirements in how data needs to be structured. If people started using this for their non-upgradable data, I expect there'll be another call for a section that's cross upgrade and independent of that. We'd just wind up with a version of FLASH_WRITABLE_INIT that's less modular.

[benpicco]

This creates a MTD device, so you can use it e.g. as a backend for FlashDB (#17612) or even a file system if you so desire. But this is just a low level component, so there is no opinion on how the data stored on it should be structured.

[chrysn]

If MTD creation is what it's about, then I suggest to build on the existing mechanism -- we have flash-to-MTD and flash partitioning, so the building blocks are there. An MTD_WRITABLE_INIT(mtd_device_name, minimum_size) should be doable directly from FLASH_WRITABLE_INIT.

[benpicco]

But FLASH_WRITABLE_INIT won't work if there are two firmware slots.

[chrysn]

I think that flash inside an application is generally used vastly differently from flash between versions -- but I'll ponder over commonalities.

A user who places data in an area, when going to upgrade-by-full-reflash to something upgradable in the field, should make a conscious decision that now this is cross-version data, and they need to be stricter about how they use that area. This relates both to the data and the size (which can't change from then on). Even if a structure like FlashDB is used that offers upgradability, suitable keys and conventions will still need to be met. Having something MTD based is certainly practical, as it reduces switching over to flipping an identifier, but it should still be a conscious decision and not something that "works for free" but inadvertently locks the user to choices that mattered less before.

[benpicco]

But how is this any different than data on an external EEPROM, SPI Flash or SD card? If you chose to store the data on an external device instead, you have to be just as careful to keep the data format between upgrades and can't arbitrarily change it's size too.

The MTD API allows to abstract this away, so you could even have an EEPROM in one hardware revision and use internal Flash in another - all without the application noticing.

[MrKevinWeiss]

If you chose to store the data on an external device instead, you have to be just as careful to keep the data format between upgrades and can't arbitrarily change it's size too.

I agree that the size should not be alterable. Making the bootloading slots understand the configuration area make sense from an "I want a production device" point of view.

AFAIU, we can set the len to 0 and manage things the current way...

The only downside as @chrysn pointed out is the potential for breaking bootloaders and possibly including this in now, then having to support it in the future even though maybe there should be something more standard.

Not that I know to much about this area but having a easy spot for config data that doesn't conflict with upgrades seems OK (and FlashDB looks really cool).

[jue89]

Great PR! I like it :)

First test:

samr30-pro

diff --git a/boards/samr30-xpro/Makefile.dep b/boards/samr30-xpro/Makefile.dep
index 4224c14c79..4afbe80d4c 100644
--- a/boards/samr30-xpro/Makefile.dep
+++ b/boards/samr30-xpro/Makefile.dep
@@ -5,3 +5,6 @@ endif
 ifneq (,$(filter saul_default,$(USEMODULE)))
   USEMODULE += saul_gpio
 endif
+
+USEMODULE += mtd_flashpage
+USEPKG += littlefs2
\ No newline at end of file
diff --git a/boards/samr30-xpro/Makefile.include b/boards/samr30-xpro/Makefile.include
index 0c4862de71..0cdccadf84 100644
--- a/boards/samr30-xpro/Makefile.include
+++ b/boards/samr30-xpro/Makefile.include
@@ -1 +1,3 @@
+SLOT_AUX_LEN := 0x8000
+
 include $(RIOTMAKE)/boards/sam0.inc.mk
diff --git a/boards/samr30-xpro/board.c b/boards/samr30-xpro/board.c
index b86c52dc37..007ee4d0ab 100644
--- a/boards/samr30-xpro/board.c
+++ b/boards/samr30-xpro/board.c
@@ -26,6 +26,10 @@
 #include "cpu.h"
 #include "periph/gpio.h"
 
+#include "vfs_default.h"
+#include "mtd_flashpage.h"
+VFS_AUTO_MOUNT(littlefs2, VFS_MTD(mtd_flash_aux_slot), VFS_DEFAULT_NVM(0), 0);
+
 void board_antenna_config(uint8_t antenna)
 {
     if (antenna == RFCTL_ANTENNA_EXT){

make -C tests/vfs_default BOARD=samr30-xpro flash term
2022-11-01 17:53:12,172 # main(): This is RIOT! (Version: 2022.10-devel-923-g065e5-HEAD)
2022-11-01 17:53:12,173 # mount points:
2022-11-01 17:53:12,174 # 	/nvm0
2022-11-01 17:53:12,175 # 
2022-11-01 17:53:12,176 # data dir: /nvm0
> vfs df
2022-11-01 17:53:19,268 # vfs df
2022-11-01 17:53:19,274 # Mountpoint              Total         Used    Available     Use%
2022-11-01 17:53:19,281 # /nvm0                  32 KiB        512 B      32256 B       1%