Dear Kumar Gala, In message <1294604972-24751-1-git-send-email-ga...@kernel.crashing.org> you wrote: > There are several users of the hwconfig APIs (8xxx DDR) before we have > the environment properly setup. This causes issues because of the > numerous ways the environment might be accessed because of the > non-volatile memory it might be stored in. Additionally the access > might be so early that memory isn't even properly setup for us. > > Towards resolving these issues we provide versions of all the hwconfig > APIs that can be passed in a buffer to parse and leave it to the caller > to determine how to allocate and populate the buffer. > > We use the _f naming convention for these new APIs even though they are > perfectly useable after relocation and the environment being ready. > > We also now warn if the non-f APIs are called before the environment is > ready to allow users to address the issues. > > Finally, we convert the 8xxx DDR code to utilize the new APIs to > hopefully address the issue once and for all. We have the 8xxx DDR code > create a buffer on the stack and populate it via getenv_f(). ...
I would like to suggest a few changes. > @@ -24,6 +32,15 @@ unsigned int populate_memctl_options(int > all_DIMMs_registered, > unsigned int ctrl_num) > { > unsigned int i; > + char buffer[HWCONFIG_BUFFER_SIZE]; > + char *buf = NULL; > + > + /* > + * Extract hwconfig from environment since we have not properly setup > + * the environment but need it for ddr config params > + */ > + if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0) > + buf = buffer; If we already know that there is no "hwconfig" setting in the environment, why do we then need to go through all the hwconfig_sub_f() and hwconfig_subarg_cmp_f() calls below? Can we not short-cut all these? > #ifdef CONFIG_DDR_SPD > + char buffer[HWCONFIG_BUFFER_SIZE]; > + char *buf = NULL; > + > + /* > + * Extract hwconfig from environment since we have not properly setup > + * the environment but need it for ddr config params > + */ > + if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0) > + buf = buffer; Ditto here. > diff --git a/common/hwconfig.c b/common/hwconfig.c > index 193863a..f909fa5 100644 > --- a/common/hwconfig.c > +++ b/common/hwconfig.c ... > -static const char *__hwconfig(const char *opt, size_t *arglen) > +static const char *__hwconfig(const char *opt, size_t *arglen, char *buf) > { > const char *env_hwconfig = NULL, *ret; > - char buf[HWCONFIG_PRE_RELOC_BUF_SIZE]; > > - if (gd->flags & GD_FLG_ENV_READY) { > - env_hwconfig = getenv("hwconfig"); > + /* if we are passed a buffer use it, otherwise try the environment */ > + if (!buf) { > + if (gd->flags & GD_FLG_ENV_READY) { > + env_hwconfig = getenv("hwconfig"); > + } else { > + printf("WARNING: Calling __hwconfig without a buffer " > + "and before environment is ready\n"); > + return NULL; > + } > } else { > - /* > - * Use our own on stack based buffer before relocation to allow > - * accessing longer hwconfig strings that might be in the > - * environment before we've relocated. This is pretty fragile > - * on both the use of stack and if the buffer is big enough. > - * However we will get a warning from getenv_f for the later. > - */ > - if ((getenv_f("hwconfig", buf, sizeof(buf))) > 0) > - env_hwconfig = buf; > + env_hwconfig = buf; > } Do we need "buf" at all? Make this: static const char *__hwconfig(const char *opt, size_t *arglen, const char *env_hwconfig) ... if (!env_hwconfig) { if (!(gd->flags & GD_FLG_ENV_READY)) { printf("WARNING: Calling __hwconfig without a buffer " "and before environment is ready\n"); return NULL; } env_hwconfig = getenv("hwconfig"); } May I ask what is the size impact of this change (i. e. all the additional parameter passing) ? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The amount of time between slipping on the peel and landing on the pavement is precisely 1 bananosecond. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot