[PATCH 0/7] disk: don't compile in partition support for spl/tpl if not really necessary

[PATCH 0/7] disk: don't compile in partition support for spl/tpl if not really necessary

[AKASHI Takahiro]

This is a reworked version of my RFC[1].
When my patch[2] is applied, the size of SPL code on some platform might
exceed the limit of ROM size due to partition support code (disk/*) being
built in even if none of partition table types (CONFIG_*_PARTITION) is
enabled on that particular platform.

This is generally inconvenient and should be fixed.
The patch#7 is a core part to fix the issue and the rest of patches
are to fix related build errors revealed/caused by this change.

[1] https://lists.denx.de/pipermail/u-boot/2022-April/481531.html
[2] https://lists.denx.de/pipermail/u-boot/2022-April/481532.html

Test:
=====
* Azure CI passed[3]

[3] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=4092&view=results


Change history:
===============
v1 (Apr, 18, 2022)
* split the original RFC into a set of patches
* remove hunks against CMD_PART and cortina_presidio-asic-emmc_defconfig
  which are no longer needed.
* use 'imply' rather than 'select' for dependency of PARTITION_UUIDS
  at EFI_LOADER, dropping DOS_PARTITON (patch#5)
* compile efi_disk.c even if BLK without PARTITION (patch#6)

RFC (Apr 15, 2021)
* initial RFC

AKASHI Takahiro (7):
  disk: include errno.h explicitly in part.h
  disk: enable function prototypes in part.h for SPL/TPL
  disk: define nullified functions for !PARTITIONS
  sandbox: move a function prototype
  efi_loader: PARTITION_UUIDS should be optional
  efi_loader: disk: compile efi_disk when CONFIG_BLK
  disk: don't compile in partition support for spl/tpl if not really
    necessary

 disk/Kconfig                     | 37 ++++++++++++++++----------------
 include/part.h                   | 15 ++++++++++---
 include/sandboxblockdev.h        |  2 ++
 lib/efi_loader/Kconfig           |  2 +-
 lib/efi_loader/Makefile          |  2 +-
 lib/efi_loader/efi_device_path.c | 11 +++++++++-
 6 files changed, 45 insertions(+), 24 deletions(-)

-- 
2.33.0

[PATCH 1/7] disk: include errno.h explicitly in part.h

[AKASHI Takahiro]

Some errno numbers are used in defining inline functions.
So "errno.h" should be explicitly included to avoid possible build errors.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/part.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/part.h b/include/part.h
index 53cfbdd87671..1196220817e4 100644
--- a/include/part.h
+++ b/include/part.h
@@ -10,6 +10,7 @@
 #include <ide.h>
 #include <uuid.h>
 #include <linker_lists.h>
+#include <linux/errno.h>
 #include <linux/list.h>
 
 struct block_drvr {
-- 
2.33.0

[PATCH 2/7] disk: enable function prototypes in part.h for SPL/TPL

[AKASHI Takahiro]

Since CONFIG_[SPL|TPL]_PARTITIONS were introduced, part.h has not been
updated. Due to this, while the build won't fail, some functionality may
possibly break as some partition-related functions are nullified even
though some partition table types are enabled for SPL/TPL.

Fixes: commit 88ca8e26958b ("disk: Add an option for partitions in SPL")
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/part.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/part.h b/include/part.h
index 1196220817e4..9975fad97121 100644
--- a/include/part.h
+++ b/include/part.h
@@ -87,7 +87,7 @@ struct disk_part {
 };
 
 /* Misc _get_dev functions */
-#ifdef CONFIG_PARTITIONS
+#if CONFIG_IS_ENABLED(PARTITIONS)
 /**
  * blk_get_dev() - get a pointer to a block device given its type and number
  *
@@ -497,7 +497,7 @@ int layout_mbr_partitions(struct disk_partition *p, int count,
 
 #endif
 
-#ifdef CONFIG_PARTITIONS
+#if CONFIG_IS_ENABLED(PARTITIONS)
 /**
  * part_driver_get_count() - get partition driver count
  *
-- 
2.33.0

[PATCH 3/7] disk: define nullified functions for !PARTITIONS

[AKASHI Takahiro]

Some defconfig enables CMD_PART even if none of any partition table
types (CONFIG_*_PARTITION) are enabled.
This will lead to the size growth in SPL/TPL code since disk/part.c
will be compiled in any way.
We will change disk/Kconfig later so that CONFIG_PARTITIONS is only
enabled when, at least, one of CONFIG_*_PARTITION is enabled.

To make the build work (in particular, "part" command) correctly,
a few functions should be defined as void functions in case of
!CONFIG_PARTITIONS.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/part.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/part.h b/include/part.h
index 9975fad97121..5f47a76bc19b 100644
--- a/include/part.h
+++ b/include/part.h
@@ -276,6 +276,15 @@ static inline int blk_get_device_part_str(const char *ifname,
 					  struct disk_partition *info,
 					  int allow_whole_dev)
 { *dev_desc = NULL; return -1; }
+static inline int part_get_info_by_name_type(struct blk_desc *dev_desc,
+					     const char *name,
+					     struct disk_partition *info,
+					     int part_type)
+{ return -ENOENT; }
+static inline int part_get_info_by_name(struct blk_desc *dev_desc,
+					const char *name,
+					struct disk_partition *info)
+{ return -ENOENT; }
 static inline int
 part_get_info_by_dev_and_name_or_num(const char *dev_iface,
 				     const char *dev_part_str,
-- 
2.33.0

Re: [PATCH 3/7] disk: define nullified functions for !PARTITIONS

[Tom Rini]

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

On Tue, Apr 19, 2022 at 10:01:54AM +0900, AKASHI Takahiro wrote:

> Some defconfig enables CMD_PART even if none of any partition table
> types (CONFIG_*_PARTITION) are enabled.
> This will lead to the size growth in SPL/TPL code since disk/part.c
> will be compiled in any way.
> We will change disk/Kconfig later so that CONFIG_PARTITIONS is only
> enabled when, at least, one of CONFIG_*_PARTITION is enabled.
> 
> To make the build work (in particular, "part" command) correctly,
> a few functions should be defined as void functions in case of
> !CONFIG_PARTITIONS.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

I guess I wonder why we don't just make CMD_PART depend on PARTITIONS
now and thus correct the few (single?) board that has this enabled
without underlying partition code by removing the can't be functional
cmd.

-- 
Tom

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

Re: [PATCH 3/7] disk: define nullified functions for !PARTITIONS

[AKASHI Takahiro]

On Mon, Apr 18, 2022 at 11:09:38PM -0400, Tom Rini wrote:
> On Tue, Apr 19, 2022 at 10:01:54AM +0900, AKASHI Takahiro wrote:
> 
> > Some defconfig enables CMD_PART even if none of any partition table
> > types (CONFIG_*_PARTITION) are enabled.
> > This will lead to the size growth in SPL/TPL code since disk/part.c
> > will be compiled in any way.
> > We will change disk/Kconfig later so that CONFIG_PARTITIONS is only
> > enabled when, at least, one of CONFIG_*_PARTITION is enabled.
> > 
> > To make the build work (in particular, "part" command) correctly,
> > a few functions should be defined as void functions in case of
> > !CONFIG_PARTITIONS.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> I guess I wonder why we don't just make CMD_PART depend on PARTITIONS
> now and thus correct the few (single?) board that has this enabled
> without underlying partition code by removing the can't be functional
> cmd.

Well, that is partially what I did in my RFC and I thought
that you declined to accept my change.
Did I misunderstand you?

-Takahiro Akashi

> -- 
> Tom

Re: [PATCH 3/7] disk: define nullified functions for !PARTITIONS

[Tom Rini]

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

On Tue, Apr 19, 2022 at 01:11:23PM +0900, AKASHI Takahiro wrote:
> On Mon, Apr 18, 2022 at 11:09:38PM -0400, Tom Rini wrote:
> > On Tue, Apr 19, 2022 at 10:01:54AM +0900, AKASHI Takahiro wrote:
> > 
> > > Some defconfig enables CMD_PART even if none of any partition table
> > > types (CONFIG_*_PARTITION) are enabled.
> > > This will lead to the size growth in SPL/TPL code since disk/part.c
> > > will be compiled in any way.
> > > We will change disk/Kconfig later so that CONFIG_PARTITIONS is only
> > > enabled when, at least, one of CONFIG_*_PARTITION is enabled.
> > > 
> > > To make the build work (in particular, "part" command) correctly,
> > > a few functions should be defined as void functions in case of
> > > !CONFIG_PARTITIONS.
> > > 
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > 
> > I guess I wonder why we don't just make CMD_PART depend on PARTITIONS
> > now and thus correct the few (single?) board that has this enabled
> > without underlying partition code by removing the can't be functional
> > cmd.
> 
> Well, that is partially what I did in my RFC and I thought
> that you declined to accept my change.
> Did I misunderstand you?

Yes, I wasn't clear, sorry for the confusion.  Just this part of the
series should be replaced with making CMD_PART depend on PARTITIONS and
if there really is a use case for 'part' without PARTITION support
(rather than it being an unintentionally enabled feature) we can deal
with it then.  The rest of the series looks good to me and I'll let
Heinrich comment on the EFI specific parts.

-- 
Tom

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

Re: [PATCH 3/7] disk: define nullified functions for !PARTITIONS

[AKASHI Takahiro]

Hi Tom,

On Tue, Apr 19, 2022 at 08:12:07AM -0400, Tom Rini wrote:
> On Tue, Apr 19, 2022 at 01:11:23PM +0900, AKASHI Takahiro wrote:
> > On Mon, Apr 18, 2022 at 11:09:38PM -0400, Tom Rini wrote:
> > > On Tue, Apr 19, 2022 at 10:01:54AM +0900, AKASHI Takahiro wrote:
> > > 
> > > > Some defconfig enables CMD_PART even if none of any partition table
> > > > types (CONFIG_*_PARTITION) are enabled.
> > > > This will lead to the size growth in SPL/TPL code since disk/part.c
> > > > will be compiled in any way.
> > > > We will change disk/Kconfig later so that CONFIG_PARTITIONS is only
> > > > enabled when, at least, one of CONFIG_*_PARTITION is enabled.
> > > > 
> > > > To make the build work (in particular, "part" command) correctly,
> > > > a few functions should be defined as void functions in case of
> > > > !CONFIG_PARTITIONS.
> > > > 
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > 
> > > I guess I wonder why we don't just make CMD_PART depend on PARTITIONS
> > > now and thus correct the few (single?) board that has this enabled
> > > without underlying partition code by removing the can't be functional
> > > cmd.
> > 
> > Well, that is partially what I did in my RFC and I thought
> > that you declined to accept my change.
> > Did I misunderstand you?
> 
> Yes, I wasn't clear, sorry for the confusion.  Just this part of the
> series should be replaced with making CMD_PART depend on PARTITIONS and
> if there really is a use case for 'part' without PARTITION support
> (rather than it being an unintentionally enabled feature) we can deal
> with it then.  The rest of the series looks good to me and I'll let
> Heinrich comment on the EFI specific parts.

I do understand what you expect here, but, what I call, "nullified
function" technique is already used in several places.
For instance, take blk_get_device_part_str() function which has
a nullified version of definition in include/part.h.
It is used without explicit dependency on CONFIG_PARTITIONS at:
        cmd/zfs.c
        cmd/disk.c
        cmd/reiser.c
        cmd/fat.c
        env/ext4.c
        env/fat.c

So I would like to propose to create another patch that you expect (and
what I did in RFC) instead of removing this patch because the latter
has no negative effect.

If you agree, I will post it as a separate patch.


-Takahiro Akashi

> -- 
> Tom

Re: [PATCH 3/7] disk: define nullified functions for !PARTITIONS

[Tom Rini]

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

On Wed, Apr 20, 2022 at 11:17:21AM +0900, AKASHI Takahiro wrote:
> Hi Tom,
> 
> On Tue, Apr 19, 2022 at 08:12:07AM -0400, Tom Rini wrote:
> > On Tue, Apr 19, 2022 at 01:11:23PM +0900, AKASHI Takahiro wrote:
> > > On Mon, Apr 18, 2022 at 11:09:38PM -0400, Tom Rini wrote:
> > > > On Tue, Apr 19, 2022 at 10:01:54AM +0900, AKASHI Takahiro wrote:
> > > > 
> > > > > Some defconfig enables CMD_PART even if none of any partition table
> > > > > types (CONFIG_*_PARTITION) are enabled.
> > > > > This will lead to the size growth in SPL/TPL code since disk/part.c
> > > > > will be compiled in any way.
> > > > > We will change disk/Kconfig later so that CONFIG_PARTITIONS is only
> > > > > enabled when, at least, one of CONFIG_*_PARTITION is enabled.
> > > > > 
> > > > > To make the build work (in particular, "part" command) correctly,
> > > > > a few functions should be defined as void functions in case of
> > > > > !CONFIG_PARTITIONS.
> > > > > 
> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > 
> > > > I guess I wonder why we don't just make CMD_PART depend on PARTITIONS
> > > > now and thus correct the few (single?) board that has this enabled
> > > > without underlying partition code by removing the can't be functional
> > > > cmd.
> > > 
> > > Well, that is partially what I did in my RFC and I thought
> > > that you declined to accept my change.
> > > Did I misunderstand you?
> > 
> > Yes, I wasn't clear, sorry for the confusion.  Just this part of the
> > series should be replaced with making CMD_PART depend on PARTITIONS and
> > if there really is a use case for 'part' without PARTITION support
> > (rather than it being an unintentionally enabled feature) we can deal
> > with it then.  The rest of the series looks good to me and I'll let
> > Heinrich comment on the EFI specific parts.
> 
> I do understand what you expect here, but, what I call, "nullified
> function" technique is already used in several places.
> For instance, take blk_get_device_part_str() function which has
> a nullified version of definition in include/part.h.
> It is used without explicit dependency on CONFIG_PARTITIONS at:
>         cmd/zfs.c
>         cmd/disk.c
>         cmd/reiser.c
>         cmd/fat.c
>         env/ext4.c
>         env/fat.c
> 
> So I would like to propose to create another patch that you expect (and
> what I did in RFC) instead of removing this patch because the latter
> has no negative effect.
> 
> If you agree, I will post it as a separate patch.

OK, lets go with that then, thanks!

-- 
Tom

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

[PATCH 4/7] sandbox: move a function prototype

[AKASHI Takahiro]

Since host_get_dev_errr() is defined in drivers/block/sandbox.c,
the associated function prototype should be in a more appropriate
header file.

Fixes: commit 4101f6879256 ("dm: Drop the block_dev_desc_t typedef")
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/part.h            | 1 -
 include/sandboxblockdev.h | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/part.h b/include/part.h
index 5f47a76bc19b..e5806edb3452 100644
--- a/include/part.h
+++ b/include/part.h
@@ -104,7 +104,6 @@ struct disk_part {
 struct blk_desc *blk_get_dev(const char *ifname, int dev);
 
 struct blk_desc *mg_disk_get_dev(int dev);
-int host_get_dev_err(int dev, struct blk_desc **blk_devp);
 
 /* disk/part.c */
 int part_get_info(struct blk_desc *dev_desc, int part,
diff --git a/include/sandboxblockdev.h b/include/sandboxblockdev.h
index 4ca9554e38a0..dc983f0417b2 100644
--- a/include/sandboxblockdev.h
+++ b/include/sandboxblockdev.h
@@ -26,4 +26,6 @@ struct host_block_dev {
  */
 int host_dev_bind(int dev, char *filename, bool removable);
 
+int host_get_dev_err(int dev, struct blk_desc **blk_devp);
+
 #endif
-- 
2.33.0

[PATCH 5/7] efi_loader: PARTITION_UUIDS should be optional

[AKASHI Takahiro]

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.akashi@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:
 			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)) {
 				log_warning(
 					"Partition no. %d: invalid guid: %s\n",
 					part, info.uuid);
+				memset(hddp->partition_signature, 0,
+				       sizeof(hddp->partition_signature));
+			}
+#else
+			memset(hddp->partition_signature, 0,
+			       sizeof(hddp->partition_signature));
+#endif
 			break;
 		}
 
-- 
2.33.0

Re: [PATCH 5/7] efi_loader: PARTITION_UUIDS should be optional

[Heinrich Schuchardt]

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.akashi@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.

>   			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.

>   				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?

Shouldn't we select CONFIG_PARTITION_UUIDS if CONFIG_EFI_LOADER=y and
CONFIG_EFI_PARTITION=y.

Best regards

Heinrich

> +#endif
>   			break;
>   		}
>

Re: [PATCH 5/7] efi_loader: PARTITION_UUIDS should be optional

[AKASHI Takahiro]

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.akashi@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;
> >   		}
> > 
> 

Re: [PATCH 5/7] efi_loader: PARTITION_UUIDS should be optional

[Tom Rini]

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

On Thu, Apr 21, 2022 at 09:57:50AM +0900, AKASHI Takahiro wrote:
> 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.akashi@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.

I wouldn't assume that they're correct in doing so and it might have
been how some other problem was addressed at the time to fix incorrect /
not yet implemented in Kconfig at the time dependencies.

-- 
Tom

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

[PATCH 6/7] efi_loader: disk: compile efi_disk when CONFIG_BLK

[AKASHI Takahiro]

Now we can build efi_loader with block device support (CONFIG_BLK) and
without CONFIG_PARTITIONS.
So change Makefile.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 034d26cf0109..aaaa25cefe01 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -68,7 +68,7 @@ obj-y += efi_watchdog.o
 obj-$(CONFIG_EFI_ESRT) += efi_esrt.o
 obj-$(CONFIG_LCD) += efi_gop.o
 obj-$(CONFIG_DM_VIDEO) += efi_gop.o
