Re: [RFC PATCH v4 28/28] board: keymile: common: Use environment to store IVM_* variables.

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3949 bytes --]

On Mon, Jun 20, 2022 at 03:27:34PM +0000, Holger Brunck wrote:
> Hi Francis,
> 
> > On Fri, Jun 17, 2022 at 12:31:58AM +0200, Francis Laniel wrote:
> > 
> > > These boards used set_local_var() to store some variables as local shell.
> > > They then used get_local_var() to retrieve the variables values.
> > >
> > > Instead of using local shell variables, they should use environment
> > > ones (like a majority of board).
> > > So, this patch converts using local variables to environment ones.
> > >
> 
> why do we need to change that? It is intended that we use this hush variable
> infrastructure from u-boot (common/hush.c) for our IVM data and not the
> standard env. We read the IVM at boot time and store these values in
> RAM. It is not intended to store them permanently in the flash or wherever
> the environment is saved. Especially in our case we have some boards where
> the environment is in a i2c EEPROM and we don't want to write down to
> the EEPROM each time when the board is starting.

So, the whole series is about updating hush to bring in a new baseline
version of the shell, from busybox.

> > > Signed-off-by: Francis Laniel <francis.laniel@amarulasolutions.com>
> > > ---
> > >  board/keymile/common/common.c | 8 ++++----
> > >  board/keymile/common/ivm.c    | 9 +--------
> > >  2 files changed, 5 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/board/keymile/common/common.c
> > > b/board/keymile/common/common.c index 3999f48719..72939af36e 100644
> > > --- a/board/keymile/common/common.c
> > > +++ b/board/keymile/common/common.c
> > > @@ -219,7 +219,7 @@ static int do_setboardid(struct cmd_tbl *cmdtp, int
> > flag, int argc,
> > >  	unsigned char buf[32];
> > >  	char *p;
> > >
> > > -	p = get_local_var("IVM_BoardId");
> > > +	p = env_get("IVM_BoardId");
> > >  	if (!p) {
> > >  		printf("can't get the IVM_Boardid\n");
> > >  		return 1;
> > > @@ -228,7 +228,7 @@ static int do_setboardid(struct cmd_tbl *cmdtp, int
> > flag, int argc,
> > >  	env_set("boardid", (char *)buf);
> > >  	printf("set boardid=%s\n", buf);
> > >
> > > -	p = get_local_var("IVM_HWKey");
> > > +	p = env_get("IVM_HWKey");
> > >  	if (!p) {
> > >  		printf("can't get the IVM_HWKey\n");
> > >  		return 1;
> > > @@ -272,14 +272,14 @@ static int do_checkboardidhwk(struct cmd_tbl
> > *cmdtp, int flag, int argc,
> > >  	 * first read out the real inventory values, these values are
> > >  	 * already stored in the local hush variables
> > >  	 */
> > > -	p = get_local_var("IVM_BoardId");
> > > +	p = env_get("IVM_BoardId");
> > >  	if (!p) {
> > >  		printf("can't get the IVM_Boardid\n");
> > >  		return 1;
> > >  	}
> > >  	rc = strict_strtoul(p, 16, &ivmbid);
> > >
> > > -	p = get_local_var("IVM_HWKey");
> > > +	p = env_get("IVM_HWKey");
> > >  	if (!p) {
> > >  		printf("can't get the IVM_HWKey\n");
> > >  		return 1;
> > > diff --git a/board/keymile/common/ivm.c b/board/keymile/common/ivm.c
> > > index 67db0c50f4..e266d7ce81 100644
> > > --- a/board/keymile/common/ivm.c
> > > +++ b/board/keymile/common/ivm.c
> > > @@ -44,14 +44,7 @@ static int ivm_calc_crc(unsigned char *buf, int
> > > len)
> > >
> > >  static int ivm_set_value(char *name, char *value)  {
> > > -	char tempbuf[256];
> > > -
> > > -	if (value) {
> > > -		sprintf(tempbuf, "%s=%s", name, value);
> > > -		return set_local_var(tempbuf, 0);
> > > -	}
> > > -	unset_local_var(name);
> > > -	return 0;
> > > +	return env_set(name, value);
> 
> this means we are now writing always down to the permanent
> environment or? And this I would really like to avoid in our case.

Note that "env_set" does not force a save of the running environment.
These variables will be exposed to the CLI run-time, which I am not sure
if they are today, so if the user then does "saveenv" they will be
written.  That I think would be a functional difference.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]