[resent RFC 00/22] efi_loader: more tightly integrate UEFI disks to device model

[resent RFC 00/22] efi_loader: more tightly integrate UEFI disks to device model

[AKASHI Takahiro]

# Resending the RFC as some of patches were deplicately submitted.
# See also
  https://git.linaro.org/people/takahiro.akashi/u-boot.git efi/dm_disk

The purpose of this RPC is to reignite the discussion about how UEFI
subystem would best be integrated into U-Boot device model.
In the past, I poposed a couple of patch series, the latest one[1],
while Heinrich revealed his idea[2], and the approach taken here is
something between them, with a focus on block device handlings.

# The code is a PoC and not well tested yet.

Disks in UEFI world:
====================
In general in UEFI world, accessing to any device is performed through
a 'protocol' interface which are installed to (or associated with) the device's
UEFI handle (or an opaque pointer to UEFI object data). Protocols are
implemented by either the UEFI system itself or UEFI drivers.

For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk
hereafter). Currently, every efi_disk may have one of two origins:
a.U-Boot's block devices or related partitions
  (lib/efi_loader/efi_disk.c)
b.UEFI objects which are implemented as a block device by UEFI drivers.
  (lib/efi_driver/efi_block_device.c)

All the efi_diskss as (a) will be enumelated and created only once at UEFI
subsystem initialization (efi_disk_register()), which is triggered by
first executing one of UEFI-related U-Boot commands, like "bootefi",
"setenv -e" or "efidebug".
EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops)
in the corresponding udevice(UCLASS_BLK).

On the other hand, efi_disk as (b) will be created each time UEFI boot
services' connect_controller() is executed in UEFI app which, as a (device)
controller, gives the method to access the device's data,
ie. EFI_BLOCK_IO_PROTOCOL.

>>> more details >>>
Internally, connect_controller() search for UEFI driver that can support
this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case,
then calls the driver's 'bind' interface, which eventually installs
the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object.
'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for
  * creating additional partitions efi_disk's, and
  * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it.
<<< <<<

Issues:
=======
1. While an efi_disk represents a device equally for either a whole disk
   or a partition in UEFI world, the device model treats only a whole
   disk as a real block device or udevice(UCLASS_BLK).

2. efi_disk holds and makes use of "blk_desc" data even though blk_desc
   in plat_data is supposed to be private and not to be accessed outside
   the device model.
   # This issue, though, exists for all the implmenetation of U-Boot
   # file systems as well.

For efi_disk(a),
3. A block device can be enumelated dynamically by 'scanning' a device bus
   in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly.
   For examples,
    => scsi rescan; efidebug devices
    => usb start; efidebug devices ... (A)
   (A) doesn't show any usb devices detected.

    => scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ...
    => scsi rescan ... (B)
    => bootefi bootmgr ... (C)
   (C) may de-reference a bogus blk_desc pointer which has been freed by (B).
   (Please note that "scsi rescan" removes all udevices/blk_desc and then
    re-create them even if nothing is changed on a bus.)

For efi_disk(b),
4. A controller (handle), combined with efi_block driver, has no
   corresponding udevice as a parent of efi_disks in DM tree, unlike, say,
   a scsi controller, even though it provides methods for block io perations.
5. There is no way supported to remove efi_disk's even after
   disconnect_controller() is called.


My approach in this RFC:
========================
Due to functional differences in semantics, it would be difficult
to identify "udevice" structure as a handle in UEFI world. Instead, we will
have to somehow maintain a relationship between a udevice and a handle.

1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions
   Currently, the uclass for paritions is not a UCLASS_BLK.
   It can be possible to define partitions as UCLASS_BLK
   (with IF_TYPE_PARTION?), but
   I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK)
   is tightly coupled with 'struct blk_desc' data which is still used
   as a "structure to a whole disk" in a lot of interfaces.
   (I hope that you understand what it means.)

   In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent:
   For instance,
       UCLASS_SCSI  --- UCLASS_BLK       --- UCLASS_PARTITION
			 (IF_TYPE_SCSI)        |
                          +- struct blk_desc   +- struct disk_part
			  +- scsi_blk_ops      +- blk_part_ops

1-2. create partition udevices in the context of device_probe() 
   part_init() is already called in blk_post_probe(). See the commit
   d0851c893706 ("blk: Call part_init() in the post_probe() method").
   Why not enumelate partitions as well in there.

2. add new block access interfaces, which takes "udevice" as a target device,
   in U-Boot and use those functions to implement efi_disk operations
   (i.e. EFI_BLOCK_IO_PROTOCOL).

3-1. maintain a bi-directional link by adding
   - a UEFI handle pointer in "struct udevice"
   - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")

3-2. use device model's post_probe/pre_remove hook to synchronize the lifetime
   of efi_disk objects in UEFI world with the device model.

4. I have no answer to issue(4) and (5) yet.


Patchs:
=======
For easy understandings, patches may be categorized into seperate groups
of changes.

Patch#1-#9: DM: create udevices for partitions
Patch#10,#11: UEFI: use udevice interfaces
Patch#12-#16: UEFI: sync up with the device model for adding
Patch#17-#19: UEFI: sync up with the device model for removing
  For removal case, we may need more consideration since removing handles
  unconditionally may end up breaking integrity of handles
  (some may still be held and referenced to by a UEFI app).
Patch#20-#22: UEFI: align efi_driver with changes by the integration


[1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html
[2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html

AKASHI Takahiro (22):
  part: call part_init() in blk_get_device_by_str() only for MMC
  scsi: call device_probe() after scanning
  usb: storage: call device_probe() after scanning
  mmc: call device_probe() after scanning
  nvme: call device_probe() after scanning
  sata: call device_probe() after scanning
  block: ide: call device_probe() after scanning
  dm: blk: add UCLASS_PARTITION
  dm: blk: add a device-probe hook for scanning disk partitions
  dm: blk: add read/write interfaces with udevice
  efi_loader: disk: use udevice instead of blk_desc
  dm: add a hidden link to efi object
  efi_loader: remove !CONFIG_BLK code from efi_disk
  efi_loader: disk: a helper function to create efi_disk objects from
    udevice
  dm: blk: call efi's device-probe hook
  efi_loader: cleanup after efi_disk-dm integration
  efi_loader: add efi_remove_handle()
  efi_loader: efi_disk: a helper function to delete efi_disk objects
  dm: blk: call efi's device-removal hook
  efi_driver: align with efi_disk-dm integration
  efi_driver: cleanup after efi_disk-dm integration
  efi_selftest: block device: adjust dp for a test disk

 common/usb_storage.c                         |   6 +
 disk/part.c                                  |   3 +-
 drivers/ata/dwc_ahsata.c                     |  10 +
 drivers/ata/fsl_sata.c                       |  11 +
 drivers/ata/sata_mv.c                        |   9 +
 drivers/ata/sata_sil.c                       |  12 +
 drivers/block/blk-uclass.c                   | 249 ++++++++++++++++
 drivers/block/ide.c                          |   6 +
 drivers/mmc/mmc-uclass.c                     |   7 +
 drivers/nvme/nvme.c                          |   6 +
 drivers/scsi/scsi.c                          |  10 +
 include/blk.h                                |  15 +
 include/dm/device.h                          |   4 +
 include/dm/uclass-id.h                       |   1 +
 include/efi_loader.h                         |   8 +-
 lib/efi_driver/efi_block_device.c            |  30 +-
 lib/efi_loader/efi_boottime.c                |   8 +
 lib/efi_loader/efi_device_path.c             |  29 ++
 lib/efi_loader/efi_disk.c                    | 286 ++++++++++---------
 lib/efi_loader/efi_setup.c                   |   5 -
 lib/efi_selftest/efi_selftest_block_device.c |  26 +-
 21 files changed, 566 insertions(+), 175 deletions(-)

-- 
2.33.0

[resent RFC 01/22] part: call part_init() in blk_get_device_by_str() only for MMC

[AKASHI Takahiro]

In blk_get_device_by_str(), the comment says: "Updates the partition table
for the specified hw partition."
Since hw partition is supported only on MMC, it makes no sense to do so
for other devices.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 disk/part.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/disk/part.c b/disk/part.c
index a6a8f7052bd3..b330103a5bc0 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -427,7 +427,8 @@ int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,
 	 * Always should be done, otherwise hw partition 0 will return stale
 	 * data after displaying a non-zero hw partition.
 	 */
-	part_init(*dev_desc);
+	if ((*dev_desc)->if_type == IF_TYPE_MMC)
+		part_init(*dev_desc);
 #endif
 
 cleanup:
-- 
2.33.0

[resent RFC 02/22] scsi: call device_probe() after scanning

[AKASHI Takahiro]

Every time a scsi bus/port is scanned and a new block device is detected,
we want to call device_probe() as it will give us a chance to run additional
post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/scsi/scsi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index d93d24192853..4865b5a86fd5 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -595,6 +595,16 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose)
 		ata_swap_buf_le16((u16 *)&bdesc->revision, sizeof(bd.revision) / 2);
 	}
 
+	ret = device_probe(bdev);
+	if (ret < 0) {
+		debug("Can't probe\n");
+		/* TODO: undo create */
+
+		ret = device_unbind(bdev);
+
+		return ret;
+	}
+
 	if (verbose) {
 		printf("  Device %d: ", bdesc->devnum);
 		dev_print(bdesc);
-- 
2.33.0

[resent RFC 03/22] usb: storage: call device_probe() after scanning

[AKASHI Takahiro]

Every time a usb bus/port is scanned and a new device is detected,
we want to call device_probe() as it will give us a chance to run additional
post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 common/usb_storage.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 946c6b2b323a..5f294f17491f 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -239,6 +239,12 @@ static int usb_stor_probe_device(struct usb_device *udev)
 			if (ret)
 				return ret;
 		}
+
+		ret = device_probe(dev);
+		if (ret) {
+			device_unbind(dev);
+			return ret;
+		}
 	}
 #else
 	/* We don't have space to even probe if we hit the maximum */
-- 
2.33.0

[resent RFC 04/22] mmc: call device_probe() after scanning

[AKASHI Takahiro]

Every time a mmc bus/port is scanned and a new device is detected,
we want to call device_probe() as it will give us a chance to run additional
post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/mmc/mmc-uclass.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
index 3ee92d03ca23..07b5c1736439 100644
--- a/drivers/mmc/mmc-uclass.c
+++ b/drivers/mmc/mmc-uclass.c
@@ -418,6 +418,13 @@ int mmc_bind(struct udevice *dev, struct mmc *mmc, const struct mmc_config *cfg)
 	bdesc->part_type = cfg->part_type;
 	mmc->dev = dev;
 	mmc->user_speed_mode = MMC_MODES_END;
+
+	ret = device_probe(dev);
+	if (ret) {
+		device_unbind(dev);
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.33.0

[resent RFC 05/22] nvme: call device_probe() after scanning

[AKASHI Takahiro]

Every time a nvme bus/port is scanned and a new device is detected,
we want to call device_probe() as it will give us a chance to run additional
post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/nvme/nvme.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
index f6465ea7f482..975bbc6dc3b7 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -909,6 +909,12 @@ static int nvme_probe(struct udevice *udev)
 					 -1, 512, 0, &ns_udev);
 		if (ret)
 			goto free_id;
+
+		ret = device_probe(ns_udev);
+		if (ret) {
+			device_unbind(ns_udev);
+			goto free_id;
+		}
 	}
 
 	free(id);
-- 
2.33.0

[resent RFC 06/22] sata: call device_probe() after scanning

[AKASHI Takahiro]

Every time a sata bus/port is scanned and a new device is detected,
we want to call device_probe() as it will give us a chance to run additional
post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/ata/dwc_ahsata.c | 10 ++++++++++
 drivers/ata/fsl_sata.c   | 11 +++++++++++
 drivers/ata/sata_mv.c    |  9 +++++++++
 drivers/ata/sata_sil.c   | 12 ++++++++++++
 4 files changed, 42 insertions(+)

diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c
index 6d42548087b3..6a51c70d1170 100644
--- a/drivers/ata/dwc_ahsata.c
+++ b/drivers/ata/dwc_ahsata.c
@@ -1026,6 +1026,16 @@ int dwc_ahsata_scan(struct udevice *dev)
 		return ret;
 	}
 
+	ret = device_probe(bdev);
+	if (ret < 0) {
+		debug("Can't probe\n");
+		/* TODO: undo create */
+
+		device_unbind(bdev);
+
+		return ret;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/ata/fsl_sata.c b/drivers/ata/fsl_sata.c
index e44db0a37458..346e9298b4c5 100644
--- a/drivers/ata/fsl_sata.c
+++ b/drivers/ata/fsl_sata.c
@@ -982,6 +982,17 @@ static int fsl_ata_probe(struct udevice *dev)
 			failed_number++;
 			continue;
 		}
+
+		ret = device_probe(bdev);
+		if (ret < 0) {
+			debug("Can't probe\n");
+			ret = fsl_unbind_device(blk);
+			if (ret)
+				return ret;
+
+			failed_number++;
+			continue;
+		}
 	}
 
 	if (failed_number == nr_ports)
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 003222d47be6..09b735779ebf 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -1099,6 +1099,15 @@ static int sata_mv_probe(struct udevice *dev)
 			continue;
 		}
 
+		ret = device_probe(bdev);
+		if (ret < 0) {
+			debug("Can't probe\n");
+			/* TODO: undo create */
+
+			device_unbind(bdev);
+			continue;
+		}
+
 		/* If we got here, the current SATA port was probed
 		 * successfully, so set the probe status to successful.
 		 */
diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index dda712f42cb2..295f7ca72303 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -864,6 +864,18 @@ static int sil_pci_probe(struct udevice *dev)
 			failed_number++;
 			continue;
 		}
+
+		ret = device_probe(bdev);
+		if (ret < 0) {
+			debug("Can't probe\n");
+			ret = sil_unbind_device(blk);
+			device_unbind(bdev);
+			if (ret)
+				return ret;
+
+			failed_number++;
+			continue;
+		}
 	}
 
 	if (failed_number == sata_info.maxport)
-- 
2.33.0

[resent RFC 07/22] block: ide: call device_probe() after scanning

[AKASHI Takahiro]

Every time an ide bus/port is scanned and a new device is detected,
we want to call device_probe() as it will give us a chance to run additional
post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/block/ide.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/block/ide.c b/drivers/block/ide.c
index c99076c6f45d..31aaed09ab70 100644
--- a/drivers/block/ide.c
+++ b/drivers/block/ide.c
@@ -1151,6 +1151,12 @@ static int ide_probe(struct udevice *udev)
 						 blksz, size, &blk_dev);
 			if (ret)
 				return ret;
+
+			ret = device_probe(blk_dev);
+			if (ret) {
+				device_unbind(blk_dev);
+				return ret;
+			}
 		}
 	}
 
-- 
2.33.0

[resent RFC 08/22] dm: blk: add UCLASS_PARTITION

[AKASHI Takahiro]

UCLASS_PARTITION device will be created as a child node of
UCLASS_BLK device.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/block/blk-uclass.c | 111 +++++++++++++++++++++++++++++++++++++
 include/blk.h              |   9 +++
 include/dm/uclass-id.h     |   1 +
 3 files changed, 121 insertions(+)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 83682dcc181a..dd7f3c0fe31e 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -12,6 +12,7 @@
 #include <log.h>
 #include <malloc.h>
 #include <part.h>
+#include <string.h>
 #include <dm/device-internal.h>
 #include <dm/lists.h>
 #include <dm/uclass-internal.h>
@@ -695,6 +696,44 @@ int blk_unbind_all(int if_type)
 	return 0;
 }
 
+int blk_create_partitions(struct udevice *parent)
+{
+	int part, count;
+	struct blk_desc *desc = dev_get_uclass_plat(parent);
+	struct disk_partition info;
+	struct disk_part *part_data;
+	char devname[32];
+	struct udevice *dev;
+	int ret;
+
+	if (!CONFIG_IS_ENABLED(PARTITIONS) ||
+	    !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
+		return 0;
+
+	/* Add devices for each partition */
+	for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
+		if (part_get_info(desc, part, &info))
+			continue;
+		snprintf(devname, sizeof(devname), "%s:%d", parent->name,
+			 part);
+
+		ret = device_bind_driver(parent, "blk_partition",
+					 strdup(devname), &dev);
+		if (ret)
+			return ret;
+
+		part_data = dev_get_uclass_plat(dev);
+		part_data->partnum = part;
+		part_data->gpt_part_info = info;
+		count++;
+
+		device_probe(dev);
+	}
+	debug("%s: %d partitions found in %s\n", __func__, count, parent->name);
+
+	return 0;
+}
+
 static int blk_post_probe(struct udevice *dev)
 {
 	if (IS_ENABLED(CONFIG_PARTITIONS) &&
@@ -713,3 +752,75 @@ UCLASS_DRIVER(blk) = {
 	.post_probe	= blk_post_probe,
 	.per_device_plat_auto	= sizeof(struct blk_desc),
 };
+
+static ulong blk_part_read(struct udevice *dev, lbaint_t start,
+			   lbaint_t blkcnt, void *buffer)
+{
+	struct udevice *parent;
+	struct disk_part *part;
+	const struct blk_ops *ops;
+
+	parent = dev_get_parent(dev);
+	ops = blk_get_ops(parent);
+	if (!ops->read)
+		return -ENOSYS;
+
+	part = dev_get_uclass_plat(dev);
+	start += part->gpt_part_info.start;
+
+	return ops->read(parent, start, blkcnt, buffer);
+}
+
+static ulong blk_part_write(struct udevice *dev, lbaint_t start,
+			    lbaint_t blkcnt, const void *buffer)
+{
+	struct udevice *parent;
+	struct disk_part *part;
+	const struct blk_ops *ops;
+
+	parent = dev_get_parent(dev);
+	ops = blk_get_ops(parent);
+	if (!ops->write)
+		return -ENOSYS;
+
+	part = dev_get_uclass_plat(dev);
+	start += part->gpt_part_info.start;
+
+	return ops->write(parent, start, blkcnt, buffer);
+}
+
+static ulong blk_part_erase(struct udevice *dev, lbaint_t start,
+			    lbaint_t blkcnt)
+{
+	struct udevice *parent;
+	struct disk_part *part;
+	const struct blk_ops *ops;
+
+	parent = dev_get_parent(dev);
+	ops = blk_get_ops(parent);
+	if (!ops->erase)
+		return -ENOSYS;
+
+	part = dev_get_uclass_plat(dev);
+	start += part->gpt_part_info.start;
+
+	return ops->erase(parent, start, blkcnt);
+}
+
+static const struct blk_ops blk_part_ops = {
+	.read	= blk_part_read,
+	.write	= blk_part_write,
+	.erase	= blk_part_erase,
+};
+
+U_BOOT_DRIVER(blk_partition) = {
+	.name		= "blk_partition",
+	.id		= UCLASS_PARTITION,
+	.ops		= &blk_part_ops,
+};
+
+UCLASS_DRIVER(partition) = {
+	.id		= UCLASS_PARTITION,
+	.per_device_plat_auto	= sizeof(struct disk_part),
+	.name		= "partition",
+};
diff --git a/include/blk.h b/include/blk.h
index 19bab081c2cd..3d883eb1db64 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -366,6 +366,15 @@ int blk_create_devicef(struct udevice *parent, const char *drv_name,
 		       const char *name, int if_type, int devnum, int blksz,
 		       lbaint_t lba, struct udevice **devp);
 
+/**
+ * blk_create_partitions - Create block devices for disk partitions
+ *
+ * Create UCLASS_PARTITION udevices for each of disk partitions in @parent
+ *
+ * @parent:	Whole disk device
+ */
+int blk_create_partitions(struct udevice *parent);
+
 /**
  * blk_unbind_all() - Unbind all device of the given interface type
  *
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index e7edd409f307..30892d01ce13 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -80,6 +80,7 @@ enum uclass_id {
 	UCLASS_P2SB,		/* (x86) Primary-to-Sideband Bus */
 	UCLASS_PANEL,		/* Display panel, such as an LCD */
 	UCLASS_PANEL_BACKLIGHT,	/* Backlight controller for panel */
+	UCLASS_PARTITION,	/* Logical disk partition device */
 	UCLASS_PCH,		/* x86 platform controller hub */
 	UCLASS_PCI,		/* PCI bus */
 	UCLASS_PCI_EP,		/* PCI endpoint device */
-- 
2.33.0

[resent RFC 09/22] dm: blk: add a device-probe hook for scanning disk partitions

[AKASHI Takahiro]

Now that all the block device drivers have enable a probe hook, we will
call blk_create_partitions() to enumerate all the partitions and create
associated udevices when a block device is detected.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/block/blk-uclass.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index dd7f3c0fe31e..6ba11a8fa7f7 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -741,11 +741,25 @@ static int blk_post_probe(struct udevice *dev)
 		struct blk_desc *desc = dev_get_uclass_plat(dev);
 
 		part_init(desc);
+
+		if (desc->part_type != PART_TYPE_UNKNOWN &&
+		    blk_create_partitions(dev))
+			debug("*** creating partitions failed\n");
 	}
 
 	return 0;
 }
 
+static int blk_part_post_probe(struct udevice *dev)
+{
+	/*
+	 * TODO:
+	 * If we call blk_creat_partitions() here, it would allow for
+	 * "partitions in a partition".
+	 */
+	return 0;
+}
+
 UCLASS_DRIVER(blk) = {
 	.id		= UCLASS_BLK,
 	.name		= "blk",
@@ -821,6 +835,7 @@ U_BOOT_DRIVER(blk_partition) = {
 
 UCLASS_DRIVER(partition) = {
 	.id		= UCLASS_PARTITION,
+	.post_probe	= blk_part_post_probe,
 	.per_device_plat_auto	= sizeof(struct disk_part),
 	.name		= "partition",
 };
-- 
2.33.0

[resent RFC 10/22] dm: blk: add read/write interfaces with udevice

[AKASHI Takahiro]

In include/blk.h, Simon suggested:
===>
/*
 * These functions should take struct udevice instead of struct blk_desc,
 * but this is convenient for migration to driver model. Add a 'd' prefix
 * to the function operations, so that blk_read(), etc. can be reserved for
 * functions with the correct arguments.
 */
unsigned long blk_dread(struct blk_desc *block_dev, lbaint_t start,
                        lbaint_t blkcnt, void *buffer);
unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t start,
                         lbaint_t blkcnt, const void *buffer);
unsigned long blk_derase(struct blk_desc *block_dev, lbaint_t start,
                         lbaint_t blkcnt);
<===

So new interfaces are provided with this patch.

They are expected to be used everywhere in U-Boot at the end. The exceptions
are block device drivers, partition drivers and efi_disk which should know
details of blk_desc structure.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/block/blk-uclass.c | 91 ++++++++++++++++++++++++++++++++++++++
 include/blk.h              |  6 +++
 2 files changed, 97 insertions(+)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 6ba11a8fa7f7..8fbec8779e1e 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -482,6 +482,97 @@ unsigned long blk_derase(struct blk_desc *block_dev, lbaint_t start,
 	return ops->erase(dev, start, blkcnt);
 }
 
+static struct blk_desc *dev_get_blk(struct udevice *dev)
+{
+	struct blk_desc *block_dev;
+
+	switch (device_get_uclass_id(dev)) {
+	case UCLASS_BLK:
+		block_dev = dev_get_uclass_plat(dev);
+		break;
+	case UCLASS_PARTITION:
+		block_dev = dev_get_uclass_plat(dev_get_parent(dev));
+		break;
+	default:
+		block_dev = NULL;
+		break;
+	}
+
+	return block_dev;
+}
+
+unsigned long blk_read(struct udevice *dev, lbaint_t start,
+		       lbaint_t blkcnt, void *buffer)
+{
+	struct blk_desc *block_dev;
+	const struct blk_ops *ops;
+	struct disk_part *part;
+	lbaint_t start_in_disk;
+	ulong blks_read;
+
+	block_dev = dev_get_blk(dev);
+	if (!block_dev)
+		return -ENOSYS;
+
+	ops = blk_get_ops(dev);
+	if (!ops->read)
+		return -ENOSYS;
+
+	start_in_disk = start;
+	if (device_get_uclass_id(dev) == UCLASS_PARTITION) {
+		part = dev_get_uclass_plat(dev);
+		start_in_disk += part->gpt_part_info.start;
+	}
+
+	if (blkcache_read(block_dev->if_type, block_dev->devnum,
+			  start_in_disk, blkcnt, block_dev->blksz, buffer))
+		return blkcnt;
+	blks_read = ops->read(dev, start, blkcnt, buffer);
+	if (blks_read == blkcnt)
+		blkcache_fill(block_dev->if_type, block_dev->devnum,
+			      start_in_disk, blkcnt, block_dev->blksz, buffer);
+
+	return blks_read;
+}
+
+unsigned long blk_write(struct udevice *dev, lbaint_t start,
+			lbaint_t blkcnt, const void *buffer)
+{
+	struct blk_desc *block_dev;
+	const struct blk_ops *ops;
+
+	block_dev = dev_get_blk(dev);
+	if (!block_dev)
+		return -ENOSYS;
+
+	ops = blk_get_ops(dev);
+	if (!ops->write)
+		return -ENOSYS;
+
+	blkcache_invalidate(block_dev->if_type, block_dev->devnum);
+
+	return ops->write(dev, start, blkcnt, buffer);
+}
+
+unsigned long blk_erase(struct udevice *dev, lbaint_t start,
+			lbaint_t blkcnt)
+{
+	struct blk_desc *block_dev;
+	const struct blk_ops *ops;
+
+	block_dev = dev_get_blk(dev);
+	if (!block_dev)
+		return -ENOSYS;
+
+	ops = blk_get_ops(dev);
+	if (!ops->erase)
+		return -ENOSYS;
+
+	blkcache_invalidate(block_dev->if_type, block_dev->devnum);
+
+	return ops->erase(dev, start, blkcnt);
+}
+
 int blk_get_from_parent(struct udevice *parent, struct udevice **devp)
 {
 	struct udevice *dev;
diff --git a/include/blk.h b/include/blk.h
index 3d883eb1db64..f5fdd6633a09 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -284,6 +284,12 @@ unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t start,
 unsigned long blk_derase(struct blk_desc *block_dev, lbaint_t start,
 			 lbaint_t blkcnt);
 
+unsigned long blk_read(struct udevice *dev, lbaint_t start,
+		       lbaint_t blkcnt, void *buffer);
+unsigned long blk_write(struct udevice *dev, lbaint_t start,
+			lbaint_t blkcnt, const void *buffer);
+unsigned long blk_erase(struct udevice *dev, lbaint_t start,
+			lbaint_t blkcnt);
 /**
  * blk_find_device() - Find a block device
  *
-- 
2.33.0

[resent RFC 11/22] efi_loader: disk: use udevice instead of blk_desc

[AKASHI Takahiro]

In most of all usages, we can avoid using blk_desc which is expected
to be data private to the device not be accessed outside device drivers.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_disk.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 988907ecb910..dfa6f898d586 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -45,7 +45,7 @@ struct efi_disk_obj {
 	unsigned int part;
 	struct efi_simple_file_system_protocol *volume;
 	lbaint_t offset;
-	struct blk_desc *desc;
+	struct udevice *dev; /* TODO: move it to efi_object */
 };
 
 /**
@@ -80,14 +80,12 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
 			void *buffer, enum efi_disk_direction direction)
 {
 	struct efi_disk_obj *diskobj;
-	struct blk_desc *desc;
 	int blksz;
 	int blocks;
 	unsigned long n;
 
 	diskobj = container_of(this, struct efi_disk_obj, ops);
-	desc = (struct blk_desc *) diskobj->desc;
-	blksz = desc->blksz;
+	blksz = diskobj->media.block_size;
 	blocks = buffer_size / blksz;
 	lba += diskobj->offset;
 
@@ -99,9 +97,9 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
 		return EFI_BAD_BUFFER_SIZE;
 
 	if (direction == EFI_DISK_READ)
-		n = blk_dread(desc, lba, blocks, buffer);
+		n = blk_read(diskobj->dev, lba, blocks, buffer);
 	else
-		n = blk_dwrite(desc, lba, blocks, buffer);
+		n = blk_write(diskobj->dev, lba, blocks, buffer);
 
 	/* We don't do interrupts, so check for timers cooperatively */
 	efi_timer_check();
@@ -443,7 +441,6 @@ static efi_status_t efi_disk_add_dev(
 	diskobj->ops = block_io_disk_template;
 	diskobj->ifname = if_typename;
 	diskobj->dev_index = dev_index;
-	diskobj->desc = desc;
 
 	/* Fill in EFI IO Media info (for read/write callbacks) */
 	diskobj->media.removable_media = desc->removable;
@@ -647,20 +644,22 @@ bool efi_disk_is_system_part(efi_handle_t handle)
 {
 	struct efi_handler *handler;
 	struct efi_disk_obj *diskobj;
-	struct disk_partition info;
+	struct udevice *dev;
+	struct disk_part *part;
 	efi_status_t ret;
-	int r;
 
 	/* check if this is a block device */
 	ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
 	if (ret != EFI_SUCCESS)
 		return false;
 
+	/* find a partition udevice */
 	diskobj = container_of(handle, struct efi_disk_obj, header);
-
-	r = part_get_info(diskobj->desc, diskobj->part, &info);
-	if (r)
+	dev = diskobj->dev;
+	if (!dev || dev->driver->id != UCLASS_PARTITION)
 		return false;
 
-	return !!(info.bootable & PART_EFI_SYSTEM_PARTITION);
+	part = dev_get_uclass_plat(dev);
+
+	return !!(part->gpt_part_info.bootable & PART_EFI_SYSTEM_PARTITION);
 }
-- 
2.33.0

[resent RFC 12/22] dm: add a hidden link to efi object

[AKASHI Takahiro]

This member field in udevice will be used to dereference from udevice
to efi_object (or efi_handle).

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

diff --git a/include/dm/device.h b/include/dm/device.h
index 0a9718a5b81a..33b09a836f06 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -190,6 +190,10 @@ struct udevice {
 #if CONFIG_IS_ENABLED(DM_DMA)
 	ulong dma_offset;
 #endif
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+	/* link to efi_object */
+	void *efi_obj;
+#endif
 };
 
 /**
-- 
2.33.0

[resent RFC 13/22] efi_loader: remove !CONFIG_BLK code from efi_disk

[AKASHI Takahiro]

The change in this patch will probably have been covered by other guy's patch.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_disk.c | 49 ---------------------------------------
 1 file changed, 49 deletions(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index dfa6f898d586..cd5528046251 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -552,7 +552,6 @@ efi_status_t efi_disk_register(void)
 	struct efi_disk_obj *disk;
 	int disks = 0;
 	efi_status_t ret;
-#ifdef CONFIG_BLK
 	struct udevice *dev;
 
 	for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
@@ -580,54 +579,6 @@ efi_status_t efi_disk_register(void)
 					&disk->header, desc, if_typename,
 					desc->devnum, dev->name);
 	}
-#else
-	int i, if_type;
-
-	/* Search for all available disk devices */
-	for (if_type = 0; if_type < IF_TYPE_COUNT; if_type++) {
-		const struct blk_driver *cur_drvr;
-		const char *if_typename;
-
-		cur_drvr = blk_driver_lookup_type(if_type);
-		if (!cur_drvr)
-			continue;
-
-		if_typename = cur_drvr->if_typename;
-		log_info("Scanning disks on %s...\n", if_typename);
-		for (i = 0; i < 4; i++) {
-			struct blk_desc *desc;
-			char devname[32] = { 0 }; /* dp->str is u16[32] long */
-
-			desc = blk_get_devnum_by_type(if_type, i);
-			if (!desc)
-				continue;
-			if (desc->type == DEV_TYPE_UNKNOWN)
-				continue;
-
-			snprintf(devname, sizeof(devname), "%s%d",
-				 if_typename, i);
-
-			/* Add block device for the full device */
-			ret = efi_disk_add_dev(NULL, NULL, if_typename, desc,
-					       i, NULL, 0, &disk);
-			if (ret == EFI_NOT_READY) {
-				log_notice("Disk %s not ready\n", devname);
-				continue;
-			}
-			if (ret) {
-				log_err("ERROR: failure to add disk device %s, r = %lu\n",
-					devname, ret & ~EFI_ERROR_MASK);
-				return ret;
-			}
-			disks++;
-
-			/* Partitions show up as block devices in EFI */
-			disks += efi_disk_create_partitions
-						(&disk->header, desc,
-						 if_typename, i, devname);
-		}
-	}
-#endif
 	log_info("Found %d disks\n", disks);
 
 	return EFI_SUCCESS;
-- 
2.33.0

[resent RFC 14/22] efi_loader: disk: a helper function to create efi_disk objects from udevice

[AKASHI Takahiro]

Add efi_disk_create() function.

Any UEFI handle created by efi_disk_create() can be treated as a efi_disk
object, the udevice is either a UCLASS_BLK (a whole raw disk) or
UCLASS_PARTITION (a disk partition).

So this function is expected to be called every time such an udevice
is detected and activated through a device model's "probe" interface.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h      |  2 +
 lib/efi_loader/efi_disk.c | 92 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index c440962fe522..751fde7fb153 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -517,6 +517,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition,
 void efi_carve_out_dt_rsv(void *fdt);
 /* Called by bootefi to make console interface available */
 efi_status_t efi_console_register(void);
+/* Called when a block devices has been probed */
+int efi_disk_create(struct udevice *dev);
 /* Called by bootefi to make all disk storage accessible as EFI objects */
 efi_status_t efi_disk_register(void);
 /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index cd5528046251..3fae40e034fb 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -10,6 +10,7 @@
 #include <common.h>
 #include <blk.h>
 #include <dm.h>
+#include <dm/device-internal.h>
 #include <efi_loader.h>
 #include <fs.h>
 #include <log.h>
@@ -484,6 +485,7 @@ error:
 	return ret;
 }
 
+#ifndef CONFIG_BLK
 /**
  * efi_disk_create_partitions() - create handles and protocols for partitions
  *
@@ -531,6 +533,96 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
 
 	return disks;
 }
+#endif /* CONFIG_BLK */
+
+/*
+ * Create a handle for a whole raw disk
+ *
+ * @dev		uclass device
+ * @return	0 on success, -1 otherwise
+ */
+static int efi_disk_create_raw(struct udevice *dev)
+{
+	struct efi_disk_obj *disk;
+	struct blk_desc *desc;
+	const char *if_typename;
+	int diskid;
+	efi_status_t ret;
+
+	desc = dev_get_uclass_plat(dev);
+	if_typename = blk_get_if_type_name(desc->if_type);
+	diskid = desc->devnum;
+
+	ret = efi_disk_add_dev(NULL, NULL, if_typename, desc,
+			       diskid, NULL, 0, &disk);
+	if (ret != EFI_SUCCESS) {
+		log_err("Adding disk %s%d failed\n", if_typename, diskid);
+		return -1;
+	}
+	disk->dev = dev;
+	dev->efi_obj = &disk->header;
+
+	return 0;
+}
+
+/*
+ * Create a handle for a disk partition
+ *
+ * @dev		uclass device
+ * @return	0 on success, -1 otherwise
+ */
+static int efi_disk_create_part(struct udevice *dev)
+{
+	efi_handle_t parent;
+	struct blk_desc *desc;
+	const char *if_typename;
+	struct disk_part *part_data;
+	struct disk_partition *info;
+	unsigned int part;
+	int diskid;
+	struct efi_device_path *dp = NULL;
+	struct efi_disk_obj *disk;
+	efi_status_t ret;
+
+	parent = dev->parent->efi_obj;
+	desc = dev_get_uclass_plat(dev->parent);
+	if_typename = blk_get_if_type_name(desc->if_type);
+	diskid = desc->devnum;
+
+	part_data = dev_get_uclass_plat(dev);
+	part = part_data->partnum;
+	info = &part_data->gpt_part_info;
+
+	/* TODO: should not use desc? */
+	dp = efi_dp_from_part(desc, 0);
+
+	ret = efi_disk_add_dev(parent, dp, if_typename, desc, diskid,
+			       info, part, &disk);
+	if (ret != EFI_SUCCESS) {
+		log_err("Adding partition %s%d:%x failed\n",
+			if_typename, diskid, part);
+		return -1;
+	}
+	disk->dev = dev;
+	dev->efi_obj = &disk->header;
+
+	return 0;
+}
+
+int efi_disk_create(struct udevice *dev)
+{
+	enum uclass_id id;
+
+	id = device_get_uclass_id(dev);
+
+	if (id == UCLASS_BLK)
+		return efi_disk_create_raw(dev);
+
+	if (id == UCLASS_PARTITION)
+		return efi_disk_create_part(dev);
+
+	return -1;
+}
 
 /**
  * efi_disk_register() - register block devices
-- 
2.33.0

[resent RFC 15/22] dm: blk: call efi's device-probe hook

[AKASHI Takahiro]

Adding this callback function, efi_disk_create() in block devices's
post_probe hook will allows for automatically creating efi_disk objects
per block device.

This will end up not only eliminating efi_disk_register() called in UEFI
initialization, but also enabling detections of new block devices even
after the initialization.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/block/blk-uclass.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 8fbec8779e1e..ce45cf0a8768 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -9,6 +9,7 @@
 #include <common.h>
 #include <blk.h>
 #include <dm.h>
+#include <efi_loader.h>
 #include <log.h>
 #include <malloc.h>
 #include <part.h>
@@ -827,6 +828,11 @@ int blk_create_partitions(struct udevice *parent)
 
 static int blk_post_probe(struct udevice *dev)
 {
+	if (CONFIG_IS_ENABLED(EFI_LOADER)) {
+		if (efi_disk_create(dev))
+			debug("*** efi_post_probe_device failed\n");
+	}
+
 	if (IS_ENABLED(CONFIG_PARTITIONS) &&
 	    IS_ENABLED(CONFIG_HAVE_BLOCK_DEVICE)) {
 		struct blk_desc *desc = dev_get_uclass_plat(dev);
@@ -843,6 +849,10 @@ static int blk_post_probe(struct udevice *dev)
 
 static int blk_part_post_probe(struct udevice *dev)
 {
+	if (CONFIG_IS_ENABLED(EFI_LOADER)) {
+		if (efi_disk_create(dev))
+			debug("*** efi_post_probe_device failed\n");
+	}
 	/*
 	 * TODO:
 	 * If we call blk_creat_partitions() here, it would allow for
-- 
2.33.0

[resent RFC 16/22] efi_loader: cleanup after efi_disk-dm integration

[AKASHI Takahiro]

efi_disk_register() will be no longer needed now that all efi_disks are
set to be created with device model thanks to efi_disk-dm integration.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h       |   2 -
 lib/efi_loader/efi_disk.c  | 102 -------------------------------------
 lib/efi_loader/efi_setup.c |   5 --
 3 files changed, 109 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 751fde7fb153..cfbe1fe659ef 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -519,8 +519,6 @@ void efi_carve_out_dt_rsv(void *fdt);
 efi_status_t efi_console_register(void);
 /* Called when a block devices has been probed */
 int efi_disk_create(struct udevice *dev);
-/* Called by bootefi to make all disk storage accessible as EFI objects */
-efi_status_t efi_disk_register(void);
 /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */
 efi_status_t efi_rng_register(void);
 /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 3fae40e034fb..74ef923d1d67 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -485,56 +485,6 @@ error:
 	return ret;
 }
 
