Hi. Le lundi 20 juin 2022, 19:33:24 CEST Tom Rini a écrit : > On Mon, Jun 20, 2022 at 04:08:32PM +0000, Holger Brunck wrote: > > > > > 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. > > > > Ok. > > > > > > > > Signed-off-by: Francis Laniel > > > > > > <francis.lan...@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. > > > > Ah yes you are right. But still for the first boot of our board we will > > call saveenv to store an initial environment and with this change it > > would also write down the IVM data which is currently only stored > > temporary in RAM. > > > > > 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. > > > > yes exactly and I wonder if this functionality will be also possible after > > the rework. I mean our use case (even it seems we are the only ones using > > it) is quite useful I think. We read out inventory data from an EEPROM > > and they are parsed and temporary stored in RAM and then we are able to > > use them as any other environment variables without the need to store > > them permanently. I also would like to avoid this as the data should be > > permanently in the IVM only and not stored a second time permanently in > > flash. > > So, I'll start by noting that the environment variable flag support > isn't as well documented as I would like. But my super quick glance > makes me wonder if ENV_FLAGS_VARACCESS_WRITEABLE does what you're > thinking about, and if not, if it would be useful to extend the flag > support to include a "hidden" flag, or otherwise way to indicate that > variables A/B/C should never be saved. This could be useful for other > cases as well.
Again, sorry for the late reply. To be honest, this board is the only one which presents this behavior. I will think about a mechanism to handle your use case. Nonetheless, as you are the exception and this series begins to be a bit big I think in a first time I will not address it. I will just add these patches to make the CI happy, but I will mark them as "do not merge". So, we can merge this series and you can still use the old hush shell, as I do not think we plan about making the new version the default right now. What do you think? Does it seem acceptable for you?