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

[E Shattow]

On 9/22/25 07:51, Marek Vasut wrote:
> 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 at 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 at 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.

nit, the use of word "alternation" here reads as splitting (no
duplication) the data one-by-one into multiple targets. I would give the
game analogy of 52 playing cards and alternating to deal the cards to
players, each player gets one card in a sequence of alternation; if
there is more than one player in the game then no player will get all
the cards from the action of dealing the cards.

>>
>> 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.

I do not understand what this description is saying because alternation
of write operations to multiple targets would not be redundant.
Alternation with the meaning of "fail-over" is similarly not redundant.

An alternative method has the meaning of "do something else, do not do
this". In the common use, an alternative may have the meaning of "do
something else if needed as fail-over from what is first being
attempted". Do you intend for the meaning of "fail-over" and "duplicate"
or "duplication" where you have written "alternate" and "alternation"?

-E