Re: [PATCH v2 1/1] drivers: bootcount: Add support for FAT filesystem

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Vasileios,

On 6/10/24 8:51 PM, Vasileios Amoiridis wrote:
 > Add support to save boot count variable in a file in a FAT filesystem.
 >
 > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
 > ---
 >   doc/README.bootcount                          | 12 ++---
 >   drivers/bootcount/Kconfig                     | 53 +++++++++++++------
 >   drivers/bootcount/Makefile                    |  2 +-
 >   .../{bootcount_ext.c => bootcount_fs.c}       | 12 ++---
 >   4 files changed, 50 insertions(+), 29 deletions(-)
 >   rename drivers/bootcount/{bootcount_ext.c => bootcount_fs.c} (81%)
 >
 > diff --git a/doc/README.bootcount b/doc/README.bootcount
 > index f6c5f82f..0f4ffb68 100644
 > --- a/doc/README.bootcount
 > +++ b/doc/README.bootcount
 > @@ -23,15 +23,15 @@ It is the responsibility of some application code 
(typically a Linux
 >   application) to reset the variable "bootcount" to 0 when the system 
booted
 >   successfully, thus allowing for more boot cycles.
 >
 > -CONFIG_BOOTCOUNT_EXT
 > +CONFIG_BOOTCOUNT_FS
 >   --------------------
 >
 > -This adds support for maintaining boot count in a file on an EXT 
filesystem.
 > -The file to use is defined by:
 > +This adds support for maintaining boot count in a file on a filesystem.
 > +Supported filesystems are FAT and EXT. The file to use is defined by:
 >
 > -CONFIG_SYS_BOOTCOUNT_EXT_INTERFACE
 > -CONFIG_SYS_BOOTCOUNT_EXT_DEVPART
 > -CONFIG_SYS_BOOTCOUNT_EXT_NAME
 > +CONFIG_SYS_BOOTCOUNT_FS_INTERFACE
 > +CONFIG_SYS_BOOTCOUNT_FS_DEVPART
 > +CONFIG_SYS_BOOTCOUNT_FS_NAME
 >
 >   The format of the file is:
 >
 > diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
 > index 3c56253b..d3679eb5 100644
 > --- a/drivers/bootcount/Kconfig
 > +++ b/drivers/bootcount/Kconfig
 > @@ -25,10 +25,9 @@ config BOOTCOUNT_GENERIC
 >   	    Set to the address where the bootcount and bootcount magic
 >   	    will be stored.
 >
 > -config BOOTCOUNT_EXT
 > -	bool "Boot counter on EXT filesystem"
 > -	depends on FS_EXT4
 > -	select EXT4_WRITE
 > +config BOOTCOUNT_FS
 > +	bool "Boot counter on a filesystem"
 > +	depends on FS_EXT4 || FS_FAT
Do we really need this 'depends on' here? Especially if we have a choice 
below...

 >   	help
 >   	  Add support for maintaining boot count in a file on an EXT
The help text is still mentioning EXT here.

I would recommend removing it, or listing the supported filesystems at 
the moment. While I assume you tested with FAT, I assume that with 
FS_ANY, any FS should be supported?

 >   	  filesystem.
 > @@ -184,26 +183,48 @@ config SYS_BOOTCOUNT_SINGLEWORD
 >   	  This option enables packing boot count magic value and boot count
 >   	  into single word (32 bits).
 >
 > -config SYS_BOOTCOUNT_EXT_INTERFACE
 > -	string "Interface on which to find boot counter EXT filesystem"
 > +if BOOTCOUNT_FS
 > +choice
 > +	prompt "Filesystem type"
 > +	default BOOTCOUNT_EXT
 > +
 > +config BOOTCOUNT_EXT
 > +	bool "Boot counter on EXT filesystem"
 > +	depends on FS_EXT4
 > +	select EXT4_WRITE
 > +	help
 > +	  Add support for maintaing boot counter in a file on EXT filesystem"
 > +
 > +config BOOTCOUNT_FAT
 > +	bool "Boot counter on FAT filesystem"
 > +	depends on FS_FAT
 > +	select FAT_WRITE
 > +	help
 > +	  Add support for maintaing boot counter in a file on FAT filesystem"
 > +
 > +endchoice
 > +endif
 > +
Since we now support FS_ANY, do we really need this choice at all?

Alternatively, should it **really** be a choice and not just a bunch of 
configs that depends on BOOTCOUNT_FS + whatever's needed to write on 
that FS instead? I think we could have both BOOTCOUNT_EXT and 
BOOTCOUNT_FAT set without issue?

Cheers,
Quentin