[PATCH 0/3] env: mmc: fix use of two separate partitions with proper type GUID

[PATCH 0/3] env: mmc: fix use of two separate partitions with proper type GUID

[Rasmus Villemoes]

I always define a disk layout with two separate partitions for the two
copies of the U-Boot environment and, being the one who introduced the
type GUID for such partitions, of course also set those partitions'
type GUID appropriately.

This has worked just fine, but, it turns out, only because I've never
had CONFIG_PARTITION_TYPE_GUID enabled on any of my boards; I've
always just set the offsets of the two partitions via the config
variables CONFIG_ENV_OFFSET(,_REDUND).

I didn't even know that env/mmc.c had learnt to look for the env
partition based on the type GUID, or that that would overrule the
ENV_OFFSET config variables, until I experienced weird random
corruption while doing bringup for an stm32 board, where
PARTITION_TYPE_GUID is automatically set because it is select'ed by
CMD_STM32PROG.

These patches try to fix the code to fit my scheme, while not changing
anything for existing setups that use the two-copies-one-partition
scheme, other than complaining loudly if the system is misconfigured
and avoiding such random corruption of neighbouring partitions.

Rasmus Villemoes (3):
  env: mmc: refactor mmc_offset_try_partition()
  env: mmc: do not return an offset before the start of the partition
  env: mmc: rework mmc_env_partition_by_guid() to work with two separate
    partitions

 env/mmc.c | 95 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 77 insertions(+), 18 deletions(-)

-- 
2.46.0

[PATCH 1/3] env: mmc: refactor mmc_offset_try_partition()

[Rasmus Villemoes]

In preparation for fixing the handling of a the case of redundant
environment defined in two separate partitions with the U-Boot env
GUID, refactor the

  for ()
    if (str)
      ...
  #ifdef CONFIG_FOO
    if (!str)
      ..
  #endif

to

  if (str)
    for ()
  else if (CONFIG_FOO && !str)
    for ()

and put those for loops in separate functions.

No functional change intended, but I did change the direct access of
info.type_guid into using the disk_partition_type_guid() helper, so
that I could avoid the #ifdef and use IS_ENABLED() in the if() statement.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 env/mmc.c | 59 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 18 deletions(-)

diff --git a/env/mmc.c b/env/mmc.c
index 0338aa6c56a..db2d35e9bd4 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -53,11 +53,45 @@ DECLARE_GLOBAL_DATA_PTR;
 #endif
 
 #if CONFIG_IS_ENABLED(OF_CONTROL)
+
+static int mmc_env_partition_by_name(struct blk_desc *desc, const char *str,
+				     struct disk_partition *info)
+{
+	int i, ret;
+
+	for (i = 1;; i++) {
+		ret = part_get_info(desc, i, info);
+		if (ret < 0)
+			return ret;
+
+		if (!strncmp((const char *)info->name, str, sizeof(info->name)))
+			return 0;
+	}
+}
+
+static int mmc_env_partition_by_guid(struct blk_desc *desc, struct disk_partition *info)
+{
+	const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
+	efi_guid_t type_guid;
+	int i, ret;
+
+	for (i = 1;; i++) {
+		ret = part_get_info(desc, i, info);
+		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;
+	}
+}
+
+
 static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
 {
 	struct disk_partition info;
 	struct blk_desc *desc;
-	int len, i, ret;
+	int len, ret;
 	char dev_str[4];
 
 	snprintf(dev_str, sizeof(dev_str), "%d", mmc_get_env_dev());
@@ -65,24 +99,13 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
 	if (ret < 0)
 		return (ret);
 
-	for (i = 1;;i++) {
-		ret = part_get_info(desc, i, &info);
-		if (ret < 0)
-			return ret;
-
-		if (str && !strncmp((const char *)info.name, str, sizeof(info.name)))
-			break;
-#ifdef CONFIG_PARTITION_TYPE_GUID
-		if (!str) {
-			const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
-			efi_guid_t type_guid;
-
-			uuid_str_to_bin(info.type_guid, type_guid.b, UUID_STR_FORMAT_GUID);
-			if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t)))
-				break;
-		}
-#endif
+	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);
 	}
+	if (ret < 0)
+		return ret;
 
 	/* round up to info.blksz */
 	len = DIV_ROUND_UP(CONFIG_ENV_SIZE, info.blksz);
-- 
2.46.0

Re: [PATCH 1/3] env: mmc: refactor mmc_offset_try_partition()

[Quentin Schulz]

Hi Rasmus,

On 9/12/24 3:41 PM, Rasmus Villemoes wrote:
> In preparation for fixing the handling of a the case of redundant
> environment defined in two separate partitions with the U-Boot env
> GUID, refactor the
> 
>    for ()
>      if (str)
>        ...
>    #ifdef CONFIG_FOO
>      if (!str)
>        ..
>    #endif
> 
> to
> 
>    if (str)
>      for ()
>    else if (CONFIG_FOO && !str)
>      for ()
> 
> and put those for loops in separate functions.
> 
> No functional change intended, but I did change the direct access of
> info.type_guid into using the disk_partition_type_guid() helper, so
> that I could avoid the #ifdef and use IS_ENABLED() in the if() statement.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   env/mmc.c | 59 ++++++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index 0338aa6c56a..db2d35e9bd4 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -53,11 +53,45 @@ DECLARE_GLOBAL_DATA_PTR;
>   #endif
>   
>   #if CONFIG_IS_ENABLED(OF_CONTROL)
> +
> +static int mmc_env_partition_by_name(struct blk_desc *desc, const char *str,
> +				     struct disk_partition *info)
> +{
> +	int i, ret;
> +
> +	for (i = 1;; i++) {
> +		ret = part_get_info(desc, i, info);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!strncmp((const char *)info->name, str, sizeof(info->name)))
> +			return 0;
> +	}
> +}
> +
> +static int mmc_env_partition_by_guid(struct blk_desc *desc, struct disk_partition *info)
> +{
> +	const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
> +	efi_guid_t type_guid;
> +	int i, ret;
> +
> +	for (i = 1;; i++) {
> +		ret = part_get_info(desc, i, info);
> +		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;
> +	}
> +}
> +
> +
>   static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
>   {
>   	struct disk_partition info;
>   	struct blk_desc *desc;
> -	int len, i, ret;
> +	int len, ret;
>   	char dev_str[4];
>   
>   	snprintf(dev_str, sizeof(dev_str), "%d", mmc_get_env_dev());
> @@ -65,24 +99,13 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
>   	if (ret < 0)
>   		return (ret);
>   
> -	for (i = 1;;i++) {
> -		ret = part_get_info(desc, i, &info);
> -		if (ret < 0)
> -			return ret;
> -
> -		if (str && !strncmp((const char *)info.name, str, sizeof(info.name)))
> -			break;
> -#ifdef CONFIG_PARTITION_TYPE_GUID
> -		if (!str) {
> -			const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
> -			efi_guid_t type_guid;
> -
> -			uuid_str_to_bin(info.type_guid, type_guid.b, UUID_STR_FORMAT_GUID);
> -			if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t)))
> -				break;
> -		}
> -#endif
> +	if (str) {
> +		ret = mmc_env_partition_by_name(desc, str, &info);
> +	} else if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) && !str) {

nitpick: it's guaranteed that !str if reaching the else if based on the 
condition of the above if condition.

Cheers,
Quentin

Re: [PATCH 1/3] env: mmc: refactor mmc_offset_try_partition()

[Rasmus Villemoes]

Quentin Schulz <quentin.schulz@cherry.de> writes:

>>   -	for (i = 1;;i++) {
>> -		ret = part_get_info(desc, i, &info);
>> -		if (ret < 0)
>> -			return ret;
>> -
>> -		if (str && !strncmp((const char *)info.name, str, sizeof(info.name)))
>> -			break;
>> -#ifdef CONFIG_PARTITION_TYPE_GUID
>> -		if (!str) {
>> -			const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
>> -			efi_guid_t type_guid;
>> -
>> -			uuid_str_to_bin(info.type_guid, type_guid.b, UUID_STR_FORMAT_GUID);
>> -			if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t)))
>> -				break;
>> -		}
>> -#endif
>> +	if (str) {
>> +		ret = mmc_env_partition_by_name(desc, str, &info);
>> +	} else if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) && !str) {
>
> nitpick: it's guaranteed that !str if reaching the else if based on
> the condition of the above if condition.

Ah, yes of course. This was just because I tried to translate the
existing

  #ifdef CONFIG_PARTITION_TYPE_GUID
    if (!str)

logic as closely as possible. I can resend. However, I plan to send some
followup cleanups and simplifications to env/mmc.c anyway later, but
didn't want to entangle those with this bugfix, so perhaps it can be
done as part of that.

Rasmus

Re: [PATCH 1/3] env: mmc: refactor mmc_offset_try_partition()

[Quentin Schulz]

Hi Rasmus,

On 9/19/24 8:53 AM, Rasmus Villemoes wrote:
> Quentin Schulz <quentin.schulz@cherry.de> writes:
> 
>>>    -	for (i = 1;;i++) {
>>> -		ret = part_get_info(desc, i, &info);
>>> -		if (ret < 0)
>>> -			return ret;
>>> -
>>> -		if (str && !strncmp((const char *)info.name, str, sizeof(info.name)))
>>> -			break;
>>> -#ifdef CONFIG_PARTITION_TYPE_GUID
>>> -		if (!str) {
>>> -			const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
>>> -			efi_guid_t type_guid;
>>> -
>>> -			uuid_str_to_bin(info.type_guid, type_guid.b, UUID_STR_FORMAT_GUID);
>>> -			if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t)))
>>> -				break;
>>> -		}
>>> -#endif
>>> +	if (str) {
>>> +		ret = mmc_env_partition_by_name(desc, str, &info);
>>> +	} else if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) && !str) {
>>
>> nitpick: it's guaranteed that !str if reaching the else if based on
>> the condition of the above if condition.
> 
> Ah, yes of course. This was just because I tried to translate the
> existing
> 
>    #ifdef CONFIG_PARTITION_TYPE_GUID
>      if (!str)
> 
> logic as closely as possible. I can resend. However, I plan to send some
> followup cleanups and simplifications to env/mmc.c anyway later, but
> didn't want to entangle those with this bugfix, so perhaps it can be
> done as part of that.
> 

It was required with the old implementation because it is not an else if 
condition. It isn't in the new implementation as we'd be now using an 
else if condition there.

If we wanted to be really pedantic, I would suggest to do:

if (str)
    foo();
if (!str && IS_ENABLED(CONFIG_PARTITION_TYPE_GUID))
    bar();

to match exactly the same logic as what is being replaced.

But that would cost an extra check which really isn't necessary. I would 
recommend just ditching the !str check.

That's a nitpick, so up to you for a resend.

Cheers,
Quentin

[PATCH 2/3] env: mmc: do not return an offset before the start of the partition

[Rasmus Villemoes]

I have an GPT layout containing two partitions with the type GUID for
U-Boot environment:

        partition U-Boot-env-1 {
                offset = 0x1fc000
                size = 0x2000
                partition-type-uuid = "3de21764-95bd-54bd-a5c3-4abe786f38a8"
        }

        partition U-Boot-env-2 {
                offset = 0x1fe000
                size = 0x2000
                partition-type-uuid = "3de21764-95bd-54bd-a5c3-4abe786f38a8"
        }

and have set CONFIG_ENV_OFFSET=0x1fc000,
CONFIG_ENV_OFFSET_REDUND=0x1fe000 and CCONFIG_ENV_SIZE=0x2000.

This usually works just fine, but on an stm32mp, I was seeing weird
behaviour. It turns out that can be tracked down to that board setting
CONFIG_PARTITION_TYPE_GUID, so the logic in mmc.c ends up only finding
the first of the two partitions, but then in the copy=1 case ends up
computing 0x1fa000 as the *val returned (that is, the end of the
partition minus two times the environment size). That is of course
outside the found partition and leads to random corruption of the
partition preceding U-Boot-env-1.

Add a sanity check that the partition found is at least as large as
needed for the "one or two copies from the end of the partition" logic
to at least produce something within that partition. That will also
catch a bug where the partition is too small for even one copy of the
environment.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 env/mmc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/env/mmc.c b/env/mmc.c
index db2d35e9bd4..5d09140655f 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -91,7 +91,8 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
 {
 	struct disk_partition info;
 	struct blk_desc *desc;
-	int len, ret;
+	lbaint_t len;
+	int ret;
 	char dev_str[4];
 
 	snprintf(dev_str, sizeof(dev_str), "%d", mmc_get_env_dev());
@@ -110,6 +111,13 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
 	/* round up to info.blksz */
 	len = DIV_ROUND_UP(CONFIG_ENV_SIZE, info.blksz);
 
+	if ((1 + copy) * len > info.size) {
+		printf("Partition '%s' [0x"LBAF"; 0x"LBAF"] too small for %senvironment, required size 0x"LBAF" blocks\n",
+		       (const char*)info.name, info.start, info.size,
+		       copy ? "two copies of " : "", (1 + copy)*len);
+		return -ENOSPC;
+	}
+
 	/* use the top of the partion for the environment */
 	*val = (info.start + info.size - (1 + copy) * len) * info.blksz;
 
-- 
2.46.0

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

[Rasmus Villemoes]

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

Re: [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@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;

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

[Rasmus Villemoes]

Quentin Schulz <quentin.schulz@cherry.de> writes:

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

That's obviously a good idea. But I don't have any idea how I'd go about
writing such tests. AFAICT, there is no existing tests of the "find env
partition by GUID" logic that I could amend, or any tests of any of the
"find the mmc partition containing the env" for that matter. Pointers
appreciated.

FWIW, on my end, I think I'll enable CONFIG_PARTITION_TYPE_GUID on all
our configs and stop defining the real values of the ENV_OFFSET, so that
our boards will start depending on the guid logic to find the env
partitions. Which will then at least eventually find a regression, but
unfortunately we usually lag some months behind on upgrading, and very
rarely have resources for checking -rcX, so we would only find it after
a release.

Rasmus

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

[Quentin Schulz]

  Hi Rasmus,

On 9/19/24 9:01 AM, Rasmus Villemoes wrote:
> Quentin Schulz <quentin.schulz@cherry.de> writes:
> 
>> Hi Rasmus,
>>
>> For this patch and the previous one, should we have test(s) to make
>> sure we don't regress?
>>
> 
> That's obviously a good idea. But I don't have any idea how I'd go about
> writing such tests. AFAICT, there is no existing tests of the "find env
> partition by GUID" logic that I could amend, or any tests of any of the
> "find the mmc partition containing the env" for that matter. Pointers
> appreciated.
> 

I have never written tests for U-Boot so wouldn't be able to provide 
pointers there, sorry :/

> FWIW, on my end, I think I'll enable CONFIG_PARTITION_TYPE_GUID on all
> our configs and stop defining the real values of the ENV_OFFSET, so that
> our boards will start depending on the guid logic to find the env
> partitions. Which will then at least eventually find a regression, but
> unfortunately we usually lag some months behind on upgrading, and very
> rarely have resources for checking -rcX, so we would only find it after
> a release.
> 

Relying on manual tests is better than none, but still bad :)

Could it be something that could be automated with labgrid somehow? 
Simon has posted some support for labgrid recently for testing U-Boot on 
real boards, maybe that's something you'd be interested in?

Cheers,
Quentin

Re: [PATCH 0/3] env: mmc: fix use of two separate partitions with proper type GUID

[Rasmus Villemoes]

Rasmus Villemoes <rasmus.villemoes@prevas.dk> writes:

> I always define a disk layout with two separate partitions for the two
> copies of the U-Boot environment and, being the one who introduced the
> type GUID for such partitions, of course also set those partitions'
> type GUID appropriately.
>
> This has worked just fine, but, it turns out, only because I've never
> had CONFIG_PARTITION_TYPE_GUID enabled on any of my boards; I've
> always just set the offsets of the two partitions via the config
> variables CONFIG_ENV_OFFSET(,_REDUND).
>
> I didn't even know that env/mmc.c had learnt to look for the env
> partition based on the type GUID, or that that would overrule the
> ENV_OFFSET config variables, until I experienced weird random
> corruption while doing bringup for an stm32 board, where
> PARTITION_TYPE_GUID is automatically set because it is select'ed by
> CMD_STM32PROG.
>
> These patches try to fix the code to fit my scheme, while not changing
> anything for existing setups that use the two-copies-one-partition
> scheme, other than complaining loudly if the system is misconfigured
> and avoiding such random corruption of neighbouring partitions.

Tom, any chance these could be picked up before 2024.10 release? And if
not, at least be put in -next so I have some stable sha1s to refer to
in my own branch.

Rasmus

Re: [PATCH 0/3] env: mmc: fix use of two separate partitions with proper type GUID

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

On Tue, Oct 01, 2024 at 03:43:23PM +0200, Rasmus Villemoes wrote:
> Rasmus Villemoes <rasmus.villemoes@prevas.dk> writes:
> 
> > I always define a disk layout with two separate partitions for the two
> > copies of the U-Boot environment and, being the one who introduced the
> > type GUID for such partitions, of course also set those partitions'
> > type GUID appropriately.
> >
> > This has worked just fine, but, it turns out, only because I've never
> > had CONFIG_PARTITION_TYPE_GUID enabled on any of my boards; I've
> > always just set the offsets of the two partitions via the config
> > variables CONFIG_ENV_OFFSET(,_REDUND).
> >
> > I didn't even know that env/mmc.c had learnt to look for the env
> > partition based on the type GUID, or that that would overrule the
> > ENV_OFFSET config variables, until I experienced weird random
> > corruption while doing bringup for an stm32 board, where
> > PARTITION_TYPE_GUID is automatically set because it is select'ed by
> > CMD_STM32PROG.
> >
> > These patches try to fix the code to fit my scheme, while not changing
> > anything for existing setups that use the two-copies-one-partition
> > scheme, other than complaining loudly if the system is misconfigured
> > and avoiding such random corruption of neighbouring partitions.
> 
> Tom, any chance these could be picked up before 2024.10 release? And if
> not, at least be put in -next so I have some stable sha1s to refer to
> in my own branch.

I'll take this to next soon, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/3] env: mmc: fix use of two separate partitions with proper type GUID

[Tom Rini]

On Thu, 12 Sep 2024 15:41:38 +0200, Rasmus Villemoes wrote:

> I always define a disk layout with two separate partitions for the two
> copies of the U-Boot environment and, being the one who introduced the
> type GUID for such partitions, of course also set those partitions'
> type GUID appropriately.
> 
> This has worked just fine, but, it turns out, only because I've never
> had CONFIG_PARTITION_TYPE_GUID enabled on any of my boards; I've
> always just set the offsets of the two partitions via the config
> variables CONFIG_ENV_OFFSET(,_REDUND).
> 
> [...]

Applied to u-boot/next, thanks!

-- 
Tom