rghaddab/mram write robustness

[rghaddab]

A series of commits that adds more robustness to the MRAM write/erase function.
It adds the support of non aligned addresses to the write_block_size
It recovers from a bad write by reading back written data and detecting a bus fault

[swift-tk]

How is this change relevant to the PR?

[rghaddab]

Well this PR depends on the MRAM_LATENCY service.
This config depends on EVENTS which also depends on MULTITHREADING
so this test doesn't make any sense for me, and it fails because we run into a Kconfig dependency loop

[bjarki-andreasen]

@anangl

[anangl]

Why not just add - CONFIG_SOC_FLASH_NRF_MRAM=n in extra_configs instead of removing this test scenario?

[d3zd3z]

So, I have a bit of a fundamental question about the misaligned write. In general, with most non-volatile storage devices cannot do this in a memory safe way, and at least looking at this implementation, I believe that is also the case here.

What problem is supporting misaligned writes trying to solve. If it isn't done in a power-safe way, what application would find that acceptable.

In general, I would say that flash drivers should never provide workarounds like this, and with how this device appears to work, I would argue that should be the case here as well.

[rghaddab]

So, I have a bit of a fundamental question about the misaligned write. In general, with most non-volatile storage devices cannot do this in a memory safe way, and at least looking at this implementation, I believe that is also the case here.

What problem is supporting misaligned writes trying to solve. If it isn't done in a power-safe way, what application would find that acceptable.

In general, I would say that flash drivers should never provide workarounds like this, and with how this device appears to work, I would argue that should be the case here as well.

Hi @d3zd3z I would agree as well that misaligned writes is not something that most application would use.
However in this implementation the driver returns -EINVAL when the address is not aligned or the length is not multiple to the MRAM_WORD_SIZE.
The only case where the misaligned writes are supported here is if the user intentionally changes the default write_block_size (== MRAM_WORD_SIZE) to a different value in the device tree.
So the default behavior is aligned writes and length. But the exception of doing a (read ->update -> modify) is only when device tree default values are intentionally changed to a different non aligned value to MRAM_WORD_SIZE.

[de-nordic]

Hi @d3zd3z I would agree as well that misaligned writes is not something that most application would use. However in this implementation the driver returns -EINVAL when the address is not aligned or the length is not multiple to the MRAM_WORD_SIZE. The only case where the misaligned writes are supported here is if the user intentionally changes the default write_block_size (== MRAM_WORD_SIZE) to a different value in the device tree. So the default behavior is aligned writes and length. But the exception of doing a (read ->update -> modify) is only when device tree default values are intentionally changed to a different non aligned value to MRAM_WORD_SIZE.

Misalignment in DTS should not be allowed. DTS should have the write-block-size set to minimal of a device required and assert in driver should not allow that to even compile.
I have completely failed to notice this in review.

The only reason sub-writes could be allowed on MRAM would be the same as it implicitly happens in EEPROMS with larger sizes.

[jonathannilsen]

I ran into a few compilation issues when trying to integrate the changes here (building from nrfconnect/sdk-zephyr#3960):

[rghaddab]

Misalignment in DTS should not be allowed. DTS should have the write-block-size set to minimal of a device required and assert in driver should not allow that to even compile. I have completely failed to notice this in review.

The only reason sub-writes could be allowed on MRAM would be the same as it implicitly happens in EEPROMS with larger sizes.

@de-nordic I will remove sub-writes support ( < MRAM_WORD_SIZE).
But what about the address alignment ? should the driver add a constraint to limit writes only on aligned addresses to MRAM_WORD_SIZE and let the above layers handle non aligned addresses ?

[karstenkoenig]

Misalignment in DTS should not be allowed. DTS should have the write-block-size set to minimal of a device required and assert in driver should not allow that to even compile. I have completely failed to notice this in review.
The only reason sub-writes could be allowed on MRAM would be the same as it implicitly happens in EEPROMS with larger sizes.

@de-nordic I will remove sub-writes support ( < MRAM_WORD_SIZE). But what about the address alignment ? should the driver add a constraint to limit writes only on aligned addresses to MRAM_WORD_SIZE and let the above layers handle non aligned addresses ?

The same would apply, either we have read/modify/write support in the flash driver or not.

[d3zd3z]

The same would apply, either we have read/modify/write support in the flash driver or not.

Flash drivers should never have read/modify/write support. It makes it impossible to use in a robust way. Robustness needs to be implemented by an FTL, or a filesystem that knows how to handle the complexity behind this.

[d3zd3z]

The only reason sub-writes could be allowed on MRAM would be the same as it implicitly happens in EEPROMS with larger sizes.

Realistically, an EEPROM driver should know about this, and prevent it from being done (at least if it isn't robust). Short of a special kind of partial_write_but_lose_my_data() API. People really don't seem to casually grasp just how bad read/modify/write operations are.

[rghaddab]

@d3zd3z can you remove the change request, as I removed the non aligned write-block-size support.

[rghaddab]

twister failures will be fixed after :
#107414
#107704
#107703

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud