drivers/mtd_flashpage: implement pagewise API, don't use raw addresses#19258
drivers/mtd_flashpage: implement pagewise API, don't use raw addresses#19258bors[bot] merged 5 commits intoRIOT-OS:masterfrom
Conversation
|
Oh, you did :D Nevermind ... |
df31831 to
6c78cc4
Compare
|
I was asking myself the same question, why the MTD flashpage sector is further divided into smaller pages. Testdiff --git a/tests/mtd_flashpage/main.c b/tests/mtd_flashpage/main.c
index 0c62d13d2b..ed04b8a242 100644
--- a/tests/mtd_flashpage/main.c
+++ b/tests/mtd_flashpage/main.c
@@ -36,7 +36,9 @@
#endif
#define TEST_ADDRESS0 ((FLASHPAGE_NUMOF - 1) - flashpage_page((void *)CPU_FLASH_BASE))
-static mtd_flashpage_t _dev = MTD_FLASHPAGE_INIT_VAL(8);
+#define PAGES_PER_SECTOR 8
+
+static mtd_flashpage_t _dev = MTD_FLASHPAGE_INIT_VAL(PAGES_PER_SECTOR);
static mtd_dev_t *dev = &_dev.base;
static void setup(void)
@@ -143,6 +145,59 @@ static void test_mtd_write_read(void)
TEST_ASSERT_EQUAL_INT(0, ret);
}
+static void test_mtd_write_read_page(void)
+{
+#if IS_USED(MODULE_MTD_WRITE_PAGE)
+ const char buf[] __attribute__ ((aligned (FLASHPAGE_WRITE_BLOCK_ALIGNMENT)))
+ = "abcdefghijklmno";
+
+ uint8_t buf_empty[3];
+ memset(buf_empty, FLASHPAGE_ERASE_STATE, sizeof(buf_empty));
+ char buf_read[sizeof(buf) + sizeof(buf_empty)];
+ memset(buf_read, 0, sizeof(buf_read));
+ int ret;
+ /* convert last flash page to MTD page */
+ uint32_t last_vpage = LAST_AVAILABLE_PAGE * dev->pages_per_sector;
+ size_t vpage_size = dev->page_size;
+
+ /* write to the beginning of last available page */
+ ret = mtd_write_page(dev, buf, last_vpage, 0, sizeof(buf));
+ TEST_ASSERT_EQUAL_INT(0, ret);
+
+ /* read back data from which some is erased */
+ ret = mtd_read_page(dev, buf_read, last_vpage, 0, sizeof(buf_read));
+ TEST_ASSERT_EQUAL_INT(0, ret);
+ TEST_ASSERT_EQUAL_INT(0, memcmp(buf, buf_read, sizeof(buf)));
+ TEST_ASSERT_EQUAL_INT(0, memcmp(buf_empty, buf_read + sizeof(buf), sizeof(buf_empty)));
+
+ /* clean */
+ ret = mtd_erase_sector(dev, LAST_AVAILABLE_PAGE, 1);
+
+ /* write to the beginning of the MTD page before the last available flash page */
+ ret = mtd_write_page(dev, buf, last_vpage - 1, 0, sizeof(buf));
+ TEST_ASSERT_EQUAL_INT(0, ret);
+
+ /* read back data from which some is erased */
+ ret = mtd_read_page(dev, buf_read, last_vpage - 1, 0, sizeof(buf_read));
+ TEST_ASSERT_EQUAL_INT(0, ret);
+ TEST_ASSERT_EQUAL_INT(0, memcmp(buf, buf_read, sizeof(buf)));
+ TEST_ASSERT_EQUAL_INT(0, memcmp(buf_empty, buf_read + sizeof(buf), sizeof(buf_empty)));
+
+ /* clean*/
+ ret = mtd_erase_sector(dev, LAST_AVAILABLE_PAGE, 1);
+
+ /* write across flash page boundary */
+ ret = mtd_write_page(dev, buf, last_vpage - 1, vpage_size - sizeof(buf) + 1, sizeof(buf));
+ TEST_ASSERT_EQUAL_INT(0, ret);
+
+ /* read back data from which some is erased */
+ ret = mtd_read_page(dev, buf_read, last_vpage - 1, vpage_size - sizeof(buf) + 1, sizeof(buf_read));
+ TEST_ASSERT_EQUAL_INT(0, ret);
+ TEST_ASSERT_EQUAL_INT(0, memcmp(buf, buf_read, sizeof(buf)));
+ TEST_ASSERT_EQUAL_INT(0, memcmp(buf_empty, buf_read + sizeof(buf), sizeof(buf_empty)));
+#endif
+}
+
Test *tests_mtd_flashpage_tests(void)
{
EMB_UNIT_TESTFIXTURES(fixtures) {
@@ -150,6 +205,7 @@ Test *tests_mtd_flashpage_tests(void)
new_TestFixture(test_mtd_erase),
new_TestFixture(test_mtd_write_erase),
new_TestFixture(test_mtd_write_read),
+ new_TestFixture(test_mtd_write_read_page),
};
EMB_UNIT_TESTCALLER(mtd_flashpage_tests, setup, teardown, fixtures);I was debugging it and found some changes to make it work. Diffdiff --git a/drivers/mtd_flashpage/mtd_flashpage.c b/drivers/mtd_flashpage/mtd_flashpage.c
index 8820e89f50..1d19ae503b 100644
--- a/drivers/mtd_flashpage/mtd_flashpage.c
+++ b/drivers/mtd_flashpage/mtd_flashpage.c
@@ -51,7 +51,9 @@ static int _read_page(mtd_dev_t *dev, void *buf, uint32_t page,
page += super->offset;
/* mtd flashpage maps multiple pages to one virtual sector for unknown reason */
- uintptr_t addr = (uintptr_t)flashpage_addr(page / dev->pages_per_sector) + offset;
+ uint32_t fpage = page / dev->pages_per_sector;
+ offset += (page % dev->pages_per_sector) * dev->page_size;
+ uintptr_t addr = (uintptr_t)flashpage_addr(fpage) + offset;
DEBUG("flashpage: read %"PRIu32" bytes from %p to %p\n", size, (void *)addr, buf);
@@ -142,7 +144,6 @@ static int _erase_page(mtd_dev_t *dev, uint32_t page, uint32_t count)
{
mtd_flashpage_t *super = container_of(dev, mtd_flashpage_t, base);
- count *= dev->pages_per_sector;
page += super->offset;
while (count--) {
DEBUG("flashpage: erase page %"PRIu32"\n", page); |
The MTD page to flash page conversion and offset correction seems to be required too for |
|
or in the I'm very much inclined to just remove this virtual sector shenanigans (but in a follow-up PR) because I really don't see what it could be useful for. I wrote @vincent-d a mail to better understand what his use-case was for adding this back then. |
|
btw, I remember you did something similar (#18141) for littleFS. |
I think 'blocks' are a software configuration. AFAIK there is no hardware block size, but the minimum block size, a filesystem is initialized with, is the size of a hardware sector, but in general it can be a multiple of it for optimization reasons. |
|
Does this make sense or did you have something else in mind? |
|
Ah right so for littleFS you combine multiple sectors into one block for more efficient handling.
|
|
Last commit brakes the test. diff --git a/drivers/mtd_flashpage/mtd_flashpage.c b/drivers/mtd_flashpage/mtd_flashpage.c
index 8be1f5bf7b..940023bb2e 100644
--- a/drivers/mtd_flashpage/mtd_flashpage.c
+++ b/drivers/mtd_flashpage/mtd_flashpage.c
@@ -38,8 +38,10 @@
static int _init(mtd_dev_t *dev)
{
- (void)dev;
+ mtd_flashpage_t *super = container_of(dev, mtd_flashpage_t, base);
+ (void)super;
assert(dev->pages_per_sector * dev->page_size == FLASHPAGE_SIZE);
+ assert(!(super->offset % dev->pages_per_sector));
return 0;
}
@@ -117,16 +119,15 @@ static int _write_page(mtd_dev_t *dev, const void *buf, uint32_t page, uint32_t
return size;
}
-static int _erase_page(mtd_dev_t *dev, uint32_t page, uint32_t count)
+static int _erase_sector(mtd_dev_t *dev, uint32_t sector, uint32_t count)
{
mtd_flashpage_t *super = container_of(dev, mtd_flashpage_t, base);
- page += super->offset;
- page /= dev->pages_per_sector;
+ sector += (super->offset / dev->pages_per_sector);
while (count--) {
- DEBUG("flashpage: erase page %"PRIu32"\n", page);
- flashpage_erase(page++);
+ DEBUG("flashpage: erase sector %"PRIu32"\n", sector);
+ flashpage_erase(sector++);
}
return 0;
@@ -136,5 +137,5 @@ const mtd_desc_t mtd_flashpage_driver = {
.init = _init,
.read_page = _read_page,
.write_page = _write_page,
- .erase_sector = _erase_page,
+ .erase_sector = _erase_sector,
}; |
|
And document what the diff --git a/drivers/include/mtd_flashpage.h b/drivers/include/mtd_flashpage.h
index 072ba8dc3d..7d555dabf9 100644
--- a/drivers/include/mtd_flashpage.h
+++ b/drivers/include/mtd_flashpage.h
@@ -57,7 +57,9 @@ extern const mtd_desc_t mtd_flashpage_driver;
*/
typedef struct {
mtd_dev_t base; /**< MTD generic device */
- size_t offset; /**< offset (in pages) from the start of the flash */
+ size_t offset; /**< Offset in terms of MTD pages, which must comprise
+ a whole number of sectors from the start of the
+ flash */
} mtd_flashpage_t;
#ifdef __cplusplus |
|
Thank you for the fixes, applied them! |
|
We should protect for out of bounds access because we crash if that happens: diff --git a/drivers/mtd_flashpage/mtd_flashpage.c b/drivers/mtd_flashpage/mtd_flashpage.c
index 940023bb2e..660e63e769 100644
--- a/drivers/mtd_flashpage/mtd_flashpage.c
+++ b/drivers/mtd_flashpage/mtd_flashpage.c
@@ -57,6 +57,12 @@ static int _read_page(mtd_dev_t *dev, void *buf, uint32_t page,
offset += (page % dev->pages_per_sector) * dev->page_size;
uintptr_t addr = (uintptr_t)flashpage_addr(fpage) + offset;
+ /* protect for out of bounds reads */
+ if (addr + size < addr ||
+ addr + size > MTD_FLASHPAGE_END_ADDR) {
+ return -EOVERFLOW;
+ }
+
DEBUG("flashpage: read %"PRIu32" bytes from %p to %p\n", size, (void *)addr, buf);
#ifndef CPU_HAS_UNALIGNED_ACCESS
@@ -90,6 +96,12 @@ static int _write_page(mtd_dev_t *dev, const void *buf, uint32_t page, uint32_t
offset += (page % dev->pages_per_sector) * dev->page_size;
uintptr_t addr = (uintptr_t)flashpage_addr(fpage) + offset;
+ /* protect for out of bounds writes */
+ if (addr + size < addr ||
+ addr + size > MTD_FLASHPAGE_END_ADDR) {
+ return -EOVERFLOW;
+ }
+
DEBUG("flashpage: write %"PRIu32" bytes from %p to %p\n", size, buf, (void *)addr);
size = MIN(flashpage_size(fpage) - offset, size);
@@ -125,6 +137,12 @@ static int _erase_sector(mtd_dev_t *dev, uint32_t sector, uint32_t count)
sector += (super->offset / dev->pages_per_sector);
+ /* protect for out of bounds erase */
+ if (sector + count < sector ||
+ sector + count > dev->sector_count) {
+ return -EOVERFLOW;
+ }
+
while (count--) {
DEBUG("flashpage: erase sector %"PRIu32"\n", sector);
flashpage_erase(sector++);
diff --git a/tests/mtd_flashpage/main.c b/tests/mtd_flashpage/main.c
index de9e89171a..1ed470252b 100644
--- a/tests/mtd_flashpage/main.c
+++ b/tests/mtd_flashpage/main.c
@@ -34,7 +34,8 @@
#define TEST_ADDRESS1 (uint32_t)((uintptr_t)flashpage_addr(LAST_AVAILABLE_PAGE) - (uintptr_t)CPU_FLASH_BASE)
#define TEST_ADDRESS2 (uint32_t)((uintptr_t)flashpage_addr(LAST_AVAILABLE_PAGE - 1) - (uintptr_t)CPU_FLASH_BASE)
#endif
-#define TEST_ADDRESS0 (FLASHPAGE_NUMOF - 1)
+/* Address of last flash page and not last available flashpage */
+#define TEST_ADDRESS0 (uint32_t)((uintptr_t)flashpage_addr((FLASHPAGE_NUMOF - 1)) - (uintptr_t)CPU_FLASH_BASE)
#define PAGES_PER_SECTOR 8
@@ -136,6 +137,14 @@ static void test_mtd_write_read(void)
ret = mtd_erase(dev, TEST_ADDRESS1, dev->pages_per_sector * dev->page_size);
TEST_ASSERT_EQUAL_INT(0, ret);
+ /* Out of bounds read */
+ ret = mtd_read(dev, buf_read, TEST_ADDRESS0 + FLASHPAGE_SIZE - 1, sizeof(buf_read));
+ TEST_ASSERT_EQUAL_INT(-EOVERFLOW, ret);
+
+ /* Out of bounds write */
+ ret = mtd_write(dev, buf, TEST_ADDRESS0 + FLASHPAGE_SIZE - 1, sizeof(buf));
+ TEST_ASSERT_EQUAL_INT(-EOVERFLOW, ret);
+
/* Unaligned write / read */
ret = mtd_write(dev, &buf[1], TEST_ADDRESS1 + sizeof(buf_empty), sizeof(buf) - 1);
TEST_ASSERT_EQUAL_INT(0, ret);
@@ -195,6 +204,16 @@ static void test_mtd_write_read_page(void)
TEST_ASSERT_EQUAL_INT(0, ret);
TEST_ASSERT_EQUAL_INT(0, memcmp(buf, buf_read, sizeof(buf)));
TEST_ASSERT_EQUAL_INT(0, memcmp(buf_empty, buf_read + sizeof(buf), sizeof(buf_empty)));
+
+ /* Out of bounds read */
+ ret = mtd_read_page(dev, buf_read,
+ FLASHPAGE_NUMOF * dev->pages_per_sector, 0, sizeof(buf_read));
+ TEST_ASSERT_EQUAL_INT(-EOVERFLOW, ret);
+
+ /* Out of bounds write */
+ ret = mtd_write_page(dev, buf,
+ FLASHPAGE_NUMOF * dev->pages_per_sector, 0, sizeof(buf));
+ TEST_ASSERT_EQUAL_INT(-EOVERFLOW, ret);
#endif
}
|
|
Since the MTD layer does not know about the |
|
Ok, please squash if you like and let´s see what the CI thinks |
260454c to
2ea7637
Compare
2ea7637 to
68d1123
Compare
I am testing with |
fabian18
left a comment
There was a problem hiding this comment.
One last thing I guess.
We could even be a bit more overflow safe.
diff --git a/drivers/include/mtd_flashpage.h b/drivers/include/mtd_flashpage.h
index 7d555dabf9..932efc2bcc 100644
--- a/drivers/include/mtd_flashpage.h
+++ b/drivers/include/mtd_flashpage.h
@@ -57,7 +57,7 @@ extern const mtd_desc_t mtd_flashpage_driver;
*/
typedef struct {
mtd_dev_t base; /**< MTD generic device */
- size_t offset; /**< Offset in terms of MTD pages, which must comprise
+ uint32_t offset; /**< Offset in terms of MTD pages, which must comprise
a whole number of sectors from the start of the
flash */
} mtd_flashpage_t;
diff --git a/drivers/mtd_flashpage/mtd_flashpage.c b/drivers/mtd_flashpage/mtd_flashpage.c
index 3ba1ba5835..d90a1fa93e 100644
--- a/drivers/mtd_flashpage/mtd_flashpage.c
+++ b/drivers/mtd_flashpage/mtd_flashpage.c
@@ -51,18 +51,22 @@ static int _read_page(mtd_dev_t *dev, void *buf, uint32_t page,
{
mtd_flashpage_t *super = container_of(dev, mtd_flashpage_t, base);
+ if (page + super->offset < page) {
+ return -EOVERFLOW;
+ }
page += super->offset;
/* mtd flashpage maps multiple pages to one virtual sector for unknown reason */
uint32_t fpage = page / dev->pages_per_sector;
offset += (page % dev->pages_per_sector) * dev->page_size;
- uintptr_t addr = (uintptr_t)flashpage_addr(fpage) + offset;
+ uintptr_t addr = (uintptr_t)flashpage_addr(fpage);
/* protect for out of bounds reads */
- if (addr + size < addr ||
- addr + size > MTD_FLASHPAGE_END_ADDR) {
+ if (addr + offset + size < addr ||
+ addr + offset + size > MTD_FLASHPAGE_END_ADDR) {
return -EOVERFLOW;
}
+ addr += offset;
DEBUG("flashpage: read %"PRIu32" bytes from %p to %p\n", size, (void *)addr, buf);
@@ -90,18 +94,22 @@ static int _write_page(mtd_dev_t *dev, const void *buf, uint32_t page, uint32_t
{
mtd_flashpage_t *super = container_of(dev, mtd_flashpage_t, base);
+ if (page + super->offset < page) {
+ return -EOVERFLOW;
+ }
page += super->offset;
/* mtd flashpage maps multiple pages to one virtual sector for unknown reason */
uint32_t fpage = page / dev->pages_per_sector;
offset += (page % dev->pages_per_sector) * dev->page_size;
- uintptr_t addr = (uintptr_t)flashpage_addr(fpage) + offset;
+ uintptr_t addr = (uintptr_t)flashpage_addr(fpage);
/* protect for out of bounds writes */
- if (addr + size < addr ||
- addr + size > MTD_FLASHPAGE_END_ADDR) {
+ if (addr + offset + size < addr ||
+ addr + offset + size > MTD_FLASHPAGE_END_ADDR) {
return -EOVERFLOW;
}
+ addr += offset;
DEBUG("flashpage: write %"PRIu32" bytes from %p to %p\n", size, buf, (void *)addr);
@@ -136,6 +144,9 @@ static int _erase_sector(mtd_dev_t *dev, uint32_t sector, uint32_t count)
{
mtd_flashpage_t *super = container_of(dev, mtd_flashpage_t, base);
+ if (sector + (super->offset / dev->pages_per_sector) < sector) {
+ return -EOVERFLOW;
+ }
sector += (super->offset / dev->pages_per_sector);
/* protect for out of bounds erase */|
Added them, but we should better add those overflow checks to We can still check for config errors to ensure |
You mean in addition to the checks with
Sure, add this if you want. You mean in |
No, they are not needed.
we can conclude that no access that passes the check in 2. will be beyond the end of the device, no matter what |
fabian18
left a comment
There was a problem hiding this comment.
Ok the out of bounds checks can be in the MTD layer because if a device has for example 10 physical sectors and offset is 1, the MTD sector_count must be initialized with 9, right?
exactly! |
fabian18
left a comment
There was a problem hiding this comment.
I think it is good. ACK one of two.
628b841 to
43fffcd
Compare
|
Or does it not need 2 ACK anaymore? |
|
Does it? The dreaded virtual pages are still there. |
|
Thought API change always needs 2 ACK. If not then even better. |
|
Depends on the severity of the change, nothing's set in stone. Thank you for the helpful suggestions btw! bors merge |
|
Build succeeded: |
Contribution description
The
mtd_flashpageAPI is a bit special in that it experts the user to pass raw flash addresses to it.This is very usual because every other MTD device will start at address 0, however with
mtd_flashpagewriting to address 0 will overwrite the firmware.If the Flash is not mapped to zero, this would just crash.
To fix this an
.offsetmember is added that should be set to the first writable page after the firmware.With this,
mtd_flashpagecan then be used like any other MTD device.#18608 will add a mechanism to set this offset at compile-time to allow for a MTD slot on the internal flash.
Testing procedure
tests/mtd_flashpageshould still work.Issues/PRs references
Split off #18608
alternative to #17964