| Message ID | 20240912134141.4143855-2-rasmus.villemoes@prevas.dk |
|---|---|
| State | Accepted |
| Commit | d7c59bfc3b20bc6d602b33ac24d7ae8698650b87 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: mmc: fix use of two separate partitions with proper type GUID | expand |
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
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
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
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);
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(-)