-#ifndef CONFIG_BLK
-/**
- * efi_disk_create_partitions() - create handles and protocols for partitions
- *
- * Create handles and protocols for the partitions of a block device.
- *
- * @parent:		handle of the parent disk
- * @desc:		block device
- * @if_typename:	interface type
- * @diskid:		device number
- * @pdevname:		device name
- * Return:		number of partitions created
- */
-int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
-			       const char *if_typename, int diskid,
-			       const char *pdevname)
-{
-	int disks = 0;
-	char devname[32] = { 0 }; /* dp->str is u16[32] long */
-	int part;
-	struct efi_device_path *dp = NULL;
-	efi_status_t ret;
-	struct efi_handler *handler;
-
-	/* Get the device path of the parent */
-	ret = efi_search_protocol(parent, &efi_guid_device_path, &handler);
-	if (ret == EFI_SUCCESS)
-		dp = handler->protocol_interface;
-
-	/* Add devices for each partition */
-	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
-		struct disk_partition info;
-
-		if (part_get_info(desc, part, &info))
-			continue;
-		snprintf(devname, sizeof(devname), "%s:%x", pdevname,
-			 part);
-		ret = efi_disk_add_dev(parent, dp, if_typename, desc, diskid,
-				       &info, part, NULL);
-		if (ret != EFI_SUCCESS) {
-			log_err("Adding partition %s failed\n", pdevname);
-			continue;
-		}
-		disks++;
-	}
-
-	return disks;
-}
-#endif /* CONFIG_BLK */
-
 /*
  * Create a handle for a whole raw disk
  *
@@ -624,58 +574,6 @@ int efi_disk_create(struct udevice *dev)
 	return -1;
 }
 
-/**
- * efi_disk_register() - register block devices
- *
- * U-Boot doesn't have a list of all online disk devices. So when running our
- * EFI payload, we scan through all of the potentially available ones and
- * store them in our object pool.
- *
- * This function is called in efi_init_obj_list().
- *
- * TODO(sjg@chromium.org): Actually with CONFIG_BLK, U-Boot does have this.
- * Consider converting the code to look up devices as needed. The EFI device
- * could be a child of the UCLASS_BLK block device, perhaps.
- *
- * Return:	status code
- */
-efi_status_t efi_disk_register(void)
-{
-	struct efi_disk_obj *disk;
-	int disks = 0;
-	efi_status_t ret;
-	struct udevice *dev;
-
-	for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
-	     uclass_next_device_check(&dev)) {
-		struct blk_desc *desc = dev_get_uclass_plat(dev);
-		const char *if_typename = blk_get_if_type_name(desc->if_type);
-
-		/* Add block device for the full device */
-		log_info("Scanning disk %s...\n", dev->name);
-		ret = efi_disk_add_dev(NULL, NULL, if_typename,
-					desc, desc->devnum, NULL, 0, &disk);
-		if (ret == EFI_NOT_READY) {
-			log_notice("Disk %s not ready\n", dev->name);
-			continue;
-		}
-		if (ret) {
-			log_err("ERROR: failure to add disk device %s, r = %lu\n",
-				dev->name, ret & ~EFI_ERROR_MASK);
-			return ret;
-		}
-		disks++;
-
-		/* Partitions show up as block devices in EFI */
-		disks += efi_disk_create_partitions(
-					&disk->header, desc, if_typename,
-					desc->devnum, dev->name);
-	}
-	log_info("Found %d disks\n", disks);
-
-	return EFI_SUCCESS;
-}
-
 /**
  * efi_disk_is_system_part() - check if handle refers to an EFI system partition
  *
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index a2338d74afac..618526eaa7c6 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -230,11 +230,6 @@ efi_status_t efi_init_obj_list(void)
 	if (ret != EFI_SUCCESS)
 		goto out;
 
-#ifdef CONFIG_PARTITIONS
-	ret = efi_disk_register();
-	if (ret != EFI_SUCCESS)
-		goto out;
-#endif
 	if (IS_ENABLED(CONFIG_EFI_RNG_PROTOCOL)) {
 		ret = efi_rng_register();
 		if (ret != EFI_SUCCESS)
-- 
2.33.0

[resent RFC 17/22] efi_loader: add efi_remove_handle()

[AKASHI Takahiro]

This function is a counterpart of efi_add_handle() and will be used
in order to remove an efi_disk object in a later patch.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h          | 2 ++
 lib/efi_loader/efi_boottime.c | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index cfbe1fe659ef..50f4119dcdfb 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -579,6 +579,8 @@ void efi_save_gd(void);
 void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
 /* Add a new object to the object list. */
 void efi_add_handle(efi_handle_t obj);
+/* Remove a object from the object list. */
+void efi_remove_handle(efi_handle_t obj);
 /* Create handle */
 efi_status_t efi_create_handle(efi_handle_t *handle);
 /* Delete handle */
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index f0283b539e46..b2503b74233b 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -503,6 +503,14 @@ void efi_add_handle(efi_handle_t handle)
 	list_add_tail(&handle->link, &efi_obj_list);
 }
 
+void efi_remove_handle(efi_handle_t handle)
+{
+	if (!handle)
+		return;
+
+	list_del(&handle->link);
+}
+
 /**
  * efi_create_handle() - create handle
  * @handle: new handle
-- 
2.33.0

[resent RFC 18/22] efi_loader: efi_disk: a helper function to delete efi_disk objects

[AKASHI Takahiro]

This function is expected to be called, in particular from dm's pre_remove
hook, when associated block devices no longer exist.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h      |  2 ++
 lib/efi_loader/efi_disk.c | 54 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 50f4119dcdfb..7079549bea70 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -519,6 +519,8 @@ void efi_carve_out_dt_rsv(void *fdt);
 efi_status_t efi_console_register(void);
 /* Called when a block devices has been probed */
 int efi_disk_create(struct udevice *dev);
+/* Called when a block devices is to be removed */
+int efi_disk_delete(struct udevice *dev);
 /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */
 efi_status_t efi_rng_register(void);
 /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 74ef923d1d67..dfd06dd31e4a 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -574,6 +574,60 @@ int efi_disk_create(struct udevice *dev)
 	return -1;
 }
 
+static int efi_disk_delete_raw(struct udevice *dev)
+{
+	efi_handle_t handle = dev->efi_obj;
+	struct blk_desc *desc;
+	struct efi_disk_obj *diskobj;
+
+	desc = dev_get_uclass_plat(dev);
+	if (desc->if_type != IF_TYPE_EFI) {
+		diskobj = container_of(handle, struct efi_disk_obj, header);
+		efi_free_pool(diskobj->dp);
+	}
+
+	/*
+	 * TODO: Can we use efi_delete_handle() here?
+	 */
+	efi_remove_all_protocols(handle);
+
+	efi_remove_handle(handle);
+	free(diskobj);
+
+	return 0;
+}
+
+static int efi_disk_delete_part(struct udevice *dev)
+{
+	efi_handle_t handle = dev->efi_obj;
+	struct efi_disk_obj *diskobj;
+
+	diskobj = container_of(handle, struct efi_disk_obj, header);
+
+	efi_free_pool(diskobj->dp);
+
+	efi_remove_all_protocols(handle);
+
+	efi_remove_handle(handle);
+	free(diskobj);
+
+	return 0;
+}
+
+int efi_disk_delete(struct udevice *dev)
+{
+	enum uclass_id id;
+
+	id = device_get_uclass_id(dev);
+
+	if (id == UCLASS_BLK)
+		return efi_disk_delete_raw(dev);
+	else if (id == UCLASS_PARTITION)
+		return efi_disk_delete_part(dev);
+	else
+		return -1;
+}
+
 /**
  * efi_disk_is_system_part() - check if handle refers to an EFI system partition
  *
-- 
2.33.0

[resent RFC 19/22] dm: blk: call efi's device-removal hook

[AKASHI Takahiro]

Adding the callback function, efi_disk_delete(), in block devices's
pre_remove hook will allows for automatically deleting efi_disk objects
per block device.

This will eliminate any improper efi_disk objects which hold a link to
non-existing udevice structures when associated block devices are physically
un-plugged or udevices are once removed (and re-created) by executing commands
like "scsi rescan."

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/block/blk-uclass.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index ce45cf0a8768..b8ad267c6c61 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -847,6 +847,16 @@ static int blk_post_probe(struct udevice *dev)
 	return 0;
 }
 
+static int blk_pre_remove(struct udevice *dev)
+{
+	if (CONFIG_IS_ENABLED(EFI_LOADER)) {
+		if (efi_disk_delete(dev))
+			debug("*** efi_pre_remove_device failed\n");
+	}
+
+	return 0;
+}
+
 static int blk_part_post_probe(struct udevice *dev)
 {
 	if (CONFIG_IS_ENABLED(EFI_LOADER)) {
@@ -861,10 +871,21 @@ static int blk_part_post_probe(struct udevice *dev)
 	return 0;
 }
 
+static int blk_part_pre_remove(struct udevice *dev)
+{
+	if (CONFIG_IS_ENABLED(EFI_LOADER)) {
+		if (efi_disk_delete(dev))
+			debug("*** efi_pre_remove_device failed\n");
+	}
+
+	return 0;
+}
+
 UCLASS_DRIVER(blk) = {
 	.id		= UCLASS_BLK,
 	.name		= "blk",
 	.post_probe	= blk_post_probe,
+	.pre_remove	= blk_pre_remove,
 	.per_device_plat_auto	= sizeof(struct blk_desc),
 };
 
@@ -937,6 +958,7 @@ U_BOOT_DRIVER(blk_partition) = {
 UCLASS_DRIVER(partition) = {
 	.id		= UCLASS_PARTITION,
 	.post_probe	= blk_part_post_probe,
+	.pre_remove	= blk_part_pre_remove,
 	.per_device_plat_auto	= sizeof(struct disk_part),
 	.name		= "partition",
 };
-- 
2.33.0

[resent RFC 20/22] efi_driver: align with efi_disk-dm integration

[AKASHI Takahiro]

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_driver/efi_block_device.c |  6 ++++++
 lib/efi_loader/efi_device_path.c  | 29 +++++++++++++++++++++++++++++
 lib/efi_loader/efi_disk.c         | 12 +++++++++++-
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
index 0937e3595a43..b6afa939e1d1 100644
--- a/lib/efi_driver/efi_block_device.c
+++ b/lib/efi_driver/efi_block_device.c
@@ -173,6 +173,12 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
 	plat->handle = handle;
 	plat->io = interface;
 
+	/*
+	 * FIXME: necessary because we won't do almost nothing in
+	 * efi_disk_create() when called from device_probe().
+	 */
+	bdev->efi_obj = handle;
+
 	ret = device_probe(bdev);
 	if (ret)
 		return ret;
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index cbdb466da41c..36c77bce9a05 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -628,6 +628,35 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev)
 			return &dp->vendor_data[1];
 			}
 #endif
+#ifdef CONFIG_EFI_LOADER
+		/*
+		 * FIXME: conflicting with CONFIG_SANDBOX
+		 * This case is necessary to support efi_disk's created by
+		 * efi_driver (and efi_driver_binding_protocol).
+		 * TODO:
+		 * The best way to work around here is to create efi_root as
+		 * udevice and put all efi_driver objects under it.
+		 */
+		case UCLASS_ROOT: {
+			struct efi_device_path_vendor *dp;
+			struct blk_desc *desc = dev_get_uclass_plat(dev);
+			/* FIXME: guid_vendor used in selftest_block_device */
+			static efi_guid_t guid_vendor =
+				EFI_GUID(0xdbca4c98, 0x6cb0, 0x694d,
+				0x08, 0x72, 0x81, 0x9c, 0x65, 0x0c, 0xb7, 0xb8);
+
+
+			dp_fill(buf, dev->parent);
+			dp = buf;
+			++dp;
+			dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
+			dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
+			dp->dp.length = sizeof(*dp) + 1;
+			memcpy(&dp->guid, &guid_vendor, sizeof(efi_guid_t));
+			dp->vendor_data[0] = desc->devnum;
+			return &dp->vendor_data[1];
+			}
+#endif
 #ifdef CONFIG_VIRTIO_BLK
 		case UCLASS_VIRTIO: {
 			struct efi_device_path_vendor *dp;
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index dfd06dd31e4a..e7cf1567929b 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -562,11 +562,21 @@ static int efi_disk_create_part(struct udevice *dev)
 int efi_disk_create(struct udevice *dev)
 {
 	enum uclass_id id;
+	struct blk_desc *desc;
 
 	id = device_get_uclass_id(dev);
 
-	if (id == UCLASS_BLK)
+	if (id == UCLASS_BLK) {
+		/*
+		 * avoid creating duplicated objects now that efi_driver
+		 * has already created an efi_disk at this moment.
+		 */
+		desc = dev_get_uclass_plat(dev);
+		if (desc->if_type == IF_TYPE_EFI)
+			return 0;
+
 		return efi_disk_create_raw(dev);
+	}
 
 	if (id == UCLASS_PARTITION)
 		return efi_disk_create_part(dev);
-- 
2.33.0

[resent RFC 21/22] efi_driver: cleanup after efi_disk-dm integration

[AKASHI Takahiro]

efi_driver-specific binding will be no longer needed now that efi_disk-
dm integration takes care of efi_driver case as well.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_driver/efi_block_device.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
index b6afa939e1d1..1f39c93f7754 100644
--- a/lib/efi_driver/efi_block_device.c
+++ b/lib/efi_driver/efi_block_device.c
@@ -106,25 +106,6 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
 	return blkcnt;
 }
 
-/**
- * Create partions for the block device.
- *
- * @handle:	EFI handle of the block device
- * @dev:	udevice of the block device
- * Return:	number of partitions created
- */
-static int efi_bl_bind_partitions(efi_handle_t handle, struct udevice *dev)
-{
-	struct blk_desc *desc;
-	const char *if_typename;
-
-	desc = dev_get_uclass_plat(dev);
-	if_typename = blk_get_if_type_name(desc->if_type);
-
-	return efi_disk_create_partitions(handle, desc, if_typename,
-					  desc->devnum, dev->name);
-}
-
 /**
  * Create a block device for a handle
  *
@@ -139,7 +120,6 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
 	char *name;
 	struct efi_object *obj = efi_search_obj(handle);
 	struct efi_block_io *io = interface;
-	int disks;
 	struct efi_blk_plat *plat;
 
 	EFI_PRINT("%s: handle %p, interface %p\n", __func__, handle, io);
@@ -184,10 +164,6 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
 		return ret;
 	EFI_PRINT("%s: block device '%s' created\n", __func__, bdev->name);
 
-	/* Create handles for the partions of the block device */
-	disks = efi_bl_bind_partitions(handle, bdev);
-	EFI_PRINT("Found %d partitions\n", disks);
-
 	return 0;
 }
 
