Jump to content

Pali

Members
  • Posts

    139
  • Joined

Recent Profile Visitors

The recent visitors block is disabled and is not being shown to other users.

  1. It is fine. I sent you private forum message with updated commit description (with tags) and comments.
  2. 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.
  3. 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.
  4. 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.
  5. Should not happen, but ok, some check could be there. Same as for adding trailing '\0' after filling all env variables.
  6. 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.
  7. If you need help then please let me know. And ideally CC to my (kernel) address.
  8. 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=
  9. 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.
  10. For every eth{,1,2,3}addr (totally 4) is reserved 30 characters. Hence totally there is 4*30+60 = 180 bytes of free space in buffer. sprintf is more complicated function, strcpy is simple and compiler can optimize it (= eliminate its usage) during LTO. Hm.. looking at the code and I'm not sure right now... So pointer must point to the location where new variable starts, so one byte after the nul byte separator. Static r/w global array. This can prove my above hypothesis how criptaddr was generated. With writing this new nul byte there should not be any criptaddr anymore and also no other variable.
  11. Something like this patch: diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c index c6ecc323bb99..0c6ac126d0e7 100644 --- a/board/Marvell/mvebu_armada-37xx/board.c +++ b/board/Marvell/mvebu_armada-37xx/board.c @@ -145,6 +145,8 @@ int board_late_init(void) strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-emmc.dtb"); else strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin.dtb"); + ptr += strlen(ptr) + 1; + *ptr = '\0'; return 0; }
  12. You are right that my patch checks every byte twice, but it should not be an issue as it is still linear. My patch is simple and I prefer simple solution. Your solution is a bit complicated at the cost to read every byte just once. But because it is complicated there is issue with it -- it took me some time to find it -- you are missing ptr--; after the outer while loop otherwise you would have two nul bytes. Anyway, I would really like to hear from somebody if my patch above fixes this criptaddr issue. Or we are still dealing with another hidden problem. I think that I should send this patch to upstream because currenly, if I understand (previous) code correctly, it drops all variables after the first valid variable. And if I'm looking at it, at the end of function is missing code which will add two nul bytes into *ptr as final terminal of variables. Hm... and maybe this could be a reason for criptaddr? First issue that code dropped all variables after the first one and then second issue that final two nul byte terminal is missing which leaded to read garbage of previous bytes in storage (which probably started with criptaddr and continued with valid env structure of remaining variables).
  13. In file include/configs/mvebu_armada-37xx.h is reserved 60 bytes for fdtfile= variable. The longest value has v7-emmc variant with 51 bytes. This should be really enough. Anyway, now I traced the whole init sequence again and I think I found issue in part which tries to find free buffer in environment array. Would you be able to test this change if it helps? Variables are separated by one nul byte and the last variable is termined by two nul bytes. diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c index c6ecc323bb99..73de2a3f002e 100644 --- a/board/Marvell/mvebu_armada-37xx/board.c +++ b/board/Marvell/mvebu_armada-37xx/board.c @@ -100,8 +100,8 @@ 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; + while (*ptr != '\0' || *(ptr+1) != '\0') ptr++; + ptr++; /* * Ensure that 'env default -a' does not erase permanent MAC addresses
  14. I have no reason to not trust you. But also I have absolutely no idea how it can happen. And because I was not able to trigger this issue on two different Espressobin boards, I cannot debug it.
×
×
  • Create New...