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

[Vasileios Amoiridis <vassilisamir@gmail.com>]

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