| Message ID | 20200311105949.23433-1-frederic.danis@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | bootcount_ext: Add flag to enable/disable bootcount | expand |
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? > 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 > 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
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
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
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); 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);
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. 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(-)