[U-Boot] [PATCH v1 1/1] mpc512x: adjust and improve AC14xx board support

[Gerhard Sittig]

On Mon, Jun 03, 2013 at 15:31 +0200, Stefano Babic wrote:
> 
> On 03/06/2013 13:10, Gerhard Sittig wrote:
> 
> > -	s = getenv("install_in_progress");
> > +	s = getenv("want_recovery");
> >  	if ((s != NULL) && (*s != '\0')) {
> > -		printf("previous installation aborted, running RECOVERY\n");
> > +		printf("detected recovery request (environment)\n");
> >  		want_recovery = 1;
> >  	}
> > -	s = getenv("install_failed");
> > +	s = getenv("install_in_progress");
> >  	if ((s != NULL) && (*s != '\0')) {
> > -		printf("previous installation FAILED, running RECOVERY\n");
> > +		printf("previous installation has not completed\n");
> >  		want_recovery = 1;
> >  	}
> > -	s = getenv("want_recovery");
> > +	s = getenv("install_failed");
> 
> I am asking myself if it is strictly required to have multiple variables
> to identify if the "recovery" script must be run or not. If a previous
> install was interrupted ("install_in_progress"
> ), or a request was initiated ("want recovery") or the last installation
> failed, is not so important. In all cases you set the want_recovery flag
> and starts the script. But using multiple variables  it is not atomic,
> and could be, due for example to a system reset, that a variable is set
> without clearing the other one.
> 
> Why is not better to set a single variable, maybe with multiple values ?
> Value could be a simple integer (0=no recovery,
> 1=install_in_progress,,..) or still a string, if you prefer this solution.

Quick answer:  holy compatibility with shipped products on one
hand, and simplicity on the initiators' sides on the other hand.
The full answer is more involved than one may think at first
glance, as usual. :)


There are at least three (or four depending on how you count it)
parties involved:  The installer within the recovery system, the
user in front of the product, and the regular application
software including potential remote access (all of them are
"initiators", and the bootloader (who is trying to help or to
mediate between the former).

Some requests are carried out willingly (the user's pressing
keys, the regular product software's or the remote access'
wanting to enter maintenance mode), others are not so much of a
willingly initiated request but instead mere consequence of
failure (the installer's not being able to finish or to succeed).
Some requests are done interactively in a moment (the user's
pressing buttons during power on), others get queued and acted
upon at any random later point in time (scheduling to enter
maintenance mode but not before the next reboot).  Notice also
that the three parts (boot loader, recovery system, product
software) may get developed and maintained by individual teams or
under differing circumstances (regarding the targetted set of
feature or supported complexity, differing schedules for releases
or updates, maybe even locks or freezes after first shipping).

The current approach is appropriate because you don't have to
coordinate the individual initiators, they don't have to agree on
one common way of expressing their request, they don't even have
to know about each other.

The evaluation of reasons in the boot loader is simple and
straight forward and just errs to the safe side.  The evaluation
may look redundant yet the cost is acceptable and the complexity
is rather low (totally cheap, mere OR, nothing else involved when
you ignore the diagnostics).

Any pending reason will make the product enter maintenance mode,
and not change any of the information which led there.
Processing the interactive request upon key press or remote
support intervention won't clobber the information about
potentially incomplete or failed installation attempts.  Error
conditions will stick as long as they apply, without further
logic needed.  The product keeps nagging and enforces that a
working status gets established (software gets installed,
completely and successfully verified please), while additional
manual intervention is possible as the need arises (one time
access to the recovery system, not voiding any of the other
information which may apply).

I feel that this is a good thing, simple to explain and to
understand (once you know the motivation), and to test and to
verify, and to maintain and support.

Introducing a uniform way of carrying the "start recovery mode,
please" request interestingly increases complexity, you need to
coordinate all the initiators.  Having the one variable hold only
one reason for the request opens a new area of unwanted
interaction, having the variable hold multiple reasons introduces
complex manipulation activities and may make you need to update
all initiators when the logic changes or gets extended.
Manipulating the variable instead of just setting or deleting it
may introduce atomicity issues, unless you add complexity by
adding locking (which all participants have to agree upon and
respect).


So I see the desire to reduce the redundancy, but I'm afraid that
this comes at the cost of increased complexity.  Which may turn
out to not be a benefit.

Are there other projects and experiences to learn from?


> > -	"rootpath=/opt/eldk/ppc_6xx\n"					\
> > +	"rootpath=/opt/eldk/ppc_6xx\0"					\
> 
> We do not set IP addresses or fix rootpath. Someone could have installed
> the rootfs on NFS on a different path. Also, IMHO rootpath should be
> simply removed.

Ah, removing the 'rootpath' spec (from the builtin set that is,
still allowing for specs in user provided environments)
eliminates my problem discussed in the other thread.  Will have
to ponder this for a moment.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de