-obj-$(CONFIG_PARTITIONS) += efi_disk.o
+obj-$(CONFIG_BLK) += efi_disk.o
 obj-$(CONFIG_NET) += efi_net.o
 obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
 obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
-- 
2.33.0

Re: [PATCH 6/7] efi_loader: disk: compile efi_disk when CONFIG_BLK

[Heinrich Schuchardt]

On 4/19/22 03:01, AKASHI Takahiro wrote:
> Now we can build efi_loader with block device support (CONFIG_BLK) and
> without CONFIG_PARTITIONS.
> So change Makefile.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   lib/efi_loader/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index 034d26cf0109..aaaa25cefe01 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -68,7 +68,7 @@ obj-y += efi_watchdog.o
>   obj-$(CONFIG_EFI_ESRT) += efi_esrt.o
>   obj-$(CONFIG_LCD) += efi_gop.o
>   obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> -obj-$(CONFIG_PARTITIONS) += efi_disk.o
> +obj-$(CONFIG_BLK) += efi_disk.o

Why do we need efi_disk.o if CONFIG_PARTITIONS=n?

How can we be UEFI compliant without partition support?

Best regards

Heinrich

>   obj-$(CONFIG_NET) += efi_net.o
>   obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
>   obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o

Re: [PATCH 6/7] efi_loader: disk: compile efi_disk when CONFIG_BLK

