drivers: disk: add loopback disk driver #69895

arbrauns · 2024-03-07T07:56:54Z

This driver creates a disk_access that transparently reads from and writes to a file in a file system.

danieldegrasse

This is an interesting feature- thanks for the PR! I feel like this might make more sense as a subsystem extension to the filesystem API though, rather than a disk driver. @de-nordic, any thoughts here?

de-nordic · 2024-04-03T16:25:47Z

This is an interesting feature- thanks for the PR! I feel like this might make more sense as a subsystem extension to the filesystem API though, rather than a disk driver. @de-nordic, any thoughts here?

Hm. The example provided here, with FATFS, actually uses the Disk Driver as backend to storage access. So in this case it makes sense to provide the driver as disk access driver.
I do not find it useful too much, as it would probably will look like FAT FS->disk access to file ->FAT FS->disk access-> MMC device (or flash api->device).
I guess that there is probably usecase where user uploads disk images to FAT FS via FTP and then mounts them.
This does not seem to break anything and may be valuable to some users.
Worth noting that this may take a toll on device life time if such image is R/W mounted from internal flash with devices that have > 512B page sizes.

I think the feature is OK, and in the right place.

danieldegrasse · 2024-04-05T14:44:23Z

This does not seem to break anything and may be valuable to some users.
Worth noting that this may take a toll on device life time if such image is R/W mounted from internal flash with devices that have > 512B page sizes.

I think the feature is OK, and in the right place

This is fair- in many ways the disk drivers are different, since they all sit under the disk subsystem. That is a good point about internal flash devices. However, I think anyone using a flash device to emulate a disk already should be aware this will cause issues regarding wear leveling and such, since filesystems like FatFS aren't explicitly designed for that.

drivers/disk/Kconfig.loopback

drivers/disk/loopback_disk.c

danieldegrasse · 2024-04-05T15:02:41Z

include/zephyr/drivers/loopback_disk.h

+ *
+ * @param ctx Preallocated context structure
+ * @param file_path Path to backing file
+ * @param disk_access_name Name of the created disk access (for disk_access_*() functions)


For the file_path and disk_access_name parameters, we should either:

document the requirement that these parameters should not be stack allocated (as they will be assigned to the ctx structure)

make the file_path and disk_access_name fields in ctx be char [] values, and use strncpy to copy the file_path and disk_access_name parameters provided by the user into those fields

As is stands right now, I worry a user might write code like the following:

struct loopback_disk_access global_ctx; const char *name = "disk_name"; void my_init_func() { const char *file_path = "my_path"; loopback_disk_access_register(&global_ctx, file_path, name); } void write_func() { .... /* Path temporary variable will have been deallocated here .. */ disk_access_read(name, data_buf, start_sector, num_sector); }

That would still be fine, since file_path is a string literal, but I get your point. I went with option 1, since I'm not a big fan of statically-sized string buffers.

I've also added an unregister function that will end the lifetime of these parameters.

That would still be fine, since file_path is a string literal, but I get your point. I went with option 1, since I'm not a big fan of statically-sized string buffers.

This is true- perhaps not the best example. Glad it highlighted the point though

I think that stack allocated things are ok as long as it is mentioned that they have to be valid through entire runtime.
This already happens when people create file in main of their code and pass it along (or use global to pass it).
And if you allow path to be passed, probably somebody would like to do the

snprintf(path, "/my_loopback/disks/%d.img", i);

at some point.

I think that stack allocated things are ok as long as it is mentioned that they have to be valid through entire runtime.

I believe that's what the doc comment says, yes.

de-nordic · 2024-04-12T16:16:43Z

tests/drivers/disk/disk_access/src/main.c

+#ifdef CONFIG_DISK_DRIVER_LOOPBACK
+#define DISK_NAME "loopback0"
+#else
+#define DISK_NAME DISK_NAME_PHYS
+#endif


Why not just add this to the above set of #if #else.
Does the FAT FS mounting work? The Fat FS has very limited number of mount points it can work with and these mountpoints are used also as disk names.

zephyr/modules/fatfs/zephyr_fatfs_config.h

Lines 84 to 95 in 51d80a9

/* Zephyr uses FF_VOLUME_STRS */

#undef FF_STR_VOLUME_ID

#define FF_STR_VOLUME_ID 1

/* By default FF_STR_VOLUME_ID in ffconf.h is 0, which means that

* FF_VOLUME_STRS is not used. Zephyr uses FF_VOLUME_STRS, which

* by default holds 8 possible strings representing mount points,

* and FF_VOLUMES needs to reflect that, which means that dolt

* value of 1 is overridden here with 8.

*/

#undef FF_VOLUMES

#define FF_VOLUMES 8

https://github.com/zephyrproject-rtos/fatfs/blob/427159bf95ea49b7680facffaa29ad506b42709b/include/ffconf.h#L174C9-L174C23

Why not just add this to the above set of #if #else.

What exactly do you mean? In the loopback case, I need both DISK_NAME (for the actual disk access tests) and DISK_NAME_PHYS (for the setup of the loopback disk access).

Does the FAT FS mounting work?

The tests pass, so I presume it does? I believe it's using the RAM disk.

Ah right. The FAT is not mounted on the loopback drive, the fat is the source of loopback image only

de-nordic · 2024-04-12T16:25:59Z

tests/drivers/disk/disk_access/testcase.yaml

+  drivers.disk.loopback:
+    extra_configs:
+      - CONFIG_DISK_DRIVER_LOOPBACK=y
+      - CONFIG_FILE_SYSTEM=y
+      - CONFIG_FILE_SYSTEM_MKFS=y
+      - CONFIG_FAT_FILESYSTEM_ELM=y
+    platform_allow: qemu_x86_64


