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 <vassilisa...@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