[AKASHI Takahiro]

On Wed, Apr 20, 2022 at 09:42:00AM +0200, Heinrich Schuchardt wrote:
> On 4/19/22 03:01, AKASHI Takahiro wrote:
> > Now we can build efi_loader with block device support (CONFIG_BLK) and
> > without CONFIG_PARTITIONS.
> > So change Makefile.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   lib/efi_loader/Makefile | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index 034d26cf0109..aaaa25cefe01 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -68,7 +68,7 @@ obj-y += efi_watchdog.o
> >   obj-$(CONFIG_EFI_ESRT) += efi_esrt.o
> >   obj-$(CONFIG_LCD) += efi_gop.o
> >   obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> > -obj-$(CONFIG_PARTITIONS) += efi_disk.o
> > +obj-$(CONFIG_BLK) += efi_disk.o
> 
> Why do we need efi_disk.o if CONFIG_PARTITIONS=n?

Why do you think so?
As you admitted in your comment at:
https://lists.denx.de/pipermail/u-boot/2022-April/481546.html

GPT(EFI_PARTITION) is optional under the current implementation
("EFI_LOADER should imply EFI_PARTITION" in your words).
As a matter of fact, EFI_LOADER (and efi_disk.o) can be compiled in
even without any partition table types.

If you don't agree, please drop this patch.
I don't care because it won't help reducing SPL code size anyway.

