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

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

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.

Cheers,
Quentin