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

minor improvements for the 'ifm AC14xx' board setup
- adjust diagnostics (reworded, silent by default)
- re-order the list of "recovery conditions"
- update and improve comments
- adjust the board configuration
  - use the builtin serial baudrate table
  - use the official 'ac14xx' name everywhere (remove 'k6' remainders)
  - fix a NUL termination issue in the rootpath spec
  - rephrase the 'muster_nr' suffix for development network boot
  - reduce the ARP timeout for faster network boot

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 board/ifm/ac14xx/ac14xx.c |   44 +++++++++++++++++++++-----------------------
 include/configs/ac14xx.h  |   25 ++++++++++++-------------
 2 files changed, 33 insertions(+), 36 deletions(-)


all of the modifications only affect the 'ifm AC14xx' board, the changes
were tested and verified on the hardware, but a style related question
remains:

shall I split this patch into a series of tiny patches each addressing a
specific aspect of the whole set, or is the aggregation acceptable since
the modifications are so small?

nothing was broken in the previous implementation given that an external
environment image existed, so the patch is not a fix but just improves
the existing board support -- except for the builtin 'rootpath' spec
which was incorrectly terminated and shadowed the 'netdev' spec, while
both variables only apply to network boot which isn't the default
configuration and exclusively relates to development support

Dear Gerhard,

In message <1370257851-27583-1-git-send-email-gsi@denx.de> you wrote:
>
> +		if (mac_diag)
> +			printf("DIAG: MAC [%s]\n", getenv("ethaddr"));

What happens if "ethaddr" is not defined in the environment?

...
> +	"rootpath=/opt/eldk/ppc_6xx\0"					\

You are really still using ELDK 4.x ?

Best regards,

Wolfgang Denk

Hi Gerhard,

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.

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

Best regards,
Stefano

On Mon, Jun 03, 2013 at 15:07 +0200, Wolfgang Denk wrote:
> 
> In message <1370257851-27583-1-git-send-email-gsi@denx.de> you wrote:
> >
> > +		if (mac_diag)
> > +			printf("DIAG: MAC [%s]\n", getenv("ethaddr"));
> 
> What happens if "ethaddr" is not defined in the environment?

The context is the mac_read_from_eeprom() routine, and the
getenv() call for the message is immediately preceeded by a
setenv() call.

Right, the lookup may fail if the setup failed as well.  That's a
yet uncaught followup failure.  Will include an improved check in
v2 of the patch.

> ...
> > +	"rootpath=/opt/eldk/ppc_6xx\0"					\
> 
> You are really still using ELDK 4.x ?

In this specific project?  Yes!  Although development setups used
to run with both 4.2 and 5.3 here for some time, the evaluation
hasn't comleted yet, so the final switch wasn't made so far.

I'll look into what else is involved in the ELDK-5 switch (using
a mere symlink here from under /opt/eldk/ppc_6xx-sign to
/opt/eldk-5.3/powerpc/rootfs-*), although /opt/eldk itself may
perfectly legally be a symlink to /opt/eldk-5.3 for those setups
which want to switch back and forth.  Wasn't this the usual
approach before ELDK-5?  Somehow I'm reluctant to encode the ELDK
version into the binary, hmm ...


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

Dear Gerhard Sittig,

In message <20130603135004.GH2571@book.gsilab.sittig.org> you wrote:
>
> > > +		if (mac_diag)
> > > +			printf("DIAG: MAC [%s]\n", getenv("ethaddr"));
> > 
> > What happens if "ethaddr" is not defined in the environment?
> 
> The context is the mac_read_from_eeprom() routine, and the
> getenv() call for the message is immediately preceeded by a
> setenv() call.

...which, as everything, can fail for a number of reasons.

It is utterly wrong to rely on results of any previous actions and
assume that nothing can go wrong.  Murphy will teach you.

> > > +	"rootpath=/opt/eldk/ppc_6xx\0"					\
> > 
> > You are really still using ELDK 4.x ?
> 
> In this specific project?  Yes!  Although development setups used
> to run with both 4.2 and 5.3 here for some time, the evaluation
> hasn't comleted yet, so the final switch wasn't made so far.

...which makes clear that setting such variables in the default config
is of questionable value.

Best regards,

Wolfgang Denk

this change set adjusts the configuration of the 'ifm AC14xx' board
to get closer to mainline philosophy and slightly improves builtin
diagnostics and robustness after setup failure


changes in v2:
- address a potential NULL pointer dereference in the diagnostics
  code path as well (previously unhandled in mainline)
- split the previous convoluted v1 patch into a series of
  individual patches, each addressing one specific issue,
  to aid in the review process


Gerhard Sittig (7):
  ac14xx: fix a potential NULL deref in diagnostics
  ac14xx: cleanup comments in the board support
  ac14xx: minor improvement in diagnostics
  ac14xx: re-order the recovery condition checks
  ac14xx: remove obsolete board config items
  ac14xx: use the official product name everywhere
  ac14xx: rephrase network boot config for development

 board/ifm/ac14xx/ac14xx.c |   50 ++++++++++++++++++++++++---------------------
 include/configs/ac14xx.h  |   28 ++++++++++---------------
 2 files changed, 38 insertions(+), 40 deletions(-)

On Wed, Jun 05, 2013 at 14:51 +0200, Gerhard Sittig wrote:
> 
> this change set adjusts the configuration of the 'ifm AC14xx' board
> to get closer to mainline philosophy and slightly improves builtin
> diagnostics and robustness after setup failure
> 
> 
> changes in v2:
> - address a potential NULL pointer dereference in the diagnostics
>   code path as well (previously unhandled in mainline)
> - split the previous convoluted v1 patch into a series of
>   individual patches, each addressing one specific issue,
>   to aid in the review process

Are there any other concerns that I shall address?  Any feedback
beyond what was mentioned before?


virtually yours
Gerhard Sittig