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

[Marek Vasut <marek.vasut@mailbox.org>]

On 9/22/25 4:02 PM, Michal Simek wrote:
> 
> 
> On 9/11/25 07:40, Michal Simek wrote:
>>
>>
>> On 9/11/25 05:28, Marek Vasut wrote:
>>> On 9/10/25 9:26 AM, Michal Simek wrote:
>>>> In case of ENV_INVALID (uninitialized variables) the first env location
>>>> should be used for storing variables.
>>>> That's why change the logic how env_valid is setup to be aligned with
>>>> offset calculation.
>>>> This will also fix the print about Valid environment is showing proper
>>>> location where variables are saved for the first time.
>>>> And also fixes behavior where the first two writes were going to the
>>>> first location instead of the first to the first location and second to
>>>> second location.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Simplify commit message
>>>>
>>>> Origin commit message with more technical details.
>>>>
>>>> When both location for variables are not valid (or empty) gd- 
>>>> >env_valid is
>>>> 0 (ENV_INVALID) which is setup by
>>>> env_sf_load()/env_import_redund()/env_check_redund() calls.
>>>>
>>>> When saveenv is called in case of SPI env_sf_save() is called which
>>>> contains logic which describes new/old variable locations.
>>>>
>>>> if (gd->env_valid == ENV_VALID) {
>>>>          env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>>          env_offset = CONFIG_ENV_OFFSET;
>>>> } else {
>>>>          env_new_offset = CONFIG_ENV_OFFSET;
>>>>          env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> }
>>>>
>>>> In case of ENV_INVALID option the first location is used (else part)
>>>> and variables are saved with ENV_REDUND_ACTIVE flag.
>>>> The second location flag is rewritten to ENV_REDUND_OBSOLETE.
>>>>
>>>> And
>>>> gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>>> is executed. env_valid is ENV_INVALID that's why ENV_REDUND is setup
>>>> but that's not correct because the first location has been written 
>>>> and not
>>>> the second one.
>>>> That is confirmed by
>>>> printf("Valid environment: %d\n", (int)gd->env_valid);
>>>> which is showing number 2 (ENV_REDUND).
>>>>
>>>> That's why change the logic how env_valid is setup to be aligned with
>>>> offset calculation which also cover the case where initial state is
>>>> ENV_INVALID.
>>>> Then print about Valid environment is showing proper location where
>>>> variables are saved for the first time.
>>>> But also it is fixing behavior where the first two writes are going 
>>>> to the
>>>> first location instead of the first to the first location and second to
>>>> second location. Alternation is happening after it.
>>>>
>>>> v1: https://lore.kernel.org/ 
>>>> r/03aa59f72f47fe0ecafd1e1ca52cbb8a5a8f0446.1755613540.git.michal.simek@amd.com
>>> Sorry for not being clear, the original commit message structure and 
>>> technical details were fine, it only needed slight rephrasing, in the 
>>> original form it was very hard to understand. Can you please 
>>> reinstate the original commit message and only clean it up slightly ?
>>
>> Can you exactly point me to part which wasn't clear? It has a lot of 
>> details about flow and obviously it makes sense to me that's why I 
>> would like you to point me to exact part which should be improved.
> 
> What about this?
> 
> When both SPI environment locations are invalid (gd->env_valid == 
> ENV_INVALID), the first call to saveenv writes to the primary location 
> and sets the active flag. However, the logic for updating gd->env_valid 
> incorrectly sets it to ENV_REDUND, which does not match the actual 
> location written. This causes the first two writes to target the same 
> location, and alternation only begins after the second write.
> 
> Update the logic to alternate gd->env_valid based on whether the last 
> write was to the primary or redundant location, ensuring the first write 
> sets ENV_VALID and subsequent writes alternate as expected. This aligns 
> env_valid with the actual storage location and fixes the alternation 
> sequence from the first write.
> 
> With this change, the "Valid environment" printout correctly reflects 
> the active location after each save, and the alternation between primary 
> and redundant locations works as intended from the start.
Yes, perfect.