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.

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

drivers/mtd_flashpage: allow to define AUX slot on flash #18608

Merged
merged 9 commits into from
Feb 28, 2024

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Sep 18, 2022

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

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Sep 18, 2022
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Sep 18, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 18, 2022
@chrysn
Copy link
Member

chrysn commented Sep 19, 2022

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
Copy link
Contributor Author

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
Copy link
Member

chrysn commented Sep 19, 2022

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
Copy link
Contributor Author

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
Copy link
Member

chrysn commented Sep 19, 2022

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
Copy link
Contributor Author

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
Copy link
Member

chrysn commented Sep 20, 2022

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
Copy link
Contributor Author

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
Copy link
Member

chrysn commented Sep 23, 2022

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
Copy link
Contributor Author

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
Copy link
Member

chrysn commented Sep 23, 2022 via email

@benpicco
Copy link
Contributor Author

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.

@benpicco
Copy link
Contributor Author

The idea is that the AUX slot would always be present (defined at board level, not by the application) similar to an SPI flash.
It can not be changed in the field.

@chrysn
Copy link
Member

chrysn commented Feb 26, 2024

OK. Is that documented at the main configuration option a user would use to enable this? Is that SLOT_AUX_LEN, and can we make that visible in the flash documentation, so that other documented items such as mtd_aux and CONFIG_SLOT_AUX_MTD_OFFSET can reference it?

@benpicco
Copy link
Contributor Author

I added documentation to CONFIG_SLOT_AUX_LEN since I think that's the only thing that shows up in Doxygen.

@fabian18
Copy link
Contributor

@chrysn are you ok with merging this PR in the current state?

@chrysn
Copy link
Member

chrysn commented Feb 28, 2024

Yes; thanks for adding the docs. I still have some reservations on whether it is the right approach w/rt bootloader interaction, but until I have a full SUIT integrated counterproposal ready, this solves real-world problems now.

@benpicco benpicco added this pull request to the merge queue Feb 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 28, 2024
@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Feb 28, 2024
@benpicco benpicco added this pull request to the merge queue Feb 28, 2024
github-merge-queue bot pushed a commit that referenced this pull request Feb 28, 2024
drivers/mtd_flashpage: allow to define AUX slot on flash
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 28, 2024
FW_ROM_LEN is supposed to be a subset of ROM_LEN that contains the firmware
(without bootloader, AUX slot).
@benpicco benpicco added this pull request to the merge queue Feb 28, 2024
Merged via the queue into RIOT-OS:master with commit 5c92409 Feb 28, 2024
25 checks passed
@benpicco benpicco deleted the mtd_flashpage-aux branch February 28, 2024 14:53
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants