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