-- 
2.33.0

[resent RFC 22/22] efi_selftest: block device: adjust dp for a test disk

[AKASHI Takahiro]

Due to efi_disk-dm integration, the resultant device path for a test disk
got slightly changed, with efi_root contained as the first component.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_selftest/efi_selftest_block_device.c | 26 ++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c
index 15f03751ac87..cac76249e6b4 100644
--- a/lib/efi_selftest/efi_selftest_block_device.c
+++ b/lib/efi_selftest/efi_selftest_block_device.c
@@ -30,6 +30,9 @@ static const efi_guid_t guid_device_path = EFI_DEVICE_PATH_PROTOCOL_GUID;
 static const efi_guid_t guid_simple_file_system_protocol =
 					EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID;
 static const efi_guid_t guid_file_system_info = EFI_FILE_SYSTEM_INFO_GUID;
+static efi_guid_t guid_uboot =
+	EFI_GUID(0xe61d73b9, 0xa384, 0x4acc, \
+		 0xae, 0xab, 0x82, 0xe8, 0x28, 0xf3, 0x62, 0x8b);
 static efi_guid_t guid_vendor =
 	EFI_GUID(0xdbca4c98, 0x6cb0, 0x694d,
 		 0x08, 0x72, 0x81, 0x9c, 0x65, 0x0c, 0xb7, 0xb8);
@@ -206,25 +209,44 @@ static int setup(const efi_handle_t handle,
 
 	ret = boottime->allocate_pool(EFI_LOADER_DATA,
 				      sizeof(struct efi_device_path_vendor) +
+				      sizeof(struct efi_device_path_vendor) +
+				      sizeof(u8) +
 				      sizeof(struct efi_device_path),
 				      (void **)&dp);
 	if (ret != EFI_SUCCESS) {
 		efi_st_error("Out of memory\n");
 		return EFI_ST_FAILURE;
 	}
+	/* first part */
 	vendor_node.dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
 	vendor_node.dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
 	vendor_node.dp.length = sizeof(struct efi_device_path_vendor);
 
-	boottime->copy_mem(&vendor_node.guid, &guid_vendor,
+	boottime->copy_mem(&vendor_node.guid, &guid_uboot,
 			   sizeof(efi_guid_t));
 	boottime->copy_mem(dp, &vendor_node,
 			   sizeof(struct efi_device_path_vendor));
+
+	/* second part */
+	vendor_node.dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
+	vendor_node.dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
+	vendor_node.dp.length = sizeof(struct efi_device_path_vendor) + 1;
+
+	boottime->copy_mem(&vendor_node.guid, &guid_vendor,
+			   sizeof(efi_guid_t));
+	boottime->copy_mem((char *)dp + sizeof(struct efi_device_path_vendor),
+			   &vendor_node,
+			   sizeof(struct efi_device_path_vendor));
+	/* vendor_data[0] */
+	*((char *)dp + sizeof(struct efi_device_path_vendor) * 2) = 0;
+
 	end_node.type = DEVICE_PATH_TYPE_END;
 	end_node.sub_type = DEVICE_PATH_SUB_TYPE_END;
 	end_node.length = sizeof(struct efi_device_path);
 
-	boottime->copy_mem((char *)dp + sizeof(struct efi_device_path_vendor),
+	boottime->copy_mem((char *)dp + sizeof(struct efi_device_path_vendor)
+			   + sizeof(struct efi_device_path_vendor)
+			   + sizeof(u8),
 			   &end_node, sizeof(struct efi_device_path));
 	ret = boottime->install_protocol_interface(&disk_handle,
 						   &guid_device_path,
-- 
2.33.0

Re: [resent RFC 00/22] efi_loader: more tightly integrate UEFI disks to device model

[Heinrich Schuchardt]

On 10/4/21 05:44, AKASHI Takahiro wrote:
> # Resending the RFC as some of patches were deplicately submitted.
> # See also
>    https://git.linaro.org/people/takahiro.akashi/u-boot.git efi/dm_disk
>
> The purpose of this RPC is to reignite the discussion about how UEFI
> subystem would best be integrated into U-Boot device model.
> In the past, I poposed a couple of patch series, the latest one[1],
> while Heinrich revealed his idea[2], and the approach taken here is
> something between them, with a focus on block device handlings.
>
> # The code is a PoC and not well tested yet.
>
> Disks in UEFI world:
> ====================
> In general in UEFI world, accessing to any device is performed through
> a 'protocol' interface which are installed to (or associated with) the device's
> UEFI handle (or an opaque pointer to UEFI object data). Protocols are
> implemented by either the UEFI system itself or UEFI drivers.
>
> For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk
> hereafter). Currently, every efi_disk may have one of two origins:
> a.U-Boot's block devices or related partitions
>    (lib/efi_loader/efi_disk.c)
> b.UEFI objects which are implemented as a block device by UEFI drivers.
>    (lib/efi_driver/efi_block_device.c)
>
> All the efi_diskss as (a) will be enumelated and created only once at UEFI
> subsystem initialization (efi_disk_register()), which is triggered by
> first executing one of UEFI-related U-Boot commands, like "bootefi",
> "setenv -e" or "efidebug".
> EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops)
> in the corresponding udevice(UCLASS_BLK).
>
> On the other hand, efi_disk as (b) will be created each time UEFI boot
> services' connect_controller() is executed in UEFI app which, as a (device)
> controller, gives the method to access the device's data,
> ie. EFI_BLOCK_IO_PROTOCOL.
>
>>>> more details >>>
> Internally, connect_controller() search for UEFI driver that can support
> this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case,
> then calls the driver's 'bind' interface, which eventually installs
> the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object.
> 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for
>    * creating additional partitions efi_disk's, and
>    * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it.
> <<< <<<
>
> Issues:
> =======
> 1. While an efi_disk represents a device equally for either a whole disk
>     or a partition in UEFI world, the device model treats only a whole
>     disk as a real block device or udevice(UCLASS_BLK).
>
> 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc
>     in plat_data is supposed to be private and not to be accessed outside
>     the device model.
>     # This issue, though, exists for all the implmenetation of U-Boot
>     # file systems as well.
>
> For efi_disk(a),
> 3. A block device can be enumelated dynamically by 'scanning' a device bus
>     in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly.
>     For examples,
>      => scsi rescan; efidebug devices
>      => usb start; efidebug devices ... (A)
>     (A) doesn't show any usb devices detected.
>
>      => scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ...
>      => scsi rescan ... (B)
>      => bootefi bootmgr ... (C)
>     (C) may de-reference a bogus blk_desc pointer which has been freed by (B).
>     (Please note that "scsi rescan" removes all udevices/blk_desc and then
>      re-create them even if nothing is changed on a bus.)
>
> For efi_disk(b),
> 4. A controller (handle), combined with efi_block driver, has no
>     corresponding udevice as a parent of efi_disks in DM tree, unlike, say,
>     a scsi controller, even though it provides methods for block io perations.
> 5. There is no way supported to remove efi_disk's even after
>     disconnect_controller() is called.
>
>
> My approach in this RFC:
> ========================
> Due to functional differences in semantics, it would be difficult
> to identify "udevice" structure as a handle in UEFI world. Instead, we will
> have to somehow maintain a relationship between a udevice and a handle.
>
> 1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions
>     Currently, the uclass for paritions is not a UCLASS_BLK.
>     It can be possible to define partitions as UCLASS_BLK
>     (with IF_TYPE_PARTION?), but
>     I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK)
>     is tightly coupled with 'struct blk_desc' data which is still used
>     as a "structure to a whole disk" in a lot of interfaces.
>     (I hope that you understand what it means.)
>
>     In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent:
>     For instance,
>         UCLASS_SCSI  --- UCLASS_BLK       --- UCLASS_PARTITION
> 			 (IF_TYPE_SCSI)        |
>                            +- struct blk_desc   +- struct disk_part
> 			  +- scsi_blk_ops      +- blk_part_ops
>
> 1-2. create partition udevices in the context of device_probe()
>     part_init() is already called in blk_post_probe(). See the commit
>     d0851c893706 ("blk: Call part_init() in the post_probe() method").
>     Why not enumelate partitions as well in there.
>
> 2. add new block access interfaces, which takes "udevice" as a target device,
>     in U-Boot and use those functions to implement efi_disk operations
>     (i.e. EFI_BLOCK_IO_PROTOCOL).
>
> 3-1. maintain a bi-directional link by adding
>     - a UEFI handle pointer in "struct udevice"
>     - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")

An EFI application can create handles with any combination of protocols,
e.g. a handle with both the block IO protocol and the simple network
protocol. This means that a udevice cannot be assigned to a handle
created by an EFI application.

When the EFI application calls ConnectController() for the handle,
U-Boot can create child controllers. If U-Boot creates a udevice for
such a child controller, it has to store the udevice pointer.
lib/efi_driver/efi_block_device.c uses a private data section but you it
could be preferable to use a field in struct efi_obj.

>
> 3-2. use device model's post_probe/pre_remove hook to synchronize the lifetime
>     of efi_disk objects in UEFI world with the device model.
>
> 4. I have no answer to issue(4) and (5) yet.

4) A udevice shall only exist for the child controller handle created by
U-Boot and not for the controller handle created by an EFI application.

5) The stop() method of the driver binding protocol has to take care of
destroying the child controllers and the associated udevices.

Best regards

Heinrich

Re: [resent RFC 00/22] efi_loader: more tightly integrate UEFI disks to device model

[Ilias Apalodimas]

On Mon, Oct 04, 2021 at 04:47:53PM +0200, Heinrich Schuchardt wrote:
> 
> 
> > 

[...]

> > My approach in this RFC:
> > ========================
> > Due to functional differences in semantics, it would be difficult
> > to identify "udevice" structure as a handle in UEFI world. Instead, we will
> > have to somehow maintain a relationship between a udevice and a handle.
> > 
> > 1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions
> >     Currently, the uclass for paritions is not a UCLASS_BLK.
> >     It can be possible to define partitions as UCLASS_BLK
> >     (with IF_TYPE_PARTION?), but
> >     I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK)
> >     is tightly coupled with 'struct blk_desc' data which is still used
> >     as a "structure to a whole disk" in a lot of interfaces.
> >     (I hope that you understand what it means.)

I think it makes more sense the way it's currently defined.  I don;t see a
point in hiding partitions within UCLASS_BLK

> > 
> >     In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent:
> >     For instance,
> >         UCLASS_SCSI  --- UCLASS_BLK       --- UCLASS_PARTITION
> > 			 (IF_TYPE_SCSI)        |
> >                            +- struct blk_desc   +- struct disk_part
> > 			  +- scsi_blk_ops      +- blk_part_ops
> > 
> > 1-2. create partition udevices in the context of device_probe()
> >     part_init() is already called in blk_post_probe(). See the commit
> >     d0851c893706 ("blk: Call part_init() in the post_probe() method").
> >     Why not enumelate partitions as well in there.
> > 
> > 2. add new block access interfaces, which takes "udevice" as a target device,
> >     in U-Boot and use those functions to implement efi_disk operations
> >     (i.e. EFI_BLOCK_IO_PROTOCOL).
> > 
> > 3-1. maintain a bi-directional link by adding
> >     - a UEFI handle pointer in "struct udevice"
> >     - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
> 
> An EFI application can create handles with any combination of protocols,
> e.g. a handle with both the block IO protocol and the simple network
> protocol. This means that a udevice cannot be assigned to a handle
> created by an EFI application.
> 
> When the EFI application calls ConnectController() for the handle,
> U-Boot can create child controllers. If U-Boot creates a udevice for
> such a child controller, it has to store the udevice pointer.
> lib/efi_driver/efi_block_device.c uses a private data section but you it
> could be preferable to use a field in struct efi_obj.
> 

I agree with Heinrich here.  Basically U-Boot has to be in charge of that.
Once ConnectController has been called U-Boot should create an 1:1 mapping
between udevice <-> handle and shouldn't be allowed to change that.

> > 
> > 3-2. use device model's post_probe/pre_remove hook to synchronize the lifetime
> >     of efi_disk objects in UEFI world with the device model.
> > 
> > 4. I have no answer to issue(4) and (5) yet.
> 
> 4) A udevice shall only exist for the child controller handle created by
> U-Boot and not for the controller handle created by an EFI application.
> 
> 5) The stop() method of the driver binding protocol has to take care of
> destroying the child controllers and the associated udevices.
> 
> Best regards
> 
> Heinrich

Thanks
/Ilias

Re: [resent RFC 08/22] dm: blk: add UCLASS_PARTITION

[Ilias Apalodimas]

Hi Akashi-san,

>  

[...]

