[PATCH] env: spi: Fix gd->env_valid for the first write

Tue Sep 9 04:24:10 CEST 2025

On 8/21/25 8:47 AM, Michal Simek wrote:
> 
> 
> On 8/20/25 22:59, Marek Vasut wrote:
>> On 8/19/25 4:25 PM, Michal Simek wrote:
>> [...]
>>
>>> ZynqMP> mtd read "U-Boot storage variables" 1000
>>
>> It would be better to spray the 0x1000 area with pattern first, 
>> otherwise, if the 'mtd read' fails, such failure might go undetected.
> 
> can be done.
> 
>>
>>> Reading 131072 byte(s) at offset 0x00000000
>>> ZynqMP> md 1000
>>> 00001000: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001010: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001020: ffffffff ffffffff ffffffff ffffffff  ................
>>
>> [...]
>>
>>> ZynqMP> savee
>>
>> "env save" is the modern equivalent.
>>
>>> Saving Environment to SPIFlash... Erasing SPI flash...Writing to SPI 
>>> flash... offset 2200000
>>> done
>>> Valid environment: 2
>>> OK
>>> ZynqMP> mtd read "U-Boot storage variables" 1000
>>> Reading 131072 byte(s) at offset 0x00000000
>>
>> This here looks odd:
>> - Where is the "offset 2200000" print coming from ?
> 
> I had additional debugs in the code when this output was created.
> 
> diff --git a/env/sf.c b/env/sf.c
> index 0b70e18b9afa..961e853eaef7 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -127,7 +127,7 @@ static int env_sf_save(void)
>          if (ret)
>                  goto done;
> 
> -       puts("Writing to SPI flash...");
> +       printf("Writing to SPI flash... offset %x\n", env_new_offset);
> 
> 
> 
> 
>> - The valid environment is indicated to be 2 , shouldn't that be 1
>>    (primary copy = 1, redundant copy = 2) ?
> 
> yes it should be 1 but this is the code before this patch is applied and 
> it is saying 2 but actual write is happening to 1.
> 
> 
>> - The dump of what looks like primary copy also seems valid, so
>>    shouldn't Valid environment be 1 (see previous bullet)
> 
> yes - dump is valid because the first write is happening to primary copy 
> but logic itself is saying that it was saved to redundant copy. And 
> that's exactly what my patch is fixing.
> 
>>
>>> ZynqMP> md 1000
>>> 00001000: 5d691224 63726101 72613d68 7561006d  $.i].arch=arm.au
>>> 00001010: 6f6c6f74 6e3d6461 6162006f 61726475  toload=no.baudra
>>> 00001020: 313d6574 30323531 6f620030 3d647261  te=115200.board=
>>> 00001030: 716e797a 6200706d 6472616f 6e616d5f  zynqmp.board_man
>>> 00001040: 63616675 65727574 49583d72 584e494c  ufacturer=XILINX
>>> 00001050: 616f6200 6e5f6472 3d656d61 4b2d4d53  .board_name=SM-K
>>> 00001060: 582d3632 47324c43 44452d43 616f6200  26-XCL2GC-ED.boa
>>> 00001070: 725f6472 423d7665 616f6200 735f6472  rd_rev=B.board_s
>>> 00001080: 61697265 30353d6c 42323735 46313131  erial=50572B111F
>>> 00001090: 62004832 6472616f 7465735f 723d7075  2H.board_setup=r
>>> 000010a0: 64206374 30207665 797a203b 706d716e  tc dev 0; zynqmp
>>> 000010b0: 696d6d20 72775f6f 20657469 46467830   mmio_write 0xFF
>>> 000010c0: 30304143 30203031 66666678 203b3020  CA0010 0xfff 0;
>>> 000010d0: 74206669 20747365 61637b24 5f316472  if test ${card1_
>>> 000010e0: 656d616e 203d207d 2d4b4353 472d564b  name} = SCK-KV-G
>>> 000010f0: 6874203b 72206e65 74206e75 6b5f6d70  ; then run tpm_k
>>> ZynqMP> mtd read "U-Boot storage variables backup" 1000
>>> Reading 131072 byte(s) at offset 0x00000000
>>> ZynqMP> md 1000
>>> 00001000: ffffffff ffffff00 ffffffff ffffffff  ................
>>> 00001010: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001020: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001030: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001040: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001050: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001060: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001070: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001080: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001090: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010a0: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010b0: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010c0: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010d0: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010e0: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010f0: ffffffff ffffffff ffffffff ffffffff  ................
>>> ZynqMP> savee
>>> Saving Environment to SPIFlash... Erasing SPI flash...Writing to SPI 
>>> flash... offset 2200000
>>> done
>>> Valid environment: 1
>>> OK
>>> ZynqMP> mtd read "U-Boot storage variables" 1000
>>
>> This behavior here looks correct .
> 
> yes for primary yes but secondary should be populated already based on 
> log above but it is not.
> On the first save you should save variables to primary, on second save 
> to secondary.
> 
> As you see from logs. On the first save - the first location is used but 
> u-boot reports that it was saved to 2 redundant location. On second save 
> it is saved to the first location and also said that it is the first.
> 
> But after this patch applied. On the first save - the first is used 
> (unchanged behavior) and it is reported as the first (fix the report). 
> On second saving the second location is used (fix) and it is reported as 
> 2 (fix).
> 
> 
>>
>>> Reading 131072 byte(s) at offset 0x00000000
>>> ZynqMP> md 1000
>>> 00001000: 5d691224 63726101 72613d68 7561006d  $.i].arch=arm.au
>>> 00001010: 6f6c6f74 6e3d6461 6162006f 61726475  toload=no.baudra
>>> 00001020: 313d6574 30323531 6f620030 3d647261  te=115200.board=
>>> 00001030: 716e797a 6200706d 6472616f 6e616d5f  zynqmp.board_man
>>> 00001040: 63616675 65727574 49583d72 584e494c  ufacturer=XILINX
>>> 00001050: 616f6200 6e5f6472 3d656d61 4b2d4d53  .board_name=SM-K
>>> 00001060: 582d3632 47324c43 44452d43 616f6200  26-XCL2GC-ED.boa
>>> 00001070: 725f6472 423d7665 616f6200 735f6472  rd_rev=B.board_s
>>> 00001080: 61697265 30353d6c 42323735 46313131  erial=50572B111F
>>> 00001090: 62004832 6472616f 7465735f 723d7075  2H.board_setup=r
>>> 000010a0: 64206374 30207665 797a203b 706d716e  tc dev 0; zynqmp
>>> 000010b0: 696d6d20 72775f6f 20657469 46467830   mmio_write 0xFF
>>> 000010c0: 30304143 30203031 66666678 203b3020  CA0010 0xfff 0;
>>> 000010d0: 74206669 20747365 61637b24 5f316472  if test ${card1_
>>> 000010e0: 656d616e 203d207d 2d4b4353 472d564b  name} = SCK-KV-G
>>> 000010f0: 6874203b 72206e65 74206e75 6b5f6d70  ; then run tpm_k
>>> ZynqMP> mtd read "U-Boot storage variables backup" 1000
>>> Reading 131072 byte(s) at offset 0x00000000
>>> ZynqMP> md 1000
>>> 00001000: ffffffff ffffff00 ffffffff ffffffff  ................
>>> 00001010: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001020: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001030: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001040: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001050: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001060: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001070: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001080: ffffffff ffffffff ffffffff ffffffff  ................
>>> 00001090: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010a0: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010b0: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010c0: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010d0: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010e0: ffffffff ffffffff ffffffff ffffffff  ................
>>> 000010f0: ffffffff ffffffff ffffffff ffffffff  ................
>>> ZynqMP> savee
>>> Saving Environment to SPIFlash... Erasing SPI flash...Writing to SPI 
>>> flash... offset 2220000
>>> done
>>> Valid environment: 2
>>> OK
>>> ZynqMP> savee
>>> Saving Environment to SPIFlash... Erasing SPI flash...Writing to SPI 
>>> flash... offset 2200000
>>> done
>>> Valid environment: 1
>> It would be good to validate that both copies of the env are actually 
>> written where they are supposed to be written . This part is missing 
>> from this test. And in fact -- it would be good if this fix did have a 
>> proper test .
> 
> I can validate it again but even now log is too long now.
> It is just demonstration that at start you get two saves to the same 
> location. When 2 saves happens behavior is correct.
> I think it will be good if you can try it on any platform which this 
> enabled to see if you can demonstrate the same problem.
> 
> Love: Can you please take a look at if this can be added to pytest?
Sorry for the delay.

The change looks valid. Test would be real good to have. Commit message 
could really use improvement/rewrite, because that one is really confusing.