[PATCH v2 0/3] part_efi: cache last scanned GPT for next partition

[PATCH v2 0/3] part_efi: cache last scanned GPT for next partition

[Neil Armstrong]

The actual architecture of the EFI part parser means the
entire GPT partition table needs to be parsed for each
part_info() callback.

Since the default part scan code will always scan up to
128 partitions for a block, and devices with an UFS chip
with up to 8 LUNs are very common in the field, this means
a complete GPT parsing and validation will be done
up to 1024 times instead of 8 on such devices.

The GPT parsing can be cached between each part_info()
call speed up to 3x on the worse condition when the CPU
i-cache and d-cache are disabled, like in the SPL.

The new part API implementation does caching each time
a correct GPT has been parsed and verified and will match
the blk_desc and LBA to serve the cached data or force
a re-parsing.

In order to allow GPT manipulation, the new API is only
called when scanning the partitions for a disk, all the
calls will be called uncached.

On the SM8650 QRD platform with a KIOXIA THGJFJT1E45BATPC
configured with 8 LUNs, the scsi scan takes 0.2s with both
CPU caches enabled, but when disabling both CPU caches it
goes up to 4s to do the full scan of all 8 LUN partitions.

With this change the scan takes only 0.18s with both CPU
caches enabled running 1.1x times faster, and with both CPU
caches disabled the full scan takes only 1.27s running
3x faster.

While 20ms could look negligeable, it's still a 20ms gain
in the boot flow and a non negligeable reduction in
calculation and memory allocation since for each scan
it would allocate and free the gpt_pte table up to
1024 times, now it would only do 8 allocations, reducing
memory fragmentation.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v2:
- add part_info_get_cached API
- only use cache when called from part_info_get_cached
- Link to v1: https://lore.kernel.org/all/20250327-u-boot-efi-part-cache-v1-1-be6b69c0698b@linaro.org

---
Neil Armstrong (3):
      part: add part_get_info_cached() API
      part: efi: add GPT PTE cache used with part_get_info_cached() API
      block: use part_get_info_cached() API when scanning partitions

 disk/part.c                |  55 +++++++++++++++++-----
 disk/part_efi.c            | 114 ++++++++++++++++++++++++++++++++++++++-------
 drivers/block/blk-uclass.c |   7 ++-
 include/part.h             |  44 +++++++++++++++++
 4 files changed, 189 insertions(+), 31 deletions(-)
---
base-commit: 5ca70325b64f760bf4190f206a0e88dda495e3d2
change-id: 20250408-topic-gpt-cache-11f770f42f1e

Best regards,
-- 
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH v2 1/3] part: add part_get_info_cached() API

[Neil Armstrong]

Introduce a new part_get_info_cached() API that's used to
get the part_info of a blk_desc allowing to use an eventual
partition scanning cache to avoid rescanning the entire disk
partition scheme for each partition number.

The part_get_info_cached_free() is also added to hint the
partition code to flush any cached data from this disk.

The equivalent ops are added that directly maps to those
added functions.

This API is designed to be used as a direct replacement of
part_get_info() in codes scanning all partitions for a same
disk, like in blk-uclass. With this, a lot of unnecessary
computation is saved, leading to a faster boot time when
partitions are scanned, especially with storage medias
with potentially multiple large hardware partitions like
UFS, NVMe or eMMC.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 disk/part.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++-----------
 include/part.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 11 deletions(-)

diff --git a/disk/part.c b/disk/part.c
index 303178161c083ec6e1b767b4f06ac5773576ca60..1d09c0511c75d457c81cab040c3f5caa924ee945 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -335,8 +335,8 @@ void part_print(struct blk_desc *desc)
 		drv->print(desc);
 }
 
-int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
-			  struct disk_partition *info)
+static int _part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
+				  struct disk_partition *info, bool cached)
 {
 	struct part_driver *drv;
 
@@ -356,24 +356,57 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
 			      desc->part_type);
 			return -EPROTONOSUPPORT;
 		}
-		if (!drv->get_info) {
-			PRINTF("## Driver %s does not have the get_info() method\n",
-			       drv->name);
-			return -ENOSYS;
-		}
-		if (drv->get_info(desc, part, info) == 0) {
-			PRINTF("## Valid %s partition found ##\n", drv->name);
-			return 0;
+		if (cached && drv->get_info_cached) {
+			if (drv->get_info_cached(desc, part, info) == 0) {
+				PRINTF("## Valid %s partition found ##\n", drv->name);
+				return 0;
+			}
+		} else {
+			if (!drv->get_info) {
+				PRINTF("## Driver %s does not have the get_info() method\n",
+				       drv->name);
+				return -ENOSYS;
+			}
+			if (drv->get_info(desc, part, info) == 0) {
+				PRINTF("## Valid %s partition found ##\n", drv->name);
+				return 0;
+			}
 		}
 	}
 
 	return -ENOENT;
 }
 
+int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
+			  struct disk_partition *info)
+{
+	return _part_get_info_by_type(desc, part, part_type, info, false);
+}
+
+int part_get_info_cached(struct blk_desc *desc, int part,
+			 struct disk_partition *info)
+{
+	return _part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info, true);
+}
+
 int part_get_info(struct blk_desc *desc, int part,
 		  struct disk_partition *info)
 {
-	return part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info);
+	return _part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info, false);
+}
+
+void part_get_info_cached_free(struct blk_desc *desc)
+{
+	struct part_driver *drv;
+
+	if (blk_enabled()) {
+		drv = part_driver_lookup_type(desc);
+		if (!drv)
+			return;
+		if (!drv->get_info_cache_free)
+			return;
+		drv->get_info_cache_free(desc);
+	}
 }
 
 int part_get_info_whole_disk(struct blk_desc *desc,
diff --git a/include/part.h b/include/part.h
index fcb3c13dea4de6346ad98d6ce320ef36747dda85..8c98865146306fb66509576068c45de01b9cedb6 100644
--- a/include/part.h
+++ b/include/part.h
@@ -216,6 +216,34 @@ struct blk_desc *mg_disk_get_dev(int dev);
  */
 int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
 			  struct disk_partition *info);
+
+/**
+ * part_get_info_cached() - Get partitions from a block device but save the
+ * disk partition map between different partitions.
+ *
+ * Call to part_get_info_cached_free() is required when scanning of the
+ * block device is finished.
+ *
+ * If the partition driver doesn't support cached part_info, the
+ * normal part_info will be called instead.
+ *
+ * @desc:	Block device descriptor
+ * @part:	Partition number to read
+ * @info:	Returned partition information
+ *
+ * Return: 0 on success, negative errno on failure
+ */
+int part_get_info_cached(struct blk_desc *desc, int part,
+			 struct disk_partition *info);
+
+/**
+ * part_get_info_cached_free() - Free the saved disk partition map
+ * from a previous call of part_get_info_cached().
+ *
+ * @desc:	Block device descriptor
+ */
+void part_get_info_cached_free(struct blk_desc *desc);
+
 int part_get_info(struct blk_desc *desc, int part,
 		  struct disk_partition *info);
 /**
@@ -463,6 +491,22 @@ struct part_driver {
 	int part_type;
 	/** @max_entries:	maximum number of partition table entries */
 	const int max_entries;