> +int blk_create_partitions(struct udevice *parent)
> +{
> +	int part, count;
> +	struct blk_desc *desc = dev_get_uclass_plat(parent);
> +	struct disk_partition info;
> +	struct disk_part *part_data;
> +	char devname[32];
> +	struct udevice *dev;
> +	int ret;
> +
> +	if (!CONFIG_IS_ENABLED(PARTITIONS) ||
> +	    !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
> +		return 0;

Would it make more sense to return an error here?

> +
> +	/* Add devices for each partition */
> +	for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> +		if (part_get_info(desc, part, &info))
> +			continue;
> +		snprintf(devname, sizeof(devname), "%s:%d", parent->name,
> +			 part);
> +
> +		ret = device_bind_driver(parent, "blk_partition",
> +					 strdup(devname), &dev);
> +		if (ret)
> +			return ret;
> +
> +		part_data = dev_get_uclass_plat(dev);
> +		part_data->partnum = part;
> +		part_data->gpt_part_info = info;
> +		count++;
> +
> +		device_probe(dev);

Probe can fail. 

> +	}
> +	debug("%s: %d partitions found in %s\n", __func__, count, parent->name);
> +
> +	return 0;
> +}
> +
>  static int blk_post_probe(struct udevice *dev)
>  {
>  	if (IS_ENABLED(CONFIG_PARTITIONS) &&
> @@ -713,3 +752,75 @@ UCLASS_DRIVER(blk) = {
>  	.post_probe	= blk_post_probe,
>  	.per_device_plat_auto	= sizeof(struct blk_desc),
>  };
[...]

Regards
/Ilias

Re: [resent RFC 06/22] sata: call device_probe() after scanning

[Ilias Apalodimas]

On Mon, Oct 04, 2021 at 12:44:14PM +0900, AKASHI Takahiro wrote:
> Every time a sata bus/port is scanned and a new device is detected,
> we want to call device_probe() as it will give us a chance to run additional
> post-processings for some purposes.
> 
> In particular, support for creating partitions on a device will be added.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  drivers/ata/dwc_ahsata.c | 10 ++++++++++
>  drivers/ata/fsl_sata.c   | 11 +++++++++++
>  drivers/ata/sata_mv.c    |  9 +++++++++
>  drivers/ata/sata_sil.c   | 12 ++++++++++++
>  4 files changed, 42 insertions(+)
> 
> diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c
> index 6d42548087b3..6a51c70d1170 100644
> --- a/drivers/ata/dwc_ahsata.c
> +++ b/drivers/ata/dwc_ahsata.c
> @@ -1026,6 +1026,16 @@ int dwc_ahsata_scan(struct udevice *dev)
>  		return ret;
>  	}
>  
> +	ret = device_probe(bdev);
> +	if (ret < 0) {
> +		debug("Can't probe\n");
> +		/* TODO: undo create */
> +
> +		device_unbind(bdev);
> +
> +		return ret;
> +	}
> +

Patches 2-6 seem to do the same thing for different subsystems.  I think
creating a function for that would make it easier. 

>  	return 0;
>  }
>  
> diff --git a/drivers/ata/fsl_sata.c b/drivers/ata/fsl_sata.c
> index e44db0a37458..346e9298b4c5 100644
> --- a/drivers/ata/fsl_sata.c
> +++ b/drivers/ata/fsl_sata.c
> @@ -982,6 +982,17 @@ static int fsl_ata_probe(struct udevice *dev)
>  			failed_number++;
>  			continue;
>  		}
> +
> +		ret = device_probe(bdev);
> +		if (ret < 0) {
> +			debug("Can't probe\n");
> +			ret = fsl_unbind_device(blk);

Apart from this exception I guess

> +			if (ret)
> +				return ret;
> +
> +			failed_number++;
> +			continue;
> +		}
>  	}
>  
>  	if (failed_number == nr_ports)
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 003222d47be6..09b735779ebf 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -1099,6 +1099,15 @@ static int sata_mv_probe(struct udevice *dev)
>  			continue;
>  		}
>  
> +		ret = device_probe(bdev);
> +		if (ret < 0) {
> +			debug("Can't probe\n");
> +			/* TODO: undo create */
> +
> +			device_unbind(bdev);
> +			continue;
> +		}
> +
>  		/* If we got here, the current SATA port was probed
>  		 * successfully, so set the probe status to successful.
>  		 */
> diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
> index dda712f42cb2..295f7ca72303 100644
> --- a/drivers/ata/sata_sil.c
> +++ b/drivers/ata/sata_sil.c
> @@ -864,6 +864,18 @@ static int sil_pci_probe(struct udevice *dev)
>  			failed_number++;
>  			continue;
>  		}
> +
> +		ret = device_probe(bdev);
> +		if (ret < 0) {
> +			debug("Can't probe\n");
> +			ret = sil_unbind_device(blk);
> +			device_unbind(bdev);
> +			if (ret)
> +				return ret;
> +
> +			failed_number++;
> +			continue;
> +		}
>  	}
>  
>  	if (failed_number == sata_info.maxport)
> -- 
> 2.33.0
> 

Re: [resent RFC 14/22] efi_loader: disk: a helper function to create efi_disk objects from udevice

[Ilias Apalodimas]

On Mon, Oct 04, 2021 at 12:44:22PM +0900, AKASHI Takahiro wrote:
> Add efi_disk_create() function.
> 
> Any UEFI handle created by efi_disk_create() can be treated as a efi_disk
> object, the udevice is either a UCLASS_BLK (a whole raw disk) or
> UCLASS_PARTITION (a disk partition).
> 
> So this function is expected to be called every time such an udevice
> is detected and activated through a device model's "probe" interface.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/efi_loader.h      |  2 +
>  lib/efi_loader/efi_disk.c | 92 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index c440962fe522..751fde7fb153 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -517,6 +517,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition,
>  void efi_carve_out_dt_rsv(void *fdt);
>  /* Called by bootefi to make console interface available */
>  efi_status_t efi_console_register(void);
> +/* Called when a block devices has been probed */
> +int efi_disk_create(struct udevice *dev);
>  /* Called by bootefi to make all disk storage accessible as EFI objects */
>  efi_status_t efi_disk_register(void);
>  /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index cd5528046251..3fae40e034fb 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -10,6 +10,7 @@
>  #include <common.h>
>  #include <blk.h>
>  #include <dm.h>
> +#include <dm/device-internal.h>
>  #include <efi_loader.h>
>  #include <fs.h>
>  #include <log.h>
> @@ -484,6 +485,7 @@ error:
>  	return ret;
>  }
>  
> +#ifndef CONFIG_BLK
>  /**
>   * efi_disk_create_partitions() - create handles and protocols for partitions
>   *
> @@ -531,6 +533,96 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
>  
>  	return disks;
>  }
> +#endif /* CONFIG_BLK */
> +
> +/*
> + * Create a handle for a whole raw disk
> + *
> + * @dev		uclass device
> + * @return	0 on success, -1 otherwise
> + */
> +static int efi_disk_create_raw(struct udevice *dev)
> +{
> +	struct efi_disk_obj *disk;
> +	struct blk_desc *desc;
> +	const char *if_typename;
> +	int diskid;
> +	efi_status_t ret;
> +
> +	desc = dev_get_uclass_plat(dev);
> +	if_typename = blk_get_if_type_name(desc->if_type);
> +	diskid = desc->devnum;
> +
> +	ret = efi_disk_add_dev(NULL, NULL, if_typename, desc,
> +			       diskid, NULL, 0, &disk);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("Adding disk %s%d failed\n", if_typename, diskid);
> +		return -1;
> +	}
> +	disk->dev = dev;
> +	dev->efi_obj = &disk->header;
> +
> +	return 0;
> +}
> +
> +/*
> + * Create a handle for a disk partition
> + *
> + * @dev		uclass device
> + * @return	0 on success, -1 otherwise
> + */
> +static int efi_disk_create_part(struct udevice *dev)
> +{
> +	efi_handle_t parent;
> +	struct blk_desc *desc;
> +	const char *if_typename;
> +	struct disk_part *part_data;
> +	struct disk_partition *info;
> +	unsigned int part;
> +	int diskid;
> +	struct efi_device_path *dp = NULL;
> +	struct efi_disk_obj *disk;
> +	efi_status_t ret;
> +
> +	parent = dev->parent->efi_obj;
> +	desc = dev_get_uclass_plat(dev->parent);
> +	if_typename = blk_get_if_type_name(desc->if_type);
> +	diskid = desc->devnum;
> +
> +	part_data = dev_get_uclass_plat(dev);
> +	part = part_data->partnum;
> +	info = &part_data->gpt_part_info;
> +
> +	/* TODO: should not use desc? */
> +	dp = efi_dp_from_part(desc, 0);
> +
> +	ret = efi_disk_add_dev(parent, dp, if_typename, desc, diskid,
> +			       info, part, &disk);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("Adding partition %s%d:%x failed\n",
> +			if_typename, diskid, part);
> +		return -1;
> +	}
> +	disk->dev = dev;
> +	dev->efi_obj = &disk->header;
> +
> +	return 0;
> +}

Can't we add a 'bool part' on this one and have a single function?

> +
> +int efi_disk_create(struct udevice *dev)
> +{
> +	enum uclass_id id;
> +
> +	id = device_get_uclass_id(dev);
> +
> +	if (id == UCLASS_BLK)
> +		return efi_disk_create_raw(dev);
> +
> +	if (id == UCLASS_PARTITION)
> +		return efi_disk_create_part(dev);
> +
> +	return -1;
> +}
>  
>  /**
>   * efi_disk_register() - register block devices
> -- 
> 2.33.0
> 

Regards
/Ilias

Re: [resent RFC 00/22] efi_loader: more tightly integrate UEFI disks to device model

[Simon Glass]

Hi Takahiro,

On Sun, 3 Oct 2021 at 21:44, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> # Resending the RFC as some of patches were deplicately submitted.
> # See also
>   https://git.linaro.org/people/takahiro.akashi/u-boot.git efi/dm_disk
>
> The purpose of this RPC is to reignite the discussion about how UEFI
> subystem would best be integrated into U-Boot device model.
> In the past, I poposed a couple of patch series, the latest one[1],
> while Heinrich revealed his idea[2], and the approach taken here is
> something between them, with a focus on block device handlings.
>
> # The code is a PoC and not well tested yet.

[..]

I expect to be able to dig into this at the end of the week.

Regards,
Simon


Regards,
Simon

Re: [resent RFC 06/22] sata: call device_probe() after scanning

[AKASHI Takahiro]

On Mon, Oct 04, 2021 at 09:45:35PM +0300, Ilias Apalodimas wrote:
> On Mon, Oct 04, 2021 at 12:44:14PM +0900, AKASHI Takahiro wrote:
> > Every time a sata bus/port is scanned and a new device is detected,
> > we want to call device_probe() as it will give us a chance to run additional
> > post-processings for some purposes.
> > 
> > In particular, support for creating partitions on a device will be added.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  drivers/ata/dwc_ahsata.c | 10 ++++++++++
> >  drivers/ata/fsl_sata.c   | 11 +++++++++++
> >  drivers/ata/sata_mv.c    |  9 +++++++++
> >  drivers/ata/sata_sil.c   | 12 ++++++++++++
> >  4 files changed, 42 insertions(+)
> > 
> > diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c
> > index 6d42548087b3..6a51c70d1170 100644
> > --- a/drivers/ata/dwc_ahsata.c
> > +++ b/drivers/ata/dwc_ahsata.c
> > @@ -1026,6 +1026,16 @@ int dwc_ahsata_scan(struct udevice *dev)
> >  		return ret;
> >  	}
> >  
> > +	ret = device_probe(bdev);
> > +	if (ret < 0) {
> > +		debug("Can't probe\n");
> > +		/* TODO: undo create */
> > +
> > +		device_unbind(bdev);
> > +
> > +		return ret;
> > +	}
> > +
> 
> Patches 2-6 seem to do the same thing for different subsystems.  I think
> creating a function for that would make it easier. 

Well, the reason why I put those changes in separate commits is
- first, different subsystems are owned by different maintainers, and
- more importantly, different subsystems may have different cleanup
  processing required.
  There are always extra setups after blk_create_device(), which should
  be reverted if device_probe() fails. For instance, sil_unbind_device()
  and fsl_unbind_device().
  So I would like to leave subsystem owners responsible for that. 

-Takahiro Akashi


> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/ata/fsl_sata.c b/drivers/ata/fsl_sata.c
> > index e44db0a37458..346e9298b4c5 100644
> > --- a/drivers/ata/fsl_sata.c
> > +++ b/drivers/ata/fsl_sata.c
> > @@ -982,6 +982,17 @@ static int fsl_ata_probe(struct udevice *dev)
> >  			failed_number++;
> >  			continue;
> >  		}
> > +
> > +		ret = device_probe(bdev);
> > +		if (ret < 0) {
> > +			debug("Can't probe\n");
> > +			ret = fsl_unbind_device(blk);
> 
> Apart from this exception I guess
> 
> > +			if (ret)
> > +				return ret;
> > +
> > +			failed_number++;
> > +			continue;
> > +		}
> >  	}
> >  
> >  	if (failed_number == nr_ports)
> > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> > index 003222d47be6..09b735779ebf 100644
> > --- a/drivers/ata/sata_mv.c
> > +++ b/drivers/ata/sata_mv.c
> > @@ -1099,6 +1099,15 @@ static int sata_mv_probe(struct udevice *dev)
> >  			continue;
> >  		}
> >  
> > +		ret = device_probe(bdev);
> > +		if (ret < 0) {
> > +			debug("Can't probe\n");
> > +			/* TODO: undo create */
> > +
> > +			device_unbind(bdev);
> > +			continue;
> > +		}
> > +
> >  		/* If we got here, the current SATA port was probed
> >  		 * successfully, so set the probe status to successful.
> >  		 */
> > diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
> > index dda712f42cb2..295f7ca72303 100644
> > --- a/drivers/ata/sata_sil.c
> > +++ b/drivers/ata/sata_sil.c
> > @@ -864,6 +864,18 @@ static int sil_pci_probe(struct udevice *dev)
> >  			failed_number++;
> >  			continue;
> >  		}
> > +
> > +		ret = device_probe(bdev);
> > +		if (ret < 0) {
> > +			debug("Can't probe\n");
> > +			ret = sil_unbind_device(blk);
> > +			device_unbind(bdev);
> > +			if (ret)
> > +				return ret;
> > +
> > +			failed_number++;
> > +			continue;
> > +		}
> >  	}
> >  
> >  	if (failed_number == sata_info.maxport)
> > -- 
> > 2.33.0
> > 

