[U-Boot] [PATCH v1 3/5] bootcount: u-boot: Do not increment bootcount if already done in SPL

[Tom Rini]

On Mon, Feb 26, 2018 at 03:53:18PM +0100, Lukasz Majewski wrote:
> Hi Tom,
> 
> > On Mon, Feb 26, 2018 at 01:22:44PM +0100, Lukasz Majewski wrote:
> > 
> > > If the CONFIG_SPL_BOOTCOUNT_LIMIT is defined, the bootcount
> > > variable is already incremented after each boot attempt.
> > > 
> > > For that reason we shall not increment it again in u-boot.
> > > 
> > > Signed-off-by: Lukasz Majewski <lukma at denx.de>
> > > ---
> > > 
> > >  common/autoboot.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/common/autoboot.c b/common/autoboot.c
> > > index 2eef7a04cc..87fca2ea92 100644
> > > --- a/common/autoboot.c
> > > +++ b/common/autoboot.c
> > > @@ -298,7 +298,9 @@ const char *bootdelay_process(void)
> > >  
> > >  #ifdef CONFIG_BOOTCOUNT_LIMIT
> > >  	bootcount = bootcount_load();
> > > +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > >  	bootcount++;
> > > +#endif
> > >  	bootcount_store(bootcount);
> > >  	env_set_ulong("bootcount", bootcount);
> > >  	bootlimit = env_get_ulong("bootlimit", 10, 0);  
> > 
> > Shouldn't we make the whole bit of code here depend on
> > !CONFIG_SPL_BOOTCOUNT_LIMIT ?  
> 
> Maybe I will try to present the scenario:
> 
> - We do use SPL to boot from either eMMC (if in falcon) or from SPI-NOR
>   (if not in falcon)
> 
> - With bootcount:
> 	-- We may want to boot into eMMC directly, so we need to
> 	increment the bootcount (no matter where it is located).
> 
> 	-- We intentionally (by setting other env variable) or not (by
> 	entering X resets) abort falcon mode boot from SPL and boot to
> 	u-boot (which is in spi-nor).
> 	In this case we do need all the bootcount limit functionality
> 	(i.e. altbootcmd) excluding increment bootcount (as it was
> 	done in SPL).
> 	
> +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT is only required to achieve this
> goal without making any more code change...

And you have one build that produces the binaries used in both cases,
OK.

> > Or for that matter, re-work the first
> > patch to instead be:
> > bool SUPPORT_BOOTCOUNT "Support bootcount in some way"
> > choice "Bootcount location"
> > bool SPL_BOOTCOUNT_LIMIT "Count boots from POV of SPL"
> > bool BOOTCOUNT_LIMIT "Count boots from POV of full U-Boot"
> > endchoice
> 
> But then SPL_BOOTCOUNT_LIMIT would select BOOTCOUNT_LIMIT to fulfil the
> above scenario.
> 
> > 
> > ?  Looking back at my old series from 2014 to do this, I just had a
> > big warning saying you need to enable bootcount for SPL or for full
> > U-Boot so you would never set BOOTCOUNT_LIMIT if doing SPL bootcount
> > instead.
> 
> Such functionality can be achieved with SPL_BOOTCOUNT_LIMIT depending
> on BOOTCOUNT_LIMIT.
> 
> If SPL_BOOTCOUNT_LIMIT selected, then "[PATCH v1 3/5] bootcount: u-boot:
> Do not increment bootcount if already done in SPL" does its job.

OK, I expect that once this code comes in, it might get complicated a
bit further by other use cases.  But I don't have a specific example I'm
going to implement currently, so I'm OK moving forward here and
expanding it as other cases come up.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180226/ccbcfa8a/attachment.sig>