mtd: Introduce write granularity

[chrysn]

Contribution description

MTD does not describe all that a user may need to know when only passed a driver-based device (which usually comes with the expectation that no more needs to be known about the device than that it provides the API).

In particular, what was missing was:

• Information that arbitrary overwrites are permitted (which they are not on flashpage_mtd devices)
• Information that writes are constrained to aligned regions. This was not super important previously (as the drivers might emulate unaligned writes, though docs already said that they might also not), but now that writes are somewhat distinguished from overwrites, a user needs to know the alignment to not accidentally perform two logically independent writes that would still constitute an overwrite (say, writing the first 4 bytes one-by-one, turned into 1 allowed write and 3 forbidden overwrites by the driver).

Status / next steps / questions

This PR introduces the terminology, but barely sets the new properties anywhere. The new flag (MTD_DRIVER_FLAG_DIRECT_WRITE "you may overwrite and only zeros will be written") is not set anywhere, the write_size is only set for mtd_flashpage, and no consumer yet requires them.

Most drivers still leave this at 0, documented to mean "driver doesn't tell" and reading which should be taken as a hint to fix the driver.

I think this is all strict improvements, and that users can be fixed (by checking their previously silent assumptions) and drivers can be made more correct (by advertising their properties) one by one as needed -- but I understand if reviewers prefer to have this extended a bit. (In that case, I'll need their help testing, for all the MTD I have access to is flashpage_mtd, native, and the IoT-Lab device).

Testing procedure

Currently, this is practically a docs-only change.

Issues/PRs references

• MTD is confusing #17663 is the original rant
• Without this, FlashDB would be configured with a write size of 1, even though some MTDs we support (like flashpage) have a write size of 4.
• SPIFFS in the default configuration relies on the ability to overwrite a 0xf0 with 0x5f and get 0x50. Even after this PR the API doesn't model all the spectrum of how OK blind writes may be, but the model should suffice to build SPIFFS on MTD_DRIVER_FLAG_CLEARING_OVERWRITE devices (where one may do blind writes) and MTD_DRIVER_FLAG_DIRECT_WRITE ones (where one may most certainly not).
• More extensive discussion on the classification of flash memories is happening in the embedded Rust community.
• (more for futher reference), the riotboot use of the flashpage API requires about one other write; if it becomes a goal to get flashpage feature parity in MTD (at least for the reasonable subset of homogenous pages), then a flag may later be introduced that gives license to do one overwrite inside a page That'd be a choice that most internal memories support. It may be necessary to sharpen that write to a single byte, just to be sure that even on devices with a small write size, only one write is needed (as expressing a number of permitted extra writes gets complex fast).

[edit: added last two items to references list]

[benpicco]

Can we have something like

    if (mtd->write_size == 0) {
        DEBUG("mtd: write granularity not set, assuming 1\n");
        mtd->write_size = 1;
    }

in mtd_init() so users don't have to special-case 0?

[chrysn]

We can, but the only safe assumption the initializing code can make that it is maximal.

(Users should never have to deal with devices with write_size 0 -- if their devices have write size 0 and it's not code that for some reason never needed to think about this, it should just fail, and the user would need to find out what the actual property of the device is and fix the driver.)

[chrysn]

Now set to page size (as that for sure can be written in one go, as it is the maximum that can).

[benpicco]

With 'users' I meant other modules like #17612 (comment)

[chrysn]

For these users I think that the new behavior (finding 0, setting pagesize) does the right thing: That user should just set write_gran = mtd->write_size (or * 8, if that's supposed to be bits).

That may easily result in runtime errors if FlashDB expects that to be smaller than some size. (Tried to find a pointer, but from a brief GitHub search ... it appears that write_gran isn't even ever read?) These errors would be justified: As long as nobody has entered the information into the system that this device supports byte granularity, there is no way to tell, and whoever builds something based on FlashDB will need to enhance the driver they use to set the correct size.

We can't just assume that every driver that doesn't set it has a write size of 1 -- mtd_flashpage didn't set it and that most certainly does not have a write size of 1. (It may easily be that that is the only MTD driver that has a write size > 1, but I don't know them all well enough to tell, and even if anyone were to go through all the datasheets of implemented drivers, people could have private MTD modules we don't find).

[benpicco]

Then let's make it

assert(mtd->write_size);

so it blows up directly instead of producing code that fails somewhere down the line or is just unnecessarily slow and hard to debug.

[chrysn]

Yes, that'd work, fixup'ing.

Still breaks applications ("needlessly" if they never access write_size, but then again chances are that's just because they're not making their assumptions explicit), and as you say it'd be easy to debug.

[benpicco]

Now this is terrible.
So far everything assumed a write granularity of 1 and it worked.
This would break every MTD backed or at least incur a costly work-around in the MTD layer for every driver.

If you really want to avoid setting an optimistic default, just set a default for each MTD driver - there are not that many 😉

[chrysn]

Setting a default for each MTD is certainly an option -- but at least for one I looked at the data sheet and it wasn't very explicit about any requirements. That probably indicates it has a granularity of 1 (because for a managed device that'd be typical). Do you know them well enough to make a general statement on everything but flashpage_mtd that we have in-tree to say that 1 is the value for them? (Or at least for some? Then I'd set these to 1 already in this PR.)

(And about "it worked", that's likely just because it hasn't been tested on flashpage_mtd on all devices that have flashpage.)

[benpicco]

The EEPROMs (at24, at25) certainly can write individual bytes. Same for SPI NOR flash.
The SD card driver only supports blocks of 512 bytes unless mtd_write_page is used (#17619).
mtd_mapper should just inherit the value of the underlying MTD device.

[chrysn]

Thanks, added them. For mtd_mapper it appears that it's not configured automatically from the parent, but there is a sanity check for some things where I added one for write_size. For mtd_spi_nor I'm setting it at init time (similar to how the sector count is set dynamically).

[benpicco]

mtd_mci and mtd_sdcard still need an update

[benpicco]

Is it intentional that this is the same as MTD_DRIVER_FLAG_DIRECT_WRITE?

[chrysn]

That was an error; thanks, fixed with the next batch.

[gschorcht]

(By the way, why is there a read in the first place? Could just set 0xffffffff & (my_word >> misalignment), couldn't we?)

Yes, we could, but that part of the code is dropped with PR #17841 anyway.

[chrysn]

Yes, we could, but that part of the code is dropped with PR #17841 anyway.

OK, then let's keep the concerns separate.

---

My summary of this would be that while there's a lot of things we could do in terms of overall deduplication, this is a first step there that provides terminology, and AFAICT all current comments have been addressed, and all implementations are described completely again.

[benpicco]

This only provides additional information via the MTD layer and does not act on it in any way.

Since all MTD backends are updated now, the assertion should also not trigger.

[benpicco]

Can't we instead just

Suggested change

	assert(backing_mtd->write_size == region->mtd.write_size);
	backing_mtd->write_size = region->mtd.write_size;

Seems pretty redundant to require the user to set this.

[chrysn]

We could, but I value consistency more highly here (page size and pages per sector are set manually as well).

[chrysn]

@jue89 your "change requested" review was not cleared (but you left a "comment" review later; that doesn't change an old "changes requested"); are you happy enough with this?

[jue89]

Yes, I happy with this! Having the write size and alignment at hand helps to write working code ;-)

But the flag MTD_DRIVER_FLAG_SETTING_OVERWRITE you described above hasn't been defined, yet. Is this intentionally?

#17683 (comment)

[chrysn]

But the flag MTD_DRIVER_FLAG_SETTING_OVERWRITE you described above hasn't been defined, yet. Is this intentionally?

Yes: We have no single driver that uses it, and from how memory is usually built it's exotic -- and it's a flag, it can be added if and when there is actual use for it. (I think that the hardware for these would be the same hardware as with the CLEARING_OVERWRITE, but with its bit semantics inverted at read and write).

[benpicco]

Failed tests are unrelated:

run_test/tests/malloc_thread_safety/esp32-wroom-32:gnurun_test/tests/periph_timer/esp32-wroom-32:gnurun_test/tests/sys_sched_round_robin/esp32-wroom-32:gnurun_test/tests/ztimer_overhead/esp32-wroom-32:gnu
run_test/tests/thread_float/samr21-xpro:gnu

[chrysn]

+ uint32_t write_size; /**< Minimum size and alignment of writes to the device */ Thinking about it, doesn't this also apply for reads?

It would (albeit as a separate parameter; eg. some STM32 flashes are written in 64bit words but read down to byte granularity), but unlike with writes, drivers can trivially* and safely (no RWM) emulate that. So we could, but unlike the write case this is barely actionable. For writes, the user (like FlashDB) might use the information to reconsider its data layout. For reads I doubt it'd influence the layout, and then when there is a large read size, it'd just shift the complexity and the stack use from the driver to the consumer (which, unlike the driver, can't employ clever tricks any more). *: Might cost an arm and a leg on the stack, or not if the driver (say) splits the 512 byte SPI read that the memory might require it to do into some bytes that are read to /dev/null before reading into the target memory.

…

-- To use raw power is to make yourself infinitely vulnerable to greater powers. -- Bene Gesserit axiom