-Takahiro Akashi

> How can we be UEFI compliant without partition support?
> 
> Best regards
> 
> Heinrich
> 
> >   obj-$(CONFIG_NET) += efi_net.o
> >   obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
> >   obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
> 

Re: [PATCH 6/7] efi_loader: disk: compile efi_disk when CONFIG_BLK

[Heinrich Schuchardt]

On 4/21/22 02:30, AKASHI Takahiro wrote:
> On Wed, Apr 20, 2022 at 09:42:00AM +0200, Heinrich Schuchardt wrote:
>> On 4/19/22 03:01, AKASHI Takahiro wrote:
>>> Now we can build efi_loader with block device support (CONFIG_BLK) and
>>> without CONFIG_PARTITIONS.
>>> So change Makefile.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>    lib/efi_loader/Makefile | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>>> index 034d26cf0109..aaaa25cefe01 100644
>>> --- a/lib/efi_loader/Makefile
>>> +++ b/lib/efi_loader/Makefile
>>> @@ -68,7 +68,7 @@ obj-y += efi_watchdog.o
>>>    obj-$(CONFIG_EFI_ESRT) += efi_esrt.o
>>>    obj-$(CONFIG_LCD) += efi_gop.o
>>>    obj-$(CONFIG_DM_VIDEO) += efi_gop.o
>>> -obj-$(CONFIG_PARTITIONS) += efi_disk.o
>>> +obj-$(CONFIG_BLK) += efi_disk.o
>>
>> Why do we need efi_disk.o if CONFIG_PARTITIONS=n?
>
> Why do you think so?
> As you admitted in your comment at:
> https://lists.denx.de/pipermail/u-boot/2022-April/481546.html
>
> GPT(EFI_PARTITION) is optional under the current implementation
> ("EFI_LOADER should imply EFI_PARTITION" in your words).
> As a matter of fact, EFI_LOADER (and efi_disk.o) can be compiled in
> even without any partition table types.
>
> If you don't agree, please drop this patch.
> I don't care because it won't help reducing SPL code size anyway.

