From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alex Kiernan <alex.kiernan@gmail.com>
Date: Tue, 08 May 2018 06:58:59 +0000
Subject: [U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to
 handle bootcount increment and error checking
In-Reply-To: <CAO5Uq5THxsosOtFA3c8bSoCu+ScYAK6Ut+P_Zj24_F+_ir6GCA@mail.gmail.com>
References: <20180502141056.23937-1-lukma@denx.de>
 <20180502141056.23937-4-lukma@denx.de>
 <CAO5Uq5THxsosOtFA3c8bSoCu+ScYAK6Ut+P_Zj24_F+_ir6GCA@mail.gmail.com>
Message-ID: <CAO5Uq5T88Ow6H6Vm-BFWU_-sGUAip+ncxsSvYdc=bRbTszTuMg@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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