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

[Simon Glass]

Hi Frédéric,

On Thu, 12 Mar 2020 at 08:36, Frédéric Danis
<frederic.danis at collabora.com> wrote:
>
> 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> 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?

I suggest doc/README.bootcount

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

It looks like we already have chosen.txt which describes the
device-tree binding. Perhaps it can fit into that?

Regards,
Simon