Re: [PATCH 1/4] bootchooser: implement locking of boot slots

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

FTR, this is meant to be exercised by RAUC with the PR being here:
https://github.com/rauc/rauc/pull/1732

On 6/13/25 16:08, Lars Schmidt wrote:
> This this new global slot lock inhibits the remaining attempts counter
> from decreasing.

It turns out, the RAUC developers tell me, that slots is a RAUC concept
and indeed there is no mention of slots anywhere in
common/bootchooser.c. Bootchooser instead has targets.

> There are ways around this, but both come with a disadvantage:
>   - If we mark the old slot as bad after a good update, we lose the
>     distinction between old slots that are unbootable and ones that
>     are bootable. Maintaining this distinction allows a health monitor
>     to explicitly boot an old slot while preventing the bootloader
>     from doing it automatically
>   - If we set the maximum attempts to a very high number, we keep
>     writing the storage every boot, even if we don't do
> In both cases, by not decreasing and increasing the remaining attempts
> counter constantly the number of write cycles is also lowered.
> 
> Signed-off-by: Lars Schmidt <l.schmidt@pengutronix.de>
> ---
>  common/bootchooser.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/common/bootchooser.c b/common/bootchooser.c
> index 58032a2b57..a50f757791 100644
> --- a/common/bootchooser.c
> +++ b/common/bootchooser.c
> @@ -49,6 +49,7 @@ struct bootchooser {
>  	struct state *state;
>  	char *state_prefix;
>  	int refs;
> +	bool slots_locked;

rename to attempts_locked.

>  
>  	int verbose;
>  	int dryrun;
> @@ -353,6 +354,7 @@ struct bootchooser *bootchooser_get(void)
>  	int ret = -EINVAL, id = 1;
>  	uint32_t last_chosen;
>  	static int attempts_resetted;
> +	uint32_t locked;
>  
>  	if (bootchooser) {
>  		bootchooser->refs++;
> @@ -397,6 +399,18 @@ struct bootchooser *bootchooser_get(void)
>  		pr_warn("using non-redundant NV instead of barebox-state\n");
>  	}
>  
> +	ret = getenv_u32(bc->state_prefix, "slots_locked", &locked);

attempts_locked.

> +	if (!ret) {
> +		bc->slots_locked = locked ? true : false;

Needlessly verbose. Just initialize attempts_locked to false and then here:

if (!ret && locked) {
	bc->attempts_locked = true;
        pr_debug(...);
}
> +		if (bc->slots_locked)
> +			pr_debug("Global slots locking is enabled\n");

"boot attempt counting is locked\n" ?

> +	} else {
> +		/* this is an optional value, so if it doesn't exist,
> +		 * we assume it's not locked
> +		 */
> +		bc->slots_locked = false;
> +	}

Can be dropped with above suggested change.

> +
>  	INIT_LIST_HEAD(&bc->targets);
>  
>  	freep = targets = xstrdup(available_targets);
> @@ -650,11 +664,14 @@ static struct bootchooser_target *bootchooser_get_target(struct bootchooser *bc)
>  	return ERR_PTR(-ENOENT);
>  
>  found:
> -	target->remaining_attempts--;
> -
> -	if (bc->verbose)
> -		pr_info("name=%s decrementing remaining_attempts to %d\n",
> -			target->name, target->remaining_attempts);
> +	if (!bc->slots_locked) {

attempts_locked

> +		target->remaining_attempts--;
> +		if (bc->verbose)
> +			pr_info("name=%s remaining_attempts %d\n", target->name,
> +				target->remaining_attempts);
> +	} else {
> +		pr_info("Slots locking is enabled, not decreasing remaining_attempts\n");
> +	}
>  
>  	if (bc->verbose)
>  		pr_info("selected target '%s', boot '%s'\n", target->name, target->boot);

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |