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

[Alex Kiernan]

On Tue, May 8, 2018 at 8:41 AM Lukasz Majewski <lukma at denx.de> wrote:

> On Tue, 08 May 2018 05:15:13 +0000
> Alex Kiernan <alex.kiernan at gmail.com> wrote:

> > On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski <lukma at denx.de> wrote:
> >
> > > Those two functions can be used to provide easy bootcount
> > > management.
> >
> > > Signed-off-by: Lukasz Majewski <lukma at denx.de>
> >
> > > Reviewed-by: Tom Rini <trini at konsulko.com>
> > > Reviewed-by: Stefan Roese <sr at 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?

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

-- 
Alex Kiernan