Skip to content

fat_fs: Add reentrant zephyr support #51484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 13, 2023
Merged

fat_fs: Add reentrant zephyr support #51484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
760c391
modules: fs: Add reentrant zephyr support
Ox11 Dec 16, 2022
8f5a889
test: fs: fat_fs_api: Add tests for reentrant zephyr support
Ox11 Dec 16, 2022
d75949e
MAINTAINERS: Add ox11 to FatFs
Ox11 Feb 27, 2023
c789f7b
doc: release-notes: Update release notes 3.4
Ox11 Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions MAINTAINERS.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1371,6 +1371,16 @@ Filesystems:
labels:
- "area: File System"

"Filesystems: FatFs reentrant support":
status: maintained
maintainers:
- ox11
files:
- modules/fatfs/zfs_ffsystem.c
Copy link
Contributor

@de-nordic de-nordic Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List is missing test_fat_file_reentrant.c file.

Copy link
Contributor Author

@Ox11 Ox11 Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it.

- tests/subsys/fs/fat_fs_api/src/test_fat_file_reentrant.c
labels:
- "area: File System"
Comment on lines +1374 to +1382
Copy link
Member

@stephanosio stephanosio Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the documentation:

We should keep the granularity of code maintainership at a manageable level

It does not make sense to have "FatFs reentrant support" as a dedicated area -- this is too much granularity.

Please revert this change or define the maintainership as "Filesystems: Fatfs."

Copy link
Contributor Author

@Ox11 Ox11 Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a maintainer of the entire FatFS module.
My suggestion would be to either create a new module like this

"Filesystems: FatFs":
  status: maintained
  maintainers:
    - de-nordic
  collaborators:
    - ox11
  files:
    - modules/fatfs
    - tests/subsys/fs/common
    - tests/subsys/fs/fat_fs_api
    - tests/subsys/fs/fat_fs_dual_drive
  labels:
    - "area: File System"

or to add myself to the collaborators in the fs area

Filesystems:
  status: maintained
  maintainers:
    - de-nordic
  collaborators:
    - Laczen
    - nashif
    - ox11
  files:
    - include/zephyr/fs/
    - samples/subsys/fs/
    - subsys/fs/
    - tests/subsys/fs/
  labels:
    - "area: File System"

The first option should IMHO be created by @de-nordic as he would be mentioned as maintainer. Additionally, is there anyone else that should be mentioned as collaborator?


Formatted Output:
status: maintained
maintainers:
Expand Down
4 changes: 4 additions & 0 deletions doc/releases/release-notes-3.4.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,10 @@ Devicetree
Libraries / Subsystems
**********************

* File systems

* Added :kconfig:option:`CONFIG_FS_FATFS_REENTRANT` to enable the FAT FS reentrant option.

HALs
****

Expand Down
5 changes: 4 additions & 1 deletion modules/fatfs/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ if(CONFIG_FAT_FILESYSTEM_ELM)

zephyr_library_sources_ifdef(CONFIG_FS_FATFS_LFN
${ZEPHYR_FATFS_MODULE_DIR}/option/ffunicode.c
${ZEPHYR_FATFS_MODULE_DIR}/option/ffsystem.c
)

if(DEFINED CONFIG_FS_FATFS_LFN OR DEFINED CONFIG_FS_FATFS_REENTRANT)
zephyr_library_sources(zfs_ffsystem.c)
endif()

zephyr_library_link_libraries(ELMFAT)
target_link_libraries(ELMFAT INTERFACE zephyr_interface)
endif()
9 changes: 9 additions & 0 deletions modules/fatfs/zephyr_fatfs_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* Copyright (c) 2022 Nordic Semiconductor ASA
* Copyright (c) 2023 Husqvarna AB
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -62,6 +63,14 @@
#define FF_FS_EXFAT CONFIG_FS_FATFS_EXFAT
#endif /* defined(CONFIG_FS_FATFS_EXFAT) */

