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 <[email protected]>
---
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
