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

[Vasileios Amoiridis <vassilisamir@gmail.com>]

On Tue, Jun 11, 2024 at 11:33:12AM +0200, Quentin Schulz wrote:
> 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...
> 
Well, probably this is redundant indeed.

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

Ahh, I missed that.

> 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?
> 

Well, I tested it with both FAT and EXT4 and it works. AFAIU, due to the
implementation of the filesystem handling code in U-Boot, if the fs supports
a write a function, then it should work. But I cannot test for other
filesystems apart from FAT and EXT4 so I think it's better to limit the
option to these two.

> >   	  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

Well, I think I kind of get the point but I am still a bit confused.
Do you mean that basically the configuration should be done the other way
around? Instead of choosing BOOTCOUNT_FS and then specifically to choose
EXT or FAT, to choose one of EXT/FAT and then to select BOOTCOUNT_FS?
If yes, what is the advantage of this approach?

Cheers,
Vasilis