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

[Lukasz Majewski]

Hi Stefan,

> 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,
> };

I will check if we do have such flags already defined. "gd" approach
looks better (more generic for sure) than the one from v3.

> 
> > +#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?

Good point - I will add it in v4.

Thanks for input.

> 
> 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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180430/b5de1d9d/attachment.sig>