[PATCH] bootcount_ext: Add flag to enable/disable bootcount

[Frédéric Danis]

Hi Simon,

On 12/03/2020 04:22, Simon Glass wrote:
> Hi Frédéric,
>
> On Wed, 11 Mar 2020 at 05:00, Frédéric Danis 
> <frederic.danis at collabora.com <mailto:frederic.danis at collabora.com>> 
> wrote:
> >
> > After a successful upgrade, multiple problem during boot sequence may
> > trigger the altbootcmd process.
> > This patch adds a 3rd byte to the bootcount file to enable/disable the
> > bootcount check.
> > When reading old bootcount file, 2 bytes long, it will consider that
> > bootcount is enabled, and update the file accordingly.
> >
>
> Is this format documented somewhere? Should it perhaps have a version 
> number so you can copy with future changes?

I will change the Magic byte and add a version number.

Can you give me advice where I should add this format documentation?

>
> > The bootcount file is only saved when `upgrade_available` is true, this
> > allows to save writes to the filesystem.
> >
> > Signed-off-by: Frédéric Danis <frederic.danis at collabora.com 
> <mailto:frederic.danis at collabora.com>>
> > ---
> >  drivers/bootcount/bootcount_ext.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/bootcount/bootcount_ext.c 
> b/drivers/bootcount/bootcount_ext.c
> > index 075e590896..740bce0296 100644
> > --- a/drivers/bootcount/bootcount_ext.c
> > +++ b/drivers/bootcount/bootcount_ext.c
> > @@ -9,6 +9,8 @@
> >
> >  #define BC_MAGIC       0xbc
> >
> > +static u8 upgrade_available = 1;
> > +
> >  void bootcount_store(ulong a)
> >  {
> >         u8 *buf;
> > @@ -21,13 +23,18 @@ void bootcount_store(ulong a)
> >                 return;
> >         }
> >
> > -       buf = map_sysmem(CONFIG_SYS_BOOTCOUNT_ADDR, 2);
> > +       /* Only update bootcount during upgrade process */
> > +       if (!upgrade_available)
> > +               return;
> > +
> > +       buf = map_sysmem(CONFIG_SYS_BOOTCOUNT_ADDR, 3);
> >         buf[0] = BC_MAGIC;
> >         buf[1] = (a & 0xff);
> > +       buf[2] = upgrade_available;
> >         unmap_sysmem(buf);
> >
> >         ret = fs_write(CONFIG_SYS_BOOTCOUNT_EXT_NAME,
> > -                      CONFIG_SYS_BOOTCOUNT_ADDR, 0, 2, &len);
> > +                      CONFIG_SYS_BOOTCOUNT_ADDR, 0, 3, &len);
>
> Separate from this patch...to me it seems ugly to put this info in 
> CONFIG. Could it not be structured as a driver, with device tree used 
> to hold the information?
>
> See for example fs_loader.txt

OK, I will try to send another patch for this … but I don't know when

>
> >         if (ret != 0)
> >                 puts("Error storing bootcount\n");
> >  }
> > @@ -45,15 +52,20 @@ ulong bootcount_load(void)
> >         }
> >
> >         ret = fs_read(CONFIG_SYS_BOOTCOUNT_EXT_NAME, 
> CONFIG_SYS_BOOTCOUNT_ADDR,
> > -                     0, 2, &len_read);
> > -       if (ret != 0 || len_read != 2) {
> > +                     0, 3, &len_read);
> > +       if (ret != 0 || len_read < 2 || len_read > 3) {
> >                 puts("Error loading bootcount\n");
> >                 return 0;
> >         }
> >
> >         buf = map_sysmem(CONFIG_SYS_BOOTCOUNT_ADDR, 2);
> > -       if (buf[0] == BC_MAGIC)
> > -               ret = buf[1];
> > +       /* Use the 3rd byte to enable/disable bootcount */
> > +       if (buf[0] == BC_MAGIC) {
> > +               if (len_read == 3)
> > +                       upgrade_available = buf[2];
> > +               if (upgrade_available)
> > +                       ret = buf[1];
> > +       }
> >
> >         unmap_sysmem(buf);
> >
> > --
> > 2.18.0
> >
>
> Regards,
> Simon

Regards,
Fred