mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH v2] bootchooser: honour reset source
@ 2023-09-29  8:53 Holger Assmann
  2023-10-04  8:19 ` Sascha Hauer
  0 siblings, 1 reply; 3+ messages in thread
From: Holger Assmann @ 2023-09-29  8:53 UTC (permalink / raw)
  To: barebox; +Cc: Holger Assmann

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;
+  	}
+
 	ret = getenv_u32(bc->state_prefix, "last_chosen", &last_chosen);
 	if (!ret && last_chosen > 0) {
 		bc->last_chosen = bootchooser_target_by_id(bc, last_chosen);
@@ -932,6 +947,8 @@ static int bootchooser_init(void)
 	globalvar_add_simple_string("bootchooser.state_prefix", &state_prefix);
 	globalvar_add_simple_int("bootchooser.default_attempts", &global_default_attempts, "%u");
 	globalvar_add_simple_int("bootchooser.default_priority", &global_default_priority, "%u");
+	globalvar_add_simple_bitmask("bootchooser.good_reset_sources", &good_reset_src_type,
+			          reset_src_names, ARRAY_SIZE(reset_src_names));
 	globalvar_add_simple_bitmask("bootchooser.reset_attempts", &reset_attempts,
 				  reset_attempts_names, ARRAY_SIZE(reset_attempts_names));
 	globalvar_add_simple_bitmask("bootchooser.reset_priorities", &reset_priorities,
@@ -951,6 +968,10 @@ BAREBOX_MAGICVAR(global.bootchooser.targets,
 		 "bootchooser: Space separated list of target names");
 BAREBOX_MAGICVAR(global.bootchooser.default_attempts,
 		 "bootchooser: Default number of attempts for a target");
+BAREBOX_MAGICVAR(global.bootchooser.reset_attempts,
+		"bootchooser: Choose condition to reset number of attempts for all targets ('power-on', 'all-zero')");
+BAREBOX_MAGICVAR(global.bootchooser.good_reset_sources,
+		"bootchooser: Space separated list of good reset sources consulted to not decrease remainin attempts counter");
 BAREBOX_MAGICVAR(global.bootchooser.default_priority,
 		 "bootchooser: Default priority for a target");
 BAREBOX_MAGICVAR(global.bootchooser.state_prefix,
diff --git a/common/reset_source.c b/common/reset_source.c
index f28be90dcb..2016329e5e 100644
--- a/common/reset_source.c
+++ b/common/reset_source.c
@@ -10,18 +10,6 @@
 #include <globalvar.h>
 #include <reset_source.h>
 
-static const char * const reset_src_names[] = {
-	[RESET_UKWN] = "unknown",
-	[RESET_POR] = "POR",
-	[RESET_RST] = "RST",
-	[RESET_WDG] = "WDG",
-	[RESET_WKE] = "WKE",
-	[RESET_JTAG] = "JTAG",
-	[RESET_THERM] = "THERM",
-	[RESET_EXT] = "EXT",
-	[RESET_BROWNOUT] = "BROWNOUT",
-};
-
 static enum reset_src_type reset_source;
 static unsigned int reset_source_priority;
 static int reset_source_instance;
diff --git a/include/reset_source.h b/include/reset_source.h
index 3766208b6d..e55cc2fadc 100644
--- a/include/reset_source.h
+++ b/include/reset_source.h
@@ -21,6 +21,18 @@ enum reset_src_type {
 	RESET_BROWNOUT,	/* Brownout Reset */
 };
 
+static const char * const reset_src_names[] = {
+	[RESET_UKWN] = "unknown",
+	[RESET_POR] = "POR",
+	[RESET_RST] = "RST",
+	[RESET_WDG] = "WDG",
+	[RESET_WKE] = "WKE",
+	[RESET_JTAG] = "JTAG",
+	[RESET_THERM] = "THERM",
+	[RESET_EXT] = "EXT",
+	[RESET_BROWNOUT] = "BROWNOUT",
+};
+
 #ifdef CONFIG_RESET_SOURCE
 
 enum reset_src_type reset_source_get(void);
-- 
2.39.2




^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] bootchooser: honour reset source
  2023-09-29  8:53 [PATCH v2] bootchooser: honour reset source Holger Assmann
@ 2023-10-04  8:19 ` Sascha Hauer
  2023-10-19 13:45   ` Holger Assmann
  0 siblings, 1 reply; 3+ messages in thread
From: Sascha Hauer @ 2023-10-04  8:19 UTC (permalink / raw)
  To: Holger Assmann; +Cc: barebox

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 |



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] bootchooser: honour reset source
  2023-10-04  8:19 ` Sascha Hauer
@ 2023-10-19 13:45   ` Holger Assmann
  0 siblings, 0 replies; 3+ messages in thread
From: Holger Assmann @ 2023-10-19 13:45 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

Hello Sascha,

Am 04.10.23 um 10:19 schrieb Sascha Hauer:
> 
> 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.

I can see your point with this valid edge case. However, this leads me
to the question why we already have a variable "last_boot_successful",
as we indeed can never know whether a previous boot really was
successful: Even something like a RAUC mark-good service can set a wrong
"success flag" i.e. when a critical error happens after that service has
already run.

This brings me to the idea of simply "inverting" the logic, changing the
code of my v2 commit from:

   if (!last_boot_successful &&
           test_bit(type, &good_reset_src_type)) {
                   last_boot_successful = true;
   }


to basically:

   if (!test_bit(type, &BAD_reset_src_type)) {
                   bootchooser_target_set_attempts(bc->last_chosen, -1);
   }

In that case, we would only reset the attempts once we have determined
that no "bad reset" has happened. Of course, that requires to gather all
unwanted reset modes and to list them in the barebox env, but the effect
on the result would be the same and we could avoid thinking about
uncertain goodness.
Scenarios like the combination of bad reset and blackout would of course
still slip through, but to me it seems like a more logical approach.

I would then further suggest to rework the concept of the variable
"last_boot_successful", as it is currently not used anyway - other than
to be set to "false" by the original code. Maybe "bad_reset_assumed"?


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

I thoroughly thought about this alternative. This might in general work
for my case, but I am not sure if the necessary changes to the code are
justified by the outcome:

We currently have the array "reset_attempts_names", which only holds the
entries "power-on" and "all-zero". As we want to be able to work with
any reset reason, we would have the respective array "reset_src_names"
(from include/reset_source.c/h) to be merged into that. As of my
understanding, this would either mean to

   - manually mirror all entries from "reset_src_names" into
     "reset_attempts_names" directly within the source code, or

   - memcpy "reset_src_names" into "reset_attempts_names" during
     bootchooser_init(), which would require to make
     the original "reset_attempts_names" not to be a const anymore.

The behaviour might also become inconsistent: As of now, "power-on" and
"all-zero" lead to ALL boot slots being reset to their default attempts
values.
In contrast to that, the evaluation good/bad reset is meant to only
affect the current active slot.
While it is possible to implement that, I don't think it is a good idea
to have the behaviour of which slots(s) will be reset depend on your
bitmask in such an intransparent way.


Any thoughts on your side?

Regards,
Holger

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



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-10-19 13:46 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-29  8:53 [PATCH v2] bootchooser: honour reset source Holger Assmann
2023-10-04  8:19 ` Sascha Hauer
2023-10-19 13:45   ` Holger Assmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox