[PATCH v2] env: spi: Fix gd->env_valid for the first write
Tom Rini
trini at konsulko.com
Tue Sep 30 16:59:19 CEST 2025
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 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.
> > > > >
> > > > > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250930/8eb81f4b/attachment.sig>
More information about the U-Boot
mailing list