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

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

[Michal Simek]

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
---
 env/sf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/sf.c b/env/sf.c
index 0b70e18b9afa..0e27a020643e 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -148,7 +148,7 @@ static int env_sf_save(void)
 
 	puts("done\n");
 
-	gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
+	gd->env_valid = gd->env_valid == ENV_VALID ? ENV_REDUND : ENV_VALID;
 
 	printf("Valid environment: %d\n", (int)gd->env_valid);
 
-- 
2.43.0

base-commit: 354392a63cf18523a6714d3b486e68dd92ba2ca7
branch: debian-sent3

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

[Marek Vasut]

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 ?

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

[Michal Simek]

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.

Thanks,
Michal

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

[Michal Simek]

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.

Thanks,
Michal

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

[Marek Vasut]

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.

Re: [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@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.

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

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

[Michal Simek]

On 9/23/25 23:18, E Shattow wrote:
> 
> 
> 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@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.
> 
> 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.

tbh before this patch I though that both copies are the same but that's not how 
it is implemented.
It is switching from Location A to B and then to A, etc.

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

How the code is written is that you have location A and B.
And when you call saveenv variables are written to A.
And next saveenv variables are written to B.
Next to A, etc.

It means unless you call saveenv twice you never get data in A and B equal.

It means it is not alternation of write operations to multiple targets.
It is alternation of location where one complete write is done.

> 
> 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"?
I am not a native speaker but the word alternation between A/B sounds the best 
for me.
Feel free to suggest how to describe it better.

Thanks,
Michal

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

[E Shattow]

Hi Michal,

On 9/29/25 00:08, Michal Simek wrote:
> 
> 
> On 9/23/25 23:18, E Shattow wrote:
>>
>>
>> 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@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.
>>
>> 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.
> 
> tbh before this patch I though that both copies are the same but that's
> not how it is implemented.
> It is switching from Location A to B and then to A, etc.
> 

Okay. That is also surprising for me.

>>
>>>>
>>>> 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.
> 
> How the code is written is that you have location A and B.
> And when you call saveenv variables are written to A.
> And next saveenv variables are written to B.
> Next to A, etc.
> 
> It means unless you call saveenv twice you never get data in A and B equal.
> 
> It means it is not alternation of write operations to multiple targets.
> It is alternation of location where one complete write is done.
> 
>>
>> 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"?
> I am not a native speaker but the word alternation between A/B sounds
> the best for me.
> Feel free to suggest how to describe it better.
> 
> Thanks,
> Michal
> 

This is not easy to describe with a small quantity of words. The common
phrases are "taking turns" and/or "round-robin".

It is clearer to say what this is not, as:

"... results in alternation of saveenv location with each invocation.
Data is not duplicated or striped between locations for any single
saveenv invocation."

I like your explanations, you understand the functionality and can
describe the problem clearly. I just do not expect that functionality
would be useful so without additional context and full explanation it
seems unlikely to be what you say it is. Thank you for expanding on the
meaning.

-E

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

[Michal Simek]

On 9/29/25 11:49, E Shattow wrote:
> Hi Michal,
> 
> On 9/29/25 00:08, Michal Simek wrote:
>>
>>
>> On 9/23/25 23:18, E Shattow wrote:
>>>
>>>
>>> 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@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.
>>>
>>> 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.
>>
>> tbh before this patch I though that both copies are the same but that's
>> not how it is implemented.
>> It is switching from Location A to B and then to A, etc.
>>
> 
> Okay. That is also surprising for me.
> 
>>>
>>>>>
>>>>> 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.
>>
>> How the code is written is that you have location A and B.
>> And when you call saveenv variables are written to A.
>> And next saveenv variables are written to B.
>> Next to A, etc.
>>
>> It means unless you call saveenv twice you never get data in A and B equal.
>>
>> It means it is not alternation of write operations to multiple targets.
>> It is alternation of location where one complete write is done.
>>
>>>
>>> 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"?
>> I am not a native speaker but the word alternation between A/B sounds
>> the best for me.
>> Feel free to suggest how to describe it better.
>>
>> Thanks,
>> Michal
>>
> 
> This is not easy to describe with a small quantity of words. The common
> phrases are "taking turns" and/or "round-robin".
> 
> It is clearer to say what this is not, as:
> 
> "... results in alternation of saveenv location with each invocation.
> Data is not duplicated or striped between locations for any single
> saveenv invocation."
> 
> I like your explanations, you understand the functionality and can
> describe the problem clearly. I just do not expect that functionality
> would be useful so without additional context and full explanation it
> seems unlikely to be what you say it is. Thank you for expanding on the
> meaning.

What about this full description?
M

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 round-robin switching only 
begins after the second write.

Be aware that data is not duplicated or striped between locations
for any single saveenv invocation.

Update the logic to perform round-robin selection of the environment location
based on whether the last write was to the primary or redundant location,
ensuring the first write sets ENV_VALID and subsequent writes follow the
round-robin pattern as expected. This keeps gd->env_valid consistent with
the actual storage location and fixes the sequence from the very first write.

With this change, the "Valid environment" printout correctly reflects the
active location after each save, and round-robin switching between primary
and redundant locations works as intended from the start.

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

[E Shattow]

On 9/29/25 05:36, Michal Simek wrote:
> 
> 
> On 9/29/25 11:49, E Shattow wrote:
>> Hi Michal,
>>
>> On 9/29/25 00:08, Michal Simek wrote:
>>>
>>>
>>> On 9/23/25 23:18, E Shattow wrote:
>>>>
>>>>
>>>> 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@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.
>>>>
>>>> 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.
>>>
>>> tbh before this patch I though that both copies are the same but that's
>>> not how it is implemented.
>>> It is switching from Location A to B and then to A, etc.
>>>
>>
>> Okay. That is also surprising for me.
>>
>>>>
>>>>>>
>>>>>> 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.
>>>
>>> How the code is written is that you have location A and B.
>>> And when you call saveenv variables are written to A.
>>> And next saveenv variables are written to B.
>>> Next to A, etc.
>>>
>>> It means unless you call saveenv twice you never get data in A and B
>>> equal.
>>>
>>> It means it is not alternation of write operations to multiple targets.
>>> It is alternation of location where one complete write is done.
>>>
>>>>
>>>> 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"?
>>> I am not a native speaker but the word alternation between A/B sounds
>>> the best for me.
>>> Feel free to suggest how to describe it better.
>>>
>>> Thanks,
>>> Michal
>>>
>>
>> This is not easy to describe with a small quantity of words. The common
>> phrases are "taking turns" and/or "round-robin".
>>
>> It is clearer to say what this is not, as:
>>
>> "... results in alternation of saveenv location with each invocation.
>> Data is not duplicated or striped between locations for any single
>> saveenv invocation."
>>
>> I like your explanations, you understand the functionality and can
>> describe the problem clearly. I just do not expect that functionality
>> would be useful so without additional context and full explanation it
>> seems unlikely to be what you say it is. Thank you for expanding on the
>> meaning.
> 
> What about this full description?
> M
> 
> 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 round-
> robin switching only begins after the second write.
> 
> Be aware that data is not duplicated or striped between locations
> for any single saveenv invocation.
> 
> Update the logic to perform round-robin selection of the environment
> location
> based on whether the last write was to the primary or redundant location,
> ensuring the first write sets ENV_VALID and subsequent writes follow the
> round-robin pattern as expected. This keeps gd->env_valid consistent with
> the actual storage location and fixes the sequence from the very first
> write.
> 
> With this change, the "Valid environment" printout correctly reflects the
> active location after each save, and round-robin switching between primary
> and redundant locations works as intended from the start.
> 

Michal, great and we can condense that to terms of location (and not so
much write operations that are now the most confusing word use)?

My edit is as following:

START

Update the environment location selection logic to perform
{alternating,round-robin} selection of the environment location based on
whether the last write from a call to saveenv was to the primary or
redundant location. Ensure the {first,initial} {write,call to saveenv}
correctly sets ENV_VALID so the subsequent calls to saveenv follow the
{alternating,round-robin} pattern as expected. This keeps gd->env_valid
consistent with the actual storage location and fixes the sequence from
the very first write.

Prior to this fix the {alternating,round-robin}
{switching,change,assignment,selection} of locations would begin only
after the second call to saveenv.

With this change, the "Valid environment" printout correctly reflects
the active location after each save, and round-robin switching between
primary and redundant locations works as intended from the start.

{in absense of a cover letter then insert triple-hyphen here so git will
ignore the remainder with more technical details not intended for the
commit message}

This corrects the existing behavior that when both SPI environment
locations are invalid (gd->env_valid == ENV_INVALID), updating
gd->env_valid is incorrectly {selected as,set to,assigned as} ENV_REDUND
and allowed the following call to saveenv to target the same location as
the first call.

Note that data write operations are not duplicated or striped between
locations for any single call to saveenv. The location switching is
being done between completed saveenv operations.

END

Here also are my notes and advice about word selection as might be
suggested from the above:

"following" may have a double meaning of both being a sequence and being
a copy of or adjoined to.

"subsequent" is more precise than "following" but not as common for easy
understanding.

"alternate" if you want the meaning of A-B-A-B for two locations, and
possibly A-B-A-C-A-B-A-C-A-B-A-C (alternating, non-circular order) for
more than two locations.

"round-robin" if you want the meaning of "A-B-C-D-A-B-C-D-A-B-C-D", in
that there will always be one-or-more locations taking turns in circular
order.

"the last write" can have the double meaning of "the previous write" and
also not the most recent write operation, but I figure this out from
context these two concepts are equivalent here.

With this I have focused my attention to give possibly too much
information :-)   Does that make more sense or is it less clear?

Best regards,

-E

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

[Michal Simek]

On 9/30/25 02:32, E Shattow wrote:
> 
> 
> On 9/29/25 05:36, Michal Simek wrote:
>>
>>
>> On 9/29/25 11:49, E Shattow wrote:
>>> Hi Michal,
>>>
>>> On 9/29/25 00:08, Michal Simek wrote:
>>>>
>>>>
>>>> On 9/23/25 23:18, E Shattow wrote:
>>>>>
>>>>>
>>>>> 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@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.
>>>>>
>>>>> 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.
>>>>
>>>> tbh before this patch I though that both copies are the same but that's
>>>> not how it is implemented.
>>>> It is switching from Location A to B and then to A, etc.
>>>>
>>>
>>> Okay. That is also surprising for me.
>>>
>>>>>
>>>>>>>
>>>>>>> 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.
>>>>
>>>> How the code is written is that you have location A and B.
>>>> And when you call saveenv variables are written to A.
>>>> And next saveenv variables are written to B.
>>>> Next to A, etc.
>>>>
>>>> It means unless you call saveenv twice you never get data in A and B
>>>> equal.
>>>>
>>>> It means it is not alternation of write operations to multiple targets.
>>>> It is alternation of location where one complete write is done.
>>>>
>>>>>
>>>>> 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"?
>>>> I am not a native speaker but the word alternation between A/B sounds
>>>> the best for me.
>>>> Feel free to suggest how to describe it better.
>>>>
>>>> Thanks,
>>>> Michal
>>>>
>>>
>>> This is not easy to describe with a small quantity of words. The common
>>> phrases are "taking turns" and/or "round-robin".
>>>
>>> It is clearer to say what this is not, as:
>>>
>>> "... results in alternation of saveenv location with each invocation.
>>> Data is not duplicated or striped between locations for any single
>>> saveenv invocation."
>>>
>>> I like your explanations, you understand the functionality and can
>>> describe the problem clearly. I just do not expect that functionality
>>> would be useful so without additional context and full explanation it
>>> seems unlikely to be what you say it is. Thank you for expanding on the
>>> meaning.
>>
>> What about this full description?
>> M
>>
>> 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 round-
>> robin switching only begins after the second write.
>>
>> Be aware that data is not duplicated or striped between locations
>> for any single saveenv invocation.
>>
>> Update the logic to perform round-robin selection of the environment
>> location
>> based on whether the last write was to the primary or redundant location,
>> ensuring the first write sets ENV_VALID and subsequent writes follow the
>> round-robin pattern as expected. This keeps gd->env_valid consistent with
>> the actual storage location and fixes the sequence from the very first
>> write.
>>
>> With this change, the "Valid environment" printout correctly reflects the
>> active location after each save, and round-robin switching between primary
>> and redundant locations works as intended from the start.
>>
> 
> Michal, great and we can condense that to terms of location (and not so
> much write operations that are now the most confusing word use)?
> 
> My edit is as following:
> 
> START
> 
> Update the environment location selection logic to perform
> {alternating,round-robin} selection of the environment location based on
> whether the last write from a call to saveenv was to the primary or
> redundant location. Ensure the {first,initial} {write,call to saveenv}
> correctly sets ENV_VALID so the subsequent calls to saveenv follow the
> {alternating,round-robin} pattern as expected. This keeps gd->env_valid
> consistent with the actual storage location and fixes the sequence from
> the very first write.
> 
> Prior to this fix the {alternating,round-robin}
> {switching,change,assignment,selection} of locations would begin only
> after the second call to saveenv.
> 
> With this change, the "Valid environment" printout correctly reflects
> the active location after each save, and round-robin switching between
> primary and redundant locations works as intended from the start.
> 
> {in absense of a cover letter then insert triple-hyphen here so git will
> ignore the remainder with more technical details not intended for the
> commit message}

Commit message is IMHO location where technical information should go.
I can't see any reason to skp them.

> 
> This corrects the existing behavior that when both SPI environment
> locations are invalid (gd->env_valid == ENV_INVALID), updating
> gd->env_valid is incorrectly {selected as,set to,assigned as} ENV_REDUND
> and allowed the following call to saveenv to target the same location as
> the first call.
> 
> Note that data write operations are not duplicated or striped between
> locations for any single call to saveenv. The location switching is
> being done between completed saveenv operations.
> 
> END
> 
> Here also are my notes and advice about word selection as might be
> suggested from the above:
> 
> "following" may have a double meaning of both being a sequence and being
> a copy of or adjoined to.
> 
> "subsequent" is more precise than "following" but not as common for easy
> understanding.
> 
> "alternate" if you want the meaning of A-B-A-B for two locations, and
> possibly A-B-A-C-A-B-A-C-A-B-A-C (alternating, non-circular order) for
> more than two locations.
> 
> "round-robin" if you want the meaning of "A-B-C-D-A-B-C-D-A-B-C-D", in
> that there will always be one-or-more locations taking turns in circular
> order.
> 
> "the last write" can have the double meaning of "the previous write" and
> also not the most recent write operation, but I figure this out from
> context these two concepts are equivalent here.
> 
> With this I have focused my attention to give possibly too much
> information :-)   Does that make more sense or is it less clear?

M

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 12929 bytes --]

On Tue, Sep 30, 2025 at 03:28:38PM +0200, Michal Simek wrote:
> 
> 
> On 9/30/25 02:32, E Shattow wrote:
> > 
> > 
> > On 9/29/25 05:36, Michal Simek wrote:
> > > 
> > > 
> > > On 9/29/25 11:49, E Shattow wrote:
> > > > Hi Michal,
> > > > 
> > > > On 9/29/25 00:08, Michal Simek wrote:
> > > > > 
> > > > > 
> > > > > On 9/23/25 23:18, E Shattow wrote:
> > > > > > 
> > > > > > 
> > > > > > 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@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.
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > tbh before this patch I though that both copies are the same but that's
> > > > > not how it is implemented.
> > > > > It is switching from Location A to B and then to A, etc.
> > > > > 
> > > > 
> > > > Okay. That is also surprising for me.
> > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > 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.
> > > > > 
> > > > > How the code is written is that you have location A and B.
> > > > > And when you call saveenv variables are written to A.
> > > > > And next saveenv variables are written to B.
> > > > > Next to A, etc.
> > > > > 
> > > > > It means unless you call saveenv twice you never get data in A and B
> > > > > equal.
> > > > > 
> > > > > It means it is not alternation of write operations to multiple targets.
> > > > > It is alternation of location where one complete write is done.
> > > > > 
> > > > > > 
> > > > > > 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"?
> > > > > I am not a native speaker but the word alternation between A/B sounds
> > > > > the best for me.
> > > > > Feel free to suggest how to describe it better.
> > > > > 
> > > > > Thanks,
> > > > > Michal
> > > > > 
> > > > 
> > > > This is not easy to describe with a small quantity of words. The common
> > > > phrases are "taking turns" and/or "round-robin".
> > > > 
> > > > It is clearer to say what this is not, as:
> > > > 
> > > > "... results in alternation of saveenv location with each invocation.
> > > > Data is not duplicated or striped between locations for any single
> > > > saveenv invocation."
> > > > 
> > > > I like your explanations, you understand the functionality and can
> > > > describe the problem clearly. I just do not expect that functionality
> > > > would be useful so without additional context and full explanation it
> > > > seems unlikely to be what you say it is. Thank you for expanding on the
> > > > meaning.
> > > 
> > > What about this full description?
> > > M
> > > 
> > > 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 round-
> > > robin switching only begins after the second write.
> > > 
> > > Be aware that data is not duplicated or striped between locations
> > > for any single saveenv invocation.
> > > 
> > > Update the logic to perform round-robin selection of the environment
> > > location
> > > based on whether the last write was to the primary or redundant location,
> > > ensuring the first write sets ENV_VALID and subsequent writes follow the
> > > round-robin pattern as expected. This keeps gd->env_valid consistent with
> > > the actual storage location and fixes the sequence from the very first
> > > write.
> > > 
> > > With this change, the "Valid environment" printout correctly reflects the
> > > active location after each save, and round-robin switching between primary
> > > and redundant locations works as intended from the start.
> > > 
> > 
> > Michal, great and we can condense that to terms of location (and not so
> > much write operations that are now the most confusing word use)?
> > 
> > My edit is as following:
> > 
> > START
> > 
> > Update the environment location selection logic to perform
> > {alternating,round-robin} selection of the environment location based on
> > whether the last write from a call to saveenv was to the primary or
> > redundant location. Ensure the {first,initial} {write,call to saveenv}
> > correctly sets ENV_VALID so the subsequent calls to saveenv follow the
> > {alternating,round-robin} pattern as expected. This keeps gd->env_valid
> > consistent with the actual storage location and fixes the sequence from
> > the very first write.
> > 
> > Prior to this fix the {alternating,round-robin}
> > {switching,change,assignment,selection} of locations would begin only
> > after the second call to saveenv.
> > 
> > With this change, the "Valid environment" printout correctly reflects
> > the active location after each save, and round-robin switching between
> > primary and redundant locations works as intended from the start.
> > 
> > {in absense of a cover letter then insert triple-hyphen here so git will
> > ignore the remainder with more technical details not intended for the
> > commit message}
> 
> Commit message is IMHO location where technical information should go.
> I can't see any reason to skp them.

I very much agree. Especially on a patch like this where we've spent a
great deal of time discussing the commit message (as the situation being
fixed is very complex). Someone looking at this in a year or three will
be much happier with a long commit message they need to digest than a
short message that leaves them with questions.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]