Re: [PATCH v2] bootchooser: honour reset source

[Sascha Hauer <sha@pengutronix.de>]

On Fri, Sep 29, 2023 at 10:53:37AM +0200, Holger Assmann wrote:
> With some systems it is possible to determine the source that triggered
> the most recent reset (see $global.system.reset).
> 
> This information can be used at the subsequent boot to evaluate whether
> the respective target works as intended by defining reset reasons that
> are to be used exclusively. Any other reset source would therefore be
> considered to be bad, which should lead to a decrease of the counter
> variable "remaining_attempts" for that target.
> Until now, such an investigation and the consecutive decision of marking
> the last boot successful has either to be done by a custom script in
> barebox or by a service that runs on the eventually booted operating
> system.
> 
> Since (if supported by the system) analyzing the reset reason is
> probably the most obvious factor for the marking decision, we might as
> well integrate the functionality in bootchooser directly.
> 
> This patch therefore introduces "$global.bootchooser.good_reset_source"
> as a new variable in which good reset sources can be listed to be
> matched against the actual reset reason.
> 
> Note: In order to allow for proper matching of the reset source with the
> possible good reasons, a bitmask is used. To make this work, we had to
> make the variable "reset_src_names[]" accessible from outside of
> "reset_source.c". It has hence been moved into the respective header
> file where it now fittingly resides alongside "reset_src_type".
> 
> Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
> ---
> 
> Changes v1 -> v2:
> 
> 1. changed matching mechanism of reset sources to an approach that utilizes a
>    bitmask and "globalvar_add_simple_bitmask", instead of
>    "globalvar_add_simple_string" and string comparison
> 2. properly handle "last_boot_successful" as boolean
> 3. removed some trailing whitespaces
> 
> ---
>  Documentation/user/bootchooser.rst | 67 +++++++++++++++++++++++++-----
>  common/bootchooser.c               | 21 ++++++++++
>  common/reset_source.c              | 12 ------
>  include/reset_source.h             | 12 ++++++
>  4 files changed, 90 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/user/bootchooser.rst b/Documentation/user/bootchooser.rst
> index db0a4f8898..4618d7cda0 100644
> --- a/Documentation/user/bootchooser.rst
> +++ b/Documentation/user/bootchooser.rst
> @@ -108,6 +108,20 @@ While the bootchooser algorithm handles attempts decrementation, retries and
>  selection of the right boot target itself, it cannot decide if the system
>  booted successfully on its own.
>  
> +However, for systems where barebox is able to detect the actual :ref:`reset reason <reset_reason>`
> +(e.g. WDG), bootchooser will look for matches in ``global.bootchooser.good_reset_sources``.
> +This variable may contain a space-separated list of those reset reasons that are
> +considered *good*. If a matching entry is found, bootchooser will mark the last
> +boot successful.
> +
> +This marking can also be performed manually or from within a script by calling
> +the barebox :ref:`bootchoser command <command_bootchooser>`::
> +
> +  bootchooser -s
> +
> +Marking the preceding boot successful will result in the ``remaining_attempts`` counter of the
> +*last chosen* slot to be reset to its default value (``reset_attempts``).
> +
>  In case only the booted system itself knows when it is in a good state,
>  it can report this to the bootchooser from Linux userspace using the
>  *barebox-state* tool from the dt-utils_ package.::
> @@ -116,16 +130,6 @@ it can report this to the bootchooser from Linux userspace using the
>    barebox-state -n system_state -s bootstate.system1.remaining_attempts=3
>    barebox-state -s system1.remaining_attempts=3
>  
> -If instead the bootchooser can detect a failed boot itself using the
> -:ref:`reset reason <reset_reason>` (WDG), one can mark the boot successful
> -using the barebox :ref:`bootchoser command <command_bootchooser>`::
> -
> -  bootchooser -s
> -
> -to mark the last boot successful.
> -This will reset the ``remaining_attempts`` counter of the *last chosen* slot to
> -its default value (``reset_attempts``).
> -
>  
>  .. _dt-utils: https://git.pengutronix.de/cgit/tools/dt-utils
>  
> @@ -147,6 +151,8 @@ options not specific to any boot target.
>    specific variable of the same name.
>  ``global.bootchooser.reset_attempts``
>    Already described in :ref:`Bootchooser Algorithm <bootchooser,algorithm>`
> +``bootchooser.good_reset_sources``
> +  Already described in :ref:`Bootchooser Algorithm <bootchooser,algorithm>`
>  ``global.bootchooser.reset_priorities``
>    A space-separated list of events that cause *bootchooser* to reset the priorities of
>    all boot targets. Possible values:
> @@ -268,6 +274,7 @@ are reset to their defaults and the first boot target is tried again.
>  Settings
>  ^^^^^^^^
>  - ``global.bootchooser.reset_attempts="all-zero"``
> +- ``global.bootchooser.good_reset_sources=""``
>  - ``global.bootchooser.reset_priorities="all-zero"``
>  - ``global.bootchooser.disable_on_zero_attempts=0``
>  - ``global.bootchooser.retry=1``
> @@ -302,6 +309,7 @@ Settings
>  ^^^^^^^^
>  
>  - ``global.bootchooser.reset_attempts=""``
> +- ``global.bootchooser.good_reset_sources=""``
>  - ``global.bootchooser.reset_priorities=""``
>  - ``global.bootchooser.disable_on_zero_attempts=0``
>  - ``global.bootchooser.retry=1``
> @@ -337,6 +345,7 @@ Settings
>  ^^^^^^^^
>  
>  - ``global.bootchooser.reset_attempts="power-on"``
> +- ``global.bootchooser.good_reset_sources=""``
>  - ``global.bootchooser.reset_priorities=""``
>  - ``global.bootchooser.disable_on_zero_attempts=1``
>  - ``global.bootchooser.retry=1``
> @@ -358,6 +367,44 @@ through due to the lack of bootable targets. This target can be:
>  - a system that will be booted as recovery.
>  - a barebox script that will be started.
>  
> +Scenario 4
> +##########
> +
> +- a system with multiple boot targets
> +- one recovery system
> +- detection of the :ref:`cause of the preceding reset <reset_reason>` is
> +  supported by the hardware
> +
> +  - POR (Power On Reset) and RST (ReSeT) are considered *good* causes
> +
> +Booting a boot target three times without success disables it.
> +
> +Settings
> +^^^^^^^^
> +
> +- ``global.bootchooser.reset_attempts=""``
> +- ``global.bootchooser.good_reset_sources="POR RST"``
> +- ``global.bootchooser.reset_priorities=""``
> +- ``global.bootchooser.disable_on_zero_attempts=1``
> +- ``global.bootchooser.retry=1``
> +- ``global.boot.default="bootchooser recovery"``
> +- bootchooser marks as good.
> +
> +Deployment
> +^^^^^^^^^^
> +
> +#. barebox or flash robot fills all boot targets with valid systems.
> +#. barebox or flash robot marks boot targets as good.
> +
> +Recovery
> +^^^^^^^^
> +
> +Done by 'recovery' boot target which is booted after the *bootchooser* falls
> +through due to the lack of bootable targets. This target can be:
> +
> +- a system that will be booted as recovery.
> +- a barebox script that will be started.
> +
>  .. _bootchooser,state_framework:
>  
>  Using the *State* Framework as Backend for Run-Time Variable Data
> diff --git a/common/bootchooser.c b/common/bootchooser.c
> index eb3dda52ab..b3499332be 100644
> --- a/common/bootchooser.c
> +++ b/common/bootchooser.c
> @@ -86,6 +86,7 @@ enum reset_priorities {
>  };
>  
>  static unsigned long reset_priorities;
> +static unsigned long good_reset_src_type;
>  
>  static int bootchooser_target_ok(struct bootchooser_target *target, const char **reason)
>  {
> @@ -348,6 +349,7 @@ struct bootchooser *bootchooser_get(void)
>  {
>  	struct bootchooser *bc;
>  	struct bootchooser_target *target;
> +	enum reset_src_type type = reset_source_get();
>  	char *targets, *str, *freep = NULL, *delim;
>  	int ret = -EINVAL, id = 1;
>  	uint32_t last_chosen;
> @@ -451,6 +453,19 @@ struct bootchooser *bootchooser_get(void)
>  		}
>  	}
>  
> +	if (!last_boot_successful &&
> +		test_bit(type, &good_reset_src_type)) {
> +		/*
> +		 * If there are reset sources defined that are considered good
> +		 * and the reset source detected by the system fits an entry of
> +		 * that list, we do not want to further decrement the
> +		 * remaining_attempts counter.
> +		 */
> +  		pr_info("Good Reset '%s', marking last boot successful\n",
> +  				reset_source_to_string(type));
> +		last_boot_successful = true;
> +  	}

I have a problem with this. When a system is interrupted with a power
failure or by pushing the reset button during boot then we can't say
that this boot was "good". We just don't know. All we can do is to
ignore that last boot for the calculation of the remaining attempts.

The end result might implement your desired behaviour, but the code
leading to that result doesn't seem logical.

Also we have this snippet:

	if (test_bit(RESET_ATTEMPTS_POWER_ON, &reset_attempts) &&
	    reset_source_get() == RESET_POR && !attempts_resetted) {
		pr_info("Power-on Reset, resetting remaining attempts\n");
		bootchooser_reset_attempts(bc);
		attempts_resetted = 1;
	}

I'm not sure if that already implements your desired behaviour, but it
at least overlaps with the case you are implementing. Would it be an
option to extend the global.bootchooser.reset_attempts variable with a
"reset" bit and adjust the above accordingly?

Sascha

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