Hi Denis,
On 2026-06-05T03:38:59, None <[email protected]> wrote:
> bootcount: gate bootcount_{inc, error}() behind !CONFIG_DM_BOOTCOUNT_GPT
>
> Autoboot calls into bootcount_inc()/bootcount_error() which rely on
> environment variable.
>
> Specifically, it may corrupt the state of a GPT-backed counter.
>
> Gate bootcount_inc()/bootcount_error() by !CONFIG_DM_BOOTCOUNT_GPT to
> compile out bootcount_store() calls on autoboot path.
>
> Signed-off-by: Denis Mukhin <[email protected]>
>
> include/bootcount.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> diff --git a/include/bootcount.h b/include/bootcount.h
> @@ -74,7 +74,8 @@ void bootcount_store(ulong);
> */
> ulong bootcount_load(void);
>
> -#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
> defined(CONFIG_TPL_BOOTCOUNT_LIMIT) || defined(CONFIG_BOOTCOUNT_LIMIT)
> +#if (defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
> defined(CONFIG_TPL_BOOTCOUNT_LIMIT) || defined(CONFIG_BOOTCOUNT_LIMIT)) && \
> + !defined(CONFIG_DM_BOOTCOUNT_GPT)
Hmm this needs a Kconfig solution...I don't like baking a specific
driver's symbol into the generic bootcount header - it inverts the
usual layering (the uclass header should not know about its drivers)
and it does not scale: the next backend that wants the same behaviour
has to add another '&& !defined(...)' here.
This also gates the inline helpers everywhere, not just on the
autoboot path. The SPL increment at common/spl/spl.c:792 becomes a
no-op too, as does any board code calling
bootcount_inc()/bootcount_error(). That is a much bigger behavioural
change than the subject suggests, and should be called out in the
commit message.
Instead, how about you introduce a generic Kconfig
(BOOTCOUNT_NO_INCREMENT, selected by DM_BOOTCOUNT_GPT) so the gate
stays driver-agnostic, or handle it inside bootcount_store()/the
uclass so the increment is silently dropped when the backend
implements its own semantics. What do you think?
> diff --git a/include/bootcount.h b/include/bootcount.h
> @@ -74,7 +74,8 @@ void bootcount_store(ulong);
> */
> ulong bootcount_load(void);
>
> -#if defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
> defined(CONFIG_TPL_BOOTCOUNT_LIMIT) || defined(CONFIG_BOOTCOUNT_LIMIT)
> +#if (defined(CONFIG_SPL_BOOTCOUNT_LIMIT) ||
> defined(CONFIG_TPL_BOOTCOUNT_LIMIT) || defined(CONFIG_BOOTCOUNT_LIMIT)) && \
> + !defined(CONFIG_DM_BOOTCOUNT_GPT)
The commit message says "may corrupt the state of a GPT-backed
counter" but does not explain why - please spell out that the top 16
bits encode structured ChromiumOS flags (priority/tries/successful),
so a plain ++ on the loaded value scrambles those fields rather than
producing a meaningful count.
Subject also has a stray space inside the brace expansion:
'bootcount_{inc, error}()' - please make it 'bootcount_{inc,error}()'
to match the body.
Regards,
Simon