Without this patch db-88f6720_defconfig does not build after applying
the rest of the series:

arm-linux-gnueabi-ld.bfd: lib/efi_loader/efi_file.o: in function
`efi_file_from_path':
lib/efi_loader/efi_file.c:1095: undefined reference to `efi_fs_from_path'
arm-linux-gnueabi-ld.bfd: lib/efi_loader/efi_var_file.o: in function
`efi_set_blk_dev_to_system_partition':
lib/efi_loader/efi_var_file.c:54: undefined reference to
`efi_system_partition'

Best regards

Heinrich

>
> -Takahiro Akashi
>
>> How can we be UEFI compliant without partition support?
>>
>> Best regards
>>
>> Heinrich
>>
>>>    obj-$(CONFIG_NET) += efi_net.o
>>>    obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
>>>    obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
>>

[PATCH 7/7] disk: don't compile in partition support for spl/tpl if not really necessary

[AKASHI Takahiro]

Under the current Kconfigs, disk/part.c will be compiled in even if none of
partition table types are enabled. This will lead to the size growth of SPL
/TPL code.

With this patch, CONFIG_PARTITIONS is selected only if, at least, one of
CONFIG_*_PARTITION is enabled.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 disk/Kconfig | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/disk/Kconfig b/disk/Kconfig
index 13700322e976..359af3b27e6d 100644
--- a/disk/Kconfig
+++ b/disk/Kconfig
@@ -2,8 +2,7 @@
 menu "Partition Types"
 
 config PARTITIONS
-	bool "Enable Partition Labels (disklabels) support"
-	default y
+	bool
 	help
 	  Partition Labels (disklabels) Supported:
 	  Zero or more of the following:
