drivers: disk_flash: Create disk_flash driver #62527
+432
−0
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # Disk flash emulator config | ||
|
|
||
| # Copyright (c) 2023 Endress+Hauser AG | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| config DISK_FLASH_DRIVER | ||
| bool "Use the flash on disk emulator" | ||
| default y | ||
| depends on DT_HAS_ZEPHYR_DISK_FLASH_ENABLED | ||
| select FLASH_HAS_DRIVER_ENABLED | ||
| help | ||
| This option enables the flash on disk emulator | ||
| which can be used to emulate a flash on a disk | ||
| device like sd-card or ramdisk. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,341 @@ | ||
| /* | ||
| * Copyright (c) 2023 Endress+Hauser AG | ||
| * | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| #define DT_DRV_COMPAT zephyr_disk_flash | ||
|
|
||
| #include <string.h> | ||
| #include <zephyr/logging/log.h> | ||
| #include <zephyr/device.h> | ||
| #include <zephyr/storage/disk_access.h> | ||
| #include <zephyr/drivers/flash.h> | ||
| #include <zephyr/drivers/disk.h> | ||
|
|
||
| LOG_MODULE_REGISTER(disk_flash, CONFIG_FLASH_LOG_LEVEL); | ||
|
|
||
| struct disk_flash_data { | ||
| struct k_sem sem; | ||
| uint8_t *const sector_buf; | ||
| uint16_t const sector_buf_size; | ||
| uint32_t disk_total_sector_cnt; | ||
| uint32_t disk_sector_size; | ||
| }; | ||
|
|
||
| struct disk_flash_config { | ||
| uint32_t flash_size; | ||
| struct flash_parameters flash_parameters; | ||
| char *disk_name; | ||
| uint32_t disk_offset; | ||
|
|
||
| IF_ENABLED(CONFIG_FLASH_PAGE_LAYOUT, (struct flash_pages_layout fpl;)) | ||
| }; | ||
|
|
||
| static void acquire_device(const struct device *dev) | ||
| { | ||
| struct disk_flash_data *const data = dev->data; | ||
|
|
||
| k_sem_take(&data->sem, K_FOREVER); | ||
| } | ||
|
|
||
| static void release_device(const struct device *dev) | ||
| { | ||
| struct disk_flash_data *const data = dev->data; | ||
|
|
||
| k_sem_give(&data->sem); | ||
| } | ||
|
|
||
| static int check_boundary(const struct device *dev, off_t offset, size_t len) | ||
| { | ||
| int ret = 0; | ||
| const struct disk_flash_config *cfg = dev->config; | ||
| size_t operation_end = offset + len; | ||
|
|
||
| if (cfg->flash_size < operation_end || offset < 0) { | ||
| LOG_ERR("Operation out of bounds"); | ||
| ret = -EINVAL; | ||
| } | ||
|
|
||
| return ret; | ||
| } | ||
|
|
||
| static int disk_flash_read(const struct device *dev, off_t offset, void *dst, size_t len) | ||
| { | ||
| int ret = 0; | ||
| uint8_t *dst_buf = dst; | ||
| const struct disk_flash_config *cfg = dev->config; | ||
| struct disk_flash_data *data = dev->data; | ||
| uint32_t write_cursor = 0; | ||
|
|
||
| acquire_device(dev); | ||
|
|
||
| ret = check_boundary(dev, offset, len); | ||
| if (ret != 0) { | ||
| goto release_device; | ||
| } | ||
|
|
||
| if (data->disk_sector_size == 0) { | ||
| ret = -EINVAL; | ||
| goto release_device; | ||
| } | ||
|
|
||
| while (write_cursor < len) { | ||
| uint32_t read_cursor = cfg->disk_offset + offset + write_cursor; | ||
| uint32_t read_sector = read_cursor / data->disk_sector_size; | ||
| uint32_t read_padding = read_cursor % data->disk_sector_size; | ||
| uint32_t len_write = (len - write_cursor); | ||
|
|
||
| if (disk_access_read(cfg->disk_name, data->sector_buf, read_sector, 1)) { | ||
| ret = -EINVAL; | ||
| goto release_device; | ||
| } | ||
|
|
||
| if (len_write > data->sector_buf_size - read_padding) { | ||
| len_write = data->sector_buf_size - read_padding; | ||
| } | ||
|
|
||
| memcpy(&dst_buf[write_cursor], &data->sector_buf[read_padding], len_write); | ||
| write_cursor += len_write; | ||
| } | ||
|
|
||
| release_device: | ||
| release_device(dev); | ||
| return ret; | ||
| } | ||
|
|
||
| static int disk_flash_write(const struct device *dev, off_t offset, const void *src, size_t len) | ||
| { | ||
| int ret = 0; | ||
| const uint8_t *src_buf = src; | ||
| const struct disk_flash_config *cfg = dev->config; | ||
| struct disk_flash_data *data = dev->data; | ||
| uint32_t read_cursor = 0; | ||
|
|
||
| acquire_device(dev); | ||
|
|
||
| ret = check_boundary(dev, offset, len); | ||
| if (ret != 0) { | ||
| goto release_device; | ||
| } | ||
|
|
||
| if (data->disk_sector_size == 0) { | ||
| ret = -EINVAL; | ||
| goto release_device; | ||
| } | ||
|
|
||
| while (read_cursor < len) { | ||
| uint32_t write_cursor = cfg->disk_offset + offset + read_cursor; | ||
| uint32_t write_sector = write_cursor / data->disk_sector_size; | ||
| uint32_t write_padding = write_cursor % data->disk_sector_size; | ||
| uint32_t len_write = (len - read_cursor); | ||
|
|
||
| if (disk_access_read(cfg->disk_name, data->sector_buf, write_sector, 1)) { | ||
| ret = -EINVAL; | ||
| goto release_device; | ||
| } | ||
|
|
||
| if (len_write > data->sector_buf_size - write_padding) { | ||
| len_write = data->sector_buf_size - write_padding; | ||
| } | ||
|
|
||
| for (size_t i = 0; i < len_write; i++) { | ||
| data->sector_buf[write_padding + i] &= src_buf[read_cursor + i]; | ||
| } | ||
|
|
||
| if (disk_access_write(cfg->disk_name, data->sector_buf, write_sector, 1)) { | ||
| ret = -EINVAL; | ||
| goto release_device; | ||
| } | ||
| read_cursor += len_write; | ||
| } | ||
|
|
||
| release_device: | ||
| release_device(dev); | ||
| return ret; | ||
| } | ||
|
|
||
| static int disk_flash_erase(const struct device *dev, off_t offset, size_t size) | ||
| { | ||
| int ret = 0; | ||
| const struct disk_flash_config *cfg = dev->config; | ||
| struct disk_flash_data *data = dev->data; | ||
| uint32_t read_cursor = 0; | ||
|
|
||
| acquire_device(dev); | ||
|
|
||
| ret = check_boundary(dev, offset, size); | ||
| if (ret != 0) { | ||
| goto release_device; | ||
| } | ||
|
|
||
| if (data->disk_sector_size == 0) { | ||
| ret = -EINVAL; | ||
| goto release_device; | ||
| } | ||
|
|
||
| while (read_cursor < size) { | ||
| uint32_t write_cursor = cfg->disk_offset + offset + read_cursor; | ||
| uint32_t write_sector = write_cursor / data->disk_sector_size; | ||
| uint32_t write_padding = write_cursor % data->disk_sector_size; | ||
| uint32_t len_write = (size - read_cursor); | ||
|
Comment on lines
+178
to
+181
There was a problem hiding this comment. I guess that both cursors will be moved by sector size or multiple of that? Looks like too much arithmetic for adding sector size. There was a problem hiding this comment. Not necessarily moved by sector size since the driver is not dependent on the underlying disk sector size partial sectors may be written or read. As such I think that much arithmetic is needed. |
||
|
|
||
| if (write_sector < 0 || write_sector >= data->disk_total_sector_cnt) { | ||
| ret = -EINVAL; | ||
| goto release_device; | ||
| } | ||
|
|
||
| if (disk_access_read(cfg->disk_name, data->sector_buf, write_sector, 1)) { | ||
| ret = -EINVAL; | ||
| goto release_device; | ||
| } | ||
|
|
||
| if (len_write > data->sector_buf_size - write_padding) { | ||
| len_write = data->sector_buf_size - write_padding; | ||
| } | ||
|
|
||
| memset(&data->sector_buf[write_padding], | ||
| cfg->flash_parameters.erase_value, | ||
| len_write); | ||
|
Comment on lines
+197
to
+199
There was a problem hiding this comment. You could do that once, before entering the loop. There was a problem hiding this comment. Since the erase supports arbitrary size and does not need to be aligned with the sector of the disk partial sectors need to be erased. Therefore I don't currently see a possibility to do this outside the loop. |
||
|
|
||
| if (disk_access_write(cfg->disk_name, data->sector_buf, write_sector, 1)) { | ||
| ret = -EINVAL; | ||
| goto release_device; | ||
| } | ||
| read_cursor += len_write; | ||
| } | ||
|
|
||
| release_device: | ||
| release_device(dev); | ||
| return ret; | ||
| } | ||
|
|
||
| #if defined(CONFIG_FLASH_PAGE_LAYOUT) | ||
| static void disk_flash_pages_layout(const struct device *dev, | ||
| const struct flash_pages_layout **layout, size_t *layout_size) | ||
| { | ||
| const struct disk_flash_config *cfg = dev->config; | ||
|
|
||
| *layout = &cfg->fpl; | ||
| *layout_size = 1; | ||
| } | ||
| #endif | ||
|
|
||
| static const struct flash_parameters *disk_flash_get_parameters(const struct device *dev) | ||
| { | ||
| const struct disk_flash_config *cfg = dev->config; | ||
|
|
||
| return &cfg->flash_parameters; | ||
| } | ||
|
|
||
| static const struct flash_driver_api disk_flash_api = { | ||
| .read = disk_flash_read, | ||
| .write = disk_flash_write, | ||
| .erase = disk_flash_erase, | ||
| .get_parameters = disk_flash_get_parameters, | ||
| #if defined(CONFIG_FLASH_PAGE_LAYOUT) | ||
| .page_layout = disk_flash_pages_layout, | ||
| #endif | ||
| #if defined(CONFIG_FLASH_JESD216_API) | ||
| .sfdp_read = NULL, | ||
| .read_jedec_id = NULL, | ||
| #endif | ||
| #if defined(CONFIG_FLASH_EX_OP_ENABLED) | ||
| .ex_op = NULL, | ||
| #endif | ||
| }; | ||
|
|
||
| static int disk_flash_init(const struct device *dev) | ||
| { | ||
| int res = 0; | ||
| struct disk_flash_data *data = dev->data; | ||
| const struct disk_flash_config *cfg = dev->config; | ||
| uint64_t disk_size = 0; | ||
|
|
||
| k_sem_init(&data->sem, 1, 1); | ||
|
|
||
| res = disk_access_init(cfg->disk_name); | ||
| if (res != 0) { | ||
| LOG_ERR("init disk failed: %d", res); | ||
| goto init_done; | ||
| } | ||
|
|
||
| res = disk_access_ioctl(cfg->disk_name, DISK_IOCTL_GET_SECTOR_COUNT, | ||
| &data->disk_total_sector_cnt); | ||
| if (res != 0) { | ||
| LOG_ERR("read total sector cnt failed: %d", res); | ||
| goto init_done; | ||
| } | ||
|
|
||
|
|
||
| res = disk_access_ioctl(cfg->disk_name, DISK_IOCTL_GET_SECTOR_SIZE, | ||
| &data->disk_sector_size); | ||
| if (res != 0) { | ||
| LOG_ERR("read sector size failed: %d", res); | ||
| goto init_done; | ||
| } | ||
|
|
||
| if (data->disk_sector_size > data->sector_buf_size) { | ||
| LOG_ERR("sector size %u of disk to big for buffer %u", data->disk_sector_size, | ||
| (uint32_t)data->sector_buf_size); | ||
| res = -EINVAL; | ||
| goto init_done; | ||
| } | ||
|
|
||
| if (cfg->disk_offset % data->disk_sector_size != 0) { | ||
| LOG_WRN("offset is not aligned with disk sectors (disk sector size: %u)", | ||
| data->disk_sector_size); | ||
| } | ||
|
|
||
| disk_size = data->disk_sector_size * data->disk_total_sector_cnt; | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. You are right. There is absolutely a possibility that large disks could be used since this driver supports partitioning. I adjusted this. |
||
| if ((disk_size - cfg->disk_offset) < cfg->flash_size) { | ||
| LOG_ERR("Underlying disk to small to support flash: %lu < %d", | ||
| (unsigned long)(disk_size - cfg->disk_offset), | ||
| cfg->flash_size); | ||
| res = -EINVAL; | ||
| goto init_done; | ||
| } | ||
|
|
||
| init_done: | ||
| return res; | ||
| } | ||
|
|
||
| #define DISK_FLASH_DEFINE(index) \ | ||
| BUILD_ASSERT(DT_INST_PROP(index, size) > 0, "flash_size needs to be > 0"); \ | ||
| BUILD_ASSERT(DT_INST_PROP(index, page_size) > 0, "page_size needs to be > 0"); \ | ||
| BUILD_ASSERT(DT_INST_PROP(index, disk_offset) >= 0, "disk_offset needs to be >= 0"); \ | ||
| uint8_t sector_buf_##index[DT_INST_PROP(index, disk_sector_size)]; \ | ||
| static struct disk_flash_data disk_flash_data_##index = { \ | ||
| .sector_buf = sector_buf_##index, \ | ||
| .sector_buf_size = sizeof(sector_buf_##index), \ | ||
| }; \ | ||
| static const struct disk_flash_config disk_flash_config_##index = { \ | ||
| .flash_size = DT_INST_PROP(index, size) / 8, \ | ||
| .flash_parameters = \ | ||
| { \ | ||
| .write_block_size = \ | ||
| COND_CODE_0(DT_INST_PROP(index, write_block_size), \ | ||
| (DT_INST_PROP(index, disk_sector_size)), \ | ||
| (DT_INST_PROP(index, write_block_size))), \ | ||
| .erase_value = 0xFF \ | ||
| }, \ | ||
| IF_ENABLED( \ | ||
| CONFIG_FLASH_PAGE_LAYOUT, \ | ||
| (.fpl = \ | ||
| { \ | ||
| .pages_count = \ | ||
| (DT_INST_PROP(index, size) / 8) \ | ||
| / DT_INST_PROP(index, page_size), \ | ||
| .pages_size = DT_INST_PROP(index, page_size) \ | ||
| }, \ | ||
| ) \ | ||
| ) \ | ||
| .disk_name = DT_INST_PROP(index, disk_name), \ | ||
| .disk_offset = DT_INST_PROP(index, disk_offset), \ | ||
| }; \ | ||
| DEVICE_DT_INST_DEFINE(index, &disk_flash_init, NULL, &disk_flash_data_##index, \ | ||
| &disk_flash_config_##index, \ | ||
| DT_STRING_TOKEN(DT_DRV_INST(index), init_level), \ | ||
| CONFIG_FLASH_INIT_PRIORITY, &disk_flash_api); | ||
|
|
||
| DT_INST_FOREACH_STATUS_OKAY(DISK_FLASH_DEFINE) | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think it is very efficient to keep recalculating these in loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@de-nordic
I agree that this is not very efficient but I don't see a way around it since the driver allows arbitrary write/read sizes and does not need to be sector aligned with the disk.
I understood you believe, that read/write/erase granularity must be equal to the underlying disk device. The idea of this flash driver is to offer a read/write/erase granularity of a single byte on top of a disk with arbitrary sector sizes. So operations typically:
-read sector,
-update sector,
-write sector,
-loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is bad. This means that half of sector may belong to something else.
And another thing is that SD card is block device. Read/write and emulated erase happens in blocks.
If you modify single byte what will happen is that entire sector needs to be re-written by a controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But unless done this way, how would you simulate a flash with potentially different properties then underlying device?
Would you prefer, that
flash_diskinherits properties from underlying device? If so, it would probably no longer be suitable for our use-case.I see the following possibilities:
Of course with 1. it is expected, that caller calls with big junks also. With 3. it is not possible to use as secondary partition for
mcubootif underlying device has different properties to main flash. Which would nullify the main benefit of theflash_diskmodule.Or am I missing something? Can primary and secondary partition for mcuboot have different properties?
If yes, then of course, the story is entirely different.
Thanks for the quick responses and instructing reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or did I completely miss your point and you suggestions is just to improve simply the code to:
disk_accessThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for elaborating your concerns, I think, that I understand better now. Did I understand your concerns correclty, that:
Are you refering to the fact, that swapping might fail when using
swap-using-moveon an image that is too big (100% of available space) because it can not move down the primary imgage block-wise when there is no free block? Whereas this can not happen when usingswap-using-scratch? Or is there another reason for recommendingswap using scratch?Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both with
swap-using-moveandswap-using-scratchthe result of a block move is written to flash. When using a disk and the write of the last sector fails you could loose all information about the process (all information is on one disk sector), while on flash only the last block move information might be incorrect. If all information is lost there is no way to continue the swap and you are left with an unrecoverable system.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood.
So it would not matter which strategy here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the strategy does not matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@de-nordic @Laczen
Shouldn't the danger of sector write fail be lowered since when not written entirely the sd-card should still contain a partially correct sector? Or how is the status of the swap tracked? By settings bytes to 0?
Also I do not currently see any mitigation for the two problems brought up in this thread. Neither for the problem with the high disk usage due to sector overwrites nor for the problem with partial writes. Do you see any mitigation possibilites here or what would/should be the way forward?