On Monday 05 December 2022 12:42:44 Stefan Roese wrote:
> Hi!
>
> On 12/4/22 11:39, Pali Rohár wrote:
> > Hello!
> >
> > I would suggest to change description of the patch from
> >
> > "mvebu: fix end-of-array check"
> >
> > to something like:
> >
> > "arm: mvebu: Espressobin: fix end-of-array check in env"
> >
> > as it affects only Espressobin boards (not other mvebu).
>
> Yes, please update the commit subject here.
>
> > Stefan, please look below as this issue/fix is important.
>
> Yes.
>
> > On Wednesday 30 November 2022 13:33:40 Derek LaHousse wrote:
> > > Properly seek the end of default_environment variables.
> > >
> > > The current algorithm overwrites from the second variable. This
> > > replacement finds the end of the array of strings.
> > >
> > > Stomped variables include "board", "soc", "loadaddr". These can be
> > > seen on a "env default -a" after patch, but they are not seen with a
> > > version before the patch.
> >
> > This is a real issue which I introduced in the past. So it some fix for
> > this issue should be pulled into the v2023.01 release.
>
> Understood.
>
> > > Signed-off-by: Derek LaHousse <de...@seaofdirac.org>
> > > ---
> > > board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/board/Marvell/mvebu_armada-37xx/board.c
> > > b/board/Marvell/mvebu_armada-37xx/board.c
> > > index c6ecc323bb..ac29ac5b95 100644
> > > --- a/board/Marvell/mvebu_armada-37xx/board.c
> > > +++ b/board/Marvell/mvebu_armada-37xx/board.c
> > > @@ -100,8 +100,11 @@ int board_late_init(void)
> > > return 0;
> > > /* Find free buffer in default_environment[] for new variables
> > > */
> > > - while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
> > > - ptr += 2;
> > > + if (*ptr != '\0') { // Defending against empty default env
> > > + while ((i = strlen(ptr)) != 0) {
> > > + ptr += i + 1;
> > > + }
> > > + }
> >
> > If I'm looking at the outer-if condition correctly then it is not
> > needed. strlen("") returns zero and so inner-while loop stops
> > immediately.
> >
> > My proposed fix for this issue was just changing correct while loop
> > check to ensure that ptr is set after the _last_ variable.
> >
> > - while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
> > - ptr += 2;
> > + while (*ptr != '\0' || *(ptr+1) != '\0') ptr++;
> > + ptr++;
> >
> > Both changes should be equivalent, but I'm not sure which one is more
> > readable. The original issue was introduced really by non-readable
> > code...
> >
> > Stefan, do you have a preference which one fix is better / more
> > readable?
>
> I would prefer to get Pali's corrected version included. Could you
> please prepare a v2 patch with this update and also with the added
> or changed patch subject.
Originally this issue was reported half year ago on the armbian forum:
https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=138136
U-Boot "changed" variable name scriptaddr to criptaddr (without leading
s) and broke booting.
Details are later in comments:
https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154062
https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154235
If you prefer my variant, I can prepare a v2 patch.
Please let me know.
> > It would be really a good idea if you try to check if any of those
> > proposed fixes/patches are now really correct.
>
> Yes, please do.
>
> Thanks,
> Stefan
