Hi Tom,
On 10/26/26 10:08 PM, Tom Rini wrote:
On Mon, Oct 26, 2026 at 03:41:02PM +0100, Clément Le Goffic wrote:
The flash environment driver has load, save and init operations but no
erase.
Erase the environment sector the way env_flash_save prepares it for a
write: unprotect it, erase it and protect it again.
When the sector is larger than the environment, the rest of the sector
is saved before the erase and written back afterwards, as the save path
does.
With a redundant copy, both sectors are erased: the active copy
alternates between them on every save, so erasing only one can leave a
valid environment behind.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Clément Le Goffic <[email protected]>
Using LLMs is discouraged in U-Boot. Patches 2 and 3 are trivial, but
Claude is tagged since presumably you directed it to implement the whole
thing.
I didn't know and already got a private mail about it.
Thanks for letting me know.
The commit message is one of the things which bugs me, but it's also a
more minor thing. Handling redundant copies correctly is an expected
requirement, not something special to be explained, and is something the
other environment stores handle too. The partial sector case is
something only "raw" flash types have to handle, but again, all do. When
handling expected functional cases, one should just acknowledge them as
being handled.
Right, I understand
But the commit message nor cover letter don't note the real problem.
That is CMD_ERASEENV can be enabled for ENV_IS_IN_FLASH but is not
implemented by the driver. The next step here would be a quick check of
which other backing stores also don't implement erase and then updating
the Kconfig logic.
Thanks for the idea, I'll fix that in v2 with the refactorisation.
---
env/flash.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/env/flash.c b/env/flash.c
index 31dd1656e721..4b402437eb77 100644
--- a/env/flash.c
+++ b/env/flash.c
@@ -310,6 +310,66 @@ done:
#endif /* CONFIG_ENV_ADDR_REDUND */
+#ifdef CMD_SAVEENV
+static int env_flash_erase_sect(ulong addr, ulong end)
+{
+ char *saved_data = NULL;
+ int rc = 1;
+#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
+ ulong tail_size = end + 1 - (addr + CONFIG_ENV_SIZE);
+
+ debug("Sector tail to keep 0x%lx\n", tail_size);
+ if (tail_size) {
+ saved_data = malloc(tail_size);
+ if (!saved_data) {
+ printf("Unable to save the rest of sector (%lu)\n",
+ tail_size);
+ return 1;
+ }
+ memcpy(saved_data, (void *)(addr + CONFIG_ENV_SIZE), tail_size);
+ }
+#endif /* CONFIG_ENV_SECT_SIZE */
+
+ debug("Protect off %08lX ... %08lX\n", addr, end);
+
+ if (flash_sect_protect(0, addr, end))
+ goto done;
+
+ if (flash_sect_erase(addr, end))
+ goto done;
+
+#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
+ if (tail_size) { /* restore the rest of sector */
+ debug("Restoring the rest of data to 0x%lx len 0x%lx\n",
+ addr + CONFIG_ENV_SIZE, tail_size);
+ rc = flash_write(saved_data, addr + CONFIG_ENV_SIZE, tail_size);
+ if (rc) {
+ flash_perror(rc);
+ goto done;
+ }
+ }
+#endif
+ rc = 0;
+done:
+ free(saved_data);
+ /* try to re-protect */
+ flash_sect_protect(1, addr, end);
+
+ return rc;
All of the CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE code is the same as
the saveenv with the variables changed. So a refactor is in order.
Ack
Best regards,
Clément