+	/**
+	 * @cache_free_cache_free:	Free any parsing data stored from get_info_cached()
+	 *
+	 * @get_info_cache_free.desc:	Block device descriptor
+	 */
+	void (*get_info_cache_free)(struct blk_desc *desc);
+	/**
+	 * @get_inf_cachedo:		Get information about a partition, and save
+	 *				the resulting parsing data for the next partition.
+	 *
+	 * @get_info_cached.desc:	Block device descriptor
+	 * @get_info_cached.part:	Partition number (1 = first)
+	 * @get_info_cached.info:	Returns partition information
+	 */
+	int (*get_info_cached)(struct blk_desc *desc, int part,
+			       struct disk_partition *info);
 	/**
 	 * @get_info:		Get information about a partition
 	 *

-- 
2.34.1

[PATCH v2 2/3] part: efi: add GPT PTE cache used with part_get_info_cached() API

[Neil Armstrong]

Implement a simple cache for part_efi to be used by the newly
introduced part_get_info_cached() API.

The cache simply stores a successfully scanned GPT PTE if called
from the part_get_info_cached() ops, and will return the cached
data if the blk_desc and lba offset matches, invalidating the
cache if not.

The cache will only operate if called from the part_get_info_cached()
API, all the over calls will operate uncached, making sure we
can still update & re-scan the GPT partitions like before.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 disk/part_efi.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 96 insertions(+), 18 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 932d058c184ce6946b7142e7c2d9637b28e4661e..949968fcd30fc18766a023e73f43ca1381dcef89 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -55,12 +55,58 @@ static inline u32 efi_crc32(const void *buf, u32 len)
 static int pmbr_part_valid(struct partition *part);
 static int is_pmbr_valid(legacy_mbr * mbr);
 static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
-			gpt_entry **pgpt_pte);
+			gpt_entry **pgpt_pte, bool cache);
 static gpt_entry *alloc_read_gpt_entries(struct blk_desc *desc,
 					 gpt_header *pgpt_head);
 static int is_pte_valid(gpt_entry * pte);
 static int find_valid_gpt(struct blk_desc *desc, gpt_header *gpt_head,
-			  gpt_entry **pgpt_pte);
+			  gpt_entry **pgpt_pte, bool cache);
+
+static struct gpt_pte_cache_data {
+	struct blk_desc *desc;
+	u64 lba;
+	gpt_entry *gpt_pte;
+	gpt_header gpt_head;
+} gpt_pte_cache;
+
+static void clear_gpt_pte_cache(void)
+{
+	if (gpt_pte_cache.desc) {
+		if (gpt_pte_cache.gpt_pte)
+			free(gpt_pte_cache.gpt_pte);
+
+		memset(&gpt_pte_cache, 0, sizeof(gpt_pte_cache));
+	}
+}
+
+static void cache_gpt_pte(struct blk_desc *desc, u64 lba,
+			  gpt_entry *gpt_pte, gpt_header *pgpt_head)
+{
+	if (gpt_pte_cache.gpt_pte)
+		free(gpt_pte_cache.gpt_pte);
+
+	gpt_pte_cache.desc = desc;
+	gpt_pte_cache.lba = lba;
+	gpt_pte_cache.gpt_pte = gpt_pte;
+	if (pgpt_head)
+		memcpy(&gpt_pte_cache.gpt_head, pgpt_head, sizeof(gpt_header));
+}
+
+static gpt_entry *get_cached_gpt_pte(struct blk_desc *desc, u64 lba,
+				     gpt_header *pgpt_head)
+{
+	if (gpt_pte_cache.desc && gpt_pte_cache.gpt_pte) {
+		if (gpt_pte_cache.desc == desc &&
+		    gpt_pte_cache.lba == lba) {
+			memcpy(pgpt_head, &gpt_pte_cache.gpt_head, sizeof(gpt_header));
+			return gpt_pte_cache.gpt_pte;
+		}
+
+		clear_gpt_pte_cache();
+	}
+
+	return NULL;
+}
 
 static char *print_efiname(gpt_entry *pte)
 {
@@ -205,7 +251,7 @@ int get_disk_guid(struct blk_desc *desc, char *guid)
 	unsigned char *guid_bin;
 
 	/* This function validates AND fills in the GPT header and PTE */
-	if (find_valid_gpt(desc, gpt_head, &gpt_pte) != 1)
+	if (find_valid_gpt(desc, gpt_head, &gpt_pte, false) != 1)
 		return -EINVAL;
 
 	guid_bin = gpt_head->disk_guid.b;
@@ -224,7 +270,7 @@ static void __maybe_unused part_print_efi(struct blk_desc *desc)
 	unsigned char *uuid;
 
 	/* This function validates AND fills in the GPT header and PTE */
-	if (find_valid_gpt(desc, gpt_head, &gpt_pte) != 1)
+	if (find_valid_gpt(desc, gpt_head, &gpt_pte, false) != 1)
 		return;
 
 	debug("%s: gpt-entry at %p\n", __func__, gpt_pte);
@@ -258,8 +304,13 @@ static void __maybe_unused part_print_efi(struct blk_desc *desc)
 	return;
 }
 
-static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
-					    struct disk_partition *info)
+static void __maybe_unused part_get_info_cache_free_efi(struct blk_desc *desc)
+{
+	clear_gpt_pte_cache();
+}
+
+static int _part_get_info_efi(struct blk_desc *desc, int part,
+			      struct disk_partition *info, bool cache)
 {
 	ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, desc->blksz);
 	gpt_entry *gpt_pte = NULL;
@@ -271,13 +322,14 @@ static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
 	}
 
 	/* This function validates AND fills in the GPT header and PTE */
-	if (find_valid_gpt(desc, gpt_head, &gpt_pte) != 1)
+	if (find_valid_gpt(desc, gpt_head, &gpt_pte, cache) != 1)
 		return -EINVAL;
 
 	if (part > le32_to_cpu(gpt_head->num_partition_entries) ||
 	    !is_pte_valid(&gpt_pte[part - 1])) {
 		log_debug("Invalid partition number %d\n", part);
-		free(gpt_pte);
+		if (!cache)
+			free(gpt_pte);
 		return -EPERM;
 	}
 
@@ -307,11 +359,23 @@ static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
 	log_debug("start 0x" LBAF ", size 0x" LBAF ", name %s\n", info->start,
 		  info->size, info->name);
 
-	/* Remember to free pte */
-	free(gpt_pte);
+	if (!cache)
+		free(gpt_pte);
 	return 0;
 }
 
+static int __maybe_unused part_get_info_cached_efi(struct blk_desc *desc, int part,
+						   struct disk_partition *info)
+{
+	return _part_get_info_efi(desc, part, info, true);
+}
+
+static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
+					    struct disk_partition *info)
+{
+	return _part_get_info_efi(desc, part, info, false);
+}
+
 static int part_test_efi(struct blk_desc *desc)
 {
 	ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, legacymbr, 1, desc->blksz);
@@ -689,7 +753,7 @@ int gpt_verify_headers(struct blk_desc *desc, gpt_header *gpt_head,
 	 */
 	if (is_gpt_valid(desc,
 			 GPT_PRIMARY_PARTITION_TABLE_LBA,
-			 gpt_head, gpt_pte) != 1) {
+			 gpt_head, gpt_pte, false) != 1) {
 		log_debug("Invalid GPT\n");
 		return -1;
 	}
@@ -706,7 +770,7 @@ int gpt_verify_headers(struct blk_desc *desc, gpt_header *gpt_head,
 	}
 
 	if (is_gpt_valid(desc, (desc->lba - 1),
-			 gpt_head, gpt_pte) != 1) {
+			 gpt_head, gpt_pte, false) != 1) {
 		log_debug("Invalid Backup GPT\n");
 		return -1;
 	}
@@ -765,9 +829,9 @@ int gpt_repair_headers(struct blk_desc *desc)
 	int ret = -1;
 
 	is_gpt1_valid = is_gpt_valid(desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
-				     gpt_h1, &gpt_e1);
+				     gpt_h1, &gpt_e1, false);
 	is_gpt2_valid = is_gpt_valid(desc, desc->lba - 1,
-				     gpt_h2, &gpt_e2);
+				     gpt_h2, &gpt_e2, false);
 
 	if (is_gpt1_valid && is_gpt2_valid) {
 		ret = 0;
@@ -1023,12 +1087,13 @@ static int is_pmbr_valid(legacy_mbr *mbr)
  * lba is the logical block address of the GPT header to test
  * gpt is a GPT header ptr, filled on return.
  * ptes is a PTEs ptr, filled on return.
+ * cache is a bool, true to use the cached gpt_pte from previous call
  *
  * Description: returns 1 if valid,  0 on error, 2 if ignored header
  * If valid, returns pointers to PTEs.
  */
 static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
-			gpt_entry **pgpt_pte)
+			gpt_entry **pgpt_pte, bool cache)
 {
 	/* Confirm valid arguments prior to allocation. */
 	if (!desc || !pgpt_head) {
@@ -1036,6 +1101,12 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
 		return 0;
 	}
 
+	if (cache) {
+		*pgpt_pte = get_cached_gpt_pte(desc, lba, pgpt_head);
+		if (*pgpt_pte)
+			return 1;
+	}
+
 	ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, mbr, 1, desc->blksz);
 
 	/* Read MBR Header from device */
@@ -1081,6 +1152,9 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
 		return 0;
 	}
 
+	if (cache)
+		cache_gpt_pte(desc, lba, *pgpt_pte, pgpt_head);
+
 	/* We're done, all's well */
 	return 1;
 }
@@ -1090,23 +1164,25 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
  *
  * gpt is a GPT header ptr, filled on return.
  * ptes is a PTEs ptr, filled on return.
+ * cache if a bool, use cached GPT if available
  *
  * Description: returns 1 if found a valid gpt,  0 on error.
  * If valid, returns pointers to PTEs.
  */
 static int find_valid_gpt(struct blk_desc *desc, gpt_header *gpt_head,
-			  gpt_entry **pgpt_pte)
+			  gpt_entry **pgpt_pte, bool cache)
 {
 	int r;
 
 	r = is_gpt_valid(desc, GPT_PRIMARY_PARTITION_TABLE_LBA, gpt_head,
-			 pgpt_pte);
+			 pgpt_pte, cache);
 
 	if (r != 1) {
 		if (r != 2)
 			log_debug("Invalid GPT\n");
 
-		if (is_gpt_valid(desc, desc->lba - 1, gpt_head, pgpt_pte)
+		if (is_gpt_valid(desc, desc->lba - 1, gpt_head, pgpt_pte,
+				 true)
 		    != 1) {
 			log_debug("Invalid Backup GPT\n");
 			return 0;
@@ -1210,6 +1286,8 @@ U_BOOT_PART_TYPE(a_efi) = {
 	.name		= "EFI",
 	.part_type	= PART_TYPE_EFI,
 	.max_entries	= GPT_ENTRY_NUMBERS,
+	.get_info_cache_free	= part_get_info_ptr(part_get_info_cache_free_efi),
+	.get_info_cached	= part_get_info_ptr(part_get_info_cached_efi),
 	.get_info	= part_get_info_ptr(part_get_info_efi),
 	.print		= part_print_ptr(part_print_efi),
 	.test		= part_test_efi,

-- 
2.34.1

[PATCH v2 3/3] block: use part_get_info_cached() API when scanning partitions

[Neil Armstrong]

Let's make use of the newly introduced part_get_info_cached() API
and the part_get_info_cached_free() when scanning all partitions
of a disk. With this, a lot of unnecessary computation is saved,
leading to a faster boot time when partitions are scanned, especially
with storage medias with potentially multiple large hardware
partitions like UFS, NVMe or eMMC.

Since the default part scan code will always scan up to
128 partitions for a block, and devices with an UFS chip
with up to 8 LUNs are very common in the field, this means
a complete GPT parsing and validation will be done
up to 1024 times instead of 8 on such devices.

On the SM8650 QRD platform with a KIOXIA THGJFJT1E45BATPC
configured with 8 LUNs, the scsi scan takes 0.2s with both
CPU caches enabled, but when disabling both CPU caches it
goes up to 4s to do the full scan of all 8 LUN partitions.

With this change the scan takes only 0.18s with both CPU
caches enabled running 1.1x times faster, and with both CPU
caches disabled the full scan takes only 1.27s running
3x faster.

While 20ms could look negligeable, it's still a 20ms gain
in the boot flow and a non negligeable reduction in
calculation and memory allocation since for each scan
it would allocate and free the gpt_pte table up to
1024 times, now it would only do 8 allocations, reducing
memory fragmentation.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/block/blk-uclass.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index f3ac8db9464311acbc96f3358e0ef143471ca59d..313fbe6a480e5bced5230c6ea03a3815cf4087b9 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -820,15 +820,17 @@ static int part_create_block_devices(struct udevice *blk_dev)
 
 	/* Add devices for each partition */
 	for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
-		if (part_get_info(desc, part, &info))
+		if (part_get_info_cached(desc, part, &info))
 			continue;
 		snprintf(devname, sizeof(devname), "%s:%d", blk_dev->name,
 			 part);
 
 		ret = device_bind_driver(blk_dev, "blk_partition",
 					 strdup(devname), &dev);
-		if (ret)
+		if (ret) {
+			part_get_info_cached_free(desc);
 			return ret;
+		}
 
 		part_data = dev_get_uclass_plat(dev);
 		part_data->partnum = part;
@@ -847,6 +849,7 @@ static int part_create_block_devices(struct udevice *blk_dev)
 	debug("%s: %d partitions found in %s\n", __func__, count,
 	      blk_dev->name);
 
+	part_get_info_cached_free(desc);
 	return 0;
 }
 

-- 
2.34.1

Re: [PATCH v2 1/3] part: add part_get_info_cached() API

[Heinrich Schuchardt]

On 08.04.25 11:13, Neil Armstrong wrote:
> Introduce a new part_get_info_cached() API that's used to
> get the part_info of a blk_desc allowing to use an eventual
> partition scanning cache to avoid rescanning the entire disk
> partition scheme for each partition number.
>
> The part_get_info_cached_free() is also added to hint the
> partition code to flush any cached data from this disk.
>
> The equivalent ops are added that directly maps to those
> added functions.
>
> This API is designed to be used as a direct replacement of
> part_get_info() in codes scanning all partitions for a same
> disk, like in blk-uclass. With this, a lot of unnecessary
> computation is saved, leading to a faster boot time when
> partitions are scanned, especially with storage medias
> with potentially multiple large hardware partitions like
> UFS, NVMe or eMMC.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>   disk/part.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++-----------
>   include/part.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 88 insertions(+), 11 deletions(-)
>
> diff --git a/disk/part.c b/disk/part.c
> index 303178161c083ec6e1b767b4f06ac5773576ca60..1d09c0511c75d457c81cab040c3f5caa924ee945 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -335,8 +335,8 @@ void part_print(struct blk_desc *desc)
>   		drv->print(desc);
>   }
>
> -int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
> -			  struct disk_partition *info)

Thank you, Neil, for pushing this forward.

All functions deserve a description.

> +static int _part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
> +				  struct disk_partition *info, bool cached)
>   {
>   	struct part_driver *drv;
>
> @@ -356,24 +356,57 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>   			      desc->part_type);
>   			return -EPROTONOSUPPORT;
>   		}
> -		if (!drv->get_info) {
> -			PRINTF("## Driver %s does not have the get_info() method\n",

Writing to the console creates screen havoc in EFI applications. Please,
use log_debug() instead of PRINTF().

And, please, remove that noisy '## '. It carries no information.

Best regards

Heinrich

> -			       drv->name);
> -			return -ENOSYS;
> -		}
> -		if (drv->get_info(desc, part, info) == 0) {
> -			PRINTF("## Valid %s partition found ##\n", drv->name);
> -			return 0;
> +		if (cached && drv->get_info_cached) {
> +			if (drv->get_info_cached(desc, part, info) == 0) {
> +				PRINTF("## Valid %s partition found ##\n", drv->name);
> +				return 0;
> +			}
> +		} else {
> +			if (!drv->get_info) {
> +				PRINTF("## Driver %s does not have the get_info() method\n",
> +				       drv->name);
> +				return -ENOSYS;
> +			}
> +			if (drv->get_info(desc, part, info) == 0) {
> +				PRINTF("## Valid %s partition found ##\n", drv->name);
> +				return 0;
> +			}
>   		}
>   	}
>
>   	return -ENOENT;
>   }
>
> +int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
> +			  struct disk_partition *info)
> +{
> +	return _part_get_info_by_type(desc, part, part_type, info, false);
> +}
> +
> +int part_get_info_cached(struct blk_desc *desc, int part,
> +			 struct disk_partition *info)
> +{
> +	return _part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info, true);
> +}
> +
>   int part_get_info(struct blk_desc *desc, int part,
>   		  struct disk_partition *info)
>   {
> -	return part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info);
> +	return _part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info, false);
> +}
> +
> +void part_get_info_cached_free(struct blk_desc *desc)
> +{
> +	struct part_driver *drv;
> +
> +	if (blk_enabled()) {
> +		drv = part_driver_lookup_type(desc);
> +		if (!drv)
> +			return;
> +		if (!drv->get_info_cache_free)
> +			return;
> +		drv->get_info_cache_free(desc);
> +	}
>   }
>
>   int part_get_info_whole_disk(struct blk_desc *desc,
> diff --git a/include/part.h b/include/part.h
> index fcb3c13dea4de6346ad98d6ce320ef36747dda85..8c98865146306fb66509576068c45de01b9cedb6 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -216,6 +216,34 @@ struct blk_desc *mg_disk_get_dev(int dev);
>    */
>   int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>   			  struct disk_partition *info);
> +
> +/**
> + * part_get_info_cached() - Get partitions from a block device but save the
> + * disk partition map between different partitions.
> + *
> + * Call to part_get_info_cached_free() is required when scanning of the
> + * block device is finished.
> + *
> + * If the partition driver doesn't support cached part_info, the
> + * normal part_info will be called instead.
> + *
> + * @desc:	Block device descriptor
> + * @part:	Partition number to read
> + * @info:	Returned partition information
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +int part_get_info_cached(struct blk_desc *desc, int part,
> +			 struct disk_partition *info);
> +
> +/**
> + * part_get_info_cached_free() - Free the saved disk partition map
> + * from a previous call of part_get_info_cached().
> + *
> + * @desc:	Block device descriptor
> + */
> +void part_get_info_cached_free(struct blk_desc *desc);
> +
>   int part_get_info(struct blk_desc *desc, int part,
>   		  struct disk_partition *info);
>   /**
> @@ -463,6 +491,22 @@ struct part_driver {
>   	int part_type;
>   	/** @max_entries:	maximum number of partition table entries */
>   	const int max_entries;
> +	/**
> +	 * @cache_free_cache_free:	Free any parsing data stored from get_info_cached()
> +	 *
> +	 * @get_info_cache_free.desc:	Block device descriptor
> +	 */
> +	void (*get_info_cache_free)(struct blk_desc *desc);
> +	/**
> +	 * @get_inf_cachedo:		Get information about a partition, and save
> +	 *				the resulting parsing data for the next partition.
> +	 *
> +	 * @get_info_cached.desc:	Block device descriptor
> +	 * @get_info_cached.part:	Partition number (1 = first)
> +	 * @get_info_cached.info:	Returns partition information
> +	 */
> +	int (*get_info_cached)(struct blk_desc *desc, int part,
> +			       struct disk_partition *info);
>   	/**
>   	 * @get_info:		Get information about a partition
>   	 *
>

Re: [PATCH v2 2/3] part: efi: add GPT PTE cache used with part_get_info_cached() API

[Heinrich Schuchardt]

On 08.04.25 11:13, Neil Armstrong wrote:
> Implement a simple cache for part_efi to be used by the newly
> introduced part_get_info_cached() API.
>
> The cache simply stores a successfully scanned GPT PTE if called
> from the part_get_info_cached() ops, and will return the cached
> data if the blk_desc and lba offset matches, invalidating the
> cache if not.
>
> The cache will only operate if called from the part_get_info_cached()
> API, all the over calls will operate uncached, making sure we
> can still update & re-scan the GPT partitions like before.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>   disk/part_efi.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 96 insertions(+), 18 deletions(-)
>
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 932d058c184ce6946b7142e7c2d9637b28e4661e..949968fcd30fc18766a023e73f43ca1381dcef89 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -55,12 +55,58 @@ static inline u32 efi_crc32(const void *buf, u32 len)
>   static int pmbr_part_valid(struct partition *part);
>   static int is_pmbr_valid(legacy_mbr * mbr);
>   static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
> -			gpt_entry **pgpt_pte);
> +			gpt_entry **pgpt_pte, bool cache);
>   static gpt_entry *alloc_read_gpt_entries(struct blk_desc *desc,
>   					 gpt_header *pgpt_head);
>   static int is_pte_valid(gpt_entry * pte);
>   static int find_valid_gpt(struct blk_desc *desc, gpt_header *gpt_head,
> -			  gpt_entry **pgpt_pte);
> +			  gpt_entry **pgpt_pte, bool cache);
> +

Please, describe the structure and its members.

https://docs.kernel.org/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation

> +static struct gpt_pte_cache_data {
> +	struct blk_desc *desc;
> +	u64 lba;
> +	gpt_entry *gpt_pte;
> +	gpt_header gpt_head;
> +} gpt_pte_cache;
> +

Please, describe all functions.

> +static void clear_gpt_pte_cache(void)
> +{
> +	if (gpt_pte_cache.desc) {
> +		if (gpt_pte_cache.gpt_pte)
> +			free(gpt_pte_cache.gpt_pte);
> +
> +		memset(&gpt_pte_cache, 0, sizeof(gpt_pte_cache));
> +	}
> +}
> +
> +static void cache_gpt_pte(struct blk_desc *desc, u64 lba,
> +			  gpt_entry *gpt_pte, gpt_header *pgpt_head)
> +{
> +	if (gpt_pte_cache.gpt_pte)
> +		free(gpt_pte_cache.gpt_pte);
> +
> +	gpt_pte_cache.desc = desc;
> +	gpt_pte_cache.lba = lba;
> +	gpt_pte_cache.gpt_pte = gpt_pte;
> +	if (pgpt_head)
> +		memcpy(&gpt_pte_cache.gpt_head, pgpt_head, sizeof(gpt_header));
> +}
> +
> +static gpt_entry *get_cached_gpt_pte(struct blk_desc *desc, u64 lba,
> +				     gpt_header *pgpt_head)
> +{
> +	if (gpt_pte_cache.desc && gpt_pte_cache.gpt_pte) {
> +		if (gpt_pte_cache.desc == desc &&
> +		    gpt_pte_cache.lba == lba) {
> +			memcpy(pgpt_head, &gpt_pte_cache.gpt_head, sizeof(gpt_header));
> +			return gpt_pte_cache.gpt_pte;
> +		}
> +
> +		clear_gpt_pte_cache();
> +	}
> +
> +	return NULL;
> +}
>
>   static char *print_efiname(gpt_entry *pte)
>   {
> @@ -205,7 +251,7 @@ int get_disk_guid(struct blk_desc *desc, char *guid)
>   	unsigned char *guid_bin;
>
>   	/* This function validates AND fills in the GPT header and PTE */
> -	if (find_valid_gpt(desc, gpt_head, &gpt_pte) != 1)
> +	if (find_valid_gpt(desc, gpt_head, &gpt_pte, false) != 1)
>   		return -EINVAL;
>
>   	guid_bin = gpt_head->disk_guid.b;
> @@ -224,7 +270,7 @@ static void __maybe_unused part_print_efi(struct blk_desc *desc)
>   	unsigned char *uuid;
>
>   	/* This function validates AND fills in the GPT header and PTE */
> -	if (find_valid_gpt(desc, gpt_head, &gpt_pte) != 1)
> +	if (find_valid_gpt(desc, gpt_head, &gpt_pte, false) != 1)
>   		return;
>
>   	debug("%s: gpt-entry at %p\n", __func__, gpt_pte);
> @@ -258,8 +304,13 @@ static void __maybe_unused part_print_efi(struct blk_desc *desc)
>   	return;
>   }
>
> -static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
> -					    struct disk_partition *info)
> +static void __maybe_unused part_get_info_cache_free_efi(struct blk_desc *desc)
> +{
> +	clear_gpt_pte_cache();
> +}
> +
> +static int _part_get_info_efi(struct blk_desc *desc, int part,
> +			      struct disk_partition *info, bool cache)
>   {
>   	ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, desc->blksz);
>   	gpt_entry *gpt_pte = NULL;
> @@ -271,13 +322,14 @@ static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
>   	}
>
>   	/* This function validates AND fills in the GPT header and PTE */
> -	if (find_valid_gpt(desc, gpt_head, &gpt_pte) != 1)
> +	if (find_valid_gpt(desc, gpt_head, &gpt_pte, cache) != 1)
>   		return -EINVAL;
>
>   	if (part > le32_to_cpu(gpt_head->num_partition_entries) ||
>   	    !is_pte_valid(&gpt_pte[part - 1])) {
>   		log_debug("Invalid partition number %d\n", part);
> -		free(gpt_pte);
> +		if (!cache)
> +			free(gpt_pte);
>   		return -EPERM;
>   	}
>
> @@ -307,11 +359,23 @@ static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
>   	log_debug("start 0x" LBAF ", size 0x" LBAF ", name %s\n", info->start,
>   		  info->size, info->name);
>
> -	/* Remember to free pte */
> -	free(gpt_pte);
> +	if (!cache)
> +		free(gpt_pte);
>   	return 0;
>   }
>
> +static int __maybe_unused part_get_info_cached_efi(struct blk_desc *desc, int part,
> +						   struct disk_partition *info)
> +{
> +	return _part_get_info_efi(desc, part, info, true);
> +}
> +
> +static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
> +					    struct disk_partition *info)
> +{
> +	return _part_get_info_efi(desc, part, info, false);
> +}
> +
>   static int part_test_efi(struct blk_desc *desc)
>   {
>   	ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, legacymbr, 1, desc->blksz);
> @@ -689,7 +753,7 @@ int gpt_verify_headers(struct blk_desc *desc, gpt_header *gpt_head,
>   	 */
>   	if (is_gpt_valid(desc,
>   			 GPT_PRIMARY_PARTITION_TABLE_LBA,
> -			 gpt_head, gpt_pte) != 1) {
> +			 gpt_head, gpt_pte, false) != 1) {
>   		log_debug("Invalid GPT\n");
>   		return -1;
>   	}
> @@ -706,7 +770,7 @@ int gpt_verify_headers(struct blk_desc *desc, gpt_header *gpt_head,
>   	}
>
>   	if (is_gpt_valid(desc, (desc->lba - 1),
> -			 gpt_head, gpt_pte) != 1) {
> +			 gpt_head, gpt_pte, false) != 1) {
>   		log_debug("Invalid Backup GPT\n");
>   		return -1;
>   	}
> @@ -765,9 +829,9 @@ int gpt_repair_headers(struct blk_desc *desc)
>   	int ret = -1;
>
>   	is_gpt1_valid = is_gpt_valid(desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
> -				     gpt_h1, &gpt_e1);
> +				     gpt_h1, &gpt_e1, false);
>   	is_gpt2_valid = is_gpt_valid(desc, desc->lba - 1,
> -				     gpt_h2, &gpt_e2);
> +				     gpt_h2, &gpt_e2, false);
>
>   	if (is_gpt1_valid && is_gpt2_valid) {
>   		ret = 0;
> @@ -1023,12 +1087,13 @@ static int is_pmbr_valid(legacy_mbr *mbr)
>    * lba is the logical block address of the GPT header to test
>    * gpt is a GPT header ptr, filled on return.
>    * ptes is a PTEs ptr, filled on return.
> + * cache is a bool, true to use the cached gpt_pte from previous call

"use the cache" or "use cached GPT if available"?

It remains unclear what will happen if cache=1 and no entry is found.

Best regards

Heinrich

>    *
>    * Description: returns 1 if valid,  0 on error, 2 if ignored header
>    * If valid, returns pointers to PTEs.
>    */
>   static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
> -			gpt_entry **pgpt_pte)
> +			gpt_entry **pgpt_pte, bool cache)
>   {
>   	/* Confirm valid arguments prior to allocation. */
>   	if (!desc || !pgpt_head) {
> @@ -1036,6 +1101,12 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
>   		return 0;
>   	}
>
> +	if (cache) {
> +		*pgpt_pte = get_cached_gpt_pte(desc, lba, pgpt_head);
> +		if (*pgpt_pte)
> +			return 1;
> +	}
> +
>   	ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, mbr, 1, desc->blksz);
>
>   	/* Read MBR Header from device */
> @@ -1081,6 +1152,9 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
>   		return 0;
>   	}
>
> +	if (cache)
> +		cache_gpt_pte(desc, lba, *pgpt_pte, pgpt_head);
> +
>   	/* We're done, all's well */
>   	return 1;
>   }
> @@ -1090,23 +1164,25 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
>    *
>    * gpt is a GPT header ptr, filled on return.
>    * ptes is a PTEs ptr, filled on return.
> + * cache if a bool, use cached GPT if available
>    *
>    * Description: returns 1 if found a valid gpt,  0 on error.
>    * If valid, returns pointers to PTEs.
>    */
>   static int find_valid_gpt(struct blk_desc *desc, gpt_header *gpt_head,
> -			  gpt_entry **pgpt_pte)
> +			  gpt_entry **pgpt_pte, bool cache)
>   {
>   	int r;
>
>   	r = is_gpt_valid(desc, GPT_PRIMARY_PARTITION_TABLE_LBA, gpt_head,
> -			 pgpt_pte);
> +			 pgpt_pte, cache);
>
>   	if (r != 1) {
>   		if (r != 2)
>   			log_debug("Invalid GPT\n");
>
> -		if (is_gpt_valid(desc, desc->lba - 1, gpt_head, pgpt_pte)
> +		if (is_gpt_valid(desc, desc->lba - 1, gpt_head, pgpt_pte,
> +				 true)
>   		    != 1) {
>   			log_debug("Invalid Backup GPT\n");
>   			return 0;
> @@ -1210,6 +1286,8 @@ U_BOOT_PART_TYPE(a_efi) = {
>   	.name		= "EFI",
>   	.part_type	= PART_TYPE_EFI,
>   	.max_entries	= GPT_ENTRY_NUMBERS,
> +	.get_info_cache_free	= part_get_info_ptr(part_get_info_cache_free_efi),
> +	.get_info_cached	= part_get_info_ptr(part_get_info_cached_efi),
>   	.get_info	= part_get_info_ptr(part_get_info_efi),
>   	.print		= part_print_ptr(part_print_efi),
>   	.test		= part_test_efi,
>

Re: [PATCH v2 1/3] part: add part_get_info_cached() API

[Neil Armstrong]

On 08/04/2025 11:55, Heinrich Schuchardt wrote:
> On 08.04.25 11:13, Neil Armstrong wrote:
>> Introduce a new part_get_info_cached() API that's used to
>> get the part_info of a blk_desc allowing to use an eventual
>> partition scanning cache to avoid rescanning the entire disk
>> partition scheme for each partition number.
>>
>> The part_get_info_cached_free() is also added to hint the
>> partition code to flush any cached data from this disk.
>>
>> The equivalent ops are added that directly maps to those
>> added functions.
>>
>> This API is designed to be used as a direct replacement of
>> part_get_info() in codes scanning all partitions for a same
>> disk, like in blk-uclass. With this, a lot of unnecessary
>> computation is saved, leading to a faster boot time when
>> partitions are scanned, especially with storage medias
>> with potentially multiple large hardware partitions like
>> UFS, NVMe or eMMC.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   disk/part.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++-----------
>>   include/part.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 88 insertions(+), 11 deletions(-)
>>
>> diff --git a/disk/part.c b/disk/part.c
>> index 303178161c083ec6e1b767b4f06ac5773576ca60..1d09c0511c75d457c81cab040c3f5caa924ee945 100644
>> --- a/disk/part.c
>> +++ b/disk/part.c
>> @@ -335,8 +335,8 @@ void part_print(struct blk_desc *desc)
>>           drv->print(desc);
>>   }
>>
>> -int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>> -              struct disk_partition *info)
> 
> Thank you, Neil, for pushing this forward.
> 
> All functions deserve a description.

Sure

> 
>> +static int _part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>> +                  struct disk_partition *info, bool cached)
>>   {
>>       struct part_driver *drv;
>>
>> @@ -356,24 +356,57 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>>                     desc->part_type);
>>               return -EPROTONOSUPPORT;
>>           }
>> -        if (!drv->get_info) {
>> -            PRINTF("## Driver %s does not have the get_info() method\n",
> 
> Writing to the console creates screen havoc in EFI applications. Please,
> use log_debug() instead of PRINTF().
> 
> And, please, remove that noisy '## '. It carries no information.

Right, those were already present, I'll add a cleanup patch before.

Thx,
Neil

> 
> Best regards
> 
> Heinrich
> 
>> -                   drv->name);
>> -            return -ENOSYS;
>> -        }
>> -        if (drv->get_info(desc, part, info) == 0) {
>> -            PRINTF("## Valid %s partition found ##\n", drv->name);
>> -            return 0;
>> +        if (cached && drv->get_info_cached) {
>> +            if (drv->get_info_cached(desc, part, info) == 0) {
>> +                PRINTF("## Valid %s partition found ##\n", drv->name);
>> +                return 0;
>> +            }
>> +        } else {
>> +            if (!drv->get_info) {
>> +                PRINTF("## Driver %s does not have the get_info() method\n",
>> +                       drv->name);
>> +                return -ENOSYS;
>> +            }
>> +            if (drv->get_info(desc, part, info) == 0) {
>> +                PRINTF("## Valid %s partition found ##\n", drv->name);
>> +                return 0;
>> +            }
>>           }
>>       }
>>
>>       return -ENOENT;
>>   }
>>
>> +int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>> +              struct disk_partition *info)
>> +{
>> +    return _part_get_info_by_type(desc, part, part_type, info, false);
>> +}
>> +
>> +int part_get_info_cached(struct blk_desc *desc, int part,
>> +             struct disk_partition *info)
>> +{
>> +    return _part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info, true);
>> +}
>> +
>>   int part_get_info(struct blk_desc *desc, int part,
>>             struct disk_partition *info)
>>   {
>> -    return part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info);
>> +    return _part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info, false);
>> +}
>> +
>> +void part_get_info_cached_free(struct blk_desc *desc)
>> +{
>> +    struct part_driver *drv;
>> +
>> +    if (blk_enabled()) {
>> +        drv = part_driver_lookup_type(desc);
>> +        if (!drv)
>> +            return;
>> +        if (!drv->get_info_cache_free)
>> +            return;
>> +        drv->get_info_cache_free(desc);
>> +    }
>>   }
>>
>>   int part_get_info_whole_disk(struct blk_desc *desc,
>> diff --git a/include/part.h b/include/part.h
>> index fcb3c13dea4de6346ad98d6ce320ef36747dda85..8c98865146306fb66509576068c45de01b9cedb6 100644
>> --- a/include/part.h
>> +++ b/include/part.h
>> @@ -216,6 +216,34 @@ struct blk_desc *mg_disk_get_dev(int dev);
>>    */
>>   int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>>                 struct disk_partition *info);
>> +
>> +/**
>> + * part_get_info_cached() - Get partitions from a block device but save the
>> + * disk partition map between different partitions.
>> + *
>> + * Call to part_get_info_cached_free() is required when scanning of the
>> + * block device is finished.
>> + *
>> + * If the partition driver doesn't support cached part_info, the
>> + * normal part_info will be called instead.
>> + *
>> + * @desc:    Block device descriptor
>> + * @part:    Partition number to read
>> + * @info:    Returned partition information
>> + *
>> + * Return: 0 on success, negative errno on failure
>> + */
>> +int part_get_info_cached(struct blk_desc *desc, int part,
>> +             struct disk_partition *info);
>> +
>> +/**
>> + * part_get_info_cached_free() - Free the saved disk partition map
>> + * from a previous call of part_get_info_cached().
>> + *
>> + * @desc:    Block device descriptor
>> + */
>> +void part_get_info_cached_free(struct blk_desc *desc);
>> +
>>   int part_get_info(struct blk_desc *desc, int part,
>>             struct disk_partition *info);
>>   /**
>> @@ -463,6 +491,22 @@ struct part_driver {
>>       int part_type;
>>       /** @max_entries:    maximum number of partition table entries */
>>       const int max_entries;
>> +    /**
>> +     * @cache_free_cache_free:    Free any parsing data stored from get_info_cached()
>> +     *
>> +     * @get_info_cache_free.desc:    Block device descriptor
>> +     */
>> +    void (*get_info_cache_free)(struct blk_desc *desc);
>> +    /**
>> +     * @get_inf_cachedo:        Get information about a partition, and save
>> +     *                the resulting parsing data for the next partition.
>> +     *
>> +     * @get_info_cached.desc:    Block device descriptor
>> +     * @get_info_cached.part:    Partition number (1 = first)
>> +     * @get_info_cached.info:    Returns partition information
>> +     */
>> +    int (*get_info_cached)(struct blk_desc *desc, int part,
>> +                   struct disk_partition *info);
>>       /**
>>        * @get_info:        Get information about a partition
>>        *
>>
> 

Re: [PATCH v2 2/3] part: efi: add GPT PTE cache used with part_get_info_cached() API

[Neil Armstrong]

On 08/04/2025 12:57, Heinrich Schuchardt wrote:
> On 08.04.25 11:13, Neil Armstrong wrote:
>> Implement a simple cache for part_efi to be used by the newly
>> introduced part_get_info_cached() API.
>>
>> The cache simply stores a successfully scanned GPT PTE if called
>> from the part_get_info_cached() ops, and will return the cached
>> data if the blk_desc and lba offset matches, invalidating the
>> cache if not.
>>
>> The cache will only operate if called from the part_get_info_cached()
>> API, all the over calls will operate uncached, making sure we
>> can still update & re-scan the GPT partitions like before.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   disk/part_efi.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 96 insertions(+), 18 deletions(-)
>>
>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>> index 932d058c184ce6946b7142e7c2d9637b28e4661e..949968fcd30fc18766a023e73f43ca1381dcef89 100644
>> --- a/disk/part_efi.c
>> +++ b/disk/part_efi.c
>> @@ -55,12 +55,58 @@ static inline u32 efi_crc32(const void *buf, u32 len)
>>   static int pmbr_part_valid(struct partition *part);
>>   static int is_pmbr_valid(legacy_mbr * mbr);
>>   static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
>> -            gpt_entry **pgpt_pte);
>> +            gpt_entry **pgpt_pte, bool cache);
>>   static gpt_entry *alloc_read_gpt_entries(struct blk_desc *desc,
>>                        gpt_header *pgpt_head);
>>   static int is_pte_valid(gpt_entry * pte);
>>   static int find_valid_gpt(struct blk_desc *desc, gpt_header *gpt_head,
>> -              gpt_entry **pgpt_pte);
>> +              gpt_entry **pgpt_pte, bool cache);
>> +
> 
> Please, describe the structure and its members.

Ack

> 
> https://docs.kernel.org/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
> 
>> +static struct gpt_pte_cache_data {
>> +    struct blk_desc *desc;
>> +    u64 lba;
>> +    gpt_entry *gpt_pte;
>> +    gpt_header gpt_head;
>> +} gpt_pte_cache;
>> +
> 
> Please, describe all functions.

Ack

> 
>> +static void clear_gpt_pte_cache(void)
>> +{
>> +    if (gpt_pte_cache.desc) {
>> +        if (gpt_pte_cache.gpt_pte)
>> +            free(gpt_pte_cache.gpt_pte);
>> +
>> +        memset(&gpt_pte_cache, 0, sizeof(gpt_pte_cache));
>> +    }
>> +}
>> +
>> +static void cache_gpt_pte(struct blk_desc *desc, u64 lba,
>> +              gpt_entry *gpt_pte, gpt_header *pgpt_head)
>> +{
>> +    if (gpt_pte_cache.gpt_pte)
>> +        free(gpt_pte_cache.gpt_pte);
>> +
>> +    gpt_pte_cache.desc = desc;
>> +    gpt_pte_cache.lba = lba;
>> +    gpt_pte_cache.gpt_pte = gpt_pte;
>> +    if (pgpt_head)
>> +        memcpy(&gpt_pte_cache.gpt_head, pgpt_head, sizeof(gpt_header));
>> +}
>> +
>> +static gpt_entry *get_cached_gpt_pte(struct blk_desc *desc, u64 lba,
>> +                     gpt_header *pgpt_head)
>> +{
>> +    if (gpt_pte_cache.desc && gpt_pte_cache.gpt_pte) {
>> +        if (gpt_pte_cache.desc == desc &&
>> +            gpt_pte_cache.lba == lba) {
>> +            memcpy(pgpt_head, &gpt_pte_cache.gpt_head, sizeof(gpt_header));
>> +            return gpt_pte_cache.gpt_pte;
>> +        }
>> +
>> +        clear_gpt_pte_cache();
>> +    }
>> +
>> +    return NULL;
>> +}
>>
>>   static char *print_efiname(gpt_entry *pte)
>>   {
>> @@ -205,7 +251,7 @@ int get_disk_guid(struct blk_desc *desc, char *guid)
>>       unsigned char *guid_bin;
>>
>>       /* This function validates AND fills in the GPT header and PTE */
>> -    if (find_valid_gpt(desc, gpt_head, &gpt_pte) != 1)
>> +    if (find_valid_gpt(desc, gpt_head, &gpt_pte, false) != 1)
>>           return -EINVAL;
>>
>>       guid_bin = gpt_head->disk_guid.b;
>> @@ -224,7 +270,7 @@ static void __maybe_unused part_print_efi(struct blk_desc *desc)
>>       unsigned char *uuid;
>>
>>       /* This function validates AND fills in the GPT header and PTE */
>> -    if (find_valid_gpt(desc, gpt_head, &gpt_pte) != 1)
>> +    if (find_valid_gpt(desc, gpt_head, &gpt_pte, false) != 1)
>>           return;
>>
>>       debug("%s: gpt-entry at %p\n", __func__, gpt_pte);
>> @@ -258,8 +304,13 @@ static void __maybe_unused part_print_efi(struct blk_desc *desc)
>>       return;
>>   }
>>
>> -static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
>> -                        struct disk_partition *info)
>> +static void __maybe_unused part_get_info_cache_free_efi(struct blk_desc *desc)
>> +{
>> +    clear_gpt_pte_cache();
>> +}
>> +
>> +static int _part_get_info_efi(struct blk_desc *desc, int part,
>> +                  struct disk_partition *info, bool cache)
>>   {
>>       ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, desc->blksz);
>>       gpt_entry *gpt_pte = NULL;
>> @@ -271,13 +322,14 @@ static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
>>       }
>>
>>       /* This function validates AND fills in the GPT header and PTE */
>> -    if (find_valid_gpt(desc, gpt_head, &gpt_pte) != 1)
>> +    if (find_valid_gpt(desc, gpt_head, &gpt_pte, cache) != 1)
>>           return -EINVAL;
>>
>>       if (part > le32_to_cpu(gpt_head->num_partition_entries) ||
>>           !is_pte_valid(&gpt_pte[part - 1])) {
>>           log_debug("Invalid partition number %d\n", part);
>> -        free(gpt_pte);
>> +        if (!cache)
>> +            free(gpt_pte);
>>           return -EPERM;
>>       }
>>
>> @@ -307,11 +359,23 @@ static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
>>       log_debug("start 0x" LBAF ", size 0x" LBAF ", name %s\n", info->start,
>>             info->size, info->name);
>>
>> -    /* Remember to free pte */
>> -    free(gpt_pte);
>> +    if (!cache)
>> +        free(gpt_pte);
>>       return 0;
>>   }
>>
>> +static int __maybe_unused part_get_info_cached_efi(struct blk_desc *desc, int part,
>> +                           struct disk_partition *info)
>> +{
>> +    return _part_get_info_efi(desc, part, info, true);
>> +}
>> +
>> +static int __maybe_unused part_get_info_efi(struct blk_desc *desc, int part,
>> +                        struct disk_partition *info)
>> +{
>> +    return _part_get_info_efi(desc, part, info, false);
>> +}
>> +
>>   static int part_test_efi(struct blk_desc *desc)
>>   {
>>       ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, legacymbr, 1, desc->blksz);
>> @@ -689,7 +753,7 @@ int gpt_verify_headers(struct blk_desc *desc, gpt_header *gpt_head,
>>        */
>>       if (is_gpt_valid(desc,
>>                GPT_PRIMARY_PARTITION_TABLE_LBA,
>> -             gpt_head, gpt_pte) != 1) {
>> +             gpt_head, gpt_pte, false) != 1) {
>>           log_debug("Invalid GPT\n");
>>           return -1;
>>       }
>> @@ -706,7 +770,7 @@ int gpt_verify_headers(struct blk_desc *desc, gpt_header *gpt_head,
>>       }
>>
>>       if (is_gpt_valid(desc, (desc->lba - 1),
>> -             gpt_head, gpt_pte) != 1) {
>> +             gpt_head, gpt_pte, false) != 1) {
>>           log_debug("Invalid Backup GPT\n");
>>           return -1;
>>       }
>> @@ -765,9 +829,9 @@ int gpt_repair_headers(struct blk_desc *desc)
>>       int ret = -1;
>>
>>       is_gpt1_valid = is_gpt_valid(desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
>> -                     gpt_h1, &gpt_e1);
>> +                     gpt_h1, &gpt_e1, false);
>>       is_gpt2_valid = is_gpt_valid(desc, desc->lba - 1,
>> -                     gpt_h2, &gpt_e2);
>> +                     gpt_h2, &gpt_e2, false);
>>
>>       if (is_gpt1_valid && is_gpt2_valid) {
>>           ret = 0;
>> @@ -1023,12 +1087,13 @@ static int is_pmbr_valid(legacy_mbr *mbr)
>>    * lba is the logical block address of the GPT header to test
>>    * gpt is a GPT header ptr, filled on return.
>>    * ptes is a PTEs ptr, filled on return.
>> + * cache is a bool, true to use the cached gpt_pte from previous call
> 
> "use the cache" or "use cached GPT if available"?
> 
> It remains unclear what will happen if cache=1 and no entry is found.

Ack, will clarify

> 
> Best regards
> 
> Heinrich
> 
>>    *
>>    * Description: returns 1 if valid,  0 on error, 2 if ignored header
>>    * If valid, returns pointers to PTEs.
>>    */
>>   static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
>> -            gpt_entry **pgpt_pte)
>> +            gpt_entry **pgpt_pte, bool cache)
>>   {
>>       /* Confirm valid arguments prior to allocation. */
>>       if (!desc || !pgpt_head) {
>> @@ -1036,6 +1101,12 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
>>           return 0;
>>       }
>>
>> +    if (cache) {
>> +        *pgpt_pte = get_cached_gpt_pte(desc, lba, pgpt_head);
>> +        if (*pgpt_pte)
>> +            return 1;
>> +    }
>> +
>>       ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, mbr, 1, desc->blksz);
>>
>>       /* Read MBR Header from device */
>> @@ -1081,6 +1152,9 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
>>           return 0;
>>       }
>>
>> +    if (cache)
>> +        cache_gpt_pte(desc, lba, *pgpt_pte, pgpt_head);
>> +
>>       /* We're done, all's well */
>>       return 1;
>>   }
>> @@ -1090,23 +1164,25 @@ static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
>>    *
>>    * gpt is a GPT header ptr, filled on return.
>>    * ptes is a PTEs ptr, filled on return.
>> + * cache if a bool, use cached GPT if available
>>    *
>>    * Description: returns 1 if found a valid gpt,  0 on error.
>>    * If valid, returns pointers to PTEs.
>>    */
>>   static int find_valid_gpt(struct blk_desc *desc, gpt_header *gpt_head,
>> -              gpt_entry **pgpt_pte)
>> +              gpt_entry **pgpt_pte, bool cache)
>>   {
>>       int r;
>>
>>       r = is_gpt_valid(desc, GPT_PRIMARY_PARTITION_TABLE_LBA, gpt_head,
>> -             pgpt_pte);
>> +             pgpt_pte, cache);
>>
>>       if (r != 1) {
>>           if (r != 2)
>>               log_debug("Invalid GPT\n");
>>
>> -        if (is_gpt_valid(desc, desc->lba - 1, gpt_head, pgpt_pte)
>> +        if (is_gpt_valid(desc, desc->lba - 1, gpt_head, pgpt_pte,
>> +                 true)
>>               != 1) {
>>               log_debug("Invalid Backup GPT\n");
>>               return 0;
>> @@ -1210,6 +1286,8 @@ U_BOOT_PART_TYPE(a_efi) = {
>>       .name        = "EFI",
>>       .part_type    = PART_TYPE_EFI,
>>       .max_entries    = GPT_ENTRY_NUMBERS,
>> +    .get_info_cache_free    = part_get_info_ptr(part_get_info_cache_free_efi),
>> +    .get_info_cached    = part_get_info_ptr(part_get_info_cached_efi),
>>       .get_info    = part_get_info_ptr(part_get_info_efi),
>>       .print        = part_print_ptr(part_print_efi),
>>       .test        = part_test_efi,
>>
> 

Re: [PATCH v2 1/3] part: add part_get_info_cached() API

[Ilias Apalodimas]

Hi Neil,

On Tue, 8 Apr 2025 at 12:13, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> Introduce a new part_get_info_cached() API that's used to
> get the part_info of a blk_desc allowing to use an eventual
> partition scanning cache to avoid rescanning the entire disk
> partition scheme for each partition number.
>
> The part_get_info_cached_free() is also added to hint the
> partition code to flush any cached data from this disk.
>
> The equivalent ops are added that directly maps to those
> added functions.
>
> This API is designed to be used as a direct replacement of
> part_get_info() in codes scanning all partitions for a same
> disk, like in blk-uclass. With this, a lot of unnecessary
> computation is saved, leading to a faster boot time when
> partitions are scanned, especially with storage medias
> with potentially multiple large hardware partitions like
> UFS, NVMe or eMMC.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  disk/part.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  include/part.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+), 11 deletions(-)
>
> diff --git a/disk/part.c b/disk/part.c
> index 303178161c083ec6e1b767b4f06ac5773576ca60..1d09c0511c75d457c81cab040c3f5caa924ee945 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -335,8 +335,8 @@ void part_print(struct blk_desc *desc)
>                 drv->print(desc);
>  }
>
> -int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
> -                         struct disk_partition *info)
> +static int _part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
> +                                 struct disk_partition *info, bool cached)
>  {
>         struct part_driver *drv;
>
> @@ -356,24 +356,57 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>                               desc->part_type);
>                         return -EPROTONOSUPPORT;
>                 }
> -               if (!drv->get_info) {
> -                       PRINTF("## Driver %s does not have the get_info() method\n",
> -                              drv->name);
> -                       return -ENOSYS;
> -               }
> -               if (drv->get_info(desc, part, info) == 0) {
> -                       PRINTF("## Valid %s partition found ##\n", drv->name);
> -                       return 0;
> +               if (cached && drv->get_info_cached) {
> +                       if (drv->get_info_cached(desc, part, info) == 0) {
> +                               PRINTF("## Valid %s partition found ##\n", drv->name);
> +                               return 0;
> +                       }
> +               } else {
> +                       if (!drv->get_info) {
> +                               PRINTF("## Driver %s does not have the get_info() method\n",
> +                                      drv->name);
> +                               return -ENOSYS;
> +                       }
> +                       if (drv->get_info(desc, part, info) == 0) {
> +                               PRINTF("## Valid %s partition found ##\n", drv->name);
> +                               return 0;
> +                       }
>                 }

That's fine but since you'll send a v3 with a cleanup on top mind also
switching the logic around on this?

if (!blk_enabled())
    return -ENOENT; etc

This will make it a bit more readable

>         }
>
>         return -ENOENT;
>  }
>
> +int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
> +                         struct disk_partition *info)
> +{
> +       return _part_get_info_by_type(desc, part, part_type, info, false);
> +}
> +
> +int part_get_info_cached(struct blk_desc *desc, int part,
> +                        struct disk_partition *info)
> +{
> +       return _part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info, true);
> +}
> +
>  int part_get_info(struct blk_desc *desc, int part,
>                   struct disk_partition *info)
>  {
> -       return part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info);
> +       return _part_get_info_by_type(desc, part, PART_TYPE_UNKNOWN, info, false);
> +}
> +
> +void part_get_info_cached_free(struct blk_desc *desc)
> +{
> +       struct part_driver *drv;
> +
> +       if (blk_enabled()) {

Same here
if (!blk_enabled())
    return;
etc

[...]

Other than that LGTM

Cheers
/Ilias

Re: [PATCH v2 2/3] part: efi: add GPT PTE cache used with part_get_info_cached() API

[Ilias Apalodimas]

Hi Neil,


On Tue, 8 Apr 2025 at 12:13, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> Implement a simple cache for part_efi to be used by the newly
> introduced part_get_info_cached() API.
>
> The cache simply stores a successfully scanned GPT PTE if called
> from the part_get_info_cached() ops, and will return the cached
> data if the blk_desc and lba offset matches, invalidating the
> cache if not.
>
> The cache will only operate if called from the part_get_info_cached()
> API, all the over calls will operate uncached, making sure we
> can still update & re-scan the GPT partitions like before.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  disk/part_efi.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 96 insertions(+), 18 deletions(-)
>
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 932d058c184ce6946b7142e7c2d9637b28e4661e..949968fcd30fc18766a023e73f43ca1381dcef89 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -55,12 +55,58 @@ static inline u32 efi_crc32(const void *buf, u32 len)
>  static int pmbr_part_valid(struct partition *part);
>  static int is_pmbr_valid(legacy_mbr * mbr);
>  static int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
> -                       gpt_entry **pgpt_pte);
> +                       gpt_entry **pgpt_pte, bool cache);
>  static gpt_entry *alloc_read_gpt_entries(struct blk_desc *desc,
>                                          gpt_header *pgpt_head);
>  static int is_pte_valid(gpt_entry * pte);
>  static int find_valid_gpt(struct blk_desc *desc, gpt_header *gpt_head,
> -                         gpt_entry **pgpt_pte);
> +                         gpt_entry **pgpt_pte, bool cache);
> +
> +static struct gpt_pte_cache_data {
> +       struct blk_desc *desc;
> +       u64 lba;
> +       gpt_entry *gpt_pte;
> +       gpt_header gpt_head;
> +} gpt_pte_cache;

My only concern is that this is global, and I don't know how it will
affect the caching with the threading framework that's under
discussion.
I don't know the block framework well enough, but it would make some
sense to store this per block device eventually.

> +
> +static void clear_gpt_pte_cache(void)
> +{
> +       if (gpt_pte_cache.desc) {
> +               if (gpt_pte_cache.gpt_pte)
> +                       free(gpt_pte_cache.gpt_pte);
> +
[...]

Other than that this looks sane to me

Thanks
/Ilias