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

Alex Kiernan
2018-05-08T06:58:59Z

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