[RFC] disk: don't compile in partition support for spl/tpl if not really necessary
AKASHI Takahiro
takahiro.akashi at linaro.org
Mon Apr 18 10:06:11 CEST 2022
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 at 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
>
More information about the U-Boot
mailing list