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

Quentin Schulz
2024-09-18T16:59:45Z

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@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;