Re: [PATCH v2 2/3] env: Add single to redundant environment upgrade path

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 12/23/25 15:31, Marek Vasut wrote:
> Add support for converting single-copy environment to redundant environment.
> In case CRC checks on both redundant environment copies fail, try one more
> CRC check on the primary environment copy and treat it as single environment.

Why would a CRC check suddenly succeed if it has failed before?

This needs some more explanation.

> If that check does pass, rewrite the single-copy environment into redundant
> environment format, indicate the environment is valid, and import that as
> usual primary copy of redundant environment. Follow up 'env save' will then
> store two environment copies and the system will continue to operate as
> regular redundant environment system.
> 
> Add test which validates this upgrade path. The test starts with spi.bin
> which is pre-populated as single-copy environment and then upgrades that
> environment to dual-copy environment.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Jerome Forissier <jerome.forissier@linaro.org>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
> V2: - Gate the option behind ENV_REDUNDANT_UPGRADE
>      - Fix up mkenvimage path in env test
> ---
>   env/Kconfig               | 11 ++++++
>   env/common.c              | 31 +++++++++++++++-
>   test/py/tests/test_env.py | 74 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 115 insertions(+), 1 deletion(-)
> 
> diff --git a/env/Kconfig b/env/Kconfig
> index 4430669964c..b312f9b5324 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -489,6 +489,17 @@ config ENV_REDUNDANT
>   	  which is used by env import/export commands which are independent of
>   	  storing variables to redundant location on a non volatile device.
>   
> +config ENV_REDUNDANT_UPGRADE
> +	bool "Enable single-copy to redundant environment upgrade support"
> +	depends on ENV_REDUNDANT
> +	help
> +	  Normally, redundant environment is expected to always operate on
> +	  two copies of the environment. However, hardware that may have
> +	  originally shipped with single-copy environment can be upgraded

%s/with single-copy/with a single-copy/
%s/can be/that can be/

> +	  to redundant environment without loss of existing environment
> +	  content by correctly configuring the location of the redundant
> +	  environment copy and by enabling this option.

Why do we have to make this an option?
Shouldn't we always try to restore the environment?

