From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukasz Majewski Date: Tue, 8 May 2018 12:27:30 +0200 Subject: [U-Boot] [PATCH v5 3/7] bootcount: Add function wrappers to handle bootcount increment and error checking In-Reply-To: References: <20180502141056.23937-1-lukma@denx.de> <20180502141056.23937-4-lukma@denx.de> <20180508094108.2e25753a@jawa> <20180508112102.2a8b8251@jawa> Message-ID: <20180508122730.28eb178b@jawa> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Alex, > On Tue, May 8, 2018 at 10:21 AM Lukasz Majewski wrote: > > > Hi Alex, > > > > On Tue, May 8, 2018 at 8:41 AM Lukasz Majewski > > > wrote: > > > > On Tue, 08 May 2018 05:15:13 +0000 > > > > Alex Kiernan wrote: > > > > > > > > On Wed, May 2, 2018 at 3:11 PM Lukasz Majewski > > > > > wrote: > > > > > > Those two functions can be used to provide easy bootcount > > > > > > management. > > > > > > > > > > > Signed-off-by: Lukasz Majewski > > > > > > > > > > > Reviewed-by: Tom Rini > > > > > > Reviewed-by: Stefan Roese > > > > > > --- > > > > > > > > > > > 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 > > > > > > #include > > > > > > > > > > > +#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). > > > There isn't one (at least not one I'm aware of), but this gives > another impossible combination which you can select, of which we've > lots already. > > I should've played around some more before commenting as I think what > you've got is everything that's needed. It is all the matter of use-case. For SPL I've assumed that we will use IRAM or some RTC special register (as it is with display5) to store the bootcount (all info - magic number with boot count in 32 bits). This seems logical and optimal - since we "assume" that SPL shall be possible small. To avoid coincidence one needs to define CONFIG_SPL_BOOTCOUNT. Without it we shall have the same behaviour as previously on all supported boards (in theory at least :-) ). > > -- > Alex Kiernan 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: