Pali

cc: @ManoftheSea @y52 you can be interested in this.

Just in case if somebody want to test or debug this issue please let me know...

Ideally check if this issue with that disk happens also on some x86 computer. In any case, you can report this issue to the linux-nvme at lists.infradead.org address and ask for help there.

Upstream u-boot (in its next branch) has now lot of 32-bit armada / mvebu patches related to SD/eMMC, SATA and UART booting. Also on mailing list are waiting some Clearfog eMMC patches. Likely you should take Helios patches and put then on top of the u-boot next.

There were problems with PCIe bus on A3720 and with lot of patches to pci-aardvark.c PCIe controller driver it could be more stable. About year or two ago I tested in Espressobin 3 or 4 randomly chosen PCIe-based NVMe M.2 disks which I bought in local shop and all worked fine. From your log it looks like an NVMe error, not the PCIe. So I guess it would not be related to PCIe bus (and its wiring or adapter). So try to choose different NVMe disk. Maybe you hit some NVMe related issue...

Perfect, thanks!

It is fine. I sent you private forum message with updated commit description (with tags) and comments.

I see that I used _wrong_ compiler - 32-bit ARM and so all my pastes are in 32-bit ARM assembler. Not in 64-bit ARM which is Espressobin. But I think you have figured it out.

I have two reasons for extra terminating nul byte: 1. If it had set then this bug would have appeared because it would have immediatelly affected all espressobin boards just by env default -a comment. 2. In case in future env allocation changes from static to dynamic (e.g. malloc-ed) then code without extra terminating nul byte cause problems because dynamic malloced memory does not have to be zeroed. Espressobin is currently the only one board which set/changes default env variables, so any such such would affect only espressobin board. And U-Boot developers do not test every core U-Boot change on Espressobin. So adding extra terminating nul byte is only for future changes in (core) u-boot code to catch possible bugs or regressions. In my opinion it is better to not expect that undefined/uninitialized data would be always zero bytes. And if other code _requires_ that after the last variable must be extra nul byte then it is better to always set it. But you are right that currently it is not needed at all and code would be correct even without it (until somebody changes it). Another option to prevent future regression in this area is to write (unit) tests which can be run during u-boot CI testing without need to have any ARM/espressobin HW. Personally I would really love to see this test but as I looked at the code it is written in such way that unit test is currently impossible without refactoring whole code which sets those default env variables. And I think it does not make sense to do this big refactoring as there was proposed better API for setting default env variables at runtime (I pointed it in u-boot email discussion as link to other patch series). In my opinion it would take same time to rewrite to the that new better API and refactoring code with ability to write unit tests. That is why I'm not going to that refactor -- but anybody else can do it and I think nobody would be against it. How to compile test.c source code to assembly file test.s: Just call gcc with -S argument, it will create file with extension small .s instead of binary a.out. You can also specify -o. So for example: arm-linux-gnueabi-gcc -Os -S test.c -o test.s And which version to choose? If we do not have an agreement what is better then the best is to ask developer responsible for u-boot platform code (which espressobin is part of), who would take and merge that patch and who is maintainer, take care to have code in a good shape and can make decision. Maintainer for mvebu is Stefan. I will send an email and ask for advice.

So... if you are not against, I would propose to combine your last change with my change and send it as patch v2 with updated description.

Should not happen, but ok, some check could be there. Same as for adding trailing '\0' after filling all env variables.

I checked my version: char *test1(char *ptr) { while (*ptr != '\0' || *(ptr+1) != '\0') ptr++; ptr++; return ptr; } And your version: #define strlen(p) __builtin_strlen(p) char *test2(char *ptr) { int i; if (*ptr != '\0') { while (i = strlen(ptr)) { ptr += i + 1; } } return ptr; } And for my version gcc generates more efficient code: test1: .L2: mov r3, r0 ldrb r2, [r3] @ zero_extendqisi2 add r0, r0, #1 cmp r2, #0 bne .L2 ldrb r2, [r0] @ zero_extendqisi2 cmp r2, #0 bne .L2 bx lr For your version gcc generates: test2: push {r4, lr} ldrb r3, [r0] @ zero_extendqisi2 mov r4, r0 cmp r3, #0 bne .L6 .L7: mov r0, r4 pop {r4, pc} .L8: add r0, r0, #1 add r4, r4, r0 .L6: mov r0, r4 bl strlen(PLT) cmp r0, #0 bne .L8 b .L7 I compiled it with arm-linux-gnueabi-gcc -Os -S test.c I specially put there __builtin_strlen so gcc would have a chance to optimize that loop. But neither with -O2 nor with -O3 it was able to eliminate it. On the other hand for my code, gcc was able to figure out that every byte is reading two times and it is not neceserry and it can be optimized to read every byte just once. So it even generate shorter code when you compare number of instructions (important for u-boot which has space limitation). So it is interesting to know this kind of information. And I'm surprised that it also eliminated the final ptr++ statement.

If you need help then please let me know. And ideally CC to my (kernel) address.

No, because printenv sort variables by name. But in default_environment[] they are not sorted. For example distro_bootcmd= is after the scriptaddr= and preboot= is before arch=

CONFIG_EXTRA_ENV_SETTINGS is put at the end of the default_environment[] array. So if you start overwritting default_environment[] in the middle it could result that you overwrite also first byte of the "scriptaddr" string as it is at the beginning of the CONFIG_EXTRA_ENV_SETTINGS, which means it is near the end of the default_environment[]. And as I described in some previous post, due to wrong logic of finding free space in default_environment[], it can really happen.