> +
>   config ENV_FAT_INTERFACE
>   	string "Name of the block device for the environment"
>   	depends on ENV_IS_IN_FAT
> diff --git a/env/common.c b/env/common.c
> index 05e78d63874..b2adbe93dbe 100644
> --- a/env/common.c
> +++ b/env/common.c
> @@ -473,14 +473,24 @@ int env_import(const char *buf, int check, int flags)
>   #ifdef CONFIG_ENV_REDUNDANT
>   static unsigned char env_flags;
>   
> +#define ENV_SINGLE_HEADER_SIZE	(sizeof(uint32_t))
> +#define ENV_SINGLE_SIZE		(CONFIG_ENV_SIZE - ENV_SINGLE_HEADER_SIZE)
> +
> +typedef struct {
> +	uint32_t	crc;			/* CRC32 over data bytes */
> +	unsigned char	data[ENV_SINGLE_SIZE];	/* Environment data */
> +} env_single_t;
> +
>   int env_check_redund(const char *buf1, int buf1_read_fail,
>   		     const char *buf2, int buf2_read_fail)
>   {
> -	int crc1_ok = 0, crc2_ok = 0;
> +	int crc1_ok = 0, crc2_ok = 0, i;
>   	env_t *tmp_env1, *tmp_env2;
> +	env_single_t *tmp_envs;
>   
>   	tmp_env1 = (env_t *)buf1;
>   	tmp_env2 = (env_t *)buf2;
> +	tmp_envs = (env_single_t *)buf1;
>   
>   	if (buf1_read_fail && buf2_read_fail) {
>   		puts("*** Error - No Valid Environment Area found\n");
> @@ -498,6 +508,25 @@ int env_check_redund(const char *buf1, int buf1_read_fail,
>   				tmp_env2->crc;
>   
>   	if (!crc1_ok && !crc2_ok) {

Do we really have a third location to copy from when both store 1 and 
store 2 are defective? I would have expected that if a vendor provides a 
single copy then exactly one of crc1_ok or crc2_ok is true and the other 
is false.

Please, provide a documentation update explaining how this all works.

Best regards

Heinrich

> +		/*
> +		 * Upgrade single-copy environment to redundant environment.
> +		 * In case CRC checks on both environment copies fail, try
> +		 * one more CRC check on the primary environment copy and
> +		 * treat it as single-copy environment. If that check does
> +		 * pass, rewrite the single-copy environment into redundant
> +		 * environment format and indicate the environment is valid.
> +		 * The follow up calls will import the environment as if it
> +		 * was a redundant environment. Follow up 'env save' will
> +		 * then store two environment copies.
> +		 */
> +		if (CONFIG_IS_ENABLED(ENV_REDUNDANT_UPGRADE) && !buf1_read_fail &&
> +		    crc32(0, tmp_envs->data, ENV_SINGLE_SIZE) == tmp_envs->crc) {
> +			for (i = ENV_SIZE - 1; i >= 0; i--)
> +				tmp_env1->data[i] = tmp_envs->data[i];
> +			tmp_env1->flags = 0;
> +			gd->env_valid = ENV_VALID;
> +			return 0;
> +		}
>   		gd->env_valid = ENV_INVALID;
>   		return -ENOMSG; /* needed for env_load() */
>   	} else if (crc1_ok && !crc2_ok) {
> diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
> index 48e31f19b3c..f8713a59ba9 100644
> --- a/test/py/tests/test_env.py
> +++ b/test/py/tests/test_env.py
> @@ -477,6 +477,22 @@ def mk_env_spi_flash(state_test_env):
>       utils.run_and_log(c, ['cp',  '-f', persistent, spi_flash_img])
>       return spi_flash_img
>   
> +def mk_env_spi_flash_single(state_test_env):
> +
> +    """Create an single-copy SPI NOR image with foo=bar entry."""
> +    c = state_test_env.ubman
> +    filename = 'spi.bin'
> +    spi_flash_img = c.config.source_dir  + '/' + filename
> +
> +    try:
> +        mkenvimage = os.path.join(c.config.build_dir, 'tools/mkenvimage')
> +        call('( echo foo=bar | %s -s 8192 -p 0x00 - ; dd if=/dev/zero bs=2088960 count=1 2>/dev/null ) > %s' % ( mkenvimage , spi_flash_img ), shell=True)
> +    except CalledProcessError:
> +        call('rm -f %s' % spi_flash_img, shell=True)
> +        raise
> +
> +    return spi_flash_img
> +
>   @pytest.mark.boardspec('sandbox')
>   @pytest.mark.buildconfigspec('cmd_echo')
>   @pytest.mark.buildconfigspec('cmd_nvedit_info')
> @@ -574,6 +590,64 @@ def test_env_spi_flash(state_test_env):
>   
>       """Test ENV in SPI NOR on sandbox."""
>       c = state_test_env.ubman
> +    spi_flash_img = ''
> +    try:
> +        spi_flash_img = mk_env_spi_flash_single(state_test_env)
> +
> +        response = c.run_command('sf probe')
> +        assert 'SF: Detected m25p16 with page size 256 Bytes, erase size 64 KiB, total 2 MiB' in response
> +
> +        # force env location: SF
> +        response = c.run_command('env select SPIFlash')
> +        assert 'Select Environment on SPIFlash: OK' in response
> +
> +        response = c.run_command('env load')
> +        assert 'Loading Environment from SPIFlash... OK' in response
> +
> +        response = c.run_command('env print foo')
> +        assert 'foo=bar' in response
> +
> +        response = c.run_command('env save')
> +        assert 'Saving Environment to SPIFlash' in response
> +
> +        response = c.run_command('env load')
> +        assert 'Loading Environment from SPIFlash... OK' in response
> +
> +        response = c.run_command('env print foo')
> +        assert 'foo=bar' in response
> +
> +        response = c.run_command('env save')
> +        assert 'Saving Environment to SPIFlash' in response
> +
> +        response = c.run_command('env save')
> +        assert 'Saving Environment to SPIFlash' in response
> +
> +        response = c.run_command('env load')
> +        assert 'Loading Environment from SPIFlash... OK' in response
> +
> +        response = c.run_command('env print foo')
> +        assert 'foo=bar' in response
> +
> +        # restore env location: NOWHERE (prio 0 in sandbox)
> +        response = c.run_command('env select nowhere')
> +        assert 'Select Environment on nowhere: OK' in response
> +
> +        response = c.run_command('env load')
> +        assert 'Loading Environment from nowhere... OK' in response
> +
> +        response = c.run_command('env info')
> +        assert 'env_valid = invalid' in response
> +        assert 'env_ready = true' in response
> +        assert 'env_use_default = true' in response
> +
> +        response = c.run_command('env info -p -d')
> +        assert 'Default environment is used' in response
> +        assert 'Environment cannot be persisted' in response
> +
> +    finally:
> +        if spi_flash_img:
> +            call('rm -f %s' % spi_flash_img, shell=True)
> +
>       spi_flash_img = ''
>       try:
>           spi_flash_img = mk_env_spi_flash(state_test_env)