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

[Lukasz Majewski <lukma@denx.de>]

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

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 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/20180508/23d18e25/attachment.sig>