Message ID | 1370257851-27583-1-git-send-email-gsi@denx.de |
---|---|
State | Superseded |
Delegated to: | Wolfgang Denk |
Headers | show |
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
diff --git a/board/ifm/ac14xx/ac14xx.c b/board/ifm/ac14xx/ac14xx.c index 7442591..e47f63d 100644 --- a/board/ifm/ac14xx/ac14xx.c +++ b/board/ifm/ac14xx/ac14xx.c @@ -23,6 +23,10 @@ #include <i2c.h> #endif +static int eeprom_diag; +static int mac_diag; +static int gpio_diag; + DECLARE_GLOBAL_DATA_PTR; static void gpio_configure(void) @@ -37,7 +41,7 @@ static void gpio_configure(void) /* * out_be32(&gpioregs->gpdir, 0xC2293020); - * workaround for a hardware affect: configure direction in pieces, + * workaround for a hardware effect: configure direction in pieces, * setting all outputs at once drops the reset line too low and * makes us lose the MII connection (breaks ethernet for us) */ @@ -126,8 +130,6 @@ static u32 gpio_querykbd(void) /* excerpt from the recovery's hw_info.h */ -static int eeprom_diag = 1; - struct __attribute__ ((__packed__)) eeprom_layout { char magic[3]; /** 'ifm' */ u8 len[2]; /** content length without magic/len fields */ @@ -230,8 +232,8 @@ int mac_read_from_eeprom(void) if (mac && is_valid_ether_addr(mac)) { eth_setenv_enetaddr("ethaddr", mac); - printf("DIAG: %s() MAC value [%s]\n", - __func__, getenv("ethaddr")); + if (mac_diag) + printf("DIAG: MAC [%s]\n", getenv("ethaddr")); } return 0; @@ -326,42 +328,38 @@ int misc_init_r(void) gpio_configure(); /* - * check the GPIO keyboard, - * enforced start of the recovery when + * enforce the start of the recovery system when * - the appropriate keys were pressed - * - a previous installation was aborted or has failed * - "some" external software told us to + * - a previous installation was aborted or has failed */ want_recovery = 0; keys = gpio_querykbd(); - printf("GPIO keyboard status [0x%08X]\n", keys); - /* XXX insist in the _exact_ combination? */ + if (gpio_diag) + printf("GPIO keyboard status [0x%02X]\n", keys); if ((keys & GPIOKEY_BITS_RECOVERY) == GPIOKEY_BITS_RECOVERY) { - printf("GPIO keyboard requested RECOVERY\n"); - /* XXX TODO - * refine the logic to detect the first keypress, and - * wait to recheck IF it was the recovery combination? - */ + printf("detected recovery request (keyboard)\n"); want_recovery = 1; } - 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"); if ((s != NULL) && (*s != '\0')) { - printf("running RECOVERY according to the request\n"); + printf("previous installation has failed\n"); want_recovery = 1; } - - if (want_recovery) + if (want_recovery) { + printf("enforced start of the recovery system\n"); setenv("bootcmd", "run recovery"); + } /* * boot the recovery system without waiting; boot the diff --git a/include/configs/ac14xx.h b/include/configs/ac14xx.h index 7cb10fb..d24d014 100644 --- a/include/configs/ac14xx.h +++ b/include/configs/ac14xx.h @@ -72,7 +72,7 @@ #define CONFIG_SYS_MAX_RAM_SIZE 0x20000000 /* - * DDR Controller Configuration XXX TODO + * DDR Controller Configuration * * SYS_CFG: * [31:31] MDDRC Soft Reset: Diabled @@ -265,7 +265,6 @@ /* * CS related parameters - * TODO document these */ /* CS0 Flash */ #define CONFIG_SYS_CS0_CFG 0x00031110 @@ -331,8 +330,6 @@ #endif #define CONFIG_BAUDRATE 115200 /* ... at 115200 bps */ -#define CONFIG_SYS_BAUDRATE_TABLE \ - {300, 600, 1200, 2400, 4800, 9600, 19200, 38400, 115200} #define CONSOLE_FIFO_TX_SIZE FIFOC_PSC3_TX_SIZE #define CONSOLE_FIFO_TX_ADDR FIFOC_PSC3_TX_ADDR @@ -506,21 +503,21 @@ #define CONFIG_BOOTDELAY 2 /* -1 disables auto-boot */ -/* XXX TODO need to specify the builtin environment */ +/* the builtin environment and standard greeting */ #define CONFIG_PREBOOT "echo;" \ "echo Type \\\"run flash_nfs\\\" to mount root filesystem over NFS;" \ "echo" #define CONFIG_EXTRA_ENV_SETTINGS_DEVEL \ - "muster_nr=00\0" \ + "muster_nr=-00\0" \ "fromram=run ramargs addip addtty; " \ - "tftp ${fdt_addr_r} k6m2/ac14xx.dtb-${muster_nr}; " \ - "tftp ${kernel_addr_r} k6m2/uImage-${muster_nr}; " \ - "tftp ${ramdisk_addr_r} k6m2/uFS-${muster_nr}; " \ + "tftp ${fdt_addr_r} ac14xx/ac14xx.dtb${muster_nr}; " \ + "tftp ${kernel_addr_r} ac14xx/uImage${muster_nr}; " \ + "tftp ${ramdisk_addr_r} ac14xx/uFS${muster_nr}; " \ "bootm ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r}\0" \ "fromnfs=run nfsargs addip addtty; " \ - "tftp ${fdt_addr_r} k6m2/ac14xx.dtb-${muster_nr}; " \ - "tftp ${kernel_addr_r} k6m2/uImage-${muster_nr}; " \ + "tftp ${fdt_addr_r} ac14xx/ac14xx.dtb${muster_nr}; " \ + "tftp ${kernel_addr_r} ac14xx/uImage${muster_nr}; " \ "bootm ${kernel_addr_r} - ${fdt_addr_r}\0" \ "fromflash=run nfsargs addip addtty; " \ "bootm fc020000 - fc000000\0" \ @@ -548,12 +545,12 @@ "u-boot=ac14xx/u-boot.bin\0" \ "bootfile=ac14xx/uImage\0" \ "fdtfile=ac14xx/ac14xx.dtb\0" \ - "rootpath=/opt/eldk/ppc_6xx\n" \ + "rootpath=/opt/eldk/ppc_6xx\0" \ "netdev=eth0\0" \ "consdev=ttyPSC0\0" \ "hostname=ac14xx\0" \ "nfsargs=setenv bootargs root=/dev/nfs rw " \ - "nfsroot=${serverip}:${rootpath}-${muster_nr}\0" \ + "nfsroot=${serverip}:${rootpath}${muster_nr}\0" \ "ramargs=setenv bootargs root=/dev/ram rw\0" \ "addip=setenv bootargs ${bootargs} " \ "ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}" \ @@ -583,6 +580,8 @@ #define CONFIG_BOOTCOMMAND "run production" +#define CONFIG_ARP_TIMEOUT 200UL + #define CONFIG_FIT 1 #define CONFIG_OF_LIBFDT 1 #define CONFIG_OF_BOARD_SETUP 1
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