Re: [resent RFC 08/22] dm: blk: add UCLASS_PARTITION

[AKASHI Takahiro]

On Mon, Oct 04, 2021 at 09:40:21PM +0300, Ilias Apalodimas wrote:
> Hi Akashi-san,
> 
> >  
> 
> [...]
> 
> > +int blk_create_partitions(struct udevice *parent)
> > +{
> > +	int part, count;
> > +	struct blk_desc *desc = dev_get_uclass_plat(parent);
> > +	struct disk_partition info;
> > +	struct disk_part *part_data;
> > +	char devname[32];
> > +	struct udevice *dev;
> > +	int ret;
> > +
> > +	if (!CONFIG_IS_ENABLED(PARTITIONS) ||
> > +	    !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
> > +		return 0;
> 
> Would it make more sense to return an error here?

!CONFIG_IS_ENABLED(PARTITIONS) means that a user doesn't want to
use partitions on their system. So simply returning 0 would be fine, I think.
This is kinda equivalence at the caller site:

	if (CONFIG_IS_ENABLED(PARTITIONS) &&
	    CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
                ret = blk_create_partitions(dev);
        else
                debug("We don't care about partitions.");

> > +
> > +	/* Add devices for each partition */
> > +	for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> > +		if (part_get_info(desc, part, &info))
> > +			continue;
> > +		snprintf(devname, sizeof(devname), "%s:%d", parent->name,
> > +			 part);
> > +
> > +		ret = device_bind_driver(parent, "blk_partition",
> > +					 strdup(devname), &dev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		part_data = dev_get_uclass_plat(dev);
> > +		part_data->partnum = part;
> > +		part_data->gpt_part_info = info;
> > +		count++;
> > +
> > +		device_probe(dev);
> 
> Probe can fail. 

Theoretically, yes. But as a matter of fact, device_probe() does almost nothing
for UCLASS_PARTITION devices under the proposed implementation here and
I don't expect it will ever fail.
Please note that, as I commented in blk_part_post_probe(), we may want to
call blk_create_partitions() in the future so that we will support
"nested" partitioning in a partition :)

-Takahiro Akashi


> > +	}
> > +	debug("%s: %d partitions found in %s\n", __func__, count, parent->name);
> > +
> > +	return 0;
> > +}
> > +
> >  static int blk_post_probe(struct udevice *dev)
> >  {
> >  	if (IS_ENABLED(CONFIG_PARTITIONS) &&
> > @@ -713,3 +752,75 @@ UCLASS_DRIVER(blk) = {
> >  	.post_probe	= blk_post_probe,
> >  	.per_device_plat_auto	= sizeof(struct blk_desc),
> >  };
> [...]
> 
> Regards
> /Ilias

Re: [resent RFC 14/22] efi_loader: disk: a helper function to create efi_disk objects from udevice

[AKASHI Takahiro]

On Mon, Oct 04, 2021 at 09:50:02PM +0300, Ilias Apalodimas wrote:
> On Mon, Oct 04, 2021 at 12:44:22PM +0900, AKASHI Takahiro wrote:
> > Add efi_disk_create() function.
> > 
> > Any UEFI handle created by efi_disk_create() can be treated as a efi_disk
> > object, the udevice is either a UCLASS_BLK (a whole raw disk) or
> > UCLASS_PARTITION (a disk partition).
> > 
> > So this function is expected to be called every time such an udevice
> > is detected and activated through a device model's "probe" interface.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/efi_loader.h      |  2 +
> >  lib/efi_loader/efi_disk.c | 92 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 94 insertions(+)
> > 
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index c440962fe522..751fde7fb153 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -517,6 +517,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition,
> >  void efi_carve_out_dt_rsv(void *fdt);
> >  /* Called by bootefi to make console interface available */
> >  efi_status_t efi_console_register(void);
> > +/* Called when a block devices has been probed */
> > +int efi_disk_create(struct udevice *dev);
> >  /* Called by bootefi to make all disk storage accessible as EFI objects */
> >  efi_status_t efi_disk_register(void);
> >  /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index cd5528046251..3fae40e034fb 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -10,6 +10,7 @@
> >  #include <common.h>
> >  #include <blk.h>
> >  #include <dm.h>
> > +#include <dm/device-internal.h>
> >  #include <efi_loader.h>
> >  #include <fs.h>
> >  #include <log.h>
> > @@ -484,6 +485,7 @@ error:
> >  	return ret;
> >  }
> >  
> > +#ifndef CONFIG_BLK
> >  /**
> >   * efi_disk_create_partitions() - create handles and protocols for partitions
> >   *
> > @@ -531,6 +533,96 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> >  
> >  	return disks;
> >  }
> > +#endif /* CONFIG_BLK */
> > +
> > +/*
> > + * Create a handle for a whole raw disk
> > + *
> > + * @dev		uclass device
> > + * @return	0 on success, -1 otherwise
> > + */
> > +static int efi_disk_create_raw(struct udevice *dev)
> > +{
> > +	struct efi_disk_obj *disk;
> > +	struct blk_desc *desc;
> > +	const char *if_typename;
> > +	int diskid;
> > +	efi_status_t ret;
> > +
> > +	desc = dev_get_uclass_plat(dev);
> > +	if_typename = blk_get_if_type_name(desc->if_type);
> > +	diskid = desc->devnum;
> > +
> > +	ret = efi_disk_add_dev(NULL, NULL, if_typename, desc,
> > +			       diskid, NULL, 0, &disk);
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("Adding disk %s%d failed\n", if_typename, diskid);
> > +		return -1;
> > +	}
> > +	disk->dev = dev;
> > +	dev->efi_obj = &disk->header;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Create a handle for a disk partition
> > + *
> > + * @dev		uclass device
> > + * @return	0 on success, -1 otherwise
> > + */
> > +static int efi_disk_create_part(struct udevice *dev)
> > +{
> > +	efi_handle_t parent;
> > +	struct blk_desc *desc;
> > +	const char *if_typename;
> > +	struct disk_part *part_data;
> > +	struct disk_partition *info;
> > +	unsigned int part;
> > +	int diskid;
> > +	struct efi_device_path *dp = NULL;
> > +	struct efi_disk_obj *disk;
> > +	efi_status_t ret;
> > +
> > +	parent = dev->parent->efi_obj;
> > +	desc = dev_get_uclass_plat(dev->parent);
> > +	if_typename = blk_get_if_type_name(desc->if_type);
> > +	diskid = desc->devnum;
> > +
> > +	part_data = dev_get_uclass_plat(dev);
> > +	part = part_data->partnum;
> > +	info = &part_data->gpt_part_info;
> > +
> > +	/* TODO: should not use desc? */
> > +	dp = efi_dp_from_part(desc, 0);
> > +
> > +	ret = efi_disk_add_dev(parent, dp, if_typename, desc, diskid,
> > +			       info, part, &disk);
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("Adding partition %s%d:%x failed\n",
> > +			if_typename, diskid, part);
> > +		return -1;
> > +	}
> > +	disk->dev = dev;
> > +	dev->efi_obj = &disk->header;
> > +
> > +	return 0;
> > +}
> 
> Can't we add a 'bool part' on this one and have a single function?

Yes, but I would like to first discuss how partitions should be treated
in the driver model. Then I will think of unifying related functions.

-Takahiro Akashi

> > +
> > +int efi_disk_create(struct udevice *dev)
> > +{
> > +	enum uclass_id id;
> > +
> > +	id = device_get_uclass_id(dev);
> > +
> > +	if (id == UCLASS_BLK)
> > +		return efi_disk_create_raw(dev);
> > +
> > +	if (id == UCLASS_PARTITION)
> > +		return efi_disk_create_part(dev);
> > +
> > +	return -1;
> > +}
> >  
> >  /**
> >   * efi_disk_register() - register block devices
> > -- 
> > 2.33.0
> > 
> 
> Regards
> /Ilias

Re: [resent RFC 00/22] efi_loader: more tightly integrate UEFI disks to device model

[AKASHI Takahiro]

On Mon, Oct 04, 2021 at 04:47:53PM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 10/4/21 05:44, AKASHI Takahiro wrote:
> > # Resending the RFC as some of patches were deplicately submitted.
> > # See also
> >    https://git.linaro.org/people/takahiro.akashi/u-boot.git efi/dm_disk
> > 
> > The purpose of this RPC is to reignite the discussion about how UEFI
> > subystem would best be integrated into U-Boot device model.
> > In the past, I poposed a couple of patch series, the latest one[1],
> > while Heinrich revealed his idea[2], and the approach taken here is
> > something between them, with a focus on block device handlings.
> > 
> > # The code is a PoC and not well tested yet.
> > 
> > Disks in UEFI world:
> > ====================
> > In general in UEFI world, accessing to any device is performed through
> > a 'protocol' interface which are installed to (or associated with) the device's
> > UEFI handle (or an opaque pointer to UEFI object data). Protocols are
> > implemented by either the UEFI system itself or UEFI drivers.
> > 
> > For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk
> > hereafter). Currently, every efi_disk may have one of two origins:
> > a.U-Boot's block devices or related partitions
> >    (lib/efi_loader/efi_disk.c)
> > b.UEFI objects which are implemented as a block device by UEFI drivers.
> >    (lib/efi_driver/efi_block_device.c)
> > 
> > All the efi_diskss as (a) will be enumelated and created only once at UEFI
> > subsystem initialization (efi_disk_register()), which is triggered by
> > first executing one of UEFI-related U-Boot commands, like "bootefi",
> > "setenv -e" or "efidebug".
> > EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops)
> > in the corresponding udevice(UCLASS_BLK).
> > 
> > On the other hand, efi_disk as (b) will be created each time UEFI boot
> > services' connect_controller() is executed in UEFI app which, as a (device)
> > controller, gives the method to access the device's data,
> > ie. EFI_BLOCK_IO_PROTOCOL.
> > 
> > > > > more details >>>
> > Internally, connect_controller() search for UEFI driver that can support
> > this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case,
> > then calls the driver's 'bind' interface, which eventually installs
> > the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object.
> > 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for
> >    * creating additional partitions efi_disk's, and
> >    * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it.
> > <<< <<<
> > 
> > Issues:
> > =======
> > 1. While an efi_disk represents a device equally for either a whole disk
> >     or a partition in UEFI world, the device model treats only a whole
> >     disk as a real block device or udevice(UCLASS_BLK).
> > 
> > 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc
> >     in plat_data is supposed to be private and not to be accessed outside
> >     the device model.
> >     # This issue, though, exists for all the implmenetation of U-Boot
> >     # file systems as well.
> > 
> > For efi_disk(a),
> > 3. A block device can be enumelated dynamically by 'scanning' a device bus
> >     in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly.
> >     For examples,
> >      => scsi rescan; efidebug devices
> >      => usb start; efidebug devices ... (A)
> >     (A) doesn't show any usb devices detected.
> > 
> >      => scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ...
> >      => scsi rescan ... (B)
> >      => bootefi bootmgr ... (C)
> >     (C) may de-reference a bogus blk_desc pointer which has been freed by (B).
> >     (Please note that "scsi rescan" removes all udevices/blk_desc and then
> >      re-create them even if nothing is changed on a bus.)
> > 
> > For efi_disk(b),
> > 4. A controller (handle), combined with efi_block driver, has no
> >     corresponding udevice as a parent of efi_disks in DM tree, unlike, say,
> >     a scsi controller, even though it provides methods for block io perations.
> > 5. There is no way supported to remove efi_disk's even after
> >     disconnect_controller() is called.
> > 
> > 
> > My approach in this RFC:
> > ========================
> > Due to functional differences in semantics, it would be difficult
> > to identify "udevice" structure as a handle in UEFI world. Instead, we will
> > have to somehow maintain a relationship between a udevice and a handle.
> > 
> > 1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions
> >     Currently, the uclass for paritions is not a UCLASS_BLK.
> >     It can be possible to define partitions as UCLASS_BLK
> >     (with IF_TYPE_PARTION?), but
> >     I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK)
> >     is tightly coupled with 'struct blk_desc' data which is still used
> >     as a "structure to a whole disk" in a lot of interfaces.
> >     (I hope that you understand what it means.)
> > 
> >     In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent:
> >     For instance,
> >         UCLASS_SCSI  --- UCLASS_BLK       --- UCLASS_PARTITION
> > 			 (IF_TYPE_SCSI)        |
> >                            +- struct blk_desc   +- struct disk_part
> > 			  +- scsi_blk_ops      +- blk_part_ops
> > 
> > 1-2. create partition udevices in the context of device_probe()
> >     part_init() is already called in blk_post_probe(). See the commit
> >     d0851c893706 ("blk: Call part_init() in the post_probe() method").
> >     Why not enumelate partitions as well in there.
> > 
> > 2. add new block access interfaces, which takes "udevice" as a target device,
> >     in U-Boot and use those functions to implement efi_disk operations
> >     (i.e. EFI_BLOCK_IO_PROTOCOL).
> > 
> > 3-1. maintain a bi-directional link by adding
> >     - a UEFI handle pointer in "struct udevice"
> >     - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
> 
> An EFI application can create handles with any combination of protocols,
> e.g. a handle with both the block IO protocol and the simple network
> protocol. This means that a udevice cannot be assigned to a handle
> created by an EFI application.

Can you please elaborate more to clarify your point/suggestion here?

> When the EFI application calls ConnectController() for the handle,
> U-Boot can create child controllers. If U-Boot creates a udevice for
> such a child controller, it has to store the udevice pointer.
> lib/efi_driver/efi_block_device.c uses a private data section but you it
> could be preferable to use a field in struct efi_obj.

Before submitting this RFC, I also thought of a possibility of
re-implementing lib/efi_loader/efi_disk.c by defining a "controller"
for each U-Boot's block device (udevice) which is essentially a source
of providing BLOCK_IO_PROTOCOL as "efi_disk" devices and then implementing
"bind" interface of DRIVER_BINDING_PROTOCOL to create a mapping between
udevice(UCLASS_BLK) and efi_disk.
(Then I hoped we could reuse efi_driver framework for the case (1) below.)
Is this similar to what you think of here?

As I mentioned, there are two paths in creating efi_disks:
1) U-Boot's block device => efi_disk
   (efi_disk_add_dev() in lib/efi_loader/efi_disk.c is responsible for this.)
2) EFI app/driver -> efi_disk => U-Boot's block device
   (efi_bl_bind() in lib/efi_driver/efi_block_device.c)

Those two methods try to establish the relationship in opposite directions.
This is somewhat a cause of confusion/misunderstanding.


> > 
> > 3-2. use device model's post_probe/pre_remove hook to synchronize the lifetime
> >     of efi_disk objects in UEFI world with the device model.
> > 
> > 4. I have no answer to issue(4) and (5) yet.
> 
> 4) A udevice shall only exist for the child controller handle created by
> U-Boot and not for the controller handle created by an EFI application.

I don't know what is a "child" controller, and will think of it.

> 5) The stop() method of the driver binding protocol has to take care of
> destroying the child controllers and the associated udevices.

That is a missing piece of code.

-Takahiro Akashi


> Best regards
> 
> Heinrich

Re: [resent RFC 00/22] efi_loader: more tightly integrate UEFI disks to device model

[AKASHI Takahiro]

On Mon, Oct 04, 2021 at 09:07:35PM +0300, Ilias Apalodimas wrote:
> On Mon, Oct 04, 2021 at 04:47:53PM +0200, Heinrich Schuchardt wrote:
> > 
> > 
> > > 
> 
> [...]
> 
> > > My approach in this RFC:
> > > ========================
> > > Due to functional differences in semantics, it would be difficult
> > > to identify "udevice" structure as a handle in UEFI world. Instead, we will
> > > have to somehow maintain a relationship between a udevice and a handle.
> > > 
> > > 1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions
> > >     Currently, the uclass for paritions is not a UCLASS_BLK.
> > >     It can be possible to define partitions as UCLASS_BLK
> > >     (with IF_TYPE_PARTION?), but
> > >     I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK)
> > >     is tightly coupled with 'struct blk_desc' data which is still used
> > >     as a "structure to a whole disk" in a lot of interfaces.
> > >     (I hope that you understand what it means.)
> 
> I think it makes more sense the way it's currently defined.  I don;t see a
> point in hiding partitions within UCLASS_BLK

Yeah. But even with my UCLASS_PARTITION, it provides block-level io's
through blk_read/blk_write() APIs.
So someone may wonder why two different type of udevices have the same
interfaces :)

> > > 
> > >     In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent:
> > >     For instance,
> > >         UCLASS_SCSI  --- UCLASS_BLK       --- UCLASS_PARTITION
> > > 			 (IF_TYPE_SCSI)        |
> > >                            +- struct blk_desc   +- struct disk_part
> > > 			  +- scsi_blk_ops      +- blk_part_ops
> > > 
> > > 1-2. create partition udevices in the context of device_probe()
> > >     part_init() is already called in blk_post_probe(). See the commit
> > >     d0851c893706 ("blk: Call part_init() in the post_probe() method").
> > >     Why not enumelate partitions as well in there.
> > > 
> > > 2. add new block access interfaces, which takes "udevice" as a target device,
> > >     in U-Boot and use those functions to implement efi_disk operations
> > >     (i.e. EFI_BLOCK_IO_PROTOCOL).
> > > 
> > > 3-1. maintain a bi-directional link by adding
> > >     - a UEFI handle pointer in "struct udevice"
> > >     - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
> > 
> > An EFI application can create handles with any combination of protocols,
> > e.g. a handle with both the block IO protocol and the simple network
> > protocol. This means that a udevice cannot be assigned to a handle
> > created by an EFI application.
> > 
> > When the EFI application calls ConnectController() for the handle,
> > U-Boot can create child controllers. If U-Boot creates a udevice for
> > such a child controller, it has to store the udevice pointer.
> > lib/efi_driver/efi_block_device.c uses a private data section but you it
> > could be preferable to use a field in struct efi_obj.
> > 
> 
> I agree with Heinrich here.  Basically U-Boot has to be in charge of that.
> Once ConnectController has been called U-Boot should create an 1:1 mapping
> between udevice <-> handle and shouldn't be allowed to change that.

Again, are you sure you're talking about the implementation of efi_disk for
U-Boot's block device (the path(1) in my previous reply to Heinrich)?

-Takahiro Akashi

> > > 
> > > 3-2. use device model's post_probe/pre_remove hook to synchronize the lifetime
> > >     of efi_disk objects in UEFI world with the device model.
> > > 
> > > 4. I have no answer to issue(4) and (5) yet.
> > 
> > 4) A udevice shall only exist for the child controller handle created by
> > U-Boot and not for the controller handle created by an EFI application.
> > 
> > 5) The stop() method of the driver binding protocol has to take care of
> > destroying the child controllers and the associated udevices.
> > 
> > Best regards
> > 
> > Heinrich
> 
> Thanks
> /Ilias

Re: [resent RFC 06/22] sata: call device_probe() after scanning

[Ilias Apalodimas]

[...]
> > > +   ret = device_probe(bdev);
> > > +   if (ret < 0) {
> > > +           debug("Can't probe\n");
> > > +           /* TODO: undo create */
> > > +
> > > +           device_unbind(bdev);
> > > +
> > > +           return ret;
> > > +   }
> > > +
> >
> > Patches 2-6 seem to do the same thing for different subsystems.  I think
> > creating a function for that would make it easier.
>
> Well, the reason why I put those changes in separate commits is
> - first, different subsystems are owned by different maintainers, and
> - more importantly, different subsystems may have different cleanup
>   processing required.

