[U-Boot] [PATCH v5 0/7] Provide SPL support for bootcount (in the case of using falcon boot mode)
This patch series provides support for controlling bootcount limits in SPL. Moreover, the common code has been identified and reused in the common/autoboot.c file. It also enables this feature on display5 board to present usage patterns.
This patch has been applied on top of u-boot/master: SHA1 : v2018.05-rc3
Test HW: Beagle Bone Black (am335x) , Display5 (imx6q) Travis-Ci: https://travis-ci.org/lmajewski/u-boot-dfu/builds/373639971
Lukasz Majewski (7): bootcount: spl: Enable bootcount support in SPL bootcount: Add include guards into bootcount.h file bootcount: Add function wrappers to handle bootcount increment and error checking bootcount: Rewrite autoboot to use wrapper functions from bootcount.h bootcount: spl: Extend SPL to support bootcount incrementation bootcount: display5: spl: Extend DISPLAY5 board SPL to support bootcount checking bootcount: display5: config: Enable boot count feature in the display5 board
board/liebherr/display5/spl.c | 3 ++- common/autoboot.c | 23 +++++--------------- common/spl/Kconfig | 9 ++++++++ common/spl/spl.c | 3 +++ configs/display5_defconfig | 4 ++++ drivers/Makefile | 1 + include/bootcount.h | 50 +++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 74 insertions(+), 19 deletions(-)
New, SPL related config option - CONFIG_SPL_BOOTCOUNT_LIMIT has been added to allow drivers/bootcount code re-usage in SPL.
This code is necessary to use and setup bootcount in SPL in the case of falcon boot mode.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
---
Changes in v5: - None
Changes in v4: - None
Changes in v3: - None
Changes in v2: - None
common/spl/Kconfig | 9 +++++++++ drivers/Makefile | 1 + 2 files changed, 10 insertions(+)
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 259f96607e..431710a93b 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -54,6 +54,15 @@ config SPL_BOOTROM_SUPPORT BOOT_DEVICE_BOOTROM (or fall-through to the next boot device in the boot device list, if not implemented for a given board)
+config SPL_BOOTCOUNT_LIMIT + bool "Support bootcount in SPL" + depends on SPL_ENV_SUPPORT + help + On some boards, which use 'falcon' mode, it is necessary to check + and increment the number of boot attempts. Such boards do not + use proper U-Boot for normal boot flow and hence needs those + adjustments to be done in the SPL. + config SPL_RAW_IMAGE_SUPPORT bool "Support SPL loading and booting of RAW images" default n if (ARCH_MX6 && (SPL_MMC_SUPPORT || SPL_SATA_SUPPORT)) diff --git a/drivers/Makefile b/drivers/Makefile index 6846d181aa..061331eadd 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_$(SPL_TPL_)TIMER) += timer/ ifndef CONFIG_TPL_BUILD ifdef CONFIG_SPL_BUILD
+obj-$(CONFIG_SPL_BOOTCOUNT_LIMIT) += bootcount/ obj-$(CONFIG_SPL_CPU_SUPPORT) += cpu/ obj-$(CONFIG_SPL_CRYPTO_SUPPORT) += crypto/ obj-$(CONFIG_SPL_GPIO_SUPPORT) += gpio/
On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski lukma@denx.de wrote:
New, SPL related config option - CONFIG_SPL_BOOTCOUNT_LIMIT has been added to allow drivers/bootcount code re-usage in SPL.
This code is necessary to use and setup bootcount in SPL in the case of falcon boot mode.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
Reviewed-by: Alex Kiernan alex.kiernan@gmail.com
Changes in v5:
- None
Changes in v4:
- None
Changes in v3:
- None
Changes in v2:
- None
common/spl/Kconfig | 9 +++++++++ drivers/Makefile | 1 + 2 files changed, 10 insertions(+)
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 259f96607e..431710a93b 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -54,6 +54,15 @@ config SPL_BOOTROM_SUPPORT BOOT_DEVICE_BOOTROM (or fall-through to the next boot device in
the
boot device list, if not implemented for a given board)
+config SPL_BOOTCOUNT_LIMIT
bool "Support bootcount in SPL"depends on SPL_ENV_SUPPORThelpOn some boards, which use 'falcon' mode, it is necessary to
check
and increment the number of boot attempts. Such boards do notuse proper U-Boot for normal boot flow and hence needs thoseadjustments to be done in the SPL.- config SPL_RAW_IMAGE_SUPPORT bool "Support SPL loading and booting of RAW images" default n if (ARCH_MX6 && (SPL_MMC_SUPPORT || SPL_SATA_SUPPORT))
diff --git a/drivers/Makefile b/drivers/Makefile index 6846d181aa..061331eadd 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_$(SPL_TPL_)TIMER) += timer/ ifndef CONFIG_TPL_BUILD ifdef CONFIG_SPL_BUILD
+obj-$(CONFIG_SPL_BOOTCOUNT_LIMIT) += bootcount/ obj-$(CONFIG_SPL_CPU_SUPPORT) += cpu/ obj-$(CONFIG_SPL_CRYPTO_SUPPORT) += crypto/ obj-$(CONFIG_SPL_GPIO_SUPPORT) += gpio/ -- 2.11.0
On Wed, May 02, 2018 at 04:10:50PM +0200, Lukasz Majewski wrote:
New, SPL related config option - CONFIG_SPL_BOOTCOUNT_LIMIT has been added to allow drivers/bootcount code re-usage in SPL.
This code is necessary to use and setup bootcount in SPL in the case of falcon boot mode.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Alex Kiernan alex.kiernan@gmail.com
Applied to u-boot/master, thanks!
This patch adds missing include guards for bootcount.h file.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
---
Changes in v5: - None
Changes in v4: - None
Changes in v3: - None
Changes in v2: - New patch
include/bootcount.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/bootcount.h b/include/bootcount.h index 06fb4d3578..e3b3f7028e 100644 --- a/include/bootcount.h +++ b/include/bootcount.h @@ -4,6 +4,8 @@ * * SPDX-License-Identifier: GPL-2.0+ */ +#ifndef _BOOTCOUNT_H__ +#define _BOOTCOUNT_H__
#include <common.h> #include <asm/io.h> @@ -38,3 +40,4 @@ static inline u32 raw_bootcount_load(volatile u32 *addr) return in_be32(addr); } #endif +#endif /* _BOOTCOUNT_H__ */
On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski lukma@denx.de wrote:
This patch adds missing include guards for bootcount.h file.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
Reviewed-by: Alex Kiernan alex.kiernan@gmail.com
Changes in v5:
- None
Changes in v4:
- None
Changes in v3:
- None
Changes in v2:
- New patch
include/bootcount.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/bootcount.h b/include/bootcount.h index 06fb4d3578..e3b3f7028e 100644 --- a/include/bootcount.h +++ b/include/bootcount.h @@ -4,6 +4,8 @@
- SPDX-License-Identifier: GPL-2.0+
*/ +#ifndef _BOOTCOUNT_H__ +#define _BOOTCOUNT_H__
#include <common.h> #include <asm/io.h> @@ -38,3 +40,4 @@ static inline u32 raw_bootcount_load(volatile u32 *addr) return in_be32(addr); } #endif
+#endif /* _BOOTCOUNT_H__ */
2.11.0
On Wed, May 02, 2018 at 04:10:51PM +0200, Lukasz Majewski wrote:
This patch adds missing include guards for bootcount.h file.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Alex Kiernan alex.kiernan@gmail.com
Applied to u-boot/master, thanks!
Those two functions can be used to provide easy bootcount management.
Signed-off-by: Lukasz Majewski lukma@denx.de
Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Stefan Roese sr@denx.de ---
Changes in v5: - Provide parenthesis for #if defined(FOO) && ...
Changes in v4: - Remove enum bootcount_context and replace it with checking gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot proper, so can be used as indication if we are in u-boot or SPL). - Do not call bootcount_store() twice when it is not needed. - Call env_set_ulong("bootcount", bootcount); only in NON SPL context - Boards with TINY_PRINTF (in newest mainline) will build break as this function requires simple_itoa() from vsprintf.c (now not always build in SPL).
Changes in v3: - Unify those functions to also work with common/autoboot.c code - Add enum bootcount_context to distinguish between u-boot proper and SPL
Changes in v2: - None
include/bootcount.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/include/bootcount.h b/include/bootcount.h index e3b3f7028e..a886c98f44 100644 --- a/include/bootcount.h +++ b/include/bootcount.h @@ -11,6 +11,8 @@ #include <asm/io.h> #include <asm/byteorder.h>
+#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) || defined(CONFIG_BOOTCOUNT_LIMIT) + #if !defined(CONFIG_SYS_BOOTCOUNT_LE) && !defined(CONFIG_SYS_BOOTCOUNT_BE) # if __BYTE_ORDER == __LITTLE_ENDIAN # define CONFIG_SYS_BOOTCOUNT_LE @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32 *addr) return in_be32(addr); } #endif + +DECLARE_GLOBAL_DATA_PTR; +static inline int bootcount_error(void) +{ + unsigned long bootcount = bootcount_load(); + unsigned long bootlimit = env_get_ulong("bootlimit", 10, 0); + + if (bootlimit && bootcount > bootlimit) { + printf("Warning: Bootlimit (%lu) exceeded.", bootlimit); + if (!(gd->flags & GD_FLG_SPL_INIT)) + printf(" Using altbootcmd."); + printf("\n"); + + return 1; + } + + return 0; +} + +static inline void bootcount_inc(void) +{ + unsigned long bootcount = bootcount_load(); + + if (gd->flags & GD_FLG_SPL_INIT) { + bootcount_store(++bootcount); + return; + } + +#ifndef CONFIG_SPL_BUILD + /* Only increment bootcount when no bootcount support in SPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT + bootcount_store(++bootcount); +#endif + env_set_ulong("bootcount", bootcount); +#endif /* !CONFIG_SPL_BUILD */ +} + +#if defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_BOOTCOUNT_LIMIT) +void bootcount_store(ulong a) {}; +ulong bootcount_load(void) { return 0; } +#endif /* CONFIG_SPL_BUILD && !CONFIG_SPL_BOOTCOUNT_LIMIT */ +#else +static inline int bootcount_error(void) { return 0; } +static inline void bootcount_inc(void) {} +#endif /* CONFIG_SPL_BOOTCOUNT_LIMIT || CONFIG_BOOTCOUNT_LIMIT */ #endif /* _BOOTCOUNT_H__ */
On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski lukma@denx.de wrote:
Those two functions can be used to provide easy bootcount management.
Signed-off-by: Lukasz Majewski lukma@denx.de
Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Stefan Roese sr@denx.de
Changes in v5:
- Provide parenthesis for #if defined(FOO) && ...
Changes in v4:
- Remove enum bootcount_context and replace it with checking gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot proper, so can be used as indication if we are in u-boot or SPL).
- Do not call bootcount_store() twice when it is not needed.
- Call env_set_ulong("bootcount", bootcount); only in NON SPL context - Boards with TINY_PRINTF (in newest mainline) will build break as this
function
requires simple_itoa() from vsprintf.c (now not always build in SPL).
Changes in v3:
- Unify those functions to also work with common/autoboot.c code
- Add enum bootcount_context to distinguish between u-boot proper and SPL
Changes in v2:
- None
include/bootcount.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/include/bootcount.h b/include/bootcount.h index e3b3f7028e..a886c98f44 100644 --- a/include/bootcount.h +++ b/include/bootcount.h @@ -11,6 +11,8 @@ #include <asm/io.h> #include <asm/byteorder.h>
+#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
defined(CONFIG_BOOTCOUNT_LIMIT)
- #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
!defined(CONFIG_SYS_BOOTCOUNT_BE)
# if __BYTE_ORDER == __LITTLE_ENDIAN # define CONFIG_SYS_BOOTCOUNT_LE @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32
*addr)
return in_be32(addr);} #endif
+DECLARE_GLOBAL_DATA_PTR; +static inline int bootcount_error(void) +{
unsigned long bootcount = bootcount_load();unsigned long bootlimit = env_get_ulong("bootlimit", 10, 0);if (bootlimit && bootcount > bootlimit) {printf("Warning: Bootlimit (%lu) exceeded.", bootlimit);if (!(gd->flags & GD_FLG_SPL_INIT))printf(" Using altbootcmd.");printf("\n");return 1;}return 0;+}
+static inline void bootcount_inc(void) +{
unsigned long bootcount = bootcount_load();if (gd->flags & GD_FLG_SPL_INIT) {bootcount_store(++bootcount);return;}+#ifndef CONFIG_SPL_BUILD
/* Only increment bootcount when no bootcount support in SPL */+#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
bootcount_store(++bootcount);+#endif
env_set_ulong("bootcount", bootcount);+#endif /* !CONFIG_SPL_BUILD */ +}
I'm kinda confused by this code... isn't this equivalent.?
static inline void bootcount_inc(void) { unsigned long bootcount = bootcount_load();
bootcount_store(++bootcount); #ifndef CONFIG_SPL_BUILD env_set_ulong("bootcount", bootcount); #endif /* !CONFIG_SPL_BUILD */ }
Also I suspect bootcount_store() will fail at link time on boards where the bootcount is stored in ext4?
+#if defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_BOOTCOUNT_LIMIT) +void bootcount_store(ulong a) {}; +ulong bootcount_load(void) { return 0; } +#endif /* CONFIG_SPL_BUILD && !CONFIG_SPL_BOOTCOUNT_LIMIT */ +#else +static inline int bootcount_error(void) { return 0; } +static inline void bootcount_inc(void) {} +#endif /* CONFIG_SPL_BOOTCOUNT_LIMIT || CONFIG_BOOTCOUNT_LIMIT */
#endif /* _BOOTCOUNT_H__ */
2.11.0
On Tue, May 8, 2018 at 6:15 AM Alex Kiernan alex.kiernan@gmail.com wrote:
On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski lukma@denx.de wrote:
Those two functions can be used to provide easy bootcount management.
Signed-off-by: Lukasz Majewski lukma@denx.de
Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Stefan Roese sr@denx.de
Changes in v5:
- Provide parenthesis for #if defined(FOO) && ...
Changes in v4:
- Remove enum bootcount_context and replace it with checking gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot
proper,
so can be used as indication if we are in u-boot or SPL).
- Do not call bootcount_store() twice when it is not needed.
- Call env_set_ulong("bootcount", bootcount); only in NON SPL context - Boards with TINY_PRINTF (in newest mainline) will build break as this
function
requires simple_itoa() from vsprintf.c (now not always build in SPL).
Changes in v3:
- Unify those functions to also work with common/autoboot.c code
- Add enum bootcount_context to distinguish between u-boot proper and
SPL
Changes in v2:
- None
include/bootcount.h | 47
+++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/include/bootcount.h b/include/bootcount.h index e3b3f7028e..a886c98f44 100644 --- a/include/bootcount.h +++ b/include/bootcount.h @@ -11,6 +11,8 @@ #include <asm/io.h> #include <asm/byteorder.h>
+#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
defined(CONFIG_BOOTCOUNT_LIMIT)
- #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
!defined(CONFIG_SYS_BOOTCOUNT_BE)
# if __BYTE_ORDER == __LITTLE_ENDIAN # define CONFIG_SYS_BOOTCOUNT_LE @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32
*addr)
return in_be32(addr);} #endif
+DECLARE_GLOBAL_DATA_PTR; +static inline int bootcount_error(void) +{
unsigned long bootcount = bootcount_load();unsigned long bootlimit = env_get_ulong("bootlimit", 10, 0);if (bootlimit && bootcount > bootlimit) {printf("Warning: Bootlimit (%lu) exceeded.", bootlimit);if (!(gd->flags & GD_FLG_SPL_INIT))printf(" Using altbootcmd.");printf("\n");return 1;}return 0;+}
+static inline void bootcount_inc(void) +{
unsigned long bootcount = bootcount_load();if (gd->flags & GD_FLG_SPL_INIT) {bootcount_store(++bootcount);return;}+#ifndef CONFIG_SPL_BUILD
/* Only increment bootcount when no bootcount support in SPL */+#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
bootcount_store(++bootcount);+#endif
env_set_ulong("bootcount", bootcount);+#endif /* !CONFIG_SPL_BUILD */ +}
I'm kinda confused by this code... isn't this equivalent.?
static inline void bootcount_inc(void) { unsigned long bootcount = bootcount_load();
bootcount_store(++bootcount); #ifndef CONFIG_SPL_BUILD env_set_ulong("bootcount", bootcount); #endif /* !CONFIG_SPL_BUILD */ }
I should've included my reasoning as I've got to be missing something... if GD_FLG_SPL_INIT is always set when we get here in SPL, then it's equivalent to the compile time guard. Which I think says I don't understand the flow to how we get here, otherwise we wouldn't need the runtime guard.
+#if defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_BOOTCOUNT_LIMIT) +void bootcount_store(ulong a) {}; +ulong bootcount_load(void) { return 0; } +#endif /* CONFIG_SPL_BUILD && !CONFIG_SPL_BOOTCOUNT_LIMIT */ +#else +static inline int bootcount_error(void) { return 0; } +static inline void bootcount_inc(void) {} +#endif /* CONFIG_SPL_BOOTCOUNT_LIMIT || CONFIG_BOOTCOUNT_LIMIT */
#endif /* _BOOTCOUNT_H__ */
2.11.0
-- Alex Kiernan
-- Alex Kiernan
On 08.05.2018 08:58, Alex Kiernan wrote:
<snip>
+static inline void bootcount_inc(void) +{
unsigned long bootcount = bootcount_load();if (gd->flags & GD_FLG_SPL_INIT) {bootcount_store(++bootcount);return;}+#ifndef CONFIG_SPL_BUILD
/* Only increment bootcount when no bootcount support in SPL */+#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
bootcount_store(++bootcount);+#endif
env_set_ulong("bootcount", bootcount);+#endif /* !CONFIG_SPL_BUILD */ +}
I'm kinda confused by this code... isn't this equivalent.?
static inline void bootcount_inc(void) { unsigned long bootcount = bootcount_load();bootcount_store(++bootcount); #ifndef CONFIG_SPL_BUILD env_set_ulong("bootcount", bootcount); #endif /* !CONFIG_SPL_BUILD */ }I should've included my reasoning as I've got to be missing something... if GD_FLG_SPL_INIT is always set when we get here in SPL, then it's equivalent to the compile time guard. Which I think says I don't understand the flow to how we get here, otherwise we wouldn't need the runtime guard.
When using with SPL and bootcounter support, this code will get called twice, first from the SPL, where the counter will get incremented. And second from main U-Boot, where we need to make sure, that the counter does not get incremented again, if SPL has already done so.
With your patch version, the bootcounter would get incremented twice in this case.
Thanks, Stefan
Hi Alex, Stefan,
On 08.05.2018 08:58, Alex Kiernan wrote:
<snip>
+static inline void bootcount_inc(void) +{
unsigned long bootcount = bootcount_load();if (gd->flags & GD_FLG_SPL_INIT) {bootcount_store(++bootcount);return;}+#ifndef CONFIG_SPL_BUILD
/* Only increment bootcount when no bootcount support inSPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
bootcount_store(++bootcount);+#endif
env_set_ulong("bootcount", bootcount);+#endif /* !CONFIG_SPL_BUILD */ +}
I'm kinda confused by this code... isn't this equivalent.?
static inline void bootcount_inc(void) { unsigned long bootcount = bootcount_load();bootcount_store(++bootcount); #ifndef CONFIG_SPL_BUILD env_set_ulong("bootcount", bootcount); #endif /* !CONFIG_SPL_BUILD */ }I should've included my reasoning as I've got to be missing something... if GD_FLG_SPL_INIT is always set when we get here in SPL, then it's equivalent to the compile time guard. Which I think says I don't understand the flow to how we get here, otherwise we wouldn't need the runtime guard.
When using with SPL and bootcounter support, this code will get called twice, first from the SPL, where the counter will get incremented. And second from main U-Boot, where we need to make sure, that the counter does not get incremented again, if SPL has already done so.
+1
With your patch version, the bootcounter would get incremented twice in this case.
Thanks, Stefan
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On Tue, May 8, 2018 at 8:11 AM Stefan Roese sr@denx.de wrote:
On 08.05.2018 08:58, Alex Kiernan wrote:
<snip>
+static inline void bootcount_inc(void) +{
unsigned long bootcount = bootcount_load();if (gd->flags & GD_FLG_SPL_INIT) {bootcount_store(++bootcount);return;}+#ifndef CONFIG_SPL_BUILD
/* Only increment bootcount when no bootcount support in SPL
*/
+#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
bootcount_store(++bootcount);+#endif
env_set_ulong("bootcount", bootcount);+#endif /* !CONFIG_SPL_BUILD */ +}
I'm kinda confused by this code... isn't this equivalent.?
static inline void bootcount_inc(void) { unsigned long bootcount = bootcount_load();bootcount_store(++bootcount); #ifndef CONFIG_SPL_BUILD env_set_ulong("bootcount", bootcount); #endif /* !CONFIG_SPL_BUILD */ }I should've included my reasoning as I've got to be missing
something... if
GD_FLG_SPL_INIT is always set when we get here in SPL, then it's
equivalent
to the compile time guard. Which I think says I don't understand the
flow
to how we get here, otherwise we wouldn't need the runtime guard.
When using with SPL and bootcounter support, this code will get called twice, first from the SPL, where the counter will get incremented. And second from main U-Boot, where we need to make sure, that the counter does not get incremented again, if SPL has already done so.
With your patch version, the bootcounter would get incremented twice in this case.
Ahh... thank you. That was the important piece!
On Tue, 08 May 2018 05:15:13 +0000 Alex Kiernan alex.kiernan@gmail.com wrote:
On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski lukma@denx.de wrote:
Those two functions can be used to provide easy bootcount management.
Signed-off-by: Lukasz Majewski lukma@denx.de
Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Stefan Roese sr@denx.de
Changes in v5:
- Provide parenthesis for #if defined(FOO) && ...
Changes in v4:
- Remove enum bootcount_context and replace it with checking
gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot proper, so can be used as indication if we are in u-boot or SPL).
- Do not call bootcount_store() twice when it is not needed.
- Call env_set_ulong("bootcount", bootcount); only in NON SPL
context - Boards with TINY_PRINTF (in newest mainline) will build break as this
function
requires simple_itoa() from vsprintf.c (now not always build in SPL).
Changes in v3:
- Unify those functions to also work with common/autoboot.c code
- Add enum bootcount_context to distinguish between u-boot proper
and SPL
Changes in v2:
- None
include/bootcount.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/include/bootcount.h b/include/bootcount.h index e3b3f7028e..a886c98f44 100644 --- a/include/bootcount.h +++ b/include/bootcount.h @@ -11,6 +11,8 @@ #include <asm/io.h> #include <asm/byteorder.h>
+#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
defined(CONFIG_BOOTCOUNT_LIMIT)
- #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
!defined(CONFIG_SYS_BOOTCOUNT_BE)
# if __BYTE_ORDER == __LITTLE_ENDIAN # define CONFIG_SYS_BOOTCOUNT_LE @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32
*addr)
return in_be32(addr);} #endif
+DECLARE_GLOBAL_DATA_PTR; +static inline int bootcount_error(void) +{
unsigned long bootcount = bootcount_load();unsigned long bootlimit = env_get_ulong("bootlimit", 10, 0);if (bootlimit && bootcount > bootlimit) {printf("Warning: Bootlimit (%lu) exceeded.",bootlimit);
if (!(gd->flags & GD_FLG_SPL_INIT))printf(" Using altbootcmd.");printf("\n");return 1;}return 0;+}
+static inline void bootcount_inc(void) +{
unsigned long bootcount = bootcount_load();if (gd->flags & GD_FLG_SPL_INIT) {bootcount_store(++bootcount);return;}+#ifndef CONFIG_SPL_BUILD
/* Only increment bootcount when no bootcount support inSPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
bootcount_store(++bootcount);+#endif
env_set_ulong("bootcount", bootcount);+#endif /* !CONFIG_SPL_BUILD */ +}
I'm kinda confused by this code... isn't this equivalent.?
static inline void bootcount_inc(void) { unsigned long bootcount = bootcount_load();
bootcount_store(++bootcount);#ifndef CONFIG_SPL_BUILD env_set_ulong("bootcount", bootcount); #endif /* !CONFIG_SPL_BUILD */ }
Also I suspect bootcount_store() will fail at link time on boards where the bootcount is stored in ext4?
I've run this patch set several times with travis-CI. No errors were present (the travis-ci link is in the cover letter).
Maybe there are some out of tree boards, which use the bootcount in some "odd" way....
+#if defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_BOOTCOUNT_LIMIT) +void bootcount_store(ulong a) {}; +ulong bootcount_load(void) { return 0; } +#endif /* CONFIG_SPL_BUILD && !CONFIG_SPL_BOOTCOUNT_LIMIT */ +#else +static inline int bootcount_error(void) { return 0; } +static inline void bootcount_inc(void) {} +#endif /* CONFIG_SPL_BOOTCOUNT_LIMIT || CONFIG_BOOTCOUNT_LIMIT */
#endif /* _BOOTCOUNT_H__ */
2.11.0
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On Tue, May 8, 2018 at 8:41 AM Lukasz Majewski lukma@denx.de wrote:
On Tue, 08 May 2018 05:15:13 +0000 Alex Kiernan alex.kiernan@gmail.com wrote:
On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski lukma@denx.de wrote:
Those two functions can be used to provide easy bootcount management.
Signed-off-by: Lukasz Majewski lukma@denx.de
Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Stefan Roese sr@denx.de
Changes in v5:
- Provide parenthesis for #if defined(FOO) && ...
Changes in v4:
- Remove enum bootcount_context and replace it with checking
gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot proper, so can be used as indication if we are in u-boot or SPL).
- Do not call bootcount_store() twice when it is not needed.
- Call env_set_ulong("bootcount", bootcount); only in NON SPL
context - Boards with TINY_PRINTF (in newest mainline) will build break as this
function
requires simple_itoa() from vsprintf.c (now not always build in SPL).
Changes in v3:
- Unify those functions to also work with common/autoboot.c code
- Add enum bootcount_context to distinguish between u-boot proper
and SPL
Changes in v2:
- None
include/bootcount.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/include/bootcount.h b/include/bootcount.h index e3b3f7028e..a886c98f44 100644 --- a/include/bootcount.h +++ b/include/bootcount.h @@ -11,6 +11,8 @@ #include <asm/io.h> #include <asm/byteorder.h>
+#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
defined(CONFIG_BOOTCOUNT_LIMIT)
- #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
!defined(CONFIG_SYS_BOOTCOUNT_BE)
# if __BYTE_ORDER == __LITTLE_ENDIAN # define CONFIG_SYS_BOOTCOUNT_LE @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32
*addr)
return in_be32(addr);} #endif
+DECLARE_GLOBAL_DATA_PTR; +static inline int bootcount_error(void) +{
unsigned long bootcount = bootcount_load();unsigned long bootlimit = env_get_ulong("bootlimit", 10, 0);if (bootlimit && bootcount > bootlimit) {printf("Warning: Bootlimit (%lu) exceeded.",bootlimit);
if (!(gd->flags & GD_FLG_SPL_INIT))printf(" Using altbootcmd.");printf("\n");return 1;}return 0;+}
+static inline void bootcount_inc(void) +{
unsigned long bootcount = bootcount_load();if (gd->flags & GD_FLG_SPL_INIT) {bootcount_store(++bootcount);return;}+#ifndef CONFIG_SPL_BUILD
/* Only increment bootcount when no bootcount support inSPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
bootcount_store(++bootcount);+#endif
env_set_ulong("bootcount", bootcount);+#endif /* !CONFIG_SPL_BUILD */ +}
I'm kinda confused by this code... isn't this equivalent.?
static inline void bootcount_inc(void) { unsigned long bootcount = bootcount_load();
bootcount_store(++bootcount);#ifndef CONFIG_SPL_BUILD env_set_ulong("bootcount", bootcount); #endif /* !CONFIG_SPL_BUILD */ }
Also I suspect bootcount_store() will fail at link time on boards where the bootcount is stored in ext4?
I've run this patch set several times with travis-CI. No errors were present (the travis-ci link is in the cover letter).
Maybe there are some out of tree boards, which use the bootcount in some "odd" way....
I'm only really aware of the ext4 stuff from when I picked through it to consolidate the bootcount into Kconfig. I don't think there would be any Travis failures for this case as you need to have bootcount in SPL which you can't have before this series. If I try building a board like this (choose ext4 for the bootcount) and it fails to link:
drivers/built-in.o: In function `bootcount_store': u-boot/drivers/bootcount/bootcount_ext.c:18: undefined reference to `fs_set_blk_dev' u-boot/drivers/bootcount/bootcount_ext.c:29: undefined reference to `fs_write' drivers/built-in.o: In function `bootcount_load': u-boot/drivers/bootcount/bootcount_ext.c:41: undefined reference to `fs_set_blk_dev' u-boot/drivers/bootcount/bootcount_ext.c:47: undefined reference to `fs_read'
But having just played around with Kconfig a bit for this case, I'm not sure there's anything that's actually any better than a link time error; anything we did in Kconfig would just end up modelling out that link error.
Hi Alex,
On Tue, May 8, 2018 at 8:41 AM Lukasz Majewski lukma@denx.de wrote:
On Tue, 08 May 2018 05:15:13 +0000 Alex Kiernan alex.kiernan@gmail.com wrote:
On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski lukma@denx.de wrote:
Those two functions can be used to provide easy bootcount management.
Signed-off-by: Lukasz Majewski lukma@denx.de
Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Stefan Roese sr@denx.de
Changes in v5:
- Provide parenthesis for #if defined(FOO) && ...
Changes in v4:
- Remove enum bootcount_context and replace it with checking
gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot proper, so can be used as indication if we are in u-boot or SPL).
- Do not call bootcount_store() twice when it is not needed.
- Call env_set_ulong("bootcount", bootcount); only in NON SPL
context - Boards with TINY_PRINTF (in newest mainline) will build break as this
function
requires simple_itoa() from vsprintf.c (now not always build in SPL).
Changes in v3:
- Unify those functions to also work with common/autoboot.c code
- Add enum bootcount_context to distinguish between u-boot
proper and SPL
Changes in v2:
- None
include/bootcount.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/include/bootcount.h b/include/bootcount.h index e3b3f7028e..a886c98f44 100644 --- a/include/bootcount.h +++ b/include/bootcount.h @@ -11,6 +11,8 @@ #include <asm/io.h> #include <asm/byteorder.h>
+#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
defined(CONFIG_BOOTCOUNT_LIMIT)
- #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
!defined(CONFIG_SYS_BOOTCOUNT_BE)
# if __BYTE_ORDER == __LITTLE_ENDIAN # define CONFIG_SYS_BOOTCOUNT_LE @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32
*addr)
return in_be32(addr);} #endif
+DECLARE_GLOBAL_DATA_PTR; +static inline int bootcount_error(void) +{
unsigned long bootcount = bootcount_load();unsigned long bootlimit = env_get_ulong("bootlimit",10, 0); +
if (bootlimit && bootcount > bootlimit) {printf("Warning: Bootlimit (%lu) exceeded.",bootlimit);
if (!(gd->flags & GD_FLG_SPL_INIT))printf(" Using altbootcmd.");printf("\n");return 1;}return 0;+}
+static inline void bootcount_inc(void) +{
unsigned long bootcount = bootcount_load();if (gd->flags & GD_FLG_SPL_INIT) {bootcount_store(++bootcount);return;}+#ifndef CONFIG_SPL_BUILD
/* Only increment bootcount when no bootcount support inSPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
bootcount_store(++bootcount);+#endif
env_set_ulong("bootcount", bootcount);+#endif /* !CONFIG_SPL_BUILD */ +}
I'm kinda confused by this code... isn't this equivalent.?
static inline void bootcount_inc(void) { unsigned long bootcount = bootcount_load();
bootcount_store(++bootcount);#ifndef CONFIG_SPL_BUILD env_set_ulong("bootcount", bootcount); #endif /* !CONFIG_SPL_BUILD */ }
Also I suspect bootcount_store() will fail at link time on boards where the bootcount is stored in ext4?
I've run this patch set several times with travis-CI. No errors were present (the travis-ci link is in the cover letter).
Maybe there are some out of tree boards, which use the bootcount in some "odd" way....
I'm only really aware of the ext4 stuff from when I picked through it to consolidate the bootcount into Kconfig. I don't think there would be any Travis failures for this case as you need to have bootcount in SPL which you can't have before this series.
Could you be more specific here?
This series adds support for bootcount in SPL. The travis-CI tests which I've performed were correct for the all boards which use it:
https://travis-ci.org/lmajewski/u-boot-dfu/builds/373639971
The bootcount_inc() is added in generic SPL code - this seems to work on all boards - boards which don't define CONFIG_SPL_BOOTCOUNT will just return from it.
However, I don't know how the code will behave if one would like to add ext4 support to it (including ext4 support to SPL).
I suppose that those boards will not enable SPL BOOTCOUNT in SPL and use the code as it is now - just in u-boot.
This patch series was designed with one main use case in mind - the "falcon" boot of Linux from SPL with bootcount support.
If I try building a board like this (choose ext4 for the bootcount) and it fails to link:
drivers/built-in.o: In function `bootcount_store': u-boot/drivers/bootcount/bootcount_ext.c:18: undefined reference to `fs_set_blk_dev' u-boot/drivers/bootcount/bootcount_ext.c:29: undefined reference to `fs_write' drivers/built-in.o: In function `bootcount_load': u-boot/drivers/bootcount/bootcount_ext.c:41: undefined reference to `fs_set_blk_dev' u-boot/drivers/bootcount/bootcount_ext.c:47: undefined reference to `fs_read'
But having just played around with Kconfig a bit for this case, I'm not sure there's anything that's actually any better than a link time error; anything we did in Kconfig would just end up modelling out that link error.
Could you share which particular board this breaks? I've tested it on Beagle Bone Black (and display5).
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On Tue, May 8, 2018 at 10:21 AM Lukasz Majewski lukma@denx.de wrote:
Hi Alex,
On Tue, May 8, 2018 at 8:41 AM Lukasz Majewski lukma@denx.de wrote:
On Tue, 08 May 2018 05:15:13 +0000 Alex Kiernan alex.kiernan@gmail.com wrote:
On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski lukma@denx.de wrote:
Those two functions can be used to provide easy bootcount management.
Signed-off-by: Lukasz Majewski lukma@denx.de
Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Stefan Roese sr@denx.de
Changes in v5:
- Provide parenthesis for #if defined(FOO) && ...
Changes in v4:
- Remove enum bootcount_context and replace it with checking
gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot proper, so can be used as indication if we are in u-boot or SPL).
- Do not call bootcount_store() twice when it is not needed.
- Call env_set_ulong("bootcount", bootcount); only in NON SPL
context - Boards with TINY_PRINTF (in newest mainline) will build break as this
function
requires simple_itoa() from vsprintf.c (now not always build in SPL).
Changes in v3:
- Unify those functions to also work with common/autoboot.c code
- Add enum bootcount_context to distinguish between u-boot
proper and SPL
Changes in v2:
- None
include/bootcount.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/include/bootcount.h b/include/bootcount.h index e3b3f7028e..a886c98f44 100644 --- a/include/bootcount.h +++ b/include/bootcount.h @@ -11,6 +11,8 @@ #include <asm/io.h> #include <asm/byteorder.h>
+#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
defined(CONFIG_BOOTCOUNT_LIMIT)
- #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
!defined(CONFIG_SYS_BOOTCOUNT_BE)
# if __BYTE_ORDER == __LITTLE_ENDIAN # define CONFIG_SYS_BOOTCOUNT_LE @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32
*addr)
return in_be32(addr);} #endif
+DECLARE_GLOBAL_DATA_PTR; +static inline int bootcount_error(void) +{
unsigned long bootcount = bootcount_load();unsigned long bootlimit = env_get_ulong("bootlimit",10, 0); +
if (bootlimit && bootcount > bootlimit) {printf("Warning: Bootlimit (%lu) exceeded.",bootlimit);
if (!(gd->flags & GD_FLG_SPL_INIT))printf(" Using altbootcmd.");printf("\n");return 1;}return 0;+}
+static inline void bootcount_inc(void) +{
unsigned long bootcount = bootcount_load();if (gd->flags & GD_FLG_SPL_INIT) {bootcount_store(++bootcount);return;}+#ifndef CONFIG_SPL_BUILD
/* Only increment bootcount when no bootcount support inSPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
bootcount_store(++bootcount);+#endif
env_set_ulong("bootcount", bootcount);+#endif /* !CONFIG_SPL_BUILD */ +}
I'm kinda confused by this code... isn't this equivalent.?
static inline void bootcount_inc(void) { unsigned long bootcount = bootcount_load();
bootcount_store(++bootcount);#ifndef CONFIG_SPL_BUILD env_set_ulong("bootcount", bootcount); #endif /* !CONFIG_SPL_BUILD */ }
Also I suspect bootcount_store() will fail at link time on boards where the bootcount is stored in ext4?
I've run this patch set several times with travis-CI. No errors were present (the travis-ci link is in the cover letter).
Maybe there are some out of tree boards, which use the bootcount in some "odd" way....
I'm only really aware of the ext4 stuff from when I picked through it to consolidate the bootcount into Kconfig. I don't think there would be any Travis failures for this case as you need to have bootcount in SPL which you can't have before this series.
Could you be more specific here?
This series adds support for bootcount in SPL. The travis-CI tests which I've performed were correct for the all boards which use it:
The bootcount_inc() is added in generic SPL code - this seems to work on all boards - boards which don't define CONFIG_SPL_BOOTCOUNT will just return from it.
However, I don't know how the code will behave if one would like to add ext4 support to it (including ext4 support to SPL).
I suppose that those boards will not enable SPL BOOTCOUNT in SPL and use the code as it is now - just in u-boot.
This patch series was designed with one main use case in mind - the "falcon" boot of Linux from SPL with bootcount support.
If I try building a board like this (choose ext4 for the bootcount) and it fails to link:
drivers/built-in.o: In function `bootcount_store': u-boot/drivers/bootcount/bootcount_ext.c:18: undefined reference to `fs_set_blk_dev' u-boot/drivers/bootcount/bootcount_ext.c:29: undefined reference to `fs_write' drivers/built-in.o: In function `bootcount_load': u-boot/drivers/bootcount/bootcount_ext.c:41: undefined reference to `fs_set_blk_dev' u-boot/drivers/bootcount/bootcount_ext.c:47: undefined reference to `fs_read'
But having just played around with Kconfig a bit for this case, I'm not sure there's anything that's actually any better than a link time error; anything we did in Kconfig would just end up modelling out that link error.
Could you share which particular board this breaks? I've tested it on Beagle Bone Black (and display5).
There isn't one (at least not one I'm aware of), but this gives another impossible combination which you can select, of which we've lots already.
I should've played around some more before commenting as I think what you've got is everything that's needed.
-- Alex Kiernan
Hi Alex,
On Tue, May 8, 2018 at 10:21 AM Lukasz Majewski lukma@denx.de wrote:
Hi Alex,
On Tue, May 8, 2018 at 8:41 AM Lukasz Majewski lukma@denx.de wrote:
On Tue, 08 May 2018 05:15:13 +0000 Alex Kiernan alex.kiernan@gmail.com wrote:
On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski lukma@denx.de wrote:
Those two functions can be used to provide easy bootcount management.
Signed-off-by: Lukasz Majewski lukma@denx.de
Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Stefan Roese sr@denx.de
Changes in v5:
- Provide parenthesis for #if defined(FOO) && ...
Changes in v4:
- Remove enum bootcount_context and replace it with checking
gd_t->flags (The GD_FLG_SPL_INIT is only set in SPL, it is cleared in u-boot proper, so can be used as indication if we are in u-boot or SPL).
- Do not call bootcount_store() twice when it is not needed.
- Call env_set_ulong("bootcount", bootcount); only in NON
SPL context - Boards with TINY_PRINTF (in newest mainline) will build break as this
function
requires simple_itoa() from vsprintf.c (now not always build in SPL).
Changes in v3:
- Unify those functions to also work with common/autoboot.c
code
- Add enum bootcount_context to distinguish between u-boot
proper and SPL
Changes in v2:
- None
include/bootcount.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/include/bootcount.h b/include/bootcount.h index e3b3f7028e..a886c98f44 100644 --- a/include/bootcount.h +++ b/include/bootcount.h @@ -11,6 +11,8 @@ #include <asm/io.h> #include <asm/byteorder.h>
+#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
defined(CONFIG_BOOTCOUNT_LIMIT)
- #if !defined(CONFIG_SYS_BOOTCOUNT_LE) &&
!defined(CONFIG_SYS_BOOTCOUNT_BE)
# if __BYTE_ORDER == __LITTLE_ENDIAN # define CONFIG_SYS_BOOTCOUNT_LE @@ -40,4 +42,49 @@ static inline u32 raw_bootcount_load(volatile u32
*addr)
return in_be32(addr);} #endif
+DECLARE_GLOBAL_DATA_PTR; +static inline int bootcount_error(void) +{
unsigned long bootcount = bootcount_load();unsigned long bootlimit = env_get_ulong("bootlimit",10, 0); +
if (bootlimit && bootcount > bootlimit) {printf("Warning: Bootlimit (%lu) exceeded.",bootlimit);
if (!(gd->flags & GD_FLG_SPL_INIT))printf(" Using altbootcmd.");printf("\n");return 1;}return 0;+}
+static inline void bootcount_inc(void) +{
unsigned long bootcount = bootcount_load();if (gd->flags & GD_FLG_SPL_INIT) {bootcount_store(++bootcount);return;}+#ifndef CONFIG_SPL_BUILD
/* Only increment bootcount when no bootcountsupport in SPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
bootcount_store(++bootcount);+#endif
env_set_ulong("bootcount", bootcount);+#endif /* !CONFIG_SPL_BUILD */ +}
I'm kinda confused by this code... isn't this equivalent.?
static inline void bootcount_inc(void) { unsigned long bootcount = bootcount_load();
bootcount_store(++bootcount);#ifndef CONFIG_SPL_BUILD env_set_ulong("bootcount", bootcount); #endif /* !CONFIG_SPL_BUILD */ }
Also I suspect bootcount_store() will fail at link time on boards where the bootcount is stored in ext4?
I've run this patch set several times with travis-CI. No errors were present (the travis-ci link is in the cover letter).
Maybe there are some out of tree boards, which use the bootcount in some "odd" way....
I'm only really aware of the ext4 stuff from when I picked through it to consolidate the bootcount into Kconfig. I don't think there would be any Travis failures for this case as you need to have bootcount in SPL which you can't have before this series.
Could you be more specific here?
This series adds support for bootcount in SPL. The travis-CI tests which I've performed were correct for the all boards which use it:
The bootcount_inc() is added in generic SPL code - this seems to work on all boards - boards which don't define CONFIG_SPL_BOOTCOUNT will just return from it.
However, I don't know how the code will behave if one would like to add ext4 support to it (including ext4 support to SPL).
I suppose that those boards will not enable SPL BOOTCOUNT in SPL and use the code as it is now - just in u-boot.
This patch series was designed with one main use case in mind - the "falcon" boot of Linux from SPL with bootcount support.
If I try building a board like this (choose ext4 for the bootcount) and it fails to link:
drivers/built-in.o: In function `bootcount_store': u-boot/drivers/bootcount/bootcount_ext.c:18: undefined reference to `fs_set_blk_dev' u-boot/drivers/bootcount/bootcount_ext.c:29: undefined reference to `fs_write' drivers/built-in.o: In function `bootcount_load': u-boot/drivers/bootcount/bootcount_ext.c:41: undefined reference to `fs_set_blk_dev' u-boot/drivers/bootcount/bootcount_ext.c:47: undefined reference to `fs_read'
But having just played around with Kconfig a bit for this case, I'm not sure there's anything that's actually any better than a link time error; anything we did in Kconfig would just end up modelling out that link error.
Could you share which particular board this breaks? I've tested it on Beagle Bone Black (and display5).
There isn't one (at least not one I'm aware of), but this gives another impossible combination which you can select, of which we've lots already.
I should've played around some more before commenting as I think what you've got is everything that's needed.
It is all the matter of use-case.
For SPL I've assumed that we will use IRAM or some RTC special register (as it is with display5) to store the bootcount (all info - magic number with boot count in 32 bits).
This seems logical and optimal - since we "assume" that SPL shall be possible small.
To avoid coincidence one needs to define CONFIG_SPL_BOOTCOUNT. Without it we shall have the same behaviour as previously on all supported boards (in theory at least :-) ).
-- Alex Kiernan
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On Wed, May 02, 2018 at 04:10:52PM +0200, Lukasz Majewski wrote:
Those two functions can be used to provide easy bootcount management.
Signed-off-by: Lukasz Majewski lukma@denx.de
Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Stefan Roese sr@denx.de
Applied to u-boot/master, thanks!
The code has been refactored to use common wrappers from bootcount.h header.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
---
Changes in v5: - None
Changes in v4: - Use global data pointer (gd_t *) instead of bootcount specific enum
Changes in v3: - New patch
Changes in v2: None
common/autoboot.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/common/autoboot.c b/common/autoboot.c index 2eef7a04cc..a0f7822c9e 100644 --- a/common/autoboot.c +++ b/common/autoboot.c @@ -14,6 +14,7 @@ #include <menu.h> #include <post.h> #include <u-boot/sha256.h> +#include <bootcount.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -291,18 +292,8 @@ const char *bootdelay_process(void) { char *s; int bootdelay; -#ifdef CONFIG_BOOTCOUNT_LIMIT - unsigned long bootcount = 0; - unsigned long bootlimit = 0; -#endif /* CONFIG_BOOTCOUNT_LIMIT */ - -#ifdef CONFIG_BOOTCOUNT_LIMIT - bootcount = bootcount_load(); - bootcount++; - bootcount_store(bootcount); - env_set_ulong("bootcount", bootcount); - bootlimit = env_get_ulong("bootlimit", 10, 0); -#endif /* CONFIG_BOOTCOUNT_LIMIT */ + + bootcount_inc();
s = env_get("bootdelay"); bootdelay = s ? (int)simple_strtol(s, NULL, 10) : CONFIG_BOOTDELAY; @@ -324,13 +315,9 @@ const char *bootdelay_process(void) s = env_get("failbootcmd"); } else #endif /* CONFIG_POST */ -#ifdef CONFIG_BOOTCOUNT_LIMIT - if (bootlimit && (bootcount > bootlimit)) { - printf("Warning: Bootlimit (%u) exceeded. Using altbootcmd.\n", - (unsigned)bootlimit); + if (bootcount_error()) s = env_get("altbootcmd"); - } else -#endif /* CONFIG_BOOTCOUNT_LIMIT */ + else s = env_get("bootcmd");
process_fdt_options(gd->fdt_blob);
On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski lukma@denx.de wrote:
The code has been refactored to use common wrappers from bootcount.h header.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
Reviewed-by: Alex Kiernan alex.kiernan@gmail.com
Changes in v5:
- None
Changes in v4:
- Use global data pointer (gd_t *) instead of bootcount specific enum
Changes in v3:
- New patch
Changes in v2: None
common/autoboot.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/common/autoboot.c b/common/autoboot.c index 2eef7a04cc..a0f7822c9e 100644 --- a/common/autoboot.c +++ b/common/autoboot.c @@ -14,6 +14,7 @@ #include <menu.h> #include <post.h> #include <u-boot/sha256.h> +#include <bootcount.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -291,18 +292,8 @@ const char *bootdelay_process(void) { char *s; int bootdelay; -#ifdef CONFIG_BOOTCOUNT_LIMIT
unsigned long bootcount = 0;unsigned long bootlimit = 0;-#endif /* CONFIG_BOOTCOUNT_LIMIT */
-#ifdef CONFIG_BOOTCOUNT_LIMIT
bootcount = bootcount_load();bootcount++;bootcount_store(bootcount);env_set_ulong("bootcount", bootcount);bootlimit = env_get_ulong("bootlimit", 10, 0);-#endif /* CONFIG_BOOTCOUNT_LIMIT */
bootcount_inc();
s = env_get("bootdelay"); bootdelay = s ? (int)simple_strtol(s, NULL, 10) :
CONFIG_BOOTDELAY;
@@ -324,13 +315,9 @@ const char *bootdelay_process(void) s = env_get("failbootcmd"); } else #endif /* CONFIG_POST */ -#ifdef CONFIG_BOOTCOUNT_LIMIT
if (bootlimit && (bootcount > bootlimit)) {printf("Warning: Bootlimit (%u) exceeded. Using
altbootcmd.\n",
(unsigned)bootlimit);
if (bootcount_error()) s = env_get("altbootcmd");
} else-#endif /* CONFIG_BOOTCOUNT_LIMIT */
else s = env_get("bootcmd");
process_fdt_options(gd->fdt_blob);-- 2.11.0
On Wed, May 02, 2018 at 04:10:53PM +0200, Lukasz Majewski wrote:
The code has been refactored to use common wrappers from bootcount.h header.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Alex Kiernan alex.kiernan@gmail.com
Applied to u-boot/master, thanks!
This patch adds support for incrementation of the bootcount in SPL. Such feature is necessary when we do want to use this feature with 'falcon' boot mode (which loads OS directly in SPL).
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
---
Changes in v5: - None
Changes in v4: - Use gd_t global data pointer instead of bootcount specific enum
Changes in v3: - Remove not needed #ifdefs - Add enum bootcount_context parameter to bootcount_inc() function
Changes in v2: - New patch - as suggested by Stefan Roese - bootcount_inc() is called in common SPL code (./common/spl/spl.c), so other boards can also reuse it without modification
common/spl/spl.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/common/spl/spl.c b/common/spl/spl.c index 794dbd0312..6eb50f3534 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -20,6 +20,7 @@ #include <dm/root.h> #include <linux/compiler.h> #include <fdt_support.h> +#include <bootcount.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -417,6 +418,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2) spl_board_init(); #endif
+ bootcount_inc(); + memset(&spl_image, '\0', sizeof(spl_image)); #ifdef CONFIG_SYS_SPL_ARGS_ADDR spl_image.arg = (void *)CONFIG_SYS_SPL_ARGS_ADDR;
On Wed, May 02, 2018 at 04:10:54PM +0200, Lukasz Majewski wrote:
This patch adds support for incrementation of the bootcount in SPL. Such feature is necessary when we do want to use this feature with 'falcon' boot mode (which loads OS directly in SPL).
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
This patch is necessary for providing basic bootcount checking in the case of using "falcon" boot mode in that board.
It forces u-boot proper boot, when we exceed the number of errors.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de
---
Changes in v5: - None
Changes in v4: - Use global data pointer (gd) instead of bootcount specific enum (SPL)
Changes in v3: - The bootcount_error now accepts enum bootcount_error input parameter
Changes in v2: - Remove bootcount_init() from SPL specific board code
board/liebherr/display5/spl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/board/liebherr/display5/spl.c b/board/liebherr/display5/spl.c index 437963e225..7712e5bc3f 100644 --- a/board/liebherr/display5/spl.c +++ b/board/liebherr/display5/spl.c @@ -20,6 +20,7 @@ #include <environment.h> #include <fsl_esdhc.h> #include <netdev.h> +#include <bootcount.h> #include "common.h"
DECLARE_GLOBAL_DATA_PTR; @@ -214,7 +215,7 @@ void board_boot_order(u32 *spl_boot_list) env_load();
s = env_get("BOOT_FROM"); - if (s && strcmp(s, "ACTIVE") == 0) { + if (s && !bootcount_error() && strcmp(s, "ACTIVE") == 0) { spl_boot_list[0] = BOOT_DEVICE_MMC1; spl_boot_list[1] = spl_boot_device(); }
On Wed, May 02, 2018 at 04:10:55PM +0200, Lukasz Majewski wrote:
This patch is necessary for providing basic bootcount checking in the case of using "falcon" boot mode in that board.
It forces u-boot proper boot, when we exceed the number of errors.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de
Applied to u-boot/master, thanks!
The boot count is enabled in both SPL and proper u-boot.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de
---
Changes in v5: - None
Changes in v4: - None
Changes in v3: - None
Changes in v2: - None
configs/display5_defconfig | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/configs/display5_defconfig b/configs/display5_defconfig index e52f4e00af..db8212ca7c 100644 --- a/configs/display5_defconfig +++ b/configs/display5_defconfig @@ -16,6 +16,7 @@ CONFIG_SPL_LOAD_FIT=y CONFIG_OF_BOARD_SETUP=y CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=arch/arm/mach-imx/spl_sd.cfg,MX6Q" CONFIG_SUPPORT_RAW_INITRD=y +CONFIG_SPL_BOOTCOUNT_LIMIT=y # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set CONFIG_SPL_DMA_SUPPORT=y CONFIG_SPL_ENV_SUPPORT=y @@ -53,6 +54,9 @@ CONFIG_EFI_PARTITION=y CONFIG_OF_CONTROL=y CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_FSL_ESDHC=y +CONFIG_BOOTCOUNT_LIMIT=y +CONFIG_SYS_BOOTCOUNT_SINGLEWORD=y +CONFIG_SYS_BOOTCOUNT_ADDR=0x020CC068 CONFIG_SPI_FLASH=y CONFIG_SPI_FLASH_BAR=y CONFIG_SPI_FLASH_SPANSION=y
On Wed, May 02, 2018 at 04:10:56PM +0200, Lukasz Majewski wrote:
The boot count is enabled in both SPL and proper u-boot.
Signed-off-by: Lukasz Majewski lukma@denx.de Reviewed-by: Stefan Roese sr@denx.de
Applied to u-boot/master, thanks!
participants (4)
-
Alex Kiernan -
Lukasz Majewski -
Stefan Roese -
Tom Rini