#if defined(CONFIG_FS_FATFS_REENTRANT)
#undef FF_FS_REENTRANT
#undef FF_FS_TIMEOUT
#include <zephyr/kernel.h>
#define FF_FS_REENTRANT CONFIG_FS_FATFS_REENTRANT
#define FF_FS_TIMEOUT K_FOREVER
#endif /* defined(CONFIG_FS_FATFS_REENTRANT) */

/*
* These options are override from default values, but have no Kconfig
* options.
Expand Down
69 changes: 69 additions & 0 deletions modules/fatfs/zfs_ffsystem.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* OS Dependent Functions for FatFs
*
* Copyright (c) 2023 Husqvarna AB
*
* SPDX-License-Identifier: Apache-2.0
*/
/* The file is based on template file by (C)ChaN, 2022, as
* available from FAT FS module source:
* https://github.com/zephyrproject-rtos/fatfs/blob/master/option/ffsystem.c
*/

#include <ff.h>

#if FF_USE_LFN == 3 /* Dynamic memory allocation */
/* Allocate a memory block */
void *ff_memalloc(UINT msize)
{
return k_malloc(msize);
}

/* Free a memory block */
void ff_memfree(void *mblock)
{
k_free(mblock);
}
#endif /* FF_USE_LFN == 3 */

#if FF_FS_REENTRANT /* Mutual exclusion */
/* Table of Zephyr mutex. One for each volume and an extra one for the ff system.
* See also the template file used as reference. Link is available in the header of this file.
*/
static struct k_mutex fs_reentrant_mutex[FF_VOLUMES + 1];

/* Create a Mutex
* Mutex ID vol: Volume mutex (0 to FF_VOLUMES - 1) or system mutex (FF_VOLUMES)
* Returns 1: Succeeded or 0: Could not create the mutex
*/
int ff_mutex_create(int vol)
{
return (int)(k_mutex_init(&fs_reentrant_mutex[vol]) == 0);
}

/* Delete a Mutex
* Mutex ID vol: Volume mutex (0 to FF_VOLUMES - 1) or system mutex (FF_VOLUMES)
*/
void ff_mutex_delete(int vol)
{
/* (nothing to do) */
(void)vol;
}

/* Request Grant to Access the Volume
* Mutex ID vol: Volume mutex (0 to FF_VOLUMES - 1) or system mutex (FF_VOLUMES)
* Returns 1: Succeeded or 0: Timeout
*/
int ff_mutex_take(int vol)
{
return (int)(k_mutex_lock(&fs_reentrant_mutex[vol], FF_FS_TIMEOUT) == 0);
}

/* Release Grant to Access the Volume
* Mutex ID vol: Volume mutex (0 to FF_VOLUMES - 1) or system mutex (FF_VOLUMES)
*/
void ff_mutex_give(int vol)
{
k_mutex_unlock(&fs_reentrant_mutex[vol]);
}
#endif /* FF_FS_REENTRANT */
9 changes: 9 additions & 0 deletions subsys/fs/Kconfig.fatfs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Copyright (c) 2016 Intel Corporation
# Copyright (c) 2020 Nordic Semiconductor ASA
# Copyright (c) 2023 Husqvarna AB
# SPDX-License-Identifier: Apache-2.0

config FAT_FILESYSTEM_ELM
Expand Down Expand Up @@ -220,6 +221,14 @@ config FS_FATFS_WINDOW_ALIGNMENT
that, in worst scenario, value provided here may cause FATFS
structure to have size of twice the value.

config FS_FATFS_REENTRANT
bool "FatFs reentrant"
depends on !FS_FATFS_LFN_MODE_BSS
help
Copy link
Contributor

@arbrauns arbrauns Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a depends on !FS_FATFS_LFN_MODE_BSS make sense here? Without, it's really easy to enable LFN in its default configuration, enable reentrancy, and not realize that it's actually still not thread safe. With the depends, there would at least be a warning because CONFIG_FS_FATFS_REENTRANT can't be selected.

Copy link
Contributor

@de-nordic de-nordic Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should probably add that.

Copy link
Contributor Author

@Ox11 Ox11 Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!
I tested it locally with depends on !FS_FATFS_LFN_MODE_BSS:
I added the CONFIG_FS_FATFS_LFN=y in the testcase.yaml for the reentrant test in the extra_configs, but the test did not fail until I set the mode manually with CONFIG_FS_FATFS_LFN_MODE_BSS=y. I also tested it by adding the depends on !FS_FATFS_REENTRANT to FS_FATFS_LFN_MODE_BSS and had the same behaviour.
The default mode is probably the most critical choise to warn about which still won't work.
Do you know more about this behaviour?
Edit: I did not check the final .config: It is set to Stack in default for the tests. I will push the changes.

Enable the FatFs re-entrancy (thread safe) option for file/directory
access for each volume. Will create a zephyr mutex object for each
FatFs volume and a FatFs system mutex.

endmenu

endif # FAT_FILESYSTEM_ELM
2 changes: 2 additions & 0 deletions tests/subsys/fs/fat_fs_api/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@ target_sources(app PRIVATE
target_sources_ifdef(CONFIG_FLASH app PRIVATE
../common/test_fs_mkfs.c
src/test_fat_mkfs.c)
target_sources_ifdef(CONFIG_FS_FATFS_REENTRANT app PRIVATE
src/test_fat_file_reentrant.c)
3 changes: 3 additions & 0 deletions tests/subsys/fs/fat_fs_api/src/common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
/*
* Copyright (c) 2016 Intel Corporation.
* Copyright (c) 2023 Husqvarna AB
*
* SPDX-License-Identifier: Apache-2.0
*/

#include "test_fat.h"

/* FatFs work area */
FATFS fat_fs;
struct fs_file_t filep;
const char test_str[] = "hello world!";

Expand Down
4 changes: 4 additions & 0 deletions tests/subsys/fs/fat_fs_api/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* Copyright (c) 2016 Intel Corporation.
* Copyright (c) 2023 Husqvarna AB
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -18,6 +19,9 @@ static void *fat_fs_basic_setup(void)
test_fat_fs();
test_fat_rename();
test_fs_open_flags();
#ifdef CONFIG_FS_FATFS_REENTRANT
test_fat_file_reentrant();
#endif /* CONFIG_FS_FATFS_REENTRANT */
test_fat_unmount();

return NULL;
Expand Down
9 changes: 8 additions & 1 deletion tests/subsys/fs/fat_fs_api/src/test_fat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
/*
* Copyright (c) 2016 Intel Corporation.
* Copyright (c) 2020 Nordic Semiconductor ASA
* Copyright (c) 2023 Husqvarna AB
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr/kernel.h>
#include <zephyr/ztest.h>
#include <zephyr/fs/fs.h>
#include <ff.h>

#ifdef CONFIG_DISK_DRIVER_RAM
#define DISK_NAME CONFIG_DISK_RAM_VOLUME_NAME
Expand All @@ -27,12 +29,14 @@
"/testlongfilenamethatsmuchlongerthan8.3chars.text"
#else
#define TEST_FILE FATFS_MNTP"/testfile.txt"
#endif /* IS_ENABLED(CONFIG_FS_FATFS_LFN) */
#endif /* CONFIG_FS_FATFS_LFN */
#define TEST_DIR FATFS_MNTP"/testdir"
#define TEST_DIR_FILE FATFS_MNTP"/testdir/testfile.txt"

extern struct fs_file_t filep;
extern const char test_str[];
/* FatFs work area */
extern FATFS fat_fs;

int check_file_dir_exists(const char *path);

Expand All @@ -42,3 +46,6 @@ void test_fat_file(void);
void test_fat_dir(void);
void test_fat_fs(void);
void test_fat_rename(void);
#ifdef CONFIG_FS_FATFS_REENTRANT
void test_fat_file_reentrant(void);
#endif /* CONFIG_FS_FATFS_REENTRANT */
Loading