That also stands if you create a common function doesn't it?
Create a function that fits most, add the calls in separate patches so
subsystems maintainers can have a look.  If one of the implementation
special, it can ignore the wrapper and go do it's own thing.

>   There are always extra setups after blk_create_device(), which should
>   be reverted if device_probe() fails. For instance, sil_unbind_device()
>   and fsl_unbind_device().
>   So I would like to leave subsystem owners responsible for that.

Regards
/Ilias

>
> -Takahiro Akashi
>
>
> > >     return 0;
> > >  }
> > >
> > > diff --git a/drivers/ata/fsl_sata.c b/drivers/ata/fsl_sata.c
> > > index e44db0a37458..346e9298b4c5 100644
> > > --- a/drivers/ata/fsl_sata.c
> > > +++ b/drivers/ata/fsl_sata.c
> > > @@ -982,6 +982,17 @@ static int fsl_ata_probe(struct udevice *dev)
> > >                     failed_number++;
> > >                     continue;
> > >             }
> > > +
> > > +           ret = device_probe(bdev);
> > > +           if (ret < 0) {
> > > +                   debug("Can't probe\n");
> > > +                   ret = fsl_unbind_device(blk);
> >
> > Apart from this exception I guess
> >
> > > +                   if (ret)
> > > +                           return ret;
> > > +
> > > +                   failed_number++;
> > > +                   continue;
> > > +           }
> > >     }
> > >
> > >     if (failed_number == nr_ports)
> > > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> > > index 003222d47be6..09b735779ebf 100644
> > > --- a/drivers/ata/sata_mv.c
> > > +++ b/drivers/ata/sata_mv.c
> > > @@ -1099,6 +1099,15 @@ static int sata_mv_probe(struct udevice *dev)
> > >                     continue;
> > >             }
> > >
> > > +           ret = device_probe(bdev);
> > > +           if (ret < 0) {
> > > +                   debug("Can't probe\n");
> > > +                   /* TODO: undo create */
> > > +
> > > +                   device_unbind(bdev);
> > > +                   continue;
> > > +           }
> > > +
> > >             /* If we got here, the current SATA port was probed
> > >              * successfully, so set the probe status to successful.
> > >              */
> > > diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
> > > index dda712f42cb2..295f7ca72303 100644
> > > --- a/drivers/ata/sata_sil.c
> > > +++ b/drivers/ata/sata_sil.c
> > > @@ -864,6 +864,18 @@ static int sil_pci_probe(struct udevice *dev)
> > >                     failed_number++;
> > >                     continue;
> > >             }
> > > +
> > > +           ret = device_probe(bdev);
> > > +           if (ret < 0) {
> > > +                   debug("Can't probe\n");
> > > +                   ret = sil_unbind_device(blk);
> > > +                   device_unbind(bdev);
> > > +                   if (ret)
> > > +                           return ret;
> > > +
> > > +                   failed_number++;
> > > +                   continue;
> > > +           }
> > >     }
> > >
> > >     if (failed_number == sata_info.maxport)
> > > --
> > > 2.33.0
> > >

Re: [resent RFC 17/22] efi_loader: add efi_remove_handle()

[Ilias Apalodimas]

On Mon, Oct 04, 2021 at 12:44:25PM +0900, AKASHI Takahiro wrote:
> This function is a counterpart of efi_add_handle() and will be used
> in order to remove an efi_disk object in a later patch.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/efi_loader.h          | 2 ++
>  lib/efi_loader/efi_boottime.c | 8 ++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index cfbe1fe659ef..50f4119dcdfb 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -579,6 +579,8 @@ void efi_save_gd(void);
>  void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
>  /* Add a new object to the object list. */
>  void efi_add_handle(efi_handle_t obj);
> +/* Remove a object from the object list. */
> +void efi_remove_handle(efi_handle_t obj);
>  /* Create handle */
>  efi_status_t efi_create_handle(efi_handle_t *handle);
>  /* Delete handle */
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index f0283b539e46..b2503b74233b 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -503,6 +503,14 @@ void efi_add_handle(efi_handle_t handle)
>  	list_add_tail(&handle->link, &efi_obj_list);
>  }
>  
> +void efi_remove_handle(efi_handle_t handle)
> +{
> +	if (!handle)
> +		return;
> +
> +	list_del(&handle->link);
> +}
> +

We already have efi_delete_handle().  You can't unconditionally remove a
handle unless all protocols are removed.  Can't you just use the existing
function?

Cheers
/Ilias
>  /**
>   * efi_create_handle() - create handle
>   * @handle: new handle
> -- 
> 2.33.0
> 

Re: [resent RFC 17/22] efi_loader: add efi_remove_handle()

[AKASHI Takahiro]

On Tue, Oct 12, 2021 at 11:16:03AM +0300, Ilias Apalodimas wrote:
> On Mon, Oct 04, 2021 at 12:44:25PM +0900, AKASHI Takahiro wrote:
> > This function is a counterpart of efi_add_handle() and will be used
> > in order to remove an efi_disk object in a later patch.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/efi_loader.h          | 2 ++
> >  lib/efi_loader/efi_boottime.c | 8 ++++++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index cfbe1fe659ef..50f4119dcdfb 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -579,6 +579,8 @@ void efi_save_gd(void);
> >  void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
> >  /* Add a new object to the object list. */
> >  void efi_add_handle(efi_handle_t obj);
> > +/* Remove a object from the object list. */
> > +void efi_remove_handle(efi_handle_t obj);
> >  /* Create handle */
> >  efi_status_t efi_create_handle(efi_handle_t *handle);
> >  /* Delete handle */
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index f0283b539e46..b2503b74233b 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -503,6 +503,14 @@ void efi_add_handle(efi_handle_t handle)
> >  	list_add_tail(&handle->link, &efi_obj_list);
> >  }
> >  
> > +void efi_remove_handle(efi_handle_t handle)
> > +{
> > +	if (!handle)
> > +		return;
> > +
> > +	list_del(&handle->link);
> > +}
> > +
> 
> We already have efi_delete_handle().  You can't unconditionally remove a
> handle unless all protocols are removed.  Can't you just use the existing
> function?

My intent was to add this function as a counterpart of efi_add_handle()
since not all the handles are dynamically allocated on its own.
As far as my usage in this patch series is concerned, however, it is always
used accompanying efi_remove_all_protocols() and free().
(See efi_disk_delete_xxx() in efi_disk.c)

So, yes we can use efi_delete_handle() instead.
(assuming 'header' is the first member in struct efi_disk_obj.)

-Takahiro Akashi

> Cheers
> /Ilias
> >  /**
> >   * efi_create_handle() - create handle
> >   * @handle: new handle
> > -- 
> > 2.33.0
> >