On Wed, Apr 20, 2022 at 09:37:39AM +0200, Heinrich Schuchardt wrote: > On 4/19/22 03:01, AKASHI Takahiro wrote: > > In the current implementation, partition table support (either GPT or DOS) > > is not mandatory. So CONFIG_PARTITION_UUIDS should not be enabled > > (selected) unconditionally. > > > > Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path > > for SIG_TYPE_GUID") > > Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org> > > --- > > lib/efi_loader/Kconfig | 2 +- > > lib/efi_loader/efi_device_path.c | 11 ++++++++++- > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > index d50cd2563d3d..bc518d7a413b 100644 > > --- a/lib/efi_loader/Kconfig > > +++ b/lib/efi_loader/Kconfig > > @@ -15,7 +15,7 @@ config EFI_LOADER > > depends on !EFI_APP > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > select LIB_UUID > > - select PARTITION_UUIDS > > + imply PARTITION_UUIDS > > select HAVE_BLOCK_DEVICE > > select REGEX > > imply FAT > > diff --git a/lib/efi_loader/efi_device_path.c > > b/lib/efi_loader/efi_device_path.c > > index 0542aaae16c7..01fb53ae8c40 100644 > > --- a/lib/efi_loader/efi_device_path.c > > +++ b/lib/efi_loader/efi_device_path.c > > @@ -864,11 +864,20 @@ static void *dp_part_node(void *buf, struct blk_desc > > *desc, int part) > > break; > > case SIG_TYPE_GUID: > > This can only be reached for CONFIG_EFI_PARTITION=y.
I know, but the code won't be exercised anyway if !CONFIG_EFI_PARTITION. > > hddp->signature_type = 2; > > +#if CONFIG_IS_ENABLED(PARTITION_UUIDS) > > + /* info.uuid exists only with PARTITION_UUIDS */ > > if (uuid_str_to_bin(info.uuid, > > - hddp->partition_signature, 1)) > > + hddp->partition_signature, 1)) { > > uuid_guid_get_bin() does not write any byte if info.uuid is not valid. What matters here is that "uuid" in struct disk_partition is defined only if CONFIG_PARTITION_UUIDS. > > log_warning( > > "Partition no. %d: invalid guid: %s\n", > > part, info.uuid); > > > + memset(hddp->partition_signature, 0, > > + sizeof(hddp->partition_signature)); > > dp_alloc() already has zeroed out the memory. Please, remove the > superfluous code. > > > + } > > +#else > > + memset(hddp->partition_signature, 0, > > + sizeof(hddp->partition_signature)); > > ditto. > > > This does not conform to the UEFI specification. Why should we support it? As I said in my previous comment at patch#6, CONFIG_EFI_PARTITION can be seen optional under the current implementation. > > Shouldn't we select CONFIG_PARTITION_UUIDS if CONFIG_EFI_LOADER=y and > CONFIG_EFI_PARTITION=y. Unfortunately some platforms enable (in other words, "select") PARTITION_UUIDS unconditionally even without explicitly enabling EFI_LOADER or EFI_PARTITION. -Takahiro Akashi > Best regards > > Heinrich > > > +#endif > > break; > > } > > >