@@ -20,8 +19,7 @@ config PARTITIONS
 	  as well.
 
 config SPL_PARTITIONS
-	bool "Enable Partition Labels (disklabels) support in SPL"
-	default y if PARTITIONS
+	bool
 	select SPL_SPRINTF
 	select SPL_STRTO
 	help
@@ -30,8 +28,7 @@ config SPL_PARTITIONS
 	  small amount of size to SPL, typically 500 bytes.
 
 config TPL_PARTITIONS
-	bool "Enable Partition Labels (disklabels) support in TPL"
-	default y if PARTITIONS
+	bool
 	select TPL_SPRINTF
 	select TPL_STRTO
 	help
@@ -41,57 +38,61 @@ config TPL_PARTITIONS
 
 config MAC_PARTITION
 	bool "Enable Apple's MacOS partition table"
-	depends on PARTITIONS
+	select PARTITIONS
 	help
 	  Say Y here if you would like to use device under U-Boot which
 	  were partitioned on a Macintosh.
 
 config SPL_MAC_PARTITION
 	bool "Enable Apple's MacOS partition table for SPL"
-	depends on SPL && PARTITIONS
+	depends on SPL
 	default y if MAC_PARTITION
+	select SPL_PARTITIONS
 
 config DOS_PARTITION
 	bool "Enable MS Dos partition table"
-	depends on PARTITIONS
 	default y if DISTRO_DEFAULTS
 	default y if x86 || CMD_FAT || USB_STORAGE
+	select PARTITIONS
 	help
 	  traditional on the Intel architecture, USB sticks, etc.
 
 config SPL_DOS_PARTITION
 	bool "Enable MS Dos partition table for SPL"
-	depends on SPL && PARTITIONS
+	depends on SPL
 	default n if ARCH_SUNXI
 	default y if DOS_PARTITION
+	select SPL_PARTITIONS
 
 config ISO_PARTITION
 	bool "Enable ISO partition table"
-	depends on PARTITIONS
 	default y if DISTRO_DEFAULTS
 	default y if MIPS || ARCH_TEGRA
+	select PARTITIONS
 
 config SPL_ISO_PARTITION
 	bool "Enable ISO partition table for SPL"
-	depends on SPL && PARTITIONS
+	depends on SPL
+	select SPL_PARTITIONS
 
 config AMIGA_PARTITION
 	bool "Enable AMIGA partition table"
-	depends on PARTITIONS
+	select PARTITIONS
 	help
 	  Say Y here if you would like to use device under U-Boot which
 	  were partitioned under AmigaOS.
 
 config SPL_AMIGA_PARTITION
 	bool "Enable AMIGA partition table for SPL"
-	depends on SPL && PARTITIONS
+	depends on SPL
 	default y if AMIGA_PARTITION
+	select SPL_PARTITIONS
 
 config EFI_PARTITION
 	bool "Enable EFI GPT partition table"
-	depends on PARTITIONS
 	default y if DISTRO_DEFAULTS
 	default y if ARCH_TEGRA
+	select PARTITIONS
 	select LIB_UUID
 	help
 	  Say Y here if you would like to use device under U-Boot which
@@ -128,9 +129,10 @@ config EFI_PARTITION_ENTRIES_OFF
 
 config SPL_EFI_PARTITION
 	bool "Enable EFI GPT partition table for SPL"
-	depends on  SPL && PARTITIONS
+	depends on  SPL
 	default n if ARCH_SUNXI
 	default y if EFI_PARTITION
+	select SPL_PARTITIONS
 
 config PARTITION_UUIDS
 	bool "Enable support of UUID for partition"
@@ -143,12 +145,11 @@ config PARTITION_UUIDS
 
 config SPL_PARTITION_UUIDS
 	bool "Enable support of UUID for partition in SPL"
-	depends on SPL && PARTITIONS
+	depends on SPL_PARTITIONS
 	default y if SPL_EFI_PARTITION
 
 config PARTITION_TYPE_GUID
 	bool "Enable support of GUID for partition type"
-	depends on PARTITIONS
 	depends on EFI_PARTITION
 	help
 	  Activate the configuration of GUID type
-- 
2.33.0