[U-Boot] [PATCH v3 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

[Stefan Roese]

Hi Lukasz,

sorry, I still have some comments to (hopefully) make this
implementation a bit more "elegant". Please see below.

On 29.04.2018 15:36, Lukasz Majewski wrote:
> Those two functions can be used to provide easy bootcount management.
> 
> Signed-off-by: Lukasz Majewski <lukma at denx.de>
> 
> ---
> 
> 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 | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 50 insertions(+)
> 
> diff --git a/include/bootcount.h b/include/bootcount.h
> index e3b3f7028e..16fc657b2a 100644
> --- a/include/bootcount.h
> +++ b/include/bootcount.h
> @@ -11,6 +11,13 @@
>   #include <asm/io.h>
>   #include <asm/byteorder.h>
>   
> +enum bootcount_context {
> +	SPL = 1,
> +	UBOOT,
> +};

Perhaps better some bootcount specific values / enums, like:

enum bootcount_context {
	BOOTCOUNT_STATE_SPL = 1,
	BOOTCOUNT_STATE_UBOOT,
};

Or even more generic, as this "boot-state" does not have to be bootcount
specific:

enum u_boot_context {
	U_BOOT_STATE_SPL = 1,
	U_BOOT_STATE_U_BOOT,
};

Or do we already have something like this, perhaps in "gd", where
its marked, in which state we are currently running? If not, it
could be added there and could be used from the bootcounter code
and other interfaces / drivers as well. The parts pre-relocation and
post-relocation fall also into this area (for the pre- / post-reloc
we definitely have a variable / flag in gd somewhere). So perhaps:

enum u_boot_context {
	U_BOOT_STATE_SPL = 1,
	U_BOOT_STATE_U_BOOT_PRE_RELOC,
	U_BOOT_STATE_U_BOOT_POST_RELOC,
};

> +#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 +47,47 @@ static inline u32 raw_bootcount_load(volatile u32 *addr)
>   	return in_be32(addr);
>   }
>   #endif
> +
> +static inline int bootcount_error(enum bootcount_context bc)
> +{
> +	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 (bc == UBOOT)
> +			printf(" Using altbootcmd.");
> +		printf("\n");
> +
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline void bootcount_inc(enum bootcount_context bc)
> +{
> +	unsigned long bootcount = bootcount_load();
> +
> +	if (bc == SPL) {
> +		bootcount_store(++bootcount);
> +		return;
> +	}
> +
> +	/* Only increment bootcount when no bootcount support in SPL */
> +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> +	bootcount++;
> +#endif
> +	bootcount_store(bootcount);
> +	env_set_ulong("bootcount", bootcount);
> +}

Why not use the same logic / code as above (the store does not need
to happen twice):

	/* Only increment bootcount when no bootcount support in SPL */
#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
	bootcount_store(++bootcount);
#endif

	env_set_ulong("bootcount", bootcount);

?

What do you think?

Thanks,
Stefan