diff mbox series

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

Message ID 20220415071156.122261-1-takahiro.akashi@linaro.org
State New
Headers show
Series [RFC] disk: don't compile in partition support for spl/tpl if not really necessary | expand

Commit Message

AKASHI Takahiro April 15, 2022, 7:11 a.m. UTC 
We see some increase of spl code size due to partition support (disk/*)
while none of particular partition types (CONFIG_SPL_XXX_PARTITION) are
enabled.
With this patch applied, part.c is no longer included unless really
necessary.

In addition, fix errors in CI build revealed after this change is made.

Fixes: commit 88ca8e26958b ("disk: Add an option for partitions in SPL")
Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path
	for SIG_TYPE_GUID")
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/Kconfig                                  |  1 +
 configs/cortina_presidio-asic-emmc_defconfig |  1 -
 disk/Kconfig                                 | 37 ++++++++++----------
 fs/fat/fat.c                                 |  3 +-
 include/part.h                               | 14 ++++++--
 include/sandboxblockdev.h                    |  2 ++
 lib/efi_loader/Kconfig                       |  1 +
 7 files changed, 37 insertions(+), 22 deletions(-)

Comments

Heinrich Schuchardt April 15, 2022, 8:21 a.m. UTC | #1 
On 4/15/22 09:11, AKASHI Takahiro wrote:
> We see some increase of spl code size due to partition support (disk/*)
> while none of particular partition types (CONFIG_SPL_XXX_PARTITION) are
> enabled.
> With this patch applied, part.c is no longer included unless really
> necessary.
>
> In addition, fix errors in CI build revealed after this change is made.
>
> Fixes: commit 88ca8e26958b ("disk: Add an option for partitions in SPL")
> Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path
> 	for SIG_TYPE_GUID")
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/Kconfig                                  |  1 +
>   configs/cortina_presidio-asic-emmc_defconfig |  1 -
>   disk/Kconfig                                 | 37 ++++++++++----------
>   fs/fat/fat.c                                 |  3 +-
>   include/part.h                               | 14 ++++++--
>   include/sandboxblockdev.h                    |  2 ++
>   lib/efi_loader/Kconfig                       |  1 +
>   7 files changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index d3abe3a06bff..b69c26912568 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1239,6 +1239,7 @@ config CMD_OSD
>
>   config CMD_PART
>   	bool "part"
> +	depends on PARTITIONS
>   	select HAVE_BLOCK_DEVICE
>   	select PARTITION_UUIDS
>   	help
> diff --git a/configs/cortina_presidio-asic-emmc_defconfig b/configs/cortina_presidio-asic-emmc_defconfig
> index c22dcef7ec05..c217a00a1cf0 100644
> --- a/configs/cortina_presidio-asic-emmc_defconfig
> +++ b/configs/cortina_presidio-asic-emmc_defconfig
> @@ -18,7 +18,6 @@ CONFIG_LAST_STAGE_INIT=y
>   CONFIG_SYS_PROMPT="G3#"
>   CONFIG_CMD_I2C=y
>   CONFIG_CMD_MMC=y
> -CONFIG_CMD_PART=y
>   CONFIG_CMD_WDT=y
>   CONFIG_BOOTP_BOOTFILESIZE=y
>   CONFIG_CMD_CACHE=y

This change seems to be unrelated to the commit message.

Why would you remove the command on a board that supports EXT2 and EXT4
as well as partitions?

> 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
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index df9ea2c028fc..a7ec1c4b759c 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -95,7 +95,8 @@ int fat_register_device(struct blk_desc *dev_desc, int part_no)
>   	cur_dev = NULL;
>
>   	/* Read the partition table, if present */
> -	if (part_get_info(dev_desc, part_no, &info)) {
> +	if (CONFIG_IS_ENABLED(DOS_PARTITION) &&
> +	    part_get_info(dev_desc, part_no, &info)) {
>   		if (part_no != 0) {
>   			printf("** Partition %d not valid on device %d **\n",
>   					part_no, dev_desc->devnum);
> diff --git a/include/part.h b/include/part.h
> index 36b76f00563f..612d4c32b5c7 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 {
> @@ -86,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
>    *
> @@ -275,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,
> @@ -513,7 +523,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
>    *
> 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
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 7dc24fbf0aa9..b615abf598b9 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -17,6 +17,7 @@ config EFI_LOADER
>   	select DM_EVENT
>   	select EVENT_DYNAMIC
>   	select LIB_UUID
> +	select DOS_PARTITION

EFI_LOADER should imply EFI_PARTITION. MBR support is not required by
the UEFI specification. You may still want to imply DOS_PARTITION.

As disk/Kconfig already has 'default y if' for both I would prefer to
add EFI_LOADER there.

diff --git a/disk/Kconfig b/disk/Kconfig
index 13700322e9..87b2e52af4 100644
--- a/disk/Kconfig
+++ b/disk/Kconfig
@@ -54,7 +54,7 @@ config SPL_MAC_PARTITION
  config DOS_PARTITION
         bool "Enable MS Dos partition table"
         depends on PARTITIONS
-       default y if DISTRO_DEFAULTS
+       default y if DISTRO_DEFAULTS || EFI_LOADER
         default y if x86 || CMD_FAT || USB_STORAGE
         help
           traditional on the Intel architecture, USB sticks, etc.
@@ -90,7 +90,7 @@ config SPL_AMIGA_PARTITION
  config EFI_PARTITION
         bool "Enable EFI GPT partition table"
         depends on PARTITIONS
-       default y if DISTRO_DEFAULTS
+       default y if DISTRO_DEFAULTS || EFI_LOADER
         default y if ARCH_TEGRA
         select LIB_UUID
         help

Best regards

Heinrich

>   	select PARTITION_UUIDS
>   	select HAVE_BLOCK_DEVICE
>   	select REGEX
Tom Rini April 15, 2022, 12:32 p.m. UTC | #2 
On Fri, Apr 15, 2022 at 10:21:39AM +0200, Heinrich Schuchardt wrote:
> On 4/15/22 09:11, AKASHI Takahiro wrote:
> > We see some increase of spl code size due to partition support (disk/*)
> > while none of particular partition types (CONFIG_SPL_XXX_PARTITION) are
> > enabled.
> > With this patch applied, part.c is no longer included unless really
> > necessary.
> > 
> > In addition, fix errors in CI build revealed after this change is made.
> > 
> > Fixes: commit 88ca8e26958b ("disk: Add an option for partitions in SPL")
> > Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path
> > 	for SIG_TYPE_GUID")
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   cmd/Kconfig                                  |  1 +
> >   configs/cortina_presidio-asic-emmc_defconfig |  1 -
> >   disk/Kconfig                                 | 37 ++++++++++----------
> >   fs/fat/fat.c                                 |  3 +-
> >   include/part.h                               | 14 ++++++--
> >   include/sandboxblockdev.h                    |  2 ++
> >   lib/efi_loader/Kconfig                       |  1 +
> >   7 files changed, 37 insertions(+), 22 deletions(-)
> > 
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index d3abe3a06bff..b69c26912568 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1239,6 +1239,7 @@ config CMD_OSD
> > 
> >   config CMD_PART
> >   	bool "part"
> > +	depends on PARTITIONS
> >   	select HAVE_BLOCK_DEVICE
> >   	select PARTITION_UUIDS
> >   	help
> > diff --git a/configs/cortina_presidio-asic-emmc_defconfig b/configs/cortina_presidio-asic-emmc_defconfig
> > index c22dcef7ec05..c217a00a1cf0 100644
> > --- a/configs/cortina_presidio-asic-emmc_defconfig
> > +++ b/configs/cortina_presidio-asic-emmc_defconfig
> > @@ -18,7 +18,6 @@ CONFIG_LAST_STAGE_INIT=y
> >   CONFIG_SYS_PROMPT="G3#"
> >   CONFIG_CMD_I2C=y
> >   CONFIG_CMD_MMC=y
> > -CONFIG_CMD_PART=y
> >   CONFIG_CMD_WDT=y
> >   CONFIG_BOOTP_BOOTFILESIZE=y
> >   CONFIG_CMD_CACHE=y
> 
> This change seems to be unrelated to the commit message.
> 
> Why would you remove the command on a board that supports EXT2 and EXT4
> as well as partitions?

I bet this is part of re-syncing the defconfigs afterwards.  It is OK to
omit these changes in general.
Tom Rini April 15, 2022, 12:33 p.m. UTC | #3 
On Fri, Apr 15, 2022 at 04:11:56PM +0900, AKASHI Takahiro wrote:

> We see some increase of spl code size due to partition support (disk/*)
> while none of particular partition types (CONFIG_SPL_XXX_PARTITION) are
> enabled.
> With this patch applied, part.c is no longer included unless really
> necessary.
> 
> In addition, fix errors in CI build revealed after this change is made.
> 
> Fixes: commit 88ca8e26958b ("disk: Add an option for partitions in SPL")
> Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path
> 	for SIG_TYPE_GUID")
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

I assume you did a spot-check for size changes before/after on this.  In
general, I think this is the right way forward and I'll do a world
before/after to check for any cases of "oops, we seem to have lost
functionality".
Tom Rini April 16, 2022, 3:01 a.m. UTC | #4 
On Fri, Apr 15, 2022 at 04:11:56PM +0900, AKASHI Takahiro wrote:
> We see some increase of spl code size due to partition support (disk/*)
> while none of particular partition types (CONFIG_SPL_XXX_PARTITION) are
> enabled.
> With this patch applied, part.c is no longer included unless really
> necessary.
> 
> In addition, fix errors in CI build revealed after this change is made.
> 
> Fixes: commit 88ca8e26958b ("disk: Add an option for partitions in SPL")
> Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path
> 	for SIG_TYPE_GUID")
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/Kconfig                                  |  1 +
>  configs/cortina_presidio-asic-emmc_defconfig |  1 -

So, the defconfig change here is wrong, CMD_PART isn't being implied
otherwise, and this board is part of a number of boards that had
EFI_LOADER before, but not DOS_PARTITION, and so do grow, but in a
valid/expected way.  There are also a number of boards that don't have
any partition type support set, but that too I think ends up being
correct.  The whole before/after is at
https://gist.github.com/trini/731ee8d50a9bc96b90e12860f8c53f14
AKASHI Takahiro April 18, 2022, 8:06 a.m. UTC | #5 
On Fri, Apr 15, 2022 at 10:21:39AM +0200, Heinrich Schuchardt wrote:
> On 4/15/22 09:11, AKASHI Takahiro wrote:
> > We see some increase of spl code size due to partition support (disk/*)
> > while none of particular partition types (CONFIG_SPL_XXX_PARTITION) are
> > enabled.
> > With this patch applied, part.c is no longer included unless really
> > necessary.
> > 
> > In addition, fix errors in CI build revealed after this change is made.
> > 
> > Fixes: commit 88ca8e26958b ("disk: Add an option for partitions in SPL")
> > Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path
> > 	for SIG_TYPE_GUID")
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   cmd/Kconfig                                  |  1 +
> >   configs/cortina_presidio-asic-emmc_defconfig |  1 -
> >   disk/Kconfig                                 | 37 ++++++++++----------
> >   fs/fat/fat.c                                 |  3 +-
> >   include/part.h                               | 14 ++++++--
> >   include/sandboxblockdev.h                    |  2 ++
> >   lib/efi_loader/Kconfig                       |  1 +
> >   7 files changed, 37 insertions(+), 22 deletions(-)
> > 
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index d3abe3a06bff..b69c26912568 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1239,6 +1239,7 @@ config CMD_OSD
> > 
> >   config CMD_PART
> >   	bool "part"
> > +	depends on PARTITIONS
> >   	select HAVE_BLOCK_DEVICE
> >   	select PARTITION_UUIDS
> >   	help
> > diff --git a/configs/cortina_presidio-asic-emmc_defconfig b/configs/cortina_presidio-asic-emmc_defconfig
> > index c22dcef7ec05..c217a00a1cf0 100644
> > --- a/configs/cortina_presidio-asic-emmc_defconfig
> > +++ b/configs/cortina_presidio-asic-emmc_defconfig
> > @@ -18,7 +18,6 @@ CONFIG_LAST_STAGE_INIT=y
> >   CONFIG_SYS_PROMPT="G3#"
> >   CONFIG_CMD_I2C=y
> >   CONFIG_CMD_MMC=y
> > -CONFIG_CMD_PART=y
> >   CONFIG_CMD_WDT=y
> >   CONFIG_BOOTP_BOOTFILESIZE=y
> >   CONFIG_CMD_CACHE=y
> 
> This change seems to be unrelated to the commit message.

I removed this option because the build failed in Azure CI due to
lack of CONFIG_PARTITIONS.
What is strange in this defconfig is that, while none of any partition table types
are enabled, but CMD_PART is enabled, which simply makes no sense.

But I will revert this change since I was able to build "part" command
even without CONFIG_PARTITIONS thanks to the change I made at include/part.h.

> Why would you remove the command on a board that supports EXT2 and EXT4
> as well as partitions?
> 
> > 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
> > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > index df9ea2c028fc..a7ec1c4b759c 100644
> > --- a/fs/fat/fat.c
> > +++ b/fs/fat/fat.c
> > @@ -95,7 +95,8 @@ int fat_register_device(struct blk_desc *dev_desc, int part_no)
> >   	cur_dev = NULL;
> > 
> >   	/* Read the partition table, if present */
> > -	if (part_get_info(dev_desc, part_no, &info)) {
> > +	if (CONFIG_IS_ENABLED(DOS_PARTITION) &&
> > +	    part_get_info(dev_desc, part_no, &info)) {
> >   		if (part_no != 0) {
> >   			printf("** Partition %d not valid on device %d **\n",
> >   					part_no, dev_desc->devnum);
> > diff --git a/include/part.h b/include/part.h
> > index 36b76f00563f..612d4c32b5c7 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 {
> > @@ -86,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
> >    *
> > @@ -275,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,
> > @@ -513,7 +523,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
> >    *
> > 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
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 7dc24fbf0aa9..b615abf598b9 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -17,6 +17,7 @@ config EFI_LOADER
> >   	select DM_EVENT
> >   	select EVENT_DYNAMIC
> >   	select LIB_UUID
> > +	select DOS_PARTITION
> 
> EFI_LOADER should imply EFI_PARTITION. MBR support is not required by
> the UEFI specification. You may still want to imply DOS_PARTITION.
> 
> As disk/Kconfig already has 'default y if' for both I would prefer to
> add EFI_LOADER there.

Basically I agree with you, but as Tom mentioned in his reply,
"imply (or select) [EFI|DOS]_PARTITION" may increase the size
of U-Boot proper on some platforms.

The reason why I added "select DOS_PARTITION" is the next line,
"select PARTITION_UUIDS".
While PARTITION_UUIDS depends on PARTITION, EFI_LOADER *selects*
PARTITION_UUIDS unconditionally, which beaks dependency as
[EFI|DOS]_PARTITION, as you said above, is not a requirement
for EFI_LOADER from a perspective of UEFI specification.

Instead of adding any "select *_PARTITION",
I will fix this issue simply by changing the dependency to
"imply PARTITION_UUIDS" with a small hack on efi_device_path.c.

> diff --git a/disk/Kconfig b/disk/Kconfig
> index 13700322e9..87b2e52af4 100644
> --- a/disk/Kconfig
> +++ b/disk/Kconfig
> @@ -54,7 +54,7 @@ config SPL_MAC_PARTITION
>  config DOS_PARTITION
>         bool "Enable MS Dos partition table"
>         depends on PARTITIONS
> -       default y if DISTRO_DEFAULTS
> +       default y if DISTRO_DEFAULTS || EFI_LOADER
>         default y if x86 || CMD_FAT || USB_STORAGE
>         help
>           traditional on the Intel architecture, USB sticks, etc.
> @@ -90,7 +90,7 @@ config SPL_AMIGA_PARTITION
>  config EFI_PARTITION
>         bool "Enable EFI GPT partition table"
>         depends on PARTITIONS
> -       default y if DISTRO_DEFAULTS
> +       default y if DISTRO_DEFAULTS || EFI_LOADER
>         default y if ARCH_TEGRA
>         select LIB_UUID
>         help

If we want to merge this hack, we will have to negotiate with Tom.

-Takahiro Akashi


> 
> Best regards
> 
> Heinrich
> 
> >   	select PARTITION_UUIDS
> >   	select HAVE_BLOCK_DEVICE
> >   	select REGEX
>
AKASHI Takahiro April 18, 2022, 8:14 a.m. UTC | #6 
Hi Tom,

Thank you for evaluating the code growth.

On Fri, Apr 15, 2022 at 11:01:48PM -0400, Tom Rini wrote:
> On Fri, Apr 15, 2022 at 04:11:56PM +0900, AKASHI Takahiro wrote:
> > We see some increase of spl code size due to partition support (disk/*)
> > while none of particular partition types (CONFIG_SPL_XXX_PARTITION) are
> > enabled.
> > With this patch applied, part.c is no longer included unless really
> > necessary.
> > 
> > In addition, fix errors in CI build revealed after this change is made.
> > 
> > Fixes: commit 88ca8e26958b ("disk: Add an option for partitions in SPL")
> > Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path
> > 	for SIG_TYPE_GUID")
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/Kconfig                                  |  1 +
> >  configs/cortina_presidio-asic-emmc_defconfig |  1 -
> 
> So, the defconfig change here is wrong, CMD_PART isn't being implied
> otherwise,

As I said in my reply to Heinrich, this defconfig seems weird.
but that's okay as I found another workaround.

> and this board is part of a number of boards that had
> EFI_LOADER before, but not DOS_PARTITION, and so do grow, but in a
> valid/expected way.  There are also a number of boards that don't have
> any partition type support set, but that too I think ends up being
> correct.  The whole before/after is at
> https://gist.github.com/trini/731ee8d50a9bc96b90e12860f8c53f14

That happens, probably, because EFI_LOADER is by default enabled
for most platforms whether users want to use UEFI or not.
I don't think that people who want to use UEFI with U-Boot will
be much careful of the code increase by this change.

Anyway, I will drop this hunk("select DOS_PARTITION) in the next version
as I found another way to fix the dependency issue.

-Takahiro Akashi



> -- 
> Tom
diff mbox series

Patch

diff --git a/cmd/Kconfig b/cmd/Kconfig
index d3abe3a06bff..b69c26912568 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1239,6 +1239,7 @@  config CMD_OSD
 
 config CMD_PART
 	bool "part"
+	depends on PARTITIONS
 	select HAVE_BLOCK_DEVICE
 	select PARTITION_UUIDS
 	help
diff --git a/configs/cortina_presidio-asic-emmc_defconfig b/configs/cortina_presidio-asic-emmc_defconfig
index c22dcef7ec05..c217a00a1cf0 100644
--- a/configs/cortina_presidio-asic-emmc_defconfig
+++ b/configs/cortina_presidio-asic-emmc_defconfig
@@ -18,7 +18,6 @@  CONFIG_LAST_STAGE_INIT=y
 CONFIG_SYS_PROMPT="G3#"
 CONFIG_CMD_I2C=y
 CONFIG_CMD_MMC=y
-CONFIG_CMD_PART=y
 CONFIG_CMD_WDT=y
 CONFIG_BOOTP_BOOTFILESIZE=y
 CONFIG_CMD_CACHE=y
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
diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index df9ea2c028fc..a7ec1c4b759c 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -95,7 +95,8 @@  int fat_register_device(struct blk_desc *dev_desc, int part_no)
 	cur_dev = NULL;
 
 	/* Read the partition table, if present */
-	if (part_get_info(dev_desc, part_no, &info)) {
+	if (CONFIG_IS_ENABLED(DOS_PARTITION) &&
+	    part_get_info(dev_desc, part_no, &info)) {
 		if (part_no != 0) {
 			printf("** Partition %d not valid on device %d **\n",
 					part_no, dev_desc->devnum);
diff --git a/include/part.h b/include/part.h
index 36b76f00563f..612d4c32b5c7 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 {
@@ -86,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
  *
@@ -275,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,
@@ -513,7 +523,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
  *
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
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 7dc24fbf0aa9..b615abf598b9 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -17,6 +17,7 @@  config EFI_LOADER
 	select DM_EVENT
 	select EVENT_DYNAMIC
 	select LIB_UUID
+	select DOS_PARTITION
 	select PARTITION_UUIDS
 	select HAVE_BLOCK_DEVICE
 	select REGEX