mtd/*: drop .write() if .write_page() is implemented #15380

benpicco · 2020-11-04T11:04:31Z

Contribution description

The old .write() function is only used as a fall-back if .write_page() is not implemented.
We can drop it.

Testing procedure

Everything will fall back to .write_page now, so no user visible change.
On the driver level, .write_page can be much simpler than .write as all the common logic is handled by MTD and we can do partial writes (only write to page boundary, MTD will split up the transaction).

AT25xxx

tests/mtd_raw

2023-03-09 23:26:41,366 # init MTD_0… OK (128 kiB)
2023-03-09 23:26:43,208 # > test 0
2023-03-09 23:26:43,208 # [START]
2023-03-09 23:26:43,286 # [SUCCESS]

2023-03-09 23:28:20,306 # > read 0 0 32
2023-03-09 23:28:20,312 # 00000000  31  32  33  34  35  36  FF  F7  6C  69  74  74  6C  65  66  73
2023-03-09 23:28:20,318 # 00000010  2F  E0  00  10  00  00  02  00  00  01  00  00  00  02  00  00

2023-03-09 23:28:52,800 # > read 0 0 32
2023-03-09 23:28:52,806 # 00000000  31  61  62  63  64  65  66  67  68  69  74  74  6C  65  66  73
2023-03-09 23:28:52,812 # 00000010  2F  E0  00  10  00  00  02  00  00  01  00  00  00  02  00  00

ESP32

tests/mtd_raw

2023-03-09 23:31:24,456 # > test 0
2023-03-09 23:31:24,457 # [START]
2023-03-09 23:31:24,603 # [SUCCESS]

2023-03-09 23:32:43,227 # > read 0 0 32
2023-03-09 23:32:43,234 # 00000000  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF
2023-03-09 23:32:43,240 # 00000010  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF

2023-03-09 23:32:48,918 # > write 0 1 abcdefgh
2023-03-09 23:32:50,963 # 00000000  FF  61  62  63  64  65  66  67  68  FF  FF  FF  FF  FF  FF  FF
2023-03-09 23:32:50,969 # 00000010  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF

2023-03-09 23:32:50,957 # > read 0 0 32
2023-03-09 23:32:50,963 # 00000000  FF  61  62  63  64  65  66  67  68  FF  FF  FF  FF  FF  FF  FF
2023-03-09 23:32:50,969 # 00000010  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF  FF

MTD SDcard

tests/mtd_raw

2023-12-13 16:55:18,923 - INFO # main(): This is RIOT! (Version: 2024.01-devel-369-ga45d2-mtd_drop_write)
2023-12-13 16:55:18,923 - INFO # Manual MTD test
2023-12-13 16:55:19,034 - INFO # init MTD_0… OK (15223 MiB)
test 0
2023-12-13 16:55:26,746 - INFO # > test 0
2023-12-13 16:55:26,747 - INFO # 
2023-12-13 16:55:26,747 - INFO # [START]
2023-12-13 16:55:27,706 - INFO # [SUCCESS]

MTD spi_nor, at24mac, MTD SDMMC (with #20180)

tests/mtd_raw

2023-12-13 17:07:07,125 - INFO # Manual MTD test
2023-12-13 17:07:07,128 - INFO # init MTD_0… OK (8 MiB)
2023-12-13 17:07:07,131 - INFO # init MTD_1… OK (256 byte)
2023-12-13 17:07:07,342 - INFO # init MTD_2… OK (15223 MiB)
test 0
2023-12-13 17:07:10,339 - INFO # > test 0
2023-12-13 17:07:10,339 - INFO # 
2023-12-13 17:07:10,339 - INFO # [START]
2023-12-13 17:07:11,608 - INFO # [SUCCESS]
test 1
2023-12-13 17:07:14,593 - INFO # > test 1
2023-12-13 17:07:14,593 - INFO # 
2023-12-13 17:07:14,594 - INFO # [START]
2023-12-13 17:07:14,648 - INFO # [SUCCESS]
test 2
2023-12-13 17:07:16,275 - INFO # > test 2
2023-12-13 17:07:16,275 - INFO # 
2023-12-13 17:07:16,275 - INFO # [START]
2023-12-13 17:07:16,963 - INFO # [SUCCESS]

Issues/PRs references

includes #15378
probably also needs #14953 and #19258

jue89 · 2020-11-04T11:44:14Z

Do you an idea how test this without owning the respective hardware?

Or can we prove that mtd's translation works in 100% of all cases?

benpicco · 2020-11-04T15:31:19Z

I don't think we have to test this on every implementation, the functionality is the same everywhere.
However, we should make sure nothing breaks when layering mtd implementations, e.g. with mtd_mapper.

I didn't do any tests yet, this is more a PoC, but the idea is to only have one write implementation per MTD backend.

bergzand · 2020-11-17T11:41:11Z

Please rebase!

benpicco · 2021-02-22T10:17:46Z

rebased

MrKevinWeiss · 2021-06-24T11:44:47Z

Any blockers for this, does it just need some testing?

fjmolinas · 2021-06-28T06:54:53Z

I don't think we have to test this on every implementation, the functionality is the same everywhere.
However, we should make sure nothing breaks when layering mtd implementations, e.g. with mtd_mapper.

I didn't do any tests yet, this is more a PoC, but the idea is to only have one write implementation per MTD backend.

Las tcomment sated this was a PoC, is this still the case?

benpicco · 2021-06-28T08:35:59Z

If .write_page() is implemented .write will not be used, so we can drop it and save some ROM.

fjmolinas · 2021-08-24T11:56:00Z

Do you have a test procedure for this?

fjmolinas · 2021-08-24T12:01:31Z

If .write_page() is implemented .write will not be used, so we can drop it and save some ROM.

I only worry that eventually, some external code could be relying on this function, so at least it needs to be tagged as removal. We could also conditionally include (with a pseudo module) and then remove it in a release or two, but maybe it's not worth the effort. Let's just make sure it will be tagged in release notes.

bors · 2023-03-07T12:10:38Z

try

Build succeeded:

The old .write() function is only used as a fall-back if .write_page() is not implemented. We can drop it.

The MTD layer now takes care of splitting up writes, so it should always work (as long as the destination address is within the memory region)

All MTD drivers should now implement the .write_page() function instead.

benpicco · 2023-12-13T16:55:25Z

All backends are tested now, the virtual backends are tested by CI.

benpicco · 2023-12-13T20:36:00Z

Thank you! 😃

benpicco requested a review from MrKevinWeiss as a code owner November 4, 2020 11:04

benpicco requested review from bergzand and vincent-d November 4, 2020 11:04

benpicco added Area: fs Area: File systems Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Nov 4, 2020

benpicco requested a review from jue89 November 4, 2020 11:05

benpicco changed the title ~~mtd/*: drop .write() if .write_page() is implemnted~~ mtd/*: drop .write() if .write_page() is implemented Nov 4, 2020

benpicco requested a review from miri64 as a code owner November 4, 2020 15:17

benpicco added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 4, 2020

benpicco force-pushed the mtd_drop_write branch from 3b4abfa to 7dbbc69 Compare November 17, 2020 11:46

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Nov 17, 2020

benpicco force-pushed the mtd_drop_write branch from 7dbbc69 to ae482d1 Compare November 19, 2020 17:37

benpicco force-pushed the mtd_drop_write branch from ae482d1 to 0531d40 Compare January 20, 2021 18:11

benpicco requested a review from fjmolinas January 20, 2021 18:11

benpicco requested a review from dylad January 29, 2021 20:50

MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

benpicco requested a review from kaspar030 August 11, 2021 09:14

fjmolinas added the Process: removal Integration Process: The PR is removing a deprecated feature or API label Aug 24, 2021

benpicco requested review from leandrolanzieri, aabadie and MichelRottleuthner as code owners February 28, 2023 14:48

bors bot added a commit that referenced this pull request Mar 7, 2023

Try #15380:

0be936c

benpicco added 9 commits December 13, 2023 16:50

cpu/esp_common: flash: drop .write()

2235dc2

The old .write() function is only used as a fall-back if .write_page() is not implemented. We can drop it.

drivers/at25xxx: drop .write()

26aba7b

The old .write() function is only used as a fall-back if .write_page() is not implemented. We can drop it.

drivers/mtd_spi_nor: drop .write()

ea105d3

The old .write() function is only used as a fall-back if .write_page() is not implemented. We can drop it.

drivers/mtd_sdcard: drop .write()

ba17c14

The old .write() function is only used as a fall-back if .write_page() is not implemented. We can drop it.

native/mtd: drop .write()

c664612

The old .write() function is only used as a fall-back if .write_page() is not implemented. We can drop it.

unittests/tests-mtd: expect mtd_write() to always succeed

d525807

The MTD layer now takes care of splitting up writes, so it should always work (as long as the destination address is within the memory region)

tests/mtd_mapper: drop .write() function

3b489cf

drivers/mtd_mapper: drop .write() function

6c28a7d

drivers/mtd_emulated: drop .write() function

cf123f9

benpicco force-pushed the mtd_drop_write branch from 3f358ca to 9dcf47d Compare December 13, 2023 15:57

drivers/mtd_sdmmc: drop .write() function

431afea

benpicco force-pushed the mtd_drop_write branch from 9dcf47d to 82cf5f3 Compare December 13, 2023 15:58

benpicco requested review from maribu and removed request for gschorcht December 13, 2023 16:39

mtd: drop .write() function

9855b02

All MTD drivers should now implement the .write_page() function instead.

benpicco force-pushed the mtd_drop_write branch from 82cf5f3 to 9855b02 Compare December 13, 2023 16:46

maribu approved these changes Dec 13, 2023

View reviewed changes

benpicco added this pull request to the merge queue Dec 13, 2023

benpicco added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Dec 13, 2023

Merged via the queue into RIOT-OS:master with commit 6eac1e1 Dec 13, 2023
27 checks passed

benpicco deleted the mtd_drop_write branch December 13, 2023 23:09

MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mtd/*: drop .write() if .write_page() is implemented #15380

mtd/*: drop .write() if .write_page() is implemented #15380

benpicco commented Nov 4, 2020 •

edited

Loading

jue89 commented Nov 4, 2020

benpicco commented Nov 4, 2020

bergzand commented Nov 17, 2020

benpicco commented Feb 22, 2021

MrKevinWeiss commented Jun 24, 2021

fjmolinas commented Jun 28, 2021

benpicco commented Jun 28, 2021

fjmolinas commented Aug 24, 2021

fjmolinas commented Aug 24, 2021

bors bot commented Mar 7, 2023

benpicco commented Dec 13, 2023

benpicco commented Dec 13, 2023

mtd/*: drop .write() if .write_page() is implemented #15380

mtd/*: drop .write() if .write_page() is implemented #15380

Conversation

benpicco commented Nov 4, 2020 • edited Loading

Contribution description

Testing procedure

tests/mtd_raw

tests/mtd_raw

tests/mtd_raw

tests/mtd_raw

Issues/PRs references

jue89 commented Nov 4, 2020

benpicco commented Nov 4, 2020

bergzand commented Nov 17, 2020

benpicco commented Feb 22, 2021

MrKevinWeiss commented Jun 24, 2021

fjmolinas commented Jun 28, 2021

benpicco commented Jun 28, 2021

fjmolinas commented Aug 24, 2021

fjmolinas commented Aug 24, 2021

bors bot commented Mar 7, 2023

try

benpicco commented Dec 13, 2023

benpicco commented Dec 13, 2023

benpicco commented Nov 4, 2020 •

edited

Loading