[PATCH 3/3] env: mmc: rework mmc_env_partition_by_guid() to work with two separate partitions

[Quentin Schulz]

Hi Rasmus,

For this patch and the previous one, should we have test(s) to make sure 
we don't regress?

Cheers,
Quentin

On 9/12/24 3:41 PM, Rasmus Villemoes wrote:
> Having two separate partitions for use in a redundant environment
> setup works just fine, if one only relies on setting CONFIG_ENV_OFFSET
> and CONFIG_ENV_OFFSET_REDUND. However, if CONFIG_PARTITION_TYPE_GUID
> is enabled, the current logic in mmc_env_partition_by_guid() means
> that only the first partition will ever be considered, and prior to
> the previous commit, lead to silent data corruption.
> 
> Extend the logic so that, when we are looking for the location for the
> second copy of the environment, we keep track of whether we have
> already found one matching partition. If a second match is found,
> return that, but also modify *copy so that the logic in the caller
> will use the last ENV_SIZE bytes of that second partition - in my
> case, and I suppose that would be typical, both partitions have been
> created with a size of exactly the desired ENV_SIZE.
> 
> When only a single matching partition exists, the behaviour is
> unchanged: We return that single partition, and *copy is left as-is,
> so the logic in the caller will either use the last (copy==0) or
> second-to-last (copy==1) ENV_SIZE bytes.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> ---
>   env/mmc.c | 46 +++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index 5d09140655f..e2f8e7ece28 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -69,21 +69,49 @@ static int mmc_env_partition_by_name(struct blk_desc *desc, const char *str,
>   	}
>   }
>   
> -static int mmc_env_partition_by_guid(struct blk_desc *desc, struct disk_partition *info)
> +/*
> + * Look for one or two partitions with the U-Boot environment GUID.
> + *
> + * If *copy is 0, return the first such partition.
> + *
> + * If *copy is 1 on entry and two partitions are found, return the
> + * second partition and set *copy = 0.
> + *
> + * If *copy is 1 on entry and only one partition is found, return that
> + * partition, leaving *copy unmodified.
> + */
> +static int mmc_env_partition_by_guid(struct blk_desc *desc, struct disk_partition *info,
> +				     int *copy)
>   {
>   	const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
>   	efi_guid_t type_guid;
> -	int i, ret;
> +	int i, ret, found = 0;
> +	struct disk_partition dp;
>   
>   	for (i = 1;; i++) {
> -		ret = part_get_info(desc, i, info);
> +		ret = part_get_info(desc, i, &dp);
>   		if (ret < 0)
> -			return ret;
> -
> -		uuid_str_to_bin(disk_partition_type_guid(info), type_guid.b, UUID_STR_FORMAT_GUID);
> -		if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t)))
> -			return 0;
> +			break;
> +
> +		uuid_str_to_bin(disk_partition_type_guid(&dp), type_guid.b, UUID_STR_FORMAT_GUID);
> +		if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t))) {
> +			memcpy(info, &dp, sizeof(dp));
> +			/* If *copy is 0, we are only looking for the first partition. */
> +			if (*copy == 0)
> +				return 0;
> +			/* This was the second such partition. */
> +			if (found) {
> +				*copy = 0;
> +				return 0;
> +			}
> +			found = 1;
> +		}
>   	}
> +
> +	/* The loop ended after finding at most one matching partition. */
> +	if (found)
> +		ret = 0;
> +	return ret;
>   }
>   
>   
> @@ -103,7 +131,7 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
>   	if (str) {
>   		ret = mmc_env_partition_by_name(desc, str, &info);
>   	} else if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) && !str) {
> -		ret = mmc_env_partition_by_guid(desc, &info);
> +		ret = mmc_env_partition_by_guid(desc, &info, &copy);
>   	}
>   	if (ret < 0)
>   		return ret;