mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Holger Assmann <h.assmann@pengutronix.de>
To: Sascha Hauer <sha@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH v2] bootchooser: honour reset source
Date: Thu, 19 Oct 2023 15:45:11 +0200	[thread overview]
Message-ID: <03c3b0c8-a286-4262-9085-961308264f18@pengutronix.de> (raw)
In-Reply-To: <20231004081914.GK637806@pengutronix.de>

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 |



      reply	other threads:[~2023-10-19 13:46 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-29  8:53 Holger Assmann
2023-10-04  8:19 ` Sascha Hauer
2023-10-19 13:45   ` Holger Assmann [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=03c3b0c8-a286-4262-9085-961308264f18@pengutronix.de \
    --to=h.assmann@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=sha@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox