diff mbox series

[U-Boot,v3,3/7] bootcount: Add function wrappers to handle bootcount increment and error checking

Message ID 20180429133633.29509-4-lukma@denx.de
State Superseded
Delegated to: Tom Rini
Headers show
Series Provide SPL support for bootcount (in the case of using falcon boot mode) | expand

Commit Message

Lukasz Majewski April 29, 2018, 1:36 p.m. UTC 
Those two functions can be used to provide easy bootcount management.

Signed-off-by: Lukasz Majewski <lukma@denx.de>

---

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 | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Stefan Roese April 30, 2018, 8:07 a.m. UTC | #1 
Hi Lukasz,

sorry, I still have some comments to (hopefully) make this
implementation a bit more "elegant". Please see below.

On 29.04.2018 15:36, Lukasz Majewski wrote:
> Those two functions can be used to provide easy bootcount management.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> 
> ---
> 
> 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 | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 50 insertions(+)
> 
> diff --git a/include/bootcount.h b/include/bootcount.h
> index e3b3f7028e..16fc657b2a 100644
> --- a/include/bootcount.h
> +++ b/include/bootcount.h
> @@ -11,6 +11,13 @@
>   #include <asm/io.h>
>   #include <asm/byteorder.h>
>   
> +enum bootcount_context {
> +	SPL = 1,
> +	UBOOT,
> +};

Perhaps better some bootcount specific values / enums, like:

enum bootcount_context {
	BOOTCOUNT_STATE_SPL = 1,
	BOOTCOUNT_STATE_UBOOT,
};

Or even more generic, as this "boot-state" does not have to be bootcount
specific:

enum u_boot_context {
	U_BOOT_STATE_SPL = 1,
	U_BOOT_STATE_U_BOOT,
};

Or do we already have something like this, perhaps in "gd", where
its marked, in which state we are currently running? If not, it
could be added there and could be used from the bootcounter code
and other interfaces / drivers as well. The parts pre-relocation and
post-relocation fall also into this area (for the pre- / post-reloc
we definitely have a variable / flag in gd somewhere). So perhaps:

enum u_boot_context {
	U_BOOT_STATE_SPL = 1,
	U_BOOT_STATE_U_BOOT_PRE_RELOC,
	U_BOOT_STATE_U_BOOT_POST_RELOC,
};

> +#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 +47,47 @@ static inline u32 raw_bootcount_load(volatile u32 *addr)
>   	return in_be32(addr);
>   }
>   #endif
> +
> +static inline int bootcount_error(enum bootcount_context bc)
> +{
> +	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 (bc == UBOOT)
> +			printf(" Using altbootcmd.");
> +		printf("\n");
> +
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline void bootcount_inc(enum bootcount_context bc)
> +{
> +	unsigned long bootcount = bootcount_load();
> +
> +	if (bc == SPL) {
> +		bootcount_store(++bootcount);
> +		return;
> +	}
> +
> +	/* Only increment bootcount when no bootcount support in SPL */
> +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> +	bootcount++;
> +#endif
> +	bootcount_store(bootcount);
> +	env_set_ulong("bootcount", bootcount);
> +}

Why not use the same logic / code as above (the store does not need
to happen twice):

	/* Only increment bootcount when no bootcount support in SPL */
#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
	bootcount_store(++bootcount);
#endif

	env_set_ulong("bootcount", bootcount);

?

What do you think?

Thanks,
Stefan
Lukasz Majewski April 30, 2018, 8:39 a.m. UTC | #2 
Hi Stefan,

> Hi Lukasz,
> 
> sorry, I still have some comments to (hopefully) make this
> implementation a bit more "elegant". Please see below.
> 
> On 29.04.2018 15:36, Lukasz Majewski wrote:
> > Those two functions can be used to provide easy bootcount
> > management.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > 
> > ---
> > 
> > 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 | 50
> > ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > 50 insertions(+)
> > 
> > diff --git a/include/bootcount.h b/include/bootcount.h
> > index e3b3f7028e..16fc657b2a 100644
> > --- a/include/bootcount.h
> > +++ b/include/bootcount.h
> > @@ -11,6 +11,13 @@
> >   #include <asm/io.h>
> >   #include <asm/byteorder.h>
> >   
> > +enum bootcount_context {
> > +	SPL = 1,
> > +	UBOOT,
> > +};  
> 
> Perhaps better some bootcount specific values / enums, like:
> 
> enum bootcount_context {
> 	BOOTCOUNT_STATE_SPL = 1,
> 	BOOTCOUNT_STATE_UBOOT,
> };
> 
> Or even more generic, as this "boot-state" does not have to be
> bootcount specific:
> 
> enum u_boot_context {
> 	U_BOOT_STATE_SPL = 1,
> 	U_BOOT_STATE_U_BOOT,
> };
> 
> Or do we already have something like this, perhaps in "gd", where
> its marked, in which state we are currently running? If not, it
> could be added there and could be used from the bootcounter code
> and other interfaces / drivers as well. The parts pre-relocation and
> post-relocation fall also into this area (for the pre- / post-reloc
> we definitely have a variable / flag in gd somewhere). So perhaps:
> 
> enum u_boot_context {
> 	U_BOOT_STATE_SPL = 1,
> 	U_BOOT_STATE_U_BOOT_PRE_RELOC,
> 	U_BOOT_STATE_U_BOOT_POST_RELOC,
> };

I will check if we do have such flags already defined. "gd" approach
looks better (more generic for sure) than the one from v3.

> 
> > +#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 +47,47 @@ static inline u32 raw_bootcount_load(volatile
> > u32 *addr) return in_be32(addr);
> >   }
> >   #endif
> > +
> > +static inline int bootcount_error(enum bootcount_context bc)
> > +{
> > +	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 (bc == UBOOT)
> > +			printf(" Using altbootcmd.");
> > +		printf("\n");
> > +
> > +		return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static inline void bootcount_inc(enum bootcount_context bc)
> > +{
> > +	unsigned long bootcount = bootcount_load();
> > +
> > +	if (bc == SPL) {
> > +		bootcount_store(++bootcount);
> > +		return;
> > +	}
> > +
> > +	/* Only increment bootcount when no bootcount support in
> > SPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > +	bootcount++;
> > +#endif
> > +	bootcount_store(bootcount);
> > +	env_set_ulong("bootcount", bootcount);
> > +}  
> 
> Why not use the same logic / code as above (the store does not need
> to happen twice):
> 
> 	/* Only increment bootcount when no bootcount support in SPL
> */ #ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> 	bootcount_store(++bootcount);
> #endif
> 
> 	env_set_ulong("bootcount", bootcount);
> 
> ?
> 
> What do you think?

Good point - I will add it in v4.

Thanks for input.

> 
> Thanks,
> Stefan


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@denx.de
diff mbox series

Patch

diff --git a/include/bootcount.h b/include/bootcount.h
index e3b3f7028e..16fc657b2a 100644
--- a/include/bootcount.h
+++ b/include/bootcount.h
@@ -11,6 +11,13 @@ 
 #include <asm/io.h>
 #include <asm/byteorder.h>
 
+enum bootcount_context {
+	SPL = 1,
+	UBOOT,
+};
+
+#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 +47,47 @@  static inline u32 raw_bootcount_load(volatile u32 *addr)
 	return in_be32(addr);
 }
 #endif
+
+static inline int bootcount_error(enum bootcount_context bc)
+{
+	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 (bc == UBOOT)
+			printf(" Using altbootcmd.");
+		printf("\n");
+
+		return 1;
+	}
+
+	return 0;
+}
+
+static inline void bootcount_inc(enum bootcount_context bc)
+{
+	unsigned long bootcount = bootcount_load();
+
+	if (bc == SPL) {
+		bootcount_store(++bootcount);
+		return;
+	}
+
+	/* Only increment bootcount when no bootcount support in SPL */
+#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
+	bootcount++;
+#endif
+	bootcount_store(bootcount);
+	env_set_ulong("bootcount", bootcount);
+}
+
+#if defined CONFIG_SPL_BUILD && !defined CONFIG_SPL_BOOTCOUNT_LIMIT
+void bootcount_store(ulong a) {};
+ulong bootcount_load(void) { return 0; }
+#endif /* CONFIG_SPL_BUILD && !CONFIG_SPL_BOOTCOUNT_LIMIT */
+#else
+static inline int bootcount_error(enum bootcount_context bc) { return 0; }
+static inline void bootcount_inc(enum bootcount_context bc) {}
+#endif /* CONFIG_SPL_BOOTCOUNT_LIMIT || CONFIG_BOOTCOUNT_LIMIT */
 #endif /* _BOOTCOUNT_H__ */