On Tue, Jun 11, 2024 at 06:29:34PM +0200, Quentin Schulz wrote:
> Hi Vasileios,
>
> On 6/11/24 6:20 PM, Vasileios Amoiridis wrote:
> > On Tue, Jun 11, 2024 at 10:06:59AM -0600, Tom Rini wrote:
> > > On Tue, Jun 11, 2024 at 05:41:19PM +0200, Quentin Schulz wrote:
> > > > Hi Vasileios,
> > > >
> > > > On 6/11/24 5:27 PM, Vasileios Amoiridis wrote:
> > > > > 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.
> > > > >
> > > >
> > > > I guess we can let people figure things out themselves and add new
> > > > options
> > > > for when they have tested them, no strong opinion here.
> > > >
> > > > > > > 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"
> > > > > > > +
> > > >
> > > > Seems like I missed a typo here as well:
> > > >
> > > > s/maintaing/maintaining/ ? At least that's what we have in
> > > > doc/README.bootcount
> > > >
> > > > > > > +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?
> > > > >
> > > >
> > > > I'm suggesting:
> > > >
> > > > """
> > > > config BOOTCOUNT_FS
> > > > bool "Boot counter on a filesystem"
> > > > help
> > > >
> > > > config BOOTCOUNT_EXT
> > > > bool "Boot counter on EXT filesystem"
> > > > default y
> > > > depends on BOOTCOUNT_FS
> > > > 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 BOOTCOUNT_FS
> > > > depends on FS_FAT
> > > > select FAT_WRITE
> > > > help
> > > > Add support for maintaing boot counter in a file on FAT
> > > > filesystem"
> > > > """
> > > >
> > > > This way we can have defconfigs where BOOTCOUNT_FAT and BOOTCOUNT_EXT
> > > > are
> > > > both selected, the user would then be free to decide if the same
> > > > partition
> > > > on two different devices but for the same purpose can be either
> > > > ext2/3/4 or
> > > > FAT, without recompiling U-Boot just for that.
> > > >
> > > > However, it would now be possible to have BOOTCOUNT_FS=y but neither
> > > > BOOTCOUNT_EXT nor BOOTCOUNT_FAT set to y (e.g. if FS_EXT4 or FS_FAT
> > > > isn't
> > > > defined).
> > > >
> > > > Finally, the other option was just to NOT have BOOTCOUNT_FAT or
> > > > BOOTCOUNT_EXT and let people select FS_EXT4/FS_FAT and
> > > > EXT4_WRITE/FAT_WRITE
> > > > themselves since the BOOTCOUNT_FAT/EXT aren't actually used in C code.
> > > > This
> > > > is less user-friendly though.
> > >
> > > I was thinking that with FS_ANY, we don't need a symbol per filesystem
> > > type but can just handle it in the help text with something like "Please
> > > ensure that you have enabled write support for the filesystem that you
> > > will be used by the partition that you configure this feature for."
> > >
> >
> > Hi Tom,
> >
> > I see that both you and Quentin are leaning towards this solution which is
> > totally fine by me. As I said before, the user already needs to specify the
> > device interface and the partition to be used so they can also be
> > responsible to enable the appropriate filesystem write functionality. Indeed
> > if more filesystems are added, it would be quite ugly to have multiple
> > symbols just to choose a config.
> >
>
> I wouldn't be surprised if some time in the future we make the device
> interface and partition configurable through a U-Boot environment.
>
> Also, there's no need for U-Boot to know the fs to write to since it can be
> auto-detected, so adding an artificial hurdle isn't necessarily the best
> design choice.
>
True.
> Basically, I can have two products based on the same HW design. I have one
> with an EXT4 partition, and another for FAT partition, they both are on the
> same storage medium (one on one product, the other on the other product) in
> the same partition number. If we had an exclusive choice, then we would have
> to have two different U-Boot for essentially the same HW just based on the
> software design decisions. We cannot always avoid this, but I think we can
> here, so I think this would be pretty nice to have :)
>
Well, that's a good example, so totally understandable, thanks!!!
> > Also, if the user forgets to enable the appropriate *_WRITE config, it will
> > trigger an error message in U-Boot that this operation is not supported
> > which will be pretty obvious what needs to be done.
> >
>
> OK, that's good news. I hope the message is explicit enough that users know
> what to do with the info. It's better than crashing U-Boot, which sometimes
> happen when in SPL or U-Boot proper pre-reloc for mysterious reasons and
> make things very frustrating to debug :)
>
> > So I can go with a v3, following your proposals.
> >
>
> Seems to me like this may be the way to go yes.
>
> Cheers,
> Quentin
Cheers,
Vasilis