Would it be possible to rather do native_sim target? (to be honest qemu is not my favourite debugging target...)

I simply copied the existing test cases, should I change those too?

should I change those too?

I am asking about code in this PR not change in entire testcase.yaml; there is already native_sim target above your addition.

Indeed there is now, there wasn't before I rebased. native_sim however does not contain the ramdisk that qemu_x86_64 does; I have copy-pasted the ramdisk declaration from the qemu overlay, hope that's how it's supposed to be.

This driver creates a disk_access that transparently reads from and writes to a file in a file system. Signed-off-by: Armin Brauns <armin.brauns@embedded-solutions.at>

arbrauns · 2024-04-15T08:05:25Z

Rebased to fix conflicts.

This sets up a backing device, creates a FAT file system, then opens a file as a loopback device and runs the disk_access tests on that. Signed-off-by: Armin Brauns <armin.brauns@embedded-solutions.at>

danieldegrasse · 2024-04-15T15:17:29Z

tests/drivers/disk/disk_access/src/main.c

@@ -46,6 +58,43 @@ static uint32_t disk_sector_size;
 /* + 4 to make sure the second buffer is dword-aligned for NVME */
 static uint8_t scratch_buf[2][SECTOR_COUNT4 * SECTOR_SIZE + 4];

+#ifdef CONFIG_DISK_DRIVER_LOOPBACK
+#define BACKING_PATH "/"DISK_NAME_PHYS":"


You could probably create this name from the DISK_NAME definition above with preprocessor concatenation, right? Might be worth looking at to avoid maintaining this name in two locations

I don't understand, isn't that exactly what is happening? DISK_NAME_PHYS is a preprocessor macro.

That is- sorry I missed this, was expecting to see # for concatenation, but this should work fine as is

de-nordic

Looks OK.

zephyrbot added the area: Disk Access label Mar 7, 2024

zephyrbot requested review from danieldegrasse, decsny and jfischer-no March 7, 2024 07:57

zephyrbot assigned danieldegrasse Mar 7, 2024

arbrauns force-pushed the loopback-disk-access branch from 4e08052 to 91be850 Compare March 7, 2024 08:04

danieldegrasse reviewed Mar 20, 2024

View reviewed changes

danieldegrasse requested a review from de-nordic March 20, 2024 20:44

danieldegrasse reviewed Apr 5, 2024

View reviewed changes

arbrauns force-pushed the loopback-disk-access branch from 91be850 to 051ae9d Compare April 8, 2024 08:49

de-nordic reviewed Apr 12, 2024

View reviewed changes

drivers: disk: add loopback disk driver

14e993c

This driver creates a disk_access that transparently reads from and writes to a file in a file system. Signed-off-by: Armin Brauns <armin.brauns@embedded-solutions.at>

arbrauns force-pushed the loopback-disk-access branch from 051ae9d to 64580ea Compare April 15, 2024 08:04

tests: add test for loopback disk access

3dbeba6

This sets up a backing device, creates a FAT file system, then opens a file as a loopback device and runs the disk_access tests on that. Signed-off-by: Armin Brauns <armin.brauns@embedded-solutions.at>

arbrauns force-pushed the loopback-disk-access branch from 64580ea to 3dbeba6 Compare April 15, 2024 12:06

danieldegrasse reviewed Apr 15, 2024

View reviewed changes

danieldegrasse approved these changes Apr 15, 2024

View reviewed changes

de-nordic approved these changes Apr 15, 2024

View reviewed changes

carlescufi merged commit 2c8ea07 into zephyrproject-rtos:main Apr 15, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: disk: add loopback disk driver #69895

drivers: disk: add loopback disk driver #69895

arbrauns commented Mar 7, 2024

danieldegrasse left a comment

de-nordic commented Apr 3, 2024

danieldegrasse commented Apr 5, 2024

danieldegrasse Apr 5, 2024

arbrauns Apr 8, 2024

danieldegrasse Apr 8, 2024

de-nordic Apr 12, 2024

arbrauns Apr 15, 2024

de-nordic Apr 12, 2024

arbrauns Apr 15, 2024

de-nordic Apr 15, 2024

de-nordic Apr 12, 2024

arbrauns Apr 15, 2024

de-nordic Apr 15, 2024

arbrauns Apr 15, 2024

arbrauns commented Apr 15, 2024

danieldegrasse Apr 15, 2024

arbrauns Apr 15, 2024

danieldegrasse Apr 15, 2024

de-nordic left a comment

	/* Zephyr uses FF_VOLUME_STRS */
	#undef FF_STR_VOLUME_ID
	#define FF_STR_VOLUME_ID 1

	/* By default FF_STR_VOLUME_ID in ffconf.h is 0, which means that
	* FF_VOLUME_STRS is not used. Zephyr uses FF_VOLUME_STRS, which
	* by default holds 8 possible strings representing mount points,
	* and FF_VOLUMES needs to reflect that, which means that dolt
	* value of 1 is overridden here with 8.
	*/
	#undef FF_VOLUMES
	#define FF_VOLUMES 8

drivers: disk: add loopback disk driver #69895

drivers: disk: add loopback disk driver #69895

Conversation

arbrauns commented Mar 7, 2024

danieldegrasse left a comment

Choose a reason for hiding this comment

de-nordic commented Apr 3, 2024

danieldegrasse commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbrauns commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-nordic left a comment

Choose a reason for hiding this comment