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

MTD is confusing #17663

Open
chrysn opened this issue Feb 16, 2022 · 8 comments
Open

MTD is confusing #17663

chrysn opened this issue Feb 16, 2022 · 8 comments
Labels
Area: doc Area: Documentation Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Comments

@chrysn
Copy link
Member

chrysn commented Feb 16, 2022

Walking between MTD and flashpage, MTD is, for lack of a clearer word, confusing (at least to me); I think that's mostly due to terminology not sufficiently explained, motivated or common.

For comparison, flash is pretty clear in that a page is something that can be erased, and writes happen in alignments and sizes of FLASHPAGE_WRITE_BLOCK_ALIGNMENT / FLASHPAGE_WRITE_BLOCK_SIZE (which I tend to mentally simplify to the maximum thereof -- that is safe, and while they do diverge on some architectures, I don't quite see how I'd use a 2-byte write in a 4-byte alignment in practice, like STM32 seems to allow for some chips).

But MTD introduces sectors and pages, and while sectors are implicitly the erasure units, pages seem to have no inherent semantics at all other than allowing addressing based on them in what vaguely reminds me of the CHS convention of the early ages. This is exacerbated by disconnect from MTD terminology as used on Linux (where I'd assume the term is borrowed from), which deliberately does not talk of pages and sectors but of eraseblocks.

Also odd (I'm leaning toward "missing feature" but with the above I'm not sure) is that while mtd_write says that some devices might enforce alignments, I don't see how the application can get that information. (It's tempting to make a conservative estimate of 8, but with LPC23xx having a FLASHPAGE_WRITE_BLOCK_SIZE of 256, conservative may need very conservative).


I'd like to make a doc PR to enhance things, but right now I don't have a sufficiently consistent mental image to do that. Could you help clarify this?

Pinging @vincent-d and and @AurelienGONCE as the original authors, @jnohlgard as reviewer and @benpicco who added the page addressed operations.

@chrysn chrysn added the Area: doc Area: Documentation label Feb 16, 2022
@benpicco
Copy link
Contributor

page_size is the largest block that can be written by one transfer, writing across that boundary causes a wrap-around.
sector_size is the erase block size.

The inconsistencies in mtd_write() are mostly historic as before the pagewise functions the user had to take care of splitting the writes if necessary. #15380 should do away with the old functions, but there is still mtd_flashpage which is not so easily converted into uniform pages.

@chrysn
Copy link
Member Author

chrysn commented Feb 16, 2022

So the concept of "in one transfer" for the page is purely one of implementation (splitting up write operations), and of performance in some backends, but not of atomicity. The performance can be relevant to implementations, but is it actionable? (I'd assume that if an application needs some bytes written, it will call the MTD layer, but the application won't gain anything from splitting the write across two writes in a run-time decision, especially because RIOT is multithreaded, and a good MTD backend will put the thread to sleep during a write).

@benpicco
Copy link
Contributor

Ideally the application should not have to care about MTD internals to begin with.
It's true that page_size only makes sense for some MTD backends and might as well be only present in the implementation specific MTD extension (e.g. mtd_spi_nor_t) , from an API perspective the sector <-> page distinction is rather arbitrary.

@chrysn
Copy link
Member Author

chrysn commented Feb 16, 2022

The application will need to care about the erase size to do proper journaling or atomic updates. (That is, unless it is happy with read-erase-write operations -- I personally think that "Do not shut off the device while saving is in progress" is something that died well after the Gamecube generation of electronics). After all, that is what sets it apart from a block device.

Working on some initial text based on the above to improve the docs...

chrysn added a commit to chrysn-pull-requests/RIOT that referenced this issue Feb 16, 2022
@chrysn
Copy link
Member Author

chrysn commented Feb 16, 2022

#17666 now summarizes how I understand things to be.

I think the MTD layer simply lacks some concepts:

  • A size indicating size-and-alignment of writes. (The driver can emulate unaligned writes, but only if the flash memory does support word rewrites, which not all backends, esp. mtd_flashpage, do.)
  • A flag indicating whether rewrites are supported.

I'd like to add these in later, then storage like the one required for OSCORE can be built on MTD rather than on flash (where at least information like FLASHPAGE_WRITE_BLOCK_SIZE is available).

There are some more details discussed in the embedde-storage crate, or even present in the flashpage driver, but I think they can be simplified:

  • Alignment and length of writes may be different (STM has a few combinations) -- but taking the larger of these as write size is not wrong (and I can't imagine it harming any practical application).
  • Allowing overwrites is not binary but actually a count over some region that may actually be the current page size (but may also be a different concept): "overwrite at will" and "overwrite never" seem to be useful extremes to cover in a flag. (Even "overwrite at will", in practice, means only up to 8 * minimal-write-size times, for then all bits are clear anyway)
  • Read alignment can be handled by the driver.
  • IIRC there are some distinctions between flashes that, on overwrite, prefer previously cleared bits to be 0, or prefer previously cleared bits to be 1. If that distinction matters to the backend, the driver can take care of it.

chrysn added a commit to chrysn-pull-requests/RIOT that referenced this issue Feb 16, 2022
@chrysn
Copy link
Member Author

chrysn commented Feb 19, 2022

Looking through some data sheets I found that at least for flash memories it's rather atypical to support arbitrary many rewrites; for example, EFM32GG (of which I thought I remembered they'd support that -- and have used that in code (AFAIK nothing blew yet...). But actually the data sheet says "write up to 2 times".

The embedded-storage classification is way more detailed than I think makes sense to store at run time. (It makes sense there as the user can adjust its data structure at compile time to work with the limitations). But what is an abstraction level that works for us?

Suggestion:

  • Introduce write granularity.
  • Writes are only allowed once after erases, and only in the granularity indicated by the former parameter.
  • Devices with DIRECT_WRITE flag ignore the "only once" limitation. (They could also ignore the granularity, but then there'd be implicit read-modify-write cycles, and the application can't build journals from that).
  • Some flash memories' "write N times" property is not exposed to the application, they'll have to make do with the pessimistic simplification.
  • (Note that all flash memories could also be expressed as DIRECT_WRITE by upping their write granularity to full erase units, and that memories that allow writing a cell twice can emulate a device with better granularity that does not support overwrites at all).

@Laczen
Copy link

Laczen commented Mar 1, 2022

@chrysn, also take a look at some datasheets for nand flashes as these are also mtd. The terminology might not be so strange. As mtd devices should also support nand it does not seem like a good idea to add some nor-flash specifics (rewritability) to the mtd api.

@chrysn
Copy link
Member Author

chrysn commented Mar 1, 2022 via email

@maribu maribu added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

No branches or pull requests

4 participants