[U-Boot] [RFC 0/3] dm, efi: integrate efi_disk into DM

[U-Boot] [RFC 0/3] dm, efi: integrate efi_disk into DM

[AKASHI Takahiro]

This patch set came from the past discussion[1] on my "removable device
support" patch and is intended to be an attempt to integrate efi_disk
(more precisely, EFI_BLOCK_IO_PROTOCOL-capable efi object) into u-boot's
Driver Model as much seamlessly as possible

[1] https://lists.denx.de/pipermail/u-boot/2019-January/354010.html

Basic idea is
* create UCLASS_PARTITION
* enumerate all the partitions when a block device is probed
* hook up a creation of UCLASS_BLK or UCLASS_PARTITION device
  to efi handler for efi_disk attributes to be properly set up

Since this patch is a prototype (or POC, proof-of-concept), the aim here
is to discuss further about how in a better shape we will be able to
merge the two worlds.


Example operation:
(Two scsi disks, one with no partition, one with two partitions)

U-Boot 2019.01-rc3-00024-ga81a6f87ad48 (Jan 29 2019 - 10:56:45 +0900)

DRAM:  1 GiB
In:    pl011 at 9000000
Out:   pl011 at 9000000
Err:   pl011 at 9000000
Net:   No ethernet found.
Hit any key to stop autoboot:  0 
=> efi devices
Device           Device Path
================ ====================
000000007ef01350 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
=> scsi rescan

Reset SCSI
scanning bus for devices...
Target spinup took 0 ms.
Target spinup took 0 ms.
SATA link 2 timeout.
SATA link 3 timeout.
SATA link 4 timeout.
SATA link 5 timeout.
AHCI 0001.0000 32 slots 6 ports 1.5 Gbps 0x3f impl SATA mode
flags: 64bit ncq only 
  Device 0: (0:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+
            Type: Hard Disk
            Capacity: 16.0 MB = 0.0 GB (32768 x 512)
  Device 0: (1:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+
            Type: Hard Disk
            Capacity: 256.0 MB = 0.2 GB (524288 x 512)
=> efi devices
Device           Device Path
================ ====================
000000007ef01350 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
000000007ef0b2c0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
000000007ef0b7b0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
000000007ef0c760 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(-1,MBR,0x086246ba,0x17ff56048,0x7eeeb770)
000000007ef0cb50 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(-1,MBR,0x086246ba,0x17ff56048,0x7eeeb770)
=> dm tree
 Class    index  Probed  Driver                Name
-----------------------------------------------------------
 root        0  [ + ]   root_driver           root_driver
 ...
 pci         0  [ + ]   pci_generic_ecam      |-- pcie at 10000000
 pci_generi  0  [   ]   pci_generic_drv       |   |-- pci_0:0.0
 virtio      32  [   ]   virtio-pci.l          |   |-- virtio-pci.l#0
 ahci        0  [ + ]   ahci_pci              |   `-- ahci_pci
 scsi        0  [ + ]   ahci_scsi             |       `-- ahci_scsi
 blk         0  [   ]   scsi_blk              |           |-- ahci_scsi.id0lun0
 blk         1  [   ]   scsi_blk              |           `-- ahci_scsi.id1lun0
 partition   0  [   ]   blk_partition         |               |-- ahci_scsi.id1lun0:1
 partition   1  [   ]   blk_partition         |               `-- ahci_scsi.id1lun0:2
 ...

Thanks,
-Takahiro Akashi

AKASHI Takahiro (3):
  dm: blk: add UCLASS_PARTITION
  efi_loader: associate BLK/PARTITION device to efi_disk
  drivers: align block device drivers with DM-efi integration

 common/usb_storage.c              |  27 ++++-
 drivers/block/blk-uclass.c        |  61 ++++++++++
 drivers/core/device.c             |   3 +
 drivers/scsi/scsi.c               |  22 ++++
 include/blk.h                     |  24 ++++
 include/dm/device.h               |   4 +
 include/dm/uclass-id.h            |   1 +
 lib/efi_driver/efi_block_device.c |  30 ++---
 lib/efi_loader/efi_disk.c         | 192 +++++++++++++++++++++---------
 9 files changed, 283 insertions(+), 81 deletions(-)

-- 
2.19.1

[U-Boot] [RFC 1/3] 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 | 52 ++++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h     |  1 +
 2 files changed, 53 insertions(+)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index baaf431e5e0c..d4ca30f23fc1 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -10,6 +10,8 @@
 #include <dm/device-internal.h>
 #include <dm/lists.h>
 #include <dm/uclass-internal.h>
+#include <part.h>
+#include <string.h>
 
 static const char *if_typename_str[IF_TYPE_COUNT] = {
 	[IF_TYPE_IDE]		= "ide",
@@ -654,3 +656,53 @@ UCLASS_DRIVER(blk) = {
 	.post_probe	= blk_post_probe,
 	.per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
 };
+
+U_BOOT_DRIVER(blk_partition) = {
+	.name		= "blk_partition",
+	.id		= UCLASS_PARTITION,
+	.platdata_auto_alloc_size = sizeof(struct disk_part),
+};
+
+UCLASS_DRIVER(partition) = {
+	.id		= UCLASS_PARTITION,
+	.name		= "partition",
+};
+
+#if defined(CONFIG_PARTITIONS) && defined(CONFIG_HAVE_BLOCK_DEVICE)
+int blk_create_partitions(struct udevice *parent)
+{
+	int part;
+	struct blk_desc *desc = dev_get_uclass_platdata(parent);
+	disk_partition_t info;
+	struct disk_part *part_data;
+	char devname[32];
+	struct udevice *dev;
+	int disks = 0, ret;
+
+	/* Add devices for each partition */
+	for (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_platdata(dev);
+		part_data->partnum = part;
+		part_data->gpt_part_info = info;
+
+		disks++;
+	}
+
+	return disks;
+}
+#else
+int blk_create_partitions(struct udevice *dev)
+{
+	return 0;
+}
+#endif
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index f3bafb3c6353..e02b5f8fda42 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -65,6 +65,7 @@ enum uclass_id {
 	UCLASS_NVME,		/* NVM Express device */
 	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_GENERIC,	/* Generic PCI bus device */
-- 
2.19.1

[U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION

[Sergey Kubushyn]

On Tue, 29 Jan 2019, AKASHI Takahiro wrote:

My $.25 -- IMHO, block device partitions in Device Tree is _NOT_ a very good
idea.

What if one decided to re-partition his drive? Or just use bigger or smaller
drive? Would he has to re-write DTS file and re-compile everything? And such
change is not something extraordinary, it is a routine action.

IMHO partitions do _NOT_ belong to Block Device. That's what partition
tables are.

It _MIGHT_ make sense for some particular devices such as MTD that don't
have partition tables but _NOT_ for Block Devices in general.

> UCLASS_PARTITION device will be created as a child node of
> UCLASS_BLK device.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************

[U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION

[Alexander Graf]

On 01/29/2019 04:17 AM, Sergey Kubushyn wrote:
> On Tue, 29 Jan 2019, AKASHI Takahiro wrote:
>
> My $.25 -- IMHO, block device partitions in Device Tree is _NOT_ a 
> very good
> idea.

This is about device model, not device tree :). Device trees indeed 
should not contain partition information. Our internal object model 
however would do well if it had them.


Alex

[U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION

[Sergey Kubushyn]

On Tue, 29 Jan 2019, Alexander Graf wrote:

> On 01/29/2019 04:17 AM, Sergey Kubushyn wrote:
>>  On Tue, 29 Jan 2019, AKASHI Takahiro wrote:
>>
>>  My $.25 -- IMHO, block device partitions in Device Tree is _NOT_ a very
>>  good
>>  idea.
>
> This is about device model, not device tree :). Device trees indeed should 
> not contain partition information. Our internal object model however would do 
> well if it had them.

DM assumes using Device Tree.

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************

[U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION

[Philipp Tomsich]

> On 29.01.2019, at 18:46, Sergey Kubushyn <ksi@koi8.net> wrote:
> 
> On Tue, 29 Jan 2019, Alexander Graf wrote:
> 
>> On 01/29/2019 04:17 AM, Sergey Kubushyn wrote:
>>> On Tue, 29 Jan 2019, AKASHI Takahiro wrote:
>>> 
>>> My $.25 -- IMHO, block device partitions in Device Tree is _NOT_ a very
>>> good
>>> idea.
>> 
>> This is about device model, not device tree :). Device trees indeed should not contain partition information. Our internal object model however would do well if it had them.
> 
> DM assumes using Device Tree.

As there’s no compatible-string, this indeed is for the internal object model only.

[U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION

[Heinrich Schuchardt]

On 1/29/19 3:59 AM, AKASHI Takahiro wrote:
> 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 | 52 ++++++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h     |  1 +
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index baaf431e5e0c..d4ca30f23fc1 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -10,6 +10,8 @@
>  #include <dm/device-internal.h>
>  #include <dm/lists.h>
>  #include <dm/uclass-internal.h>
> +#include <part.h>
> +#include <string.h>
>  
>  static const char *if_typename_str[IF_TYPE_COUNT] = {
>  	[IF_TYPE_IDE]		= "ide",
> @@ -654,3 +656,53 @@ UCLASS_DRIVER(blk) = {
>  	.post_probe	= blk_post_probe,
>  	.per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
>  };
> +
> +U_BOOT_DRIVER(blk_partition) = {
> +	.name		= "blk_partition",
> +	.id		= UCLASS_PARTITION,
> +	.platdata_auto_alloc_size = sizeof(struct disk_part),
> +};
> +
> +UCLASS_DRIVER(partition) = {
> +	.id		= UCLASS_PARTITION,
> +	.name		= "partition",
> +};
> +
> +#if defined(CONFIG_PARTITIONS) && defined(CONFIG_HAVE_BLOCK_DEVICE)
> +int blk_create_partitions(struct udevice *parent)
> +{
> +	int part;
> +	struct blk_desc *desc = dev_get_uclass_platdata(parent);
> +	disk_partition_t info;
> +	struct disk_part *part_data;
> +	char devname[32];
> +	struct udevice *dev;
> +	int disks = 0, ret;
> +
> +	/* Add devices for each partition */
> +	for (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)

This looks like a memory leak for the output of strdup().

> +			return ret;

Why would we leave here if one partition fails?
Does this imply that all further partitions will fail?
Should we use continue here?

Best regards

Heinrich

> +
> +		part_data = dev_get_uclass_platdata(dev);
> +		part_data->partnum = part;
> +		part_data->gpt_part_info = info;
> +
> +		disks++;
> +	}
> +
> +	return disks;
> +}
> +#else
> +int blk_create_partitions(struct udevice *dev)
> +{
> +	return 0;
> +}
> +#endif
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index f3bafb3c6353..e02b5f8fda42 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -65,6 +65,7 @@ enum uclass_id {
>  	UCLASS_NVME,		/* NVM Express device */
>  	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_GENERIC,	/* Generic PCI bus device */
> 

[U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION

[AKASHI Takahiro]

On Tue, Jan 29, 2019 at 11:20:01PM +0100, Heinrich Schuchardt wrote:
> On 1/29/19 3:59 AM, AKASHI Takahiro wrote:
> > 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 | 52 ++++++++++++++++++++++++++++++++++++++
> >  include/dm/uclass-id.h     |  1 +
> >  2 files changed, 53 insertions(+)
> > 
> > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> > index baaf431e5e0c..d4ca30f23fc1 100644
> > --- a/drivers/block/blk-uclass.c
> > +++ b/drivers/block/blk-uclass.c
> > @@ -10,6 +10,8 @@
> >  #include <dm/device-internal.h>
> >  #include <dm/lists.h>
> >  #include <dm/uclass-internal.h>
> > +#include <part.h>
> > +#include <string.h>
> >  
> >  static const char *if_typename_str[IF_TYPE_COUNT] = {
> >  	[IF_TYPE_IDE]		= "ide",
> > @@ -654,3 +656,53 @@ UCLASS_DRIVER(blk) = {
> >  	.post_probe	= blk_post_probe,
> >  	.per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
> >  };
> > +
> > +U_BOOT_DRIVER(blk_partition) = {
> > +	.name		= "blk_partition",
> > +	.id		= UCLASS_PARTITION,
> > +	.platdata_auto_alloc_size = sizeof(struct disk_part),
> > +};
> > +
> > +UCLASS_DRIVER(partition) = {
> > +	.id		= UCLASS_PARTITION,
> > +	.name		= "partition",
> > +};
> > +
> > +#if defined(CONFIG_PARTITIONS) && defined(CONFIG_HAVE_BLOCK_DEVICE)
> > +int blk_create_partitions(struct udevice *parent)
> > +{
> > +	int part;
> > +	struct blk_desc *desc = dev_get_uclass_platdata(parent);
> > +	disk_partition_t info;
> > +	struct disk_part *part_data;
> > +	char devname[32];
> > +	struct udevice *dev;
> > +	int disks = 0, ret;
> > +
> > +	/* Add devices for each partition */
> > +	for (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)
> 
> This looks like a memory leak for the output of strdup().

Yes, I'm aware of that, but please note that this is a prototype
and so I haven't paid much attention to failure cases (error recovery).
First of all, even in the current implementation, we don't support
*unplugging* (or unbind in EFI jargon?) devices.
It's a more fundamental issue.

> > +			return ret;
> 
> Why would we leave here if one partition fails?
> Does this imply that all further partitions will fail?
> Should we use continue here?

Ditto. Please be patient for the time being :)

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +
> > +		part_data = dev_get_uclass_platdata(dev);
> > +		part_data->partnum = part;
> > +		part_data->gpt_part_info = info;
> > +
> > +		disks++;
> > +	}
> > +
> > +	return disks;
> > +}
> > +#else
> > +int blk_create_partitions(struct udevice *dev)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index f3bafb3c6353..e02b5f8fda42 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -65,6 +65,7 @@ enum uclass_id {
> >  	UCLASS_NVME,		/* NVM Express device */
> >  	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_GENERIC,	/* Generic PCI bus device */
> > 
> 

[U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION

[Simon Glass]

Hi Takahiro,

On Mon, 28 Jan 2019 at 19:59, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> 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 | 52 ++++++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h     |  1 +
>  2 files changed, 53 insertions(+)
>
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index baaf431e5e0c..d4ca30f23fc1 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -10,6 +10,8 @@
>  #include <dm/device-internal.h>
>  #include <dm/lists.h>
>  #include <dm/uclass-internal.h>
> +#include <part.h>
> +#include <string.h>
>
>  static const char *if_typename_str[IF_TYPE_COUNT] = {
>         [IF_TYPE_IDE]           = "ide",
> @@ -654,3 +656,53 @@ UCLASS_DRIVER(blk) = {
>         .post_probe     = blk_post_probe,
>         .per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
>  };
> +
> +U_BOOT_DRIVER(blk_partition) = {
> +       .name           = "blk_partition",
> +       .id             = UCLASS_PARTITION,
> +       .platdata_auto_alloc_size = sizeof(struct disk_part),
> +};
> +
> +UCLASS_DRIVER(partition) = {
> +       .id             = UCLASS_PARTITION,
> +       .name           = "partition",
> +};
> +
> +#if defined(CONFIG_PARTITIONS) && defined(CONFIG_HAVE_BLOCK_DEVICE)
> +int blk_create_partitions(struct udevice *parent)
> +{
> +       int part;
> +       struct blk_desc *desc = dev_get_uclass_platdata(parent);
> +       disk_partition_t info;
> +       struct disk_part *part_data;
> +       char devname[32];
> +       struct udevice *dev;
> +       int disks = 0, ret;
> +
> +       /* Add devices for each partition */
> +       for (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_platdata(dev);
> +               part_data->partnum = part;
> +               part_data->gpt_part_info = info;
> +
> +               disks++;
> +       }
> +
> +       return disks;
> +}
> +#else
> +int blk_create_partitions(struct udevice *dev)
> +{
> +       return 0;
> +}
> +#endif
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index f3bafb3c6353..e02b5f8fda42 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -65,6 +65,7 @@ enum uclass_id {
>         UCLASS_NVME,            /* NVM Express device */
>         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_GENERIC,     /* Generic PCI bus device */
> --
> 2.19.1
>

This looks OK to me. It does need a test in test/dm/partition.c for
the new uclass.

Regards,
Simon

[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[AKASHI Takahiro]

efi_disk_create() will initialize efi_disk attributes for each device,
either UCLASS_BLK or UCLASS_PARTITION.

Currently (temporarily), efi_disk_obj structure is embedded into
blk_desc to hold efi-specific attributes.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 drivers/block/blk-uclass.c |   9 ++
 drivers/core/device.c      |   3 +
 include/blk.h              |  24 +++++
 include/dm/device.h        |   4 +
 lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
 5 files changed, 174 insertions(+), 58 deletions(-)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index d4ca30f23fc1..28c45d724113 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -657,6 +657,9 @@ UCLASS_DRIVER(blk) = {
 	.per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
 };
 
+/* FIXME */
+extern int efi_disk_create(struct udevice *dev);
+
 U_BOOT_DRIVER(blk_partition) = {
 	.name		= "blk_partition",
 	.id		= UCLASS_PARTITION,
@@ -695,6 +698,12 @@ int blk_create_partitions(struct udevice *parent)
 		part_data->partnum = part;
 		part_data->gpt_part_info = info;
 
+		ret = efi_disk_create(dev);
+		if (ret) {
+			device_unbind(dev);
+			return ret;
+		}
+
 		disks++;
 	}
 
diff --git a/drivers/core/device.c b/drivers/core/device.c
index 0d15e5062b66..8625fccb0dcb 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -67,6 +67,9 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv,
 	dev->parent = parent;
 	dev->driver = drv;
 	dev->uclass = uc;
+#ifdef CONFIG_EFI_LOADER
+	INIT_LIST_HEAD(&dev->efi_obj.protocols);
+#endif
 
 	dev->seq = -1;
 	dev->req_seq = -1;
diff --git a/include/blk.h b/include/blk.h
index d0c033aece0f..405f6f790d66 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -8,6 +8,7 @@
 #define BLK_H
 
 #include <efi.h>
+#include <efi_api.h>
 
 #ifdef CONFIG_SYS_64BIT_LBA
 typedef uint64_t lbaint_t;
@@ -53,6 +54,26 @@ enum sig_type {
 	SIG_TYPE_COUNT			/* Number of signature types */
 };
 
+/* FIXME */
+/**
+ * struct efi_disk_obj - EFI disk object
+ *
+ * @ops:	EFI disk I/O protocol interface
+ * @media:	block I/O media information
+ * @dp:		device path to the block device
+ * @part:	partition
+ * @volume:	simple file system protocol of the partition
+ * @offset:	offset into disk for simple partition
+ */
+struct efi_disk_obj {
+	struct efi_block_io ops;
+	struct efi_block_io_media media;
+	struct efi_device_path *dp;
+	unsigned int part;
+	struct efi_simple_file_system_protocol *volume;
+	lbaint_t offset;
+};
+
 /*
  * With driver model (CONFIG_BLK) this is uclass platform data, accessible
  * with dev_get_uclass_platdata(dev)
@@ -92,6 +113,9 @@ struct blk_desc {
 	 * device. Once these functions are removed we can drop this field.
 	 */
 	struct udevice *bdev;
+#ifdef CONFIG_EFI_LOADER
+	struct efi_disk_obj efi_disk;
+#endif
 #else
 	unsigned long	(*block_read)(struct blk_desc *block_dev,
 				      lbaint_t start,
diff --git a/include/dm/device.h b/include/dm/device.h
index 27a6d7b9fdb0..121bfb46b1a0 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -12,6 +12,7 @@
 
 #include <dm/ofnode.h>
 #include <dm/uclass-id.h>
+#include <efi_loader.h>
 #include <fdtdec.h>
 #include <linker_lists.h>
 #include <linux/compat.h>
@@ -146,6 +147,9 @@ struct udevice {
 #ifdef CONFIG_DEVRES
 	struct list_head devres_head;
 #endif
+#ifdef CONFIG_EFI_LOADER
+	struct efi_object efi_obj;
+#endif
 };
 
 /* Maximum sequence number supported */
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index c037526ad2d0..84fa0ddfba14 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -14,33 +14,6 @@
 
 const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
 
-/**
- * struct efi_disk_obj - EFI disk object
- *
- * @header:	EFI object header
- * @ops:	EFI disk I/O protocol interface
- * @ifname:	interface name for block device
- * @dev_index:	device index of block device
- * @media:	block I/O media information
- * @dp:		device path to the block device
- * @part:	partition
- * @volume:	simple file system protocol of the partition
- * @offset:	offset into disk for simple partition
- * @desc:	internal block device descriptor
- */
-struct efi_disk_obj {
-	struct efi_object header;
-	struct efi_block_io ops;
-	const char *ifname;
-	int dev_index;
-	struct efi_block_io_media media;
-	struct efi_device_path *dp;
-	unsigned int part;
-	struct efi_simple_file_system_protocol *volume;
-	lbaint_t offset;
-	struct blk_desc *desc;
-};
-
 static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this,
 			char extended_verification)
 {
@@ -64,7 +37,7 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
 	unsigned long n;
 
 	diskobj = container_of(this, struct efi_disk_obj, ops);
-	desc = (struct blk_desc *) diskobj->desc;
+	desc = container_of(diskobj, struct blk_desc, efi_disk);
 	blksz = desc->blksz;
 	blocks = buffer_size / blksz;
 	lba += diskobj->offset;
@@ -217,6 +190,7 @@ efi_fs_from_path(struct efi_device_path *full_path)
 	return handler->protocol_interface;
 }
 
+#ifndef CONFIG_BLK
 /*
  * Create a handle for a partition or disk
  *
@@ -343,6 +317,136 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
 
 	return disks;
 }
+#else
+static int efi_disk_create_common(efi_handle_t handle,
+				  struct efi_disk_obj *disk,
+				  struct blk_desc *desc)
+{
+	efi_status_t ret;
+
+	/* Hook up to the device list */
+	efi_add_handle(handle);
+
+	/* Fill in EFI IO Media info (for read/write callbacks) */
+	disk->media.removable_media = desc->removable;
+	disk->media.media_present = 1;
+	disk->media.block_size = desc->blksz;
+	disk->media.io_align = desc->blksz;
+	disk->media.last_block = desc->lba - disk->offset;
+	disk->ops.media = &disk->media;
+
+	/* add protocols */
+	disk->ops = block_io_disk_template;
+	ret = efi_add_protocol(handle, &efi_block_io_guid, &disk->ops);
+	if (ret != EFI_SUCCESS)
+		goto err;
+
+	ret = efi_add_protocol(handle, &efi_guid_device_path, disk->dp);
+	if (ret != EFI_SUCCESS)
+		goto err;
+
+	return 0;
+
+err:
+	/* FIXME: undo adding protocols */
+	return -1;
+}
+
+/*
+ * Create a handle for a raw disk
+ *
+ * @dev		uclass device
+ * @return	0 on success, -1 otherwise
+ */
+int efi_disk_create_raw(struct udevice *dev)
+{
+	struct blk_desc *desc = dev_get_uclass_platdata(dev);
+	struct efi_disk_obj *disk = &desc->efi_disk;
+
+	/* Don't add empty devices */
+	if (!desc->lba)
+		return -1;
+
+	/* raw block device */
+	disk->offset = 0;
+	disk->part = 0;
+	disk->dp = efi_dp_from_part(desc, 0);
+
+	/* efi IO media */
+	disk->media.logical_partition = 0;
+
+	return efi_disk_create_common(&dev->efi_obj, disk, desc);
+}
+
+/*
+ * Create a handle for a partition
+ *
+ * @dev		uclass device
+ * @return	0 on success, -1 otherwise
+ */
+int efi_disk_create_part(struct udevice *dev)
+{
+	struct udevice *parent = dev->parent;
+	struct blk_desc *desc = dev_get_uclass_platdata(parent);
+	struct blk_desc *this;
+	struct disk_part *pdata = dev_get_uclass_platdata(dev);
+	struct efi_disk_obj *disk;
+	struct efi_device_path *node;
+
+	int ret;
+
+	/* dummy block device */
+	this = malloc(sizeof(*this));
+	if (!this)
+		return -1;
+	disk = &this->efi_disk;
+
+	/* logical disk partition */
+	disk->offset = pdata->gpt_part_info.start;
+	disk->part = pdata->partnum;
+
+	node = efi_dp_part_node(desc, disk->part);
+	disk->dp = efi_dp_append_node(desc->efi_disk.dp, node);
+	efi_free_pool(node);
+
+	/* efi IO media */
+	disk->media.logical_partition = 1;
+
+	ret = efi_disk_create_common(&dev->efi_obj, disk, desc);
+	if (ret)
+		goto err;
+
+	/* partition may support file system access */
+	disk->volume = efi_simple_file_system(desc, disk->part, disk->dp);
+	ret = efi_add_protocol(&dev->efi_obj,
+			       &efi_simple_file_system_protocol_guid,
+			       disk->volume);
+	if (ret != EFI_SUCCESS)
+		goto err;
+
+	return 0;
+
+err:
+	free(this);
+	/* FIXME: undo create */
+
+	return -1;
+}
+
+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);
+	else if (id == UCLASS_PARTITION)
+		return efi_disk_create_part(dev);
+	else
+		return -1;
+}
+#endif /* CONFIG_BLK */
 
 /*
  * U-Boot doesn't have a list of all online disk devices. So when running our
@@ -357,38 +461,10 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
  */
 efi_status_t efi_disk_register(void)
 {
+#ifndef CONFIG_BLK
 	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;
-	     uclass_next_device_check(&dev)) {
-		struct blk_desc *desc = dev_get_uclass_platdata(dev);
-		const char *if_typename = blk_get_if_type_name(desc->if_type);
-
-		/* Add block device for the full device */
-		printf("Scanning disk %s...\n", dev->name);
-		ret = efi_disk_add_dev(NULL, NULL, if_typename,
-					desc, desc->devnum, 0, 0, &disk);
-		if (ret == EFI_NOT_READY) {
-			printf("Disk %s not ready\n", dev->name);
-			continue;
-		}
-		if (ret) {
-			printf("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);
-	}
-#else
 	int i, if_type;
 
 	/* Search for all available disk devices */
@@ -435,8 +511,8 @@ efi_status_t efi_disk_register(void)
 						 if_typename, i, devname);
 		}
 	}
-#endif
 	printf("Found %d disks\n", disks);
+#endif
 
 	return EFI_SUCCESS;
 }
-- 
2.19.1

[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[Heinrich Schuchardt]

On 1/29/19 3:59 AM, AKASHI Takahiro wrote:
> efi_disk_create() will initialize efi_disk attributes for each device,
> either UCLASS_BLK or UCLASS_PARTITION.
> 
> Currently (temporarily), efi_disk_obj structure is embedded into
> blk_desc to hold efi-specific attributes.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  drivers/block/blk-uclass.c |   9 ++
>  drivers/core/device.c      |   3 +
>  include/blk.h              |  24 +++++
>  include/dm/device.h        |   4 +
>  lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
>  5 files changed, 174 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index d4ca30f23fc1..28c45d724113 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -657,6 +657,9 @@ UCLASS_DRIVER(blk) = {
>  	.per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
>  };
>  
> +/* FIXME */
> +extern int efi_disk_create(struct udevice *dev);
> +
>  U_BOOT_DRIVER(blk_partition) = {
>  	.name		= "blk_partition",
>  	.id		= UCLASS_PARTITION,
> @@ -695,6 +698,12 @@ int blk_create_partitions(struct udevice *parent)
>  		part_data->partnum = part;
>  		part_data->gpt_part_info = info;
>  
> +		ret = efi_disk_create(dev);
> +		if (ret) {
> +			device_unbind(dev);
> +			return ret;
> +		}
> +
>  		disks++;
>  	}
>  
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 0d15e5062b66..8625fccb0dcb 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -67,6 +67,9 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv,
>  	dev->parent = parent;
>  	dev->driver = drv;
>  	dev->uclass = uc;
> +#ifdef CONFIG_EFI_LOADER
> +	INIT_LIST_HEAD(&dev->efi_obj.protocols);

This looks like an incomplete initialization of efi_obj. Why don't we
use efi_create_handle.

Why should a device be aware of struct efi_obj? We only need a handle to
install protocols.

> +#endif
>  
>  	dev->seq = -1;
>  	dev->req_seq = -1;
> diff --git a/include/blk.h b/include/blk.h
> index d0c033aece0f..405f6f790d66 100644
> --- a/include/blk.h
> +++ b/include/blk.h
> @@ -8,6 +8,7 @@
>  #define BLK_H
>  
>  #include <efi.h>
> +#include <efi_api.h>
>  
>  #ifdef CONFIG_SYS_64BIT_LBA
>  typedef uint64_t lbaint_t;
> @@ -53,6 +54,26 @@ enum sig_type {
>  	SIG_TYPE_COUNT			/* Number of signature types */
>  };
>  
> +/* FIXME */

Fix what?

> +/**
> + * struct efi_disk_obj - EFI disk object
> + *
> + * @ops:	EFI disk I/O protocol interface
> + * @media:	block I/O media information
> + * @dp:		device path to the block device
> + * @part:	partition
> + * @volume:	simple file system protocol of the partition
> + * @offset:	offset into disk for simple partition
> + */
> +struct efi_disk_obj {
> +	struct efi_block_io ops;
> +	struct efi_block_io_media media;
> +	struct efi_device_path *dp;
> +	unsigned int part;
> +	struct efi_simple_file_system_protocol *volume;
> +	lbaint_t offset;
> +};
> +
>  /*
>   * With driver model (CONFIG_BLK) this is uclass platform data, accessible
>   * with dev_get_uclass_platdata(dev)
> @@ -92,6 +113,9 @@ struct blk_desc {
>  	 * device. Once these functions are removed we can drop this field.
>  	 */
>  	struct udevice *bdev;
> +#ifdef CONFIG_EFI_LOADER
> +	struct efi_disk_obj efi_disk;

This must be a handle (i.e. a pointer). Otherwise when the last protocol
is removed we try to free memory that was never malloc'ed.

> +#endif
>  #else
>  	unsigned long	(*block_read)(struct blk_desc *block_dev,
>  				      lbaint_t start,
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 27a6d7b9fdb0..121bfb46b1a0 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -12,6 +12,7 @@
>  
>  #include <dm/ofnode.h>
>  #include <dm/uclass-id.h>
> +#include <efi_loader.h>
>  #include <fdtdec.h>
>  #include <linker_lists.h>
>  #include <linux/compat.h>
> @@ -146,6 +147,9 @@ struct udevice {
>  #ifdef CONFIG_DEVRES
>  	struct list_head devres_head;
>  #endif
> +#ifdef CONFIG_EFI_LOADER
> +	struct efi_object efi_obj;
> +#endif
>  };
>  
>  /* Maximum sequence number supported */
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index c037526ad2d0..84fa0ddfba14 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -14,33 +14,6 @@
>  
>  const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
>  
> -/**
> - * struct efi_disk_obj - EFI disk object
> - *
> - * @header:	EFI object header
> - * @ops:	EFI disk I/O protocol interface
> - * @ifname:	interface name for block device
> - * @dev_index:	device index of block device
> - * @media:	block I/O media information
> - * @dp:		device path to the block device
> - * @part:	partition
> - * @volume:	simple file system protocol of the partition
> - * @offset:	offset into disk for simple partition
> - * @desc:	internal block device descriptor
> - */
> -struct efi_disk_obj {
> -	struct efi_object header;
> -	struct efi_block_io ops;
> -	const char *ifname;
> -	int dev_index;
> -	struct efi_block_io_media media;
> -	struct efi_device_path *dp;
> -	unsigned int part;
> -	struct efi_simple_file_system_protocol *volume;
> -	lbaint_t offset;
> -	struct blk_desc *desc;
> -};
> -
>  static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this,
>  			char extended_verification)
>  {
> @@ -64,7 +37,7 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
>  	unsigned long n;
>  
>  	diskobj = container_of(this, struct efi_disk_obj, ops);
> -	desc = (struct blk_desc *) diskobj->desc;
> +	desc = container_of(diskobj, struct blk_desc, efi_disk);
>  	blksz = desc->blksz;
>  	blocks = buffer_size / blksz;
>  	lba += diskobj->offset;
> @@ -217,6 +190,7 @@ efi_fs_from_path(struct efi_device_path *full_path)
>  	return handler->protocol_interface;
>  }
>  
> +#ifndef CONFIG_BLK
>  /*
>   * Create a handle for a partition or disk
>   *
> @@ -343,6 +317,136 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
>  
>  	return disks;
>  }
> +#else
> +static int efi_disk_create_common(efi_handle_t handle,
> +				  struct efi_disk_obj *disk,
> +				  struct blk_desc *desc)
> +{
> +	efi_status_t ret;
> +
> +	/* Hook up to the device list */
> +	efi_add_handle(handle);
> +
> +	/* Fill in EFI IO Media info (for read/write callbacks) */
> +	disk->media.removable_media = desc->removable;
> +	disk->media.media_present = 1;
> +	disk->media.block_size = desc->blksz;
> +	disk->media.io_align = desc->blksz;
> +	disk->media.last_block = desc->lba - disk->offset;
> +	disk->ops.media = &disk->media;
> +
> +	/* add protocols */
> +	disk->ops = block_io_disk_template;
> +	ret = efi_add_protocol(handle, &efi_block_io_guid, &disk->ops);
> +	if (ret != EFI_SUCCESS)
> +		goto err;
> +
> +	ret = efi_add_protocol(handle, &efi_guid_device_path, disk->dp);
> +	if (ret != EFI_SUCCESS)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	/* FIXME: undo adding protocols */
> +	return -1;
> +}
> +
> +/*
> + * Create a handle for a raw disk
> + *
> + * @dev		uclass device
> + * @return	0 on success, -1 otherwise
> + */
> +int efi_disk_create_raw(struct udevice *dev)
> +{
> +	struct blk_desc *desc = dev_get_uclass_platdata(dev);
> +	struct efi_disk_obj *disk = &desc->efi_disk;
> +
> +	/* Don't add empty devices */
> +	if (!desc->lba)
> +		return -1;
> +
> +	/* raw block device */
> +	disk->offset = 0;
> +	disk->part = 0;
> +	disk->dp = efi_dp_from_part(desc, 0);
> +
> +	/* efi IO media */
> +	disk->media.logical_partition = 0;
> +
> +	return efi_disk_create_common(&dev->efi_obj, disk, desc);
> +}
> +
> +/*
> + * Create a handle for a partition
> + *
> + * @dev		uclass device
> + * @return	0 on success, -1 otherwise
> + */
> +int efi_disk_create_part(struct udevice *dev)
> +{
> +	struct udevice *parent = dev->parent;
> +	struct blk_desc *desc = dev_get_uclass_platdata(parent);
> +	struct blk_desc *this;
> +	struct disk_part *pdata = dev_get_uclass_platdata(dev);
> +	struct efi_disk_obj *disk;
> +	struct efi_device_path *node;
> +
> +	int ret;
> +
> +	/* dummy block device */
> +	this = malloc(sizeof(*this));
> +	if (!this)
> +		return -1;
> +	disk = &this->efi_disk;
> +
> +	/* logical disk partition */
> +	disk->offset = pdata->gpt_part_info.start;
> +	disk->part = pdata->partnum;
> +
> +	node = efi_dp_part_node(desc, disk->part);
> +	disk->dp = efi_dp_append_node(desc->efi_disk.dp, node);
> +	efi_free_pool(node);
> +
> +	/* efi IO media */
> +	disk->media.logical_partition = 1;
> +
> +	ret = efi_disk_create_common(&dev->efi_obj, disk, desc);
> +	if (ret)
> +		goto err;
> +
> +	/* partition may support file system access */
> +	disk->volume = efi_simple_file_system(desc, disk->part, disk->dp);
> +	ret = efi_add_protocol(&dev->efi_obj,
> +			       &efi_simple_file_system_protocol_guid,
> +			       disk->volume);
> +	if (ret != EFI_SUCCESS)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	free(this);
> +	/* FIXME: undo create */
> +
> +	return -1;
> +}
> +
> +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);
> +	else if (id == UCLASS_PARTITION)
> +		return efi_disk_create_part(dev);
> +	else
> +		return -1;
> +}
> +#endif /* CONFIG_BLK */
>  
>  /*
>   * U-Boot doesn't have a list of all online disk devices. So when running our
> @@ -357,38 +461,10 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
>   */
>  efi_status_t efi_disk_register(void)
>  {
> +#ifndef CONFIG_BLK
>  	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;
> -	     uclass_next_device_check(&dev)) {
> -		struct blk_desc *desc = dev_get_uclass_platdata(dev);
> -		const char *if_typename = blk_get_if_type_name(desc->if_type);
> -
> -		/* Add block device for the full device */
> -		printf("Scanning disk %s...\n", dev->name);

Who cares for this output? If really needed make it debug().

> -		ret = efi_disk_add_dev(NULL, NULL, if_typename,
> -					desc, desc->devnum, 0, 0, &disk);
> -		if (ret == EFI_NOT_READY) {
> -			printf("Disk %s not ready\n", dev->name);

Who minds if it is a CD-ROM drive with no disk inserted? Debug() might
be adequate here.

> -			continue;
> -		}
> -		if (ret) {
> -			printf("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);
> -	}
> -#else
>  	int i, if_type;
>  
>  	/* Search for all available disk devices */
> @@ -435,8 +511,8 @@ efi_status_t efi_disk_register(void)
>  						 if_typename, i, devname);
>  		}
>  	}
> -#endif
>  	printf("Found %d disks\n", disks);

I would prefer this to be debug() or eliminated.

Best regards

Heinrich

> +#endif
>  
>  	return EFI_SUCCESS;
>  }
> 

[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[AKASHI Takahiro]

On Tue, Jan 29, 2019 at 11:33:31PM +0100, Heinrich Schuchardt wrote:
> On 1/29/19 3:59 AM, AKASHI Takahiro wrote:
> > efi_disk_create() will initialize efi_disk attributes for each device,
> > either UCLASS_BLK or UCLASS_PARTITION.
> > 
> > Currently (temporarily), efi_disk_obj structure is embedded into
> > blk_desc to hold efi-specific attributes.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  drivers/block/blk-uclass.c |   9 ++
> >  drivers/core/device.c      |   3 +
> >  include/blk.h              |  24 +++++
> >  include/dm/device.h        |   4 +
> >  lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
> >  5 files changed, 174 insertions(+), 58 deletions(-)
> > 
> > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> > index d4ca30f23fc1..28c45d724113 100644
> > --- a/drivers/block/blk-uclass.c
> > +++ b/drivers/block/blk-uclass.c
> > @@ -657,6 +657,9 @@ UCLASS_DRIVER(blk) = {
> >  	.per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
> >  };
> >  
> > +/* FIXME */
> > +extern int efi_disk_create(struct udevice *dev);
> > +
> >  U_BOOT_DRIVER(blk_partition) = {
> >  	.name		= "blk_partition",
> >  	.id		= UCLASS_PARTITION,
> > @@ -695,6 +698,12 @@ int blk_create_partitions(struct udevice *parent)
> >  		part_data->partnum = part;
> >  		part_data->gpt_part_info = info;
> >  
> > +		ret = efi_disk_create(dev);
> > +		if (ret) {
> > +			device_unbind(dev);
> > +			return ret;
> > +		}
> > +
> >  		disks++;
> >  	}
> >  
> > diff --git a/drivers/core/device.c b/drivers/core/device.c
> > index 0d15e5062b66..8625fccb0dcb 100644
> > --- a/drivers/core/device.c
> > +++ b/drivers/core/device.c
> > @@ -67,6 +67,9 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv,
> >  	dev->parent = parent;
> >  	dev->driver = drv;
> >  	dev->uclass = uc;
> > +#ifdef CONFIG_EFI_LOADER
> > +	INIT_LIST_HEAD(&dev->efi_obj.protocols);
> 
> This looks like an incomplete initialization of efi_obj. Why don't we
> use efi_create_handle.

I think that it is more or less a matter of implementation.
I will address this issue later if necessary.

> Why should a device be aware of struct efi_obj? We only need a handle to
> install protocols.
> 
> > +#endif
> >  
> >  	dev->seq = -1;
> >  	dev->req_seq = -1;
> > diff --git a/include/blk.h b/include/blk.h
> > index d0c033aece0f..405f6f790d66 100644
> > --- a/include/blk.h
> > +++ b/include/blk.h
> > @@ -8,6 +8,7 @@
> >  #define BLK_H
> >  
> >  #include <efi.h>
> > +#include <efi_api.h>
> >  
> >  #ifdef CONFIG_SYS_64BIT_LBA
> >  typedef uint64_t lbaint_t;
> > @@ -53,6 +54,26 @@ enum sig_type {
> >  	SIG_TYPE_COUNT			/* Number of signature types */
> >  };
> >  
> > +/* FIXME */
> 
> Fix what?

For simplicity, this data structure was copied from efi_disk.c
in this initial release.
As the implementation goes *sophisticated*, some members may go away
or move somewhere, say in blk_desc itself.

> > +/**
> > + * struct efi_disk_obj - EFI disk object
> > + *
> > + * @ops:	EFI disk I/O protocol interface
> > + * @media:	block I/O media information
> > + * @dp:		device path to the block device
> > + * @part:	partition
> > + * @volume:	simple file system protocol of the partition
> > + * @offset:	offset into disk for simple partition
> > + */
> > +struct efi_disk_obj {
> > +	struct efi_block_io ops;
> > +	struct efi_block_io_media media;
> > +	struct efi_device_path *dp;
> > +	unsigned int part;
> > +	struct efi_simple_file_system_protocol *volume;
> > +	lbaint_t offset;
> > +};
> > +
> >  /*
> >   * With driver model (CONFIG_BLK) this is uclass platform data, accessible
> >   * with dev_get_uclass_platdata(dev)
> > @@ -92,6 +113,9 @@ struct blk_desc {
> >  	 * device. Once these functions are removed we can drop this field.
> >  	 */
> >  	struct udevice *bdev;
> > +#ifdef CONFIG_EFI_LOADER
> > +	struct efi_disk_obj efi_disk;
> 
> This must be a handle (i.e. a pointer). Otherwise when the last protocol
> is removed we try to free memory that was never malloc'ed.

Who actually frees?

> > +#endif
> >  #else
> >  	unsigned long	(*block_read)(struct blk_desc *block_dev,
> >  				      lbaint_t start,
> > diff --git a/include/dm/device.h b/include/dm/device.h
> > index 27a6d7b9fdb0..121bfb46b1a0 100644
> > --- a/include/dm/device.h
> > +++ b/include/dm/device.h
> > @@ -12,6 +12,7 @@
> >  
> >  #include <dm/ofnode.h>
> >  #include <dm/uclass-id.h>
> > +#include <efi_loader.h>
> >  #include <fdtdec.h>
> >  #include <linker_lists.h>
> >  #include <linux/compat.h>
> > @@ -146,6 +147,9 @@ struct udevice {
> >  #ifdef CONFIG_DEVRES
> >  	struct list_head devres_head;
> >  #endif
> > +#ifdef CONFIG_EFI_LOADER
> > +	struct efi_object efi_obj;
> > +#endif
> >  };
> >  
> >  /* Maximum sequence number supported */
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index c037526ad2d0..84fa0ddfba14 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -14,33 +14,6 @@
> >  
> >  const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
> >  
> > -/**
> > - * struct efi_disk_obj - EFI disk object
> > - *
> > - * @header:	EFI object header
> > - * @ops:	EFI disk I/O protocol interface
> > - * @ifname:	interface name for block device
> > - * @dev_index:	device index of block device
> > - * @media:	block I/O media information
> > - * @dp:		device path to the block device
> > - * @part:	partition
> > - * @volume:	simple file system protocol of the partition
> > - * @offset:	offset into disk for simple partition
> > - * @desc:	internal block device descriptor
> > - */
> > -struct efi_disk_obj {
> > -	struct efi_object header;
> > -	struct efi_block_io ops;
> > -	const char *ifname;
> > -	int dev_index;
> > -	struct efi_block_io_media media;
> > -	struct efi_device_path *dp;
> > -	unsigned int part;
> > -	struct efi_simple_file_system_protocol *volume;
> > -	lbaint_t offset;
> > -	struct blk_desc *desc;
> > -};
> > -
> >  static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this,
> >  			char extended_verification)
> >  {
> > @@ -64,7 +37,7 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
> >  	unsigned long n;
> >  
> >  	diskobj = container_of(this, struct efi_disk_obj, ops);
> > -	desc = (struct blk_desc *) diskobj->desc;
> > +	desc = container_of(diskobj, struct blk_desc, efi_disk);
> >  	blksz = desc->blksz;
> >  	blocks = buffer_size / blksz;
> >  	lba += diskobj->offset;
> > @@ -217,6 +190,7 @@ efi_fs_from_path(struct efi_device_path *full_path)
> >  	return handler->protocol_interface;
> >  }
> >  
> > +#ifndef CONFIG_BLK
> >  /*
> >   * Create a handle for a partition or disk
> >   *
> > @@ -343,6 +317,136 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> >  
> >  	return disks;
> >  }
> > +#else
> > +static int efi_disk_create_common(efi_handle_t handle,
> > +				  struct efi_disk_obj *disk,
> > +				  struct blk_desc *desc)
> > +{
> > +	efi_status_t ret;
> > +
> > +	/* Hook up to the device list */
> > +	efi_add_handle(handle);
> > +
> > +	/* Fill in EFI IO Media info (for read/write callbacks) */
> > +	disk->media.removable_media = desc->removable;
> > +	disk->media.media_present = 1;
> > +	disk->media.block_size = desc->blksz;
> > +	disk->media.io_align = desc->blksz;
> > +	disk->media.last_block = desc->lba - disk->offset;
> > +	disk->ops.media = &disk->media;
> > +
> > +	/* add protocols */
> > +	disk->ops = block_io_disk_template;
> > +	ret = efi_add_protocol(handle, &efi_block_io_guid, &disk->ops);
> > +	if (ret != EFI_SUCCESS)
> > +		goto err;
> > +
> > +	ret = efi_add_protocol(handle, &efi_guid_device_path, disk->dp);
> > +	if (ret != EFI_SUCCESS)
> > +		goto err;
> > +
> > +	return 0;
> > +
> > +err:
> > +	/* FIXME: undo adding protocols */
> > +	return -1;
> > +}
> > +
> > +/*
> > + * Create a handle for a raw disk
> > + *
> > + * @dev		uclass device
> > + * @return	0 on success, -1 otherwise
> > + */
> > +int efi_disk_create_raw(struct udevice *dev)
> > +{
> > +	struct blk_desc *desc = dev_get_uclass_platdata(dev);
> > +	struct efi_disk_obj *disk = &desc->efi_disk;
> > +
> > +	/* Don't add empty devices */
> > +	if (!desc->lba)
> > +		return -1;
> > +
> > +	/* raw block device */
> > +	disk->offset = 0;
> > +	disk->part = 0;
> > +	disk->dp = efi_dp_from_part(desc, 0);
> > +
> > +	/* efi IO media */
> > +	disk->media.logical_partition = 0;
> > +
> > +	return efi_disk_create_common(&dev->efi_obj, disk, desc);
> > +}
> > +
> > +/*
> > + * Create a handle for a partition
> > + *
> > + * @dev		uclass device
> > + * @return	0 on success, -1 otherwise
> > + */
> > +int efi_disk_create_part(struct udevice *dev)
> > +{
> > +	struct udevice *parent = dev->parent;
> > +	struct blk_desc *desc = dev_get_uclass_platdata(parent);
> > +	struct blk_desc *this;
> > +	struct disk_part *pdata = dev_get_uclass_platdata(dev);
> > +	struct efi_disk_obj *disk;
> > +	struct efi_device_path *node;
> > +
> > +	int ret;
> > +
> > +	/* dummy block device */
> > +	this = malloc(sizeof(*this));
> > +	if (!this)
> > +		return -1;
> > +	disk = &this->efi_disk;
> > +
> > +	/* logical disk partition */
> > +	disk->offset = pdata->gpt_part_info.start;
> > +	disk->part = pdata->partnum;
> > +
> > +	node = efi_dp_part_node(desc, disk->part);
> > +	disk->dp = efi_dp_append_node(desc->efi_disk.dp, node);
> > +	efi_free_pool(node);
> > +
> > +	/* efi IO media */
> > +	disk->media.logical_partition = 1;
> > +
> > +	ret = efi_disk_create_common(&dev->efi_obj, disk, desc);
> > +	if (ret)
> > +		goto err;
> > +
> > +	/* partition may support file system access */
> > +	disk->volume = efi_simple_file_system(desc, disk->part, disk->dp);
> > +	ret = efi_add_protocol(&dev->efi_obj,
> > +			       &efi_simple_file_system_protocol_guid,
> > +			       disk->volume);
> > +	if (ret != EFI_SUCCESS)
> > +		goto err;
> > +
> > +	return 0;
> > +
> > +err:
> > +	free(this);
> > +	/* FIXME: undo create */
> > +
> > +	return -1;
> > +}
> > +
> > +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);
> > +	else if (id == UCLASS_PARTITION)
> > +		return efi_disk_create_part(dev);
> > +	else
> > +		return -1;
> > +}
> > +#endif /* CONFIG_BLK */
> >  
> >  /*
> >   * U-Boot doesn't have a list of all online disk devices. So when running our
> > @@ -357,38 +461,10 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> >   */
> >  efi_status_t efi_disk_register(void)
> >  {
> > +#ifndef CONFIG_BLK
> >  	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;
> > -	     uclass_next_device_check(&dev)) {
> > -		struct blk_desc *desc = dev_get_uclass_platdata(dev);
> > -		const char *if_typename = blk_get_if_type_name(desc->if_type);
> > -
> > -		/* Add block device for the full device */
> > -		printf("Scanning disk %s...\n", dev->name);
> 
> Who cares for this output? If really needed make it debug().

Please note that this is a line to be deleted.

> > -		ret = efi_disk_add_dev(NULL, NULL, if_typename,
> > -					desc, desc->devnum, 0, 0, &disk);
> > -		if (ret == EFI_NOT_READY) {
> > -			printf("Disk %s not ready\n", dev->name);
> 
> Who minds if it is a CD-ROM drive with no disk inserted? Debug() might
> be adequate here.

Ditto

> > -			continue;
> > -		}
> > -		if (ret) {
> > -			printf("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);
> > -	}
> > -#else
> >  	int i, if_type;
> >  
> >  	/* Search for all available disk devices */
> > @@ -435,8 +511,8 @@ efi_status_t efi_disk_register(void)
> >  						 if_typename, i, devname);
> >  		}
> >  	}
> > -#endif
> >  	printf("Found %d disks\n", disks);
> 
> I would prefer this to be debug() or eliminated.

I didn't change anything on this line and the function, efi_disk_register(),
will eventually go away.

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > +#endif
> >  
> >  	return EFI_SUCCESS;
> >  }
> > 
> 

[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[Heinrich Schuchardt]

On 1/30/19 6:48 AM, AKASHI Takahiro wrote:
> On Tue, Jan 29, 2019 at 11:33:31PM +0100, Heinrich Schuchardt wrote:
>> On 1/29/19 3:59 AM, AKASHI Takahiro wrote:
>>> efi_disk_create() will initialize efi_disk attributes for each device,
>>> either UCLASS_BLK or UCLASS_PARTITION.
>>>
>>> Currently (temporarily), efi_disk_obj structure is embedded into
>>> blk_desc to hold efi-specific attributes.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  drivers/block/blk-uclass.c |   9 ++
>>>  drivers/core/device.c      |   3 +
>>>  include/blk.h              |  24 +++++
>>>  include/dm/device.h        |   4 +
>>>  lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
>>>  5 files changed, 174 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
>>> index d4ca30f23fc1..28c45d724113 100644
>>> --- a/drivers/block/blk-uclass.c
>>> +++ b/drivers/block/blk-uclass.c
>>> @@ -657,6 +657,9 @@ UCLASS_DRIVER(blk) = {
>>>  	.per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
>>>  };
>>>  
>>> +/* FIXME */
>>> +extern int efi_disk_create(struct udevice *dev);
>>> +
>>>  U_BOOT_DRIVER(blk_partition) = {
>>>  	.name		= "blk_partition",
>>>  	.id		= UCLASS_PARTITION,
>>> @@ -695,6 +698,12 @@ int blk_create_partitions(struct udevice *parent)
>>>  		part_data->partnum = part;
>>>  		part_data->gpt_part_info = info;
>>>  
>>> +		ret = efi_disk_create(dev);
>>> +		if (ret) {
>>> +			device_unbind(dev);
>>> +			return ret;
>>> +		}
>>> +
>>>  		disks++;
>>>  	}
>>>  
>>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>>> index 0d15e5062b66..8625fccb0dcb 100644
>>> --- a/drivers/core/device.c
>>> +++ b/drivers/core/device.c
>>> @@ -67,6 +67,9 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv,
>>>  	dev->parent = parent;
>>>  	dev->driver = drv;
>>>  	dev->uclass = uc;
>>> +#ifdef CONFIG_EFI_LOADER
>>> +	INIT_LIST_HEAD(&dev->efi_obj.protocols);
>>
>> This looks like an incomplete initialization of efi_obj. Why don't we
>> use efi_create_handle.
> 
> I think that it is more or less a matter of implementation.
> I will address this issue later if necessary.
> 
>> Why should a device be aware of struct efi_obj? We only need a handle to
>> install protocols.
>>
>>> +#endif
>>>  
>>>  	dev->seq = -1;
>>>  	dev->req_seq = -1;
>>> diff --git a/include/blk.h b/include/blk.h
>>> index d0c033aece0f..405f6f790d66 100644
>>> --- a/include/blk.h
>>> +++ b/include/blk.h
>>> @@ -8,6 +8,7 @@
>>>  #define BLK_H
>>>  
>>>  #include <efi.h>
>>> +#include <efi_api.h>
>>>  
>>>  #ifdef CONFIG_SYS_64BIT_LBA
>>>  typedef uint64_t lbaint_t;
>>> @@ -53,6 +54,26 @@ enum sig_type {
>>>  	SIG_TYPE_COUNT			/* Number of signature types */
>>>  };
>>>  
>>> +/* FIXME */
>>
>> Fix what?
> 
> For simplicity, this data structure was copied from efi_disk.c
> in this initial release.
> As the implementation goes *sophisticated*, some members may go away
> or move somewhere, say in blk_desc itself.
> 
>>> +/**
>>> + * struct efi_disk_obj - EFI disk object
>>> + *
>>> + * @ops:	EFI disk I/O protocol interface
>>> + * @media:	block I/O media information
>>> + * @dp:		device path to the block device
>>> + * @part:	partition
>>> + * @volume:	simple file system protocol of the partition
>>> + * @offset:	offset into disk for simple partition
>>> + */
>>> +struct efi_disk_obj {
>>> +	struct efi_block_io ops;
>>> +	struct efi_block_io_media media;
>>> +	struct efi_device_path *dp;
>>> +	unsigned int part;
>>> +	struct efi_simple_file_system_protocol *volume;
>>> +	lbaint_t offset;
>>> +};
>>> +
>>>  /*
>>>   * With driver model (CONFIG_BLK) this is uclass platform data, accessible
>>>   * with dev_get_uclass_platdata(dev)
>>> @@ -92,6 +113,9 @@ struct blk_desc {
>>>  	 * device. Once these functions are removed we can drop this field.
>>>  	 */
>>>  	struct udevice *bdev;
>>> +#ifdef CONFIG_EFI_LOADER
>>> +	struct efi_disk_obj efi_disk;
>>
>> This must be a handle (i.e. a pointer). Otherwise when the last protocol
>> is removed we try to free memory that was never malloc'ed.
> 
> Who actually frees?

see UEFI spec for UninstallProtocolInterface():

"If the last protocol interface is removed from a handle, the handle is
freed and is no longer valid."

Best regards

Heinrich

> 
>>> +#endif
>>>  #else
>>>  	unsigned long	(*block_read)(struct blk_desc *block_dev,
>>>  				      lbaint_t start,
>>> diff --git a/include/dm/device.h b/include/dm/device.h
>>> index 27a6d7b9fdb0..121bfb46b1a0 100644
>>> --- a/include/dm/device.h
>>> +++ b/include/dm/device.h
>>> @@ -12,6 +12,7 @@
>>>  
>>>  #include <dm/ofnode.h>
>>>  #include <dm/uclass-id.h>
>>> +#include <efi_loader.h>
>>>  #include <fdtdec.h>
>>>  #include <linker_lists.h>
>>>  #include <linux/compat.h>
>>> @@ -146,6 +147,9 @@ struct udevice {
>>>  #ifdef CONFIG_DEVRES
>>>  	struct list_head devres_head;
>>>  #endif
>>> +#ifdef CONFIG_EFI_LOADER
>>> +	struct efi_object efi_obj;
>>> +#endif
>>>  };
>>>  
>>>  /* Maximum sequence number supported */
>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>>> index c037526ad2d0..84fa0ddfba14 100644
>>> --- a/lib/efi_loader/efi_disk.c
>>> +++ b/lib/efi_loader/efi_disk.c
>>> @@ -14,33 +14,6 @@
>>>  
>>>  const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
>>>  
>>> -/**
>>> - * struct efi_disk_obj - EFI disk object
>>> - *
>>> - * @header:	EFI object header
>>> - * @ops:	EFI disk I/O protocol interface
>>> - * @ifname:	interface name for block device
>>> - * @dev_index:	device index of block device
>>> - * @media:	block I/O media information
>>> - * @dp:		device path to the block device
>>> - * @part:	partition
>>> - * @volume:	simple file system protocol of the partition
>>> - * @offset:	offset into disk for simple partition
>>> - * @desc:	internal block device descriptor
>>> - */
>>> -struct efi_disk_obj {
>>> -	struct efi_object header;
>>> -	struct efi_block_io ops;
>>> -	const char *ifname;
>>> -	int dev_index;
>>> -	struct efi_block_io_media media;
>>> -	struct efi_device_path *dp;
>>> -	unsigned int part;
>>> -	struct efi_simple_file_system_protocol *volume;
>>> -	lbaint_t offset;
>>> -	struct blk_desc *desc;
>>> -};
>>> -
>>>  static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this,
>>>  			char extended_verification)
>>>  {
>>> @@ -64,7 +37,7 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
>>>  	unsigned long n;
>>>  
>>>  	diskobj = container_of(this, struct efi_disk_obj, ops);
>>> -	desc = (struct blk_desc *) diskobj->desc;
>>> +	desc = container_of(diskobj, struct blk_desc, efi_disk);
>>>  	blksz = desc->blksz;
>>>  	blocks = buffer_size / blksz;
>>>  	lba += diskobj->offset;
>>> @@ -217,6 +190,7 @@ efi_fs_from_path(struct efi_device_path *full_path)
>>>  	return handler->protocol_interface;
>>>  }
>>>  
>>> +#ifndef CONFIG_BLK
>>>  /*
>>>   * Create a handle for a partition or disk
>>>   *
>>> @@ -343,6 +317,136 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
>>>  
>>>  	return disks;
>>>  }
>>> +#else
>>> +static int efi_disk_create_common(efi_handle_t handle,
>>> +				  struct efi_disk_obj *disk,
>>> +				  struct blk_desc *desc)
>>> +{
>>> +	efi_status_t ret;
>>> +
>>> +	/* Hook up to the device list */
>>> +	efi_add_handle(handle);
>>> +
>>> +	/* Fill in EFI IO Media info (for read/write callbacks) */
>>> +	disk->media.removable_media = desc->removable;
>>> +	disk->media.media_present = 1;
>>> +	disk->media.block_size = desc->blksz;
>>> +	disk->media.io_align = desc->blksz;
>>> +	disk->media.last_block = desc->lba - disk->offset;
>>> +	disk->ops.media = &disk->media;
>>> +
>>> +	/* add protocols */
>>> +	disk->ops = block_io_disk_template;
>>> +	ret = efi_add_protocol(handle, &efi_block_io_guid, &disk->ops);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto err;
>>> +
>>> +	ret = efi_add_protocol(handle, &efi_guid_device_path, disk->dp);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto err;
>>> +
>>> +	return 0;
>>> +
>>> +err:
>>> +	/* FIXME: undo adding protocols */
>>> +	return -1;
>>> +}
>>> +
>>> +/*
>>> + * Create a handle for a raw disk
>>> + *
>>> + * @dev		uclass device
>>> + * @return	0 on success, -1 otherwise
>>> + */
>>> +int efi_disk_create_raw(struct udevice *dev)
>>> +{
>>> +	struct blk_desc *desc = dev_get_uclass_platdata(dev);
>>> +	struct efi_disk_obj *disk = &desc->efi_disk;
>>> +
>>> +	/* Don't add empty devices */
>>> +	if (!desc->lba)
>>> +		return -1;
>>> +
>>> +	/* raw block device */
>>> +	disk->offset = 0;
>>> +	disk->part = 0;
>>> +	disk->dp = efi_dp_from_part(desc, 0);
>>> +
>>> +	/* efi IO media */
>>> +	disk->media.logical_partition = 0;
>>> +
>>> +	return efi_disk_create_common(&dev->efi_obj, disk, desc);
>>> +}
>>> +
>>> +/*
>>> + * Create a handle for a partition
>>> + *
>>> + * @dev		uclass device
>>> + * @return	0 on success, -1 otherwise
>>> + */
>>> +int efi_disk_create_part(struct udevice *dev)
>>> +{
>>> +	struct udevice *parent = dev->parent;
>>> +	struct blk_desc *desc = dev_get_uclass_platdata(parent);
>>> +	struct blk_desc *this;
>>> +	struct disk_part *pdata = dev_get_uclass_platdata(dev);
>>> +	struct efi_disk_obj *disk;
>>> +	struct efi_device_path *node;
>>> +
>>> +	int ret;
>>> +
>>> +	/* dummy block device */
>>> +	this = malloc(sizeof(*this));
>>> +	if (!this)
>>> +		return -1;
>>> +	disk = &this->efi_disk;
>>> +
>>> +	/* logical disk partition */
>>> +	disk->offset = pdata->gpt_part_info.start;
>>> +	disk->part = pdata->partnum;
>>> +
>>> +	node = efi_dp_part_node(desc, disk->part);
>>> +	disk->dp = efi_dp_append_node(desc->efi_disk.dp, node);
>>> +	efi_free_pool(node);
>>> +
>>> +	/* efi IO media */
>>> +	disk->media.logical_partition = 1;
>>> +
>>> +	ret = efi_disk_create_common(&dev->efi_obj, disk, desc);
>>> +	if (ret)
>>> +		goto err;
>>> +
>>> +	/* partition may support file system access */
>>> +	disk->volume = efi_simple_file_system(desc, disk->part, disk->dp);
>>> +	ret = efi_add_protocol(&dev->efi_obj,
>>> +			       &efi_simple_file_system_protocol_guid,
>>> +			       disk->volume);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto err;
>>> +
>>> +	return 0;
>>> +
>>> +err:
>>> +	free(this);
>>> +	/* FIXME: undo create */
>>> +
>>> +	return -1;
>>> +}
>>> +
>>> +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);
>>> +	else if (id == UCLASS_PARTITION)
>>> +		return efi_disk_create_part(dev);
>>> +	else
>>> +		return -1;
>>> +}
>>> +#endif /* CONFIG_BLK */
>>>  
>>>  /*
>>>   * U-Boot doesn't have a list of all online disk devices. So when running our
>>> @@ -357,38 +461,10 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
>>>   */
>>>  efi_status_t efi_disk_register(void)
>>>  {
>>> +#ifndef CONFIG_BLK
>>>  	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;
>>> -	     uclass_next_device_check(&dev)) {
>>> -		struct blk_desc *desc = dev_get_uclass_platdata(dev);
>>> -		const char *if_typename = blk_get_if_type_name(desc->if_type);
>>> -
>>> -		/* Add block device for the full device */
>>> -		printf("Scanning disk %s...\n", dev->name);
>>
>> Who cares for this output? If really needed make it debug().
> 
> Please note that this is a line to be deleted.
> 
>>> -		ret = efi_disk_add_dev(NULL, NULL, if_typename,
>>> -					desc, desc->devnum, 0, 0, &disk);
>>> -		if (ret == EFI_NOT_READY) {
>>> -			printf("Disk %s not ready\n", dev->name);
>>
>> Who minds if it is a CD-ROM drive with no disk inserted? Debug() might
>> be adequate here.
> 
> Ditto
> 
>>> -			continue;
>>> -		}
>>> -		if (ret) {
>>> -			printf("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);
>>> -	}
>>> -#else
>>>  	int i, if_type;
>>>  
>>>  	/* Search for all available disk devices */
>>> @@ -435,8 +511,8 @@ efi_status_t efi_disk_register(void)
>>>  						 if_typename, i, devname);
>>>  		}
>>>  	}
>>> -#endif
>>>  	printf("Found %d disks\n", disks);
>>
>> I would prefer this to be debug() or eliminated.
> 
> I didn't change anything on this line and the function, efi_disk_register(),
> will eventually go away.
> 
> Thanks,
> -Takahiro Akashi
> 
> 
>> Best regards
>>
>> Heinrich
>>
>>> +#endif
>>>  
>>>  	return EFI_SUCCESS;
>>>  }
>>>
>>
> 

[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[AKASHI Takahiro]

On Wed, Jan 30, 2019 at 07:49:37AM +0100, Heinrich Schuchardt wrote:
> On 1/30/19 6:48 AM, AKASHI Takahiro wrote:
> > On Tue, Jan 29, 2019 at 11:33:31PM +0100, Heinrich Schuchardt wrote:
> >> On 1/29/19 3:59 AM, AKASHI Takahiro wrote:
> >>> efi_disk_create() will initialize efi_disk attributes for each device,
> >>> either UCLASS_BLK or UCLASS_PARTITION.
> >>>
> >>> Currently (temporarily), efi_disk_obj structure is embedded into
> >>> blk_desc to hold efi-specific attributes.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  drivers/block/blk-uclass.c |   9 ++
> >>>  drivers/core/device.c      |   3 +
> >>>  include/blk.h              |  24 +++++
> >>>  include/dm/device.h        |   4 +
> >>>  lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
> >>>  5 files changed, 174 insertions(+), 58 deletions(-)
> >>>
> >>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> >>> index d4ca30f23fc1..28c45d724113 100644
> >>> --- a/drivers/block/blk-uclass.c
> >>> +++ b/drivers/block/blk-uclass.c
> >>> @@ -657,6 +657,9 @@ UCLASS_DRIVER(blk) = {
> >>>  	.per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
> >>>  };
> >>>  
> >>> +/* FIXME */
> >>> +extern int efi_disk_create(struct udevice *dev);
> >>> +
> >>>  U_BOOT_DRIVER(blk_partition) = {
> >>>  	.name		= "blk_partition",
> >>>  	.id		= UCLASS_PARTITION,
> >>> @@ -695,6 +698,12 @@ int blk_create_partitions(struct udevice *parent)
> >>>  		part_data->partnum = part;
> >>>  		part_data->gpt_part_info = info;
> >>>  
> >>> +		ret = efi_disk_create(dev);
> >>> +		if (ret) {
> >>> +			device_unbind(dev);
> >>> +			return ret;
> >>> +		}
> >>> +
> >>>  		disks++;
> >>>  	}
> >>>  
> >>> diff --git a/drivers/core/device.c b/drivers/core/device.c
> >>> index 0d15e5062b66..8625fccb0dcb 100644
> >>> --- a/drivers/core/device.c
> >>> +++ b/drivers/core/device.c
> >>> @@ -67,6 +67,9 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv,
> >>>  	dev->parent = parent;
> >>>  	dev->driver = drv;
> >>>  	dev->uclass = uc;
> >>> +#ifdef CONFIG_EFI_LOADER
> >>> +	INIT_LIST_HEAD(&dev->efi_obj.protocols);
> >>
> >> This looks like an incomplete initialization of efi_obj. Why don't we
> >> use efi_create_handle.
> > 
> > I think that it is more or less a matter of implementation.
> > I will address this issue later if necessary.
> > 
> >> Why should a device be aware of struct efi_obj? We only need a handle to
> >> install protocols.
> >>
> >>> +#endif
> >>>  
> >>>  	dev->seq = -1;
> >>>  	dev->req_seq = -1;
> >>> diff --git a/include/blk.h b/include/blk.h
> >>> index d0c033aece0f..405f6f790d66 100644
> >>> --- a/include/blk.h
> >>> +++ b/include/blk.h
> >>> @@ -8,6 +8,7 @@
> >>>  #define BLK_H
> >>>  
> >>>  #include <efi.h>
> >>> +#include <efi_api.h>
> >>>  
> >>>  #ifdef CONFIG_SYS_64BIT_LBA
> >>>  typedef uint64_t lbaint_t;
> >>> @@ -53,6 +54,26 @@ enum sig_type {
> >>>  	SIG_TYPE_COUNT			/* Number of signature types */
> >>>  };
> >>>  
> >>> +/* FIXME */
> >>
> >> Fix what?
> > 
> > For simplicity, this data structure was copied from efi_disk.c
> > in this initial release.
> > As the implementation goes *sophisticated*, some members may go away
> > or move somewhere, say in blk_desc itself.
> > 
> >>> +/**
> >>> + * struct efi_disk_obj - EFI disk object
> >>> + *
> >>> + * @ops:	EFI disk I/O protocol interface
> >>> + * @media:	block I/O media information
> >>> + * @dp:		device path to the block device
> >>> + * @part:	partition
> >>> + * @volume:	simple file system protocol of the partition
> >>> + * @offset:	offset into disk for simple partition
> >>> + */
> >>> +struct efi_disk_obj {
> >>> +	struct efi_block_io ops;
> >>> +	struct efi_block_io_media media;
> >>> +	struct efi_device_path *dp;
> >>> +	unsigned int part;
> >>> +	struct efi_simple_file_system_protocol *volume;
> >>> +	lbaint_t offset;
> >>> +};
> >>> +
> >>>  /*
> >>>   * With driver model (CONFIG_BLK) this is uclass platform data, accessible
> >>>   * with dev_get_uclass_platdata(dev)
> >>> @@ -92,6 +113,9 @@ struct blk_desc {
> >>>  	 * device. Once these functions are removed we can drop this field.
> >>>  	 */
> >>>  	struct udevice *bdev;
> >>> +#ifdef CONFIG_EFI_LOADER
> >>> +	struct efi_disk_obj efi_disk;
> >>
> >> This must be a handle (i.e. a pointer). Otherwise when the last protocol
> >> is removed we try to free memory that was never malloc'ed.
> > 
> > Who actually frees?
> 
> see UEFI spec for UninstallProtocolInterface():
> 
> "If the last protocol interface is removed from a handle, the handle is
> freed and is no longer valid."

Got it.
So, under the current implementation, any efi_object must be allocated
by efi code itself and all derived efi objects have an efi_object
as the first member.
(We can lift this restriction by adding object-specific "free" function,
like calling (handle->free)(handle) instead of free(handle) though.)

Umm. This will make it a bit difficult to remove efi_disk_obj from
our code.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> >>> +#endif
> >>>  #else
> >>>  	unsigned long	(*block_read)(struct blk_desc *block_dev,
> >>>  				      lbaint_t start,
> >>> diff --git a/include/dm/device.h b/include/dm/device.h
> >>> index 27a6d7b9fdb0..121bfb46b1a0 100644
> >>> --- a/include/dm/device.h
> >>> +++ b/include/dm/device.h
> >>> @@ -12,6 +12,7 @@
> >>>  
> >>>  #include <dm/ofnode.h>
> >>>  #include <dm/uclass-id.h>
> >>> +#include <efi_loader.h>
> >>>  #include <fdtdec.h>
> >>>  #include <linker_lists.h>
> >>>  #include <linux/compat.h>
> >>> @@ -146,6 +147,9 @@ struct udevice {
> >>>  #ifdef CONFIG_DEVRES
> >>>  	struct list_head devres_head;
> >>>  #endif
> >>> +#ifdef CONFIG_EFI_LOADER
> >>> +	struct efi_object efi_obj;
> >>> +#endif
> >>>  };
> >>>  
> >>>  /* Maximum sequence number supported */
> >>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >>> index c037526ad2d0..84fa0ddfba14 100644
> >>> --- a/lib/efi_loader/efi_disk.c
> >>> +++ b/lib/efi_loader/efi_disk.c
> >>> @@ -14,33 +14,6 @@
> >>>  
> >>>  const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
> >>>  
> >>> -/**
> >>> - * struct efi_disk_obj - EFI disk object
> >>> - *
> >>> - * @header:	EFI object header
> >>> - * @ops:	EFI disk I/O protocol interface
> >>> - * @ifname:	interface name for block device
> >>> - * @dev_index:	device index of block device
> >>> - * @media:	block I/O media information
> >>> - * @dp:		device path to the block device
> >>> - * @part:	partition
> >>> - * @volume:	simple file system protocol of the partition
> >>> - * @offset:	offset into disk for simple partition
> >>> - * @desc:	internal block device descriptor
> >>> - */
> >>> -struct efi_disk_obj {
> >>> -	struct efi_object header;
> >>> -	struct efi_block_io ops;
> >>> -	const char *ifname;
> >>> -	int dev_index;
> >>> -	struct efi_block_io_media media;
> >>> -	struct efi_device_path *dp;
> >>> -	unsigned int part;
> >>> -	struct efi_simple_file_system_protocol *volume;
> >>> -	lbaint_t offset;
> >>> -	struct blk_desc *desc;
> >>> -};
> >>> -
> >>>  static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this,
> >>>  			char extended_verification)
> >>>  {
> >>> @@ -64,7 +37,7 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
> >>>  	unsigned long n;
> >>>  
> >>>  	diskobj = container_of(this, struct efi_disk_obj, ops);
> >>> -	desc = (struct blk_desc *) diskobj->desc;
> >>> +	desc = container_of(diskobj, struct blk_desc, efi_disk);
> >>>  	blksz = desc->blksz;
> >>>  	blocks = buffer_size / blksz;
> >>>  	lba += diskobj->offset;
> >>> @@ -217,6 +190,7 @@ efi_fs_from_path(struct efi_device_path *full_path)
> >>>  	return handler->protocol_interface;
> >>>  }
> >>>  
> >>> +#ifndef CONFIG_BLK
> >>>  /*
> >>>   * Create a handle for a partition or disk
> >>>   *
> >>> @@ -343,6 +317,136 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> >>>  
> >>>  	return disks;
> >>>  }
> >>> +#else
> >>> +static int efi_disk_create_common(efi_handle_t handle,
> >>> +				  struct efi_disk_obj *disk,
> >>> +				  struct blk_desc *desc)
> >>> +{
> >>> +	efi_status_t ret;
> >>> +
> >>> +	/* Hook up to the device list */
> >>> +	efi_add_handle(handle);
> >>> +
> >>> +	/* Fill in EFI IO Media info (for read/write callbacks) */
> >>> +	disk->media.removable_media = desc->removable;
> >>> +	disk->media.media_present = 1;
> >>> +	disk->media.block_size = desc->blksz;
> >>> +	disk->media.io_align = desc->blksz;
> >>> +	disk->media.last_block = desc->lba - disk->offset;
> >>> +	disk->ops.media = &disk->media;
> >>> +
> >>> +	/* add protocols */
> >>> +	disk->ops = block_io_disk_template;
> >>> +	ret = efi_add_protocol(handle, &efi_block_io_guid, &disk->ops);
> >>> +	if (ret != EFI_SUCCESS)
> >>> +		goto err;
> >>> +
> >>> +	ret = efi_add_protocol(handle, &efi_guid_device_path, disk->dp);
> >>> +	if (ret != EFI_SUCCESS)
> >>> +		goto err;
> >>> +
> >>> +	return 0;
> >>> +
> >>> +err:
> >>> +	/* FIXME: undo adding protocols */
> >>> +	return -1;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Create a handle for a raw disk
> >>> + *
> >>> + * @dev		uclass device
> >>> + * @return	0 on success, -1 otherwise
> >>> + */
> >>> +int efi_disk_create_raw(struct udevice *dev)
> >>> +{
> >>> +	struct blk_desc *desc = dev_get_uclass_platdata(dev);
> >>> +	struct efi_disk_obj *disk = &desc->efi_disk;
> >>> +
> >>> +	/* Don't add empty devices */
> >>> +	if (!desc->lba)
> >>> +		return -1;
> >>> +
> >>> +	/* raw block device */
> >>> +	disk->offset = 0;
> >>> +	disk->part = 0;
> >>> +	disk->dp = efi_dp_from_part(desc, 0);
> >>> +
> >>> +	/* efi IO media */
> >>> +	disk->media.logical_partition = 0;
> >>> +
> >>> +	return efi_disk_create_common(&dev->efi_obj, disk, desc);
> >>> +}
> >>> +
> >>> +/*
> >>> + * Create a handle for a partition
> >>> + *
> >>> + * @dev		uclass device
> >>> + * @return	0 on success, -1 otherwise
> >>> + */
> >>> +int efi_disk_create_part(struct udevice *dev)
> >>> +{
> >>> +	struct udevice *parent = dev->parent;
> >>> +	struct blk_desc *desc = dev_get_uclass_platdata(parent);
> >>> +	struct blk_desc *this;
> >>> +	struct disk_part *pdata = dev_get_uclass_platdata(dev);
> >>> +	struct efi_disk_obj *disk;
> >>> +	struct efi_device_path *node;
> >>> +
> >>> +	int ret;
> >>> +
> >>> +	/* dummy block device */
> >>> +	this = malloc(sizeof(*this));
> >>> +	if (!this)
> >>> +		return -1;
> >>> +	disk = &this->efi_disk;
> >>> +
> >>> +	/* logical disk partition */
> >>> +	disk->offset = pdata->gpt_part_info.start;
> >>> +	disk->part = pdata->partnum;
> >>> +
> >>> +	node = efi_dp_part_node(desc, disk->part);
> >>> +	disk->dp = efi_dp_append_node(desc->efi_disk.dp, node);
> >>> +	efi_free_pool(node);
> >>> +
> >>> +	/* efi IO media */
> >>> +	disk->media.logical_partition = 1;
> >>> +
> >>> +	ret = efi_disk_create_common(&dev->efi_obj, disk, desc);
> >>> +	if (ret)
> >>> +		goto err;
> >>> +
> >>> +	/* partition may support file system access */
> >>> +	disk->volume = efi_simple_file_system(desc, disk->part, disk->dp);
> >>> +	ret = efi_add_protocol(&dev->efi_obj,
> >>> +			       &efi_simple_file_system_protocol_guid,
> >>> +			       disk->volume);
> >>> +	if (ret != EFI_SUCCESS)
> >>> +		goto err;
> >>> +
> >>> +	return 0;
> >>> +
> >>> +err:
> >>> +	free(this);
> >>> +	/* FIXME: undo create */
> >>> +
> >>> +	return -1;
> >>> +}
> >>> +
> >>> +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);
> >>> +	else if (id == UCLASS_PARTITION)
> >>> +		return efi_disk_create_part(dev);
> >>> +	else
> >>> +		return -1;
> >>> +}
> >>> +#endif /* CONFIG_BLK */
> >>>  
> >>>  /*
> >>>   * U-Boot doesn't have a list of all online disk devices. So when running our
> >>> @@ -357,38 +461,10 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> >>>   */
> >>>  efi_status_t efi_disk_register(void)
> >>>  {
> >>> +#ifndef CONFIG_BLK
> >>>  	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;
> >>> -	     uclass_next_device_check(&dev)) {
> >>> -		struct blk_desc *desc = dev_get_uclass_platdata(dev);
> >>> -		const char *if_typename = blk_get_if_type_name(desc->if_type);
> >>> -
> >>> -		/* Add block device for the full device */
> >>> -		printf("Scanning disk %s...\n", dev->name);
> >>
> >> Who cares for this output? If really needed make it debug().
> > 
> > Please note that this is a line to be deleted.
> > 
> >>> -		ret = efi_disk_add_dev(NULL, NULL, if_typename,
> >>> -					desc, desc->devnum, 0, 0, &disk);
> >>> -		if (ret == EFI_NOT_READY) {
> >>> -			printf("Disk %s not ready\n", dev->name);
> >>
> >> Who minds if it is a CD-ROM drive with no disk inserted? Debug() might
> >> be adequate here.
> > 
> > Ditto
> > 
> >>> -			continue;
> >>> -		}
> >>> -		if (ret) {
> >>> -			printf("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);
> >>> -	}
> >>> -#else
> >>>  	int i, if_type;
> >>>  
> >>>  	/* Search for all available disk devices */
> >>> @@ -435,8 +511,8 @@ efi_status_t efi_disk_register(void)
> >>>  						 if_typename, i, devname);
> >>>  		}
> >>>  	}
> >>> -#endif
> >>>  	printf("Found %d disks\n", disks);
> >>
> >> I would prefer this to be debug() or eliminated.
> > 
> > I didn't change anything on this line and the function, efi_disk_register(),
> > will eventually go away.
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> > 
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +#endif
> >>>  
> >>>  	return EFI_SUCCESS;
> >>>  }
> >>>
> >>
> > 
> 

[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[Simon Glass]

Hi AKASHI,

On Mon, 28 Jan 2019 at 19:59, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> efi_disk_create() will initialize efi_disk attributes for each device,
> either UCLASS_BLK or UCLASS_PARTITION.
>
> Currently (temporarily), efi_disk_obj structure is embedded into
> blk_desc to hold efi-specific attributes.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  drivers/block/blk-uclass.c |   9 ++
>  drivers/core/device.c      |   3 +
>  include/blk.h              |  24 +++++
>  include/dm/device.h        |   4 +
>  lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
>  5 files changed, 174 insertions(+), 58 deletions(-)
>

I think the objective should be to drop the EFI data structures.

So your approach of just including them in DM structures seems about
right to me, as a short-term migration measure. Given the large amount
of code that has built up I don't think it is possible to do it any
other way.

It is very unfortunate though.

So basically migration could be something like this:

1. Move each of the EFI structs into the DM structs one by one
2. Drop struct members that are not needed and can be calculated from DM members
3. Update DM to have 1:1 uclasses for each EFI protocol
4. Move all the protocol structs into the DM uclasses
5. Whittle these down until they are just shells used by the API, with
everything going through normal DM calls

Or would it be better to just start again and rewrite it with the
existing code as a base?

Looking at it, efi_object is not very useful in DM. It contains two members:

1. link - linked list link, which devices already have, although we
don't have a single list of all them. Instead they are linked into
separate lists for each uclass

2. protocols - list of protocols. But DM devices support only one
protocol. Multiple protocols are handled using child devices. E.g a
UCLASS_PMIC device that supports UCLASS_GPIO, UCLASS_REGULATOR and
UCLASS_AUDIO_CODEC would have three children, one for each uclass. So
perhaps with EFI we should create a separate child for each protocol
in a similar way?

Which comes back to the idea of creating an EFI child device for every
DM device. Perhaps instead we create one EFI child for each protocol
supported by the parent device?

Another point is that the operations supported by EFI should be in DM
operations structs. For example I think struct
efi_simple_text_output_protocol should have methods which call into
the corresponding uclass operations.

It is confusing that an EFI disk is in fact a partition. Or do I have
that wrong?

Anyway that's all the thoughts I have at present. Thanks for your
efforts on this.

Regards,
Simon

[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[AKASHI Takahiro]

Hi Simon,

Thank you for suggestive comments.
I've got no idea of making DM class for EFI protocol.

On Wed, Jan 30, 2019 at 06:22:47PM -0700, Simon Glass wrote:
> Hi AKASHI,
> 
> On Mon, 28 Jan 2019 at 19:59, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > efi_disk_create() will initialize efi_disk attributes for each device,
> > either UCLASS_BLK or UCLASS_PARTITION.
> >
> > Currently (temporarily), efi_disk_obj structure is embedded into
> > blk_desc to hold efi-specific attributes.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  drivers/block/blk-uclass.c |   9 ++
> >  drivers/core/device.c      |   3 +
> >  include/blk.h              |  24 +++++
> >  include/dm/device.h        |   4 +
> >  lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
> >  5 files changed, 174 insertions(+), 58 deletions(-)
> >
> 
> I think the objective should be to drop the EFI data structures.

More or less so, yes.

> So your approach of just including them in DM structures seems about
> right to me, as a short-term migration measure. Given the large amount
> of code that has built up I don't think it is possible to do it any
> other way.

Right.

> It is very unfortunate though.
> 
> So basically migration could be something like this:
> 
> 1. Move each of the EFI structs into the DM structs one by one
> 2. Drop struct members that are not needed and can be calculated from DM members
> 3. Update DM to have 1:1 uclasses for each EFI protocol
> 4. Move all the protocol structs into the DM uclasses
> 5. Whittle these down until they are just shells used by the API, with
> everything going through normal DM calls
> 
> Or would it be better to just start again and rewrite it with the
> existing code as a base?
> 
> Looking at it, efi_object is not very useful in DM. It contains two members:
> 
> 1. link - linked list link, which devices already have, although we
> don't have a single list of all them. Instead they are linked into
> separate lists for each uclass
>
> 2. protocols - list of protocols. But DM devices support only one
> protocol. Multiple protocols are handled using child devices. E.g a
> UCLASS_PMIC device that supports UCLASS_GPIO, UCLASS_REGULATOR and
> UCLASS_AUDIO_CODEC would have three children, one for each uclass. So
> perhaps with EFI we should create a separate child for each protocol
> in a similar way?
> 
> Which comes back to the idea of creating an EFI child device for every
> DM device. Perhaps instead we create one EFI child for each protocol
> supported by the parent device?

Well, "child device as a EFI protocol" is really workable, but
I have some concerns:
* Can a DM device be an abstract instance with no real hardware?
* There may be two different types of "children" mixed for an EFI object
   - some physical hierarchy, say disk partitions for a raw disk
   - these EFI protocols
  That is, for example, one raw disk has
         - partition 1 has
                 - block io protocol
                 - device path protocol
                 - simple file system protocol
         - partition 2 has
                 - block io protocol
                 - device path protocol
                 - simple file system protocol
         - block io protocol
         - device path protocol
* device path protocol can be annoying as almost all devices (visible
  to UEFI) have some sort of device path, while corresponding u-boot
  notion is, say, "scsi 0:1" which only appears on command interfaces.

I'm not sure if those concerns are acceptable.

> Another point is that the operations supported by EFI should be in DM
> operations structs. For example I think struct
> efi_simple_text_output_protocol should have methods which call into
> the corresponding uclass operations.

I have no idea on those "console" devices yet.

> It is confusing that an EFI disk is in fact a partition. Or do I have
> that wrong?

IMO, EFI disk is any type of EFI object which supports EFI_BLOCK_IO_PROTOCOL.
So a raw disk(UCLASS_BLK) as well as its partitions(UCLASS_PARTITION) are
EFI disks as well.
Is this an answer to you?

Those said, your suggestion is truly worth considering.

Thanks,
-Takahiro Akashi


> Anyway that's all the thoughts I have at present. Thanks for your
> efforts on this.
> 
> Regards,
> Simon

[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[Simon Glass]

Hi,

On Thu, 31 Jan 2019 at 22:53, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Hi Simon,
>
> Thank you for suggestive comments.
> I've got no idea of making DM class for EFI protocol.
>
> On Wed, Jan 30, 2019 at 06:22:47PM -0700, Simon Glass wrote:
> > Hi AKASHI,
> >
> > On Mon, 28 Jan 2019 at 19:59, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > efi_disk_create() will initialize efi_disk attributes for each device,
> > > either UCLASS_BLK or UCLASS_PARTITION.
> > >
> > > Currently (temporarily), efi_disk_obj structure is embedded into
> > > blk_desc to hold efi-specific attributes.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >  drivers/block/blk-uclass.c |   9 ++
> > >  drivers/core/device.c      |   3 +
> > >  include/blk.h              |  24 +++++
> > >  include/dm/device.h        |   4 +
> > >  lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
> > >  5 files changed, 174 insertions(+), 58 deletions(-)
> > >
> >
> > I think the objective should be to drop the EFI data structures.
>
> More or less so, yes.
>
> > So your approach of just including them in DM structures seems about
> > right to me, as a short-term migration measure. Given the large amount
> > of code that has built up I don't think it is possible to do it any
> > other way.
>
> Right.
>
> > It is very unfortunate though.
> >
> > So basically migration could be something like this:
> >
> > 1. Move each of the EFI structs into the DM structs one by one
> > 2. Drop struct members that are not needed and can be calculated from DM members
> > 3. Update DM to have 1:1 uclasses for each EFI protocol
> > 4. Move all the protocol structs into the DM uclasses
> > 5. Whittle these down until they are just shells used by the API, with
> > everything going through normal DM calls
> >
> > Or would it be better to just start again and rewrite it with the
> > existing code as a base?
> >
> > Looking at it, efi_object is not very useful in DM. It contains two members:
> >
> > 1. link - linked list link, which devices already have, although we
> > don't have a single list of all them. Instead they are linked into
> > separate lists for each uclass
> >
> > 2. protocols - list of protocols. But DM devices support only one
> > protocol. Multiple protocols are handled using child devices. E.g a
> > UCLASS_PMIC device that supports UCLASS_GPIO, UCLASS_REGULATOR and
> > UCLASS_AUDIO_CODEC would have three children, one for each uclass. So
> > perhaps with EFI we should create a separate child for each protocol
> > in a similar way?
> >
> > Which comes back to the idea of creating an EFI child device for every
> > DM device. Perhaps instead we create one EFI child for each protocol
> > supported by the parent device?
>
> Well, "child device as a EFI protocol" is really workable, but
> I have some concerns:
> * Can a DM device be an abstract instance with no real hardware?

Yes we do that quite a bit. Even UCLASS_BLK is like this, if you think about it.

> * There may be two different types of "children" mixed for an EFI object
>    - some physical hierarchy, say disk partitions for a raw disk
>    - these EFI protocols
>   That is, for example, one raw disk has
>          - partition 1 has
>                  - block io protocol
>                  - device path protocol
>                  - simple file system protocol
>          - partition 2 has
>                  - block io protocol
>                  - device path protocol
>                  - simple file system protocol
>          - block io protocol
>          - device path protocol
> * device path protocol can be annoying as almost all devices (visible
>   to UEFI) have some sort of device path, while corresponding u-boot
>   notion is, say, "scsi 0:1" which only appears on command interfaces.

Yes. We could use the device path from concatenating device names from
all parents, perhaps. I've been thinking about adding that to the
command line as an option.

>
> I'm not sure if those concerns are acceptable.
>
> > Another point is that the operations supported by EFI should be in DM
> > operations structs. For example I think struct
> > efi_simple_text_output_protocol should have methods which call into
> > the corresponding uclass operations.
>
> I have no idea on those "console" devices yet.
>
> > It is confusing that an EFI disk is in fact a partition. Or do I have
> > that wrong?
>
> IMO, EFI disk is any type of EFI object which supports EFI_BLOCK_IO_PROTOCOL.
> So a raw disk(UCLASS_BLK) as well as its partitions(UCLASS_PARTITION) are
> EFI disks as well.
> Is this an answer to you?

Yes OK, I see.

>
> Those said, your suggestion is truly worth considering.

OK, good. Certainly an interesting project.

Regards,
Simon

[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[AKASHI Takahiro]

On Sat, Feb 02, 2019 at 07:15:53AM -0700, Simon Glass wrote:
> Hi,
> 
> On Thu, 31 Jan 2019 at 22:53, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > Thank you for suggestive comments.
> > I've got no idea of making DM class for EFI protocol.
> >
> > On Wed, Jan 30, 2019 at 06:22:47PM -0700, Simon Glass wrote:
> > > Hi AKASHI,
> > >
> > > On Mon, 28 Jan 2019 at 19:59, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > efi_disk_create() will initialize efi_disk attributes for each device,
> > > > either UCLASS_BLK or UCLASS_PARTITION.
> > > >
> > > > Currently (temporarily), efi_disk_obj structure is embedded into
> > > > blk_desc to hold efi-specific attributes.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > >  drivers/block/blk-uclass.c |   9 ++
> > > >  drivers/core/device.c      |   3 +
> > > >  include/blk.h              |  24 +++++
> > > >  include/dm/device.h        |   4 +
> > > >  lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
> > > >  5 files changed, 174 insertions(+), 58 deletions(-)
> > > >
> > >
> > > I think the objective should be to drop the EFI data structures.
> >
> > More or less so, yes.
> >
> > > So your approach of just including them in DM structures seems about
> > > right to me, as a short-term migration measure. Given the large amount
> > > of code that has built up I don't think it is possible to do it any
> > > other way.
> >
> > Right.
> >
> > > It is very unfortunate though.
> > >
> > > So basically migration could be something like this:
> > >
> > > 1. Move each of the EFI structs into the DM structs one by one
> > > 2. Drop struct members that are not needed and can be calculated from DM members
> > > 3. Update DM to have 1:1 uclasses for each EFI protocol
> > > 4. Move all the protocol structs into the DM uclasses
> > > 5. Whittle these down until they are just shells used by the API, with
> > > everything going through normal DM calls
> > >
> > > Or would it be better to just start again and rewrite it with the
> > > existing code as a base?
> > >
> > > Looking at it, efi_object is not very useful in DM. It contains two members:
> > >
> > > 1. link - linked list link, which devices already have, although we
> > > don't have a single list of all them. Instead they are linked into
> > > separate lists for each uclass
> > >
> > > 2. protocols - list of protocols. But DM devices support only one
> > > protocol. Multiple protocols are handled using child devices. E.g a
> > > UCLASS_PMIC device that supports UCLASS_GPIO, UCLASS_REGULATOR and
> > > UCLASS_AUDIO_CODEC would have three children, one for each uclass. So
> > > perhaps with EFI we should create a separate child for each protocol
> > > in a similar way?
> > >
> > > Which comes back to the idea of creating an EFI child device for every
> > > DM device. Perhaps instead we create one EFI child for each protocol
> > > supported by the parent device?
> >
> > Well, "child device as a EFI protocol" is really workable, but
> > I have some concerns:
> > * Can a DM device be an abstract instance with no real hardware?
> 
> Yes we do that quite a bit. Even UCLASS_BLK is like this, if you think about it.

OK

> > * There may be two different types of "children" mixed for an EFI object
> >    - some physical hierarchy, say disk partitions for a raw disk
> >    - these EFI protocols
> >   That is, for example, one raw disk has
> >          - partition 1 has
> >                  - block io protocol
> >                  - device path protocol
> >                  - simple file system protocol
> >          - partition 2 has
> >                  - block io protocol
> >                  - device path protocol
> >                  - simple file system protocol
> >          - block io protocol
> >          - device path protocol
> > * device path protocol can be annoying as almost all devices (visible
> >   to UEFI) have some sort of device path, while corresponding u-boot
> >   notion is, say, "scsi 0:1" which only appears on command interfaces.
> 
> Yes. We could use the device path from concatenating device names from
> all parents, perhaps. I've been thinking about adding that to the
> command line as an option.

Device path is kinda device hierarchy of DM, so it's easily confusing.
Please see an example below.

> >
> > I'm not sure if those concerns are acceptable.
> >
> > > Another point is that the operations supported by EFI should be in DM
> > > operations structs. For example I think struct
> > > efi_simple_text_output_protocol should have methods which call into
> > > the corresponding uclass operations.
> >
> > I have no idea on those "console" devices yet.
> >
> > > It is confusing that an EFI disk is in fact a partition. Or do I have
> > > that wrong?
> >
> > IMO, EFI disk is any type of EFI object which supports EFI_BLOCK_IO_PROTOCOL.
> > So a raw disk(UCLASS_BLK) as well as its partitions(UCLASS_PARTITION) are
> > EFI disks as well.
> > Is this an answer to you?
> 
> Yes OK, I see.
> 
> >
> > Those said, your suggestion is truly worth considering.
> 
> OK, good. Certainly an interesting project.

Conversion of efi protocols to uclass device(UCLASS_PROTOCOL) was done
and efi_handle_t is now 'struct udevice *'!

I could do this almost "systematically," but the code size doesn't decrease
much. This is no surprise because we rely on udevice only for traversing
efi handles and protocols. We still need lots of "wrapper" layers
on top of block devices, file systems and so on due to differences
of APIs and their semantics.
(Because of this, I don't have good confidence yet that efi protocol
should also be a device in DM.)

Here is a sample operation:

=> efi dev
Device           Device Path
================ ====================
000000007eef9590 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
=> scsi rescan

Reset SCSI
scanning bus for devices...
Target spinup took 0 ms.
Target spinup took 0 ms.
SATA link 2 timeout.
SATA link 3 timeout.
SATA link 4 timeout.
SATA link 5 timeout.
AHCI 0001.0000 32 slots 6 ports 1.5 Gbps 0x3f impl SATA mode
flags: 64bit ncq only 
  Device 0: (0:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+
            Type: Hard Disk
            Capacity: 16.0 MB = 0.0 GB (32768 x 512)
  Device 0: (1:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+
            Type: Hard Disk
            Capacity: 256.0 MB = 0.2 GB (524288 x 512)
=> efi devices
Device           Device Path
================ ====================
000000007eef9590 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
000000007ef04c50 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
000000007ef02d10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
000000007ef05c70 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(335544330,MBR,0x086246ba,0x17ff501a0,0x7eee4770)
000000007ef06270 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(335544330,MBR,0x086246ba,0x17ff501a0,0x7eee4770)
=> efi dh
Handle           Protocols
================ ====================
000000007eef9590 Device Path, Device Path To Text, Device Path Utilities, Unicode Collation 2
000000007ef00970 Driver Binding
000000007eef7b80 Simple Text Output, Simple Text Input, Simple Text Input Ex
000000007ef04c50 Block IO, Device Path
000000007ef02d10 Block IO, Device Path
000000007ef05c70 Block IO, Device Path, Simple File System
000000007ef06270 Block IO, Device Path, Simple File System
=> dm tree
 Class    index  Probed  Driver                Name
-----------------------------------------------------------
 root        0  [ + ]   root_driver           root_driver
 simple_bus  0  [   ]   generic_simple_bus    |-- platform at c000000
 virtio      0  [ + ]   virtio-mmio           |-- virtio_mmio at a000000

 ...

 pci         0  [ + ]   pci_generic_ecam      |-- pcie at 10000000
 pci_generi  0  [   ]   pci_generic_drv       |   |-- pci_0:0.0
 virtio      32  [   ]   virtio-pci.l          |   |-- virtio-pci.l#0
 ahci        0  [ + ]   ahci_pci              |   `-- ahci_pci
 scsi        0  [ + ]   ahci_scsi             |       `-- ahci_scsi
 blk         0  [   ]   scsi_blk              |           |-- ahci_scsi.id0lun0
 efi_protoc  8  [   ]   efi_protocol          |           |   |-- (PROTO)
 efi_protoc  9  [   ]   efi_protocol          |           |   `-- (PROTO)
 blk         1  [   ]   scsi_blk              |           `-- ahci_scsi.id1lun0
 efi_protoc  10  [   ]   efi_protocol          |               |-- (PROTO)
 efi_protoc  11  [   ]   efi_protocol          |               |-- (PROTO)
 partition   0  [   ]   blk_partition         |               |-- ahci_scsi.id1lun0:1
 efi_protoc  12  [   ]   efi_protocol          |               |   |-- (PROTO)
 efi_protoc  13  [   ]   efi_protocol          |               |   |-- (PROTO)
 efi_protoc  14  [   ]   efi_protocol          |               |   `-- (PROTO)
 partition   1  [   ]   blk_partition         |               `-- ahci_scsi.id1lun0:2
 efi_protoc  15  [   ]   efi_protocol          |                   |-- (PROTO)
 efi_protoc  16  [   ]   efi_protocol          |                   |-- (PROTO)
 efi_protoc  17  [   ]   efi_protocol          |                   `-- (PROTO)
 rtc         0  [   ]   rtc-pl031             |-- pl031 at 9010000
 serial      0  [   ]   serial_pl01x          |-- pl011 at 9050000
 serial      1  [ + ]   serial_pl01x          |-- pl011 at 9000000
 efi_protoc  5  [   ]   efi_protocol          |   |-- (PROTO)
 efi_protoc  6  [   ]   efi_protocol          |   |-- (PROTO)
 efi_protoc  7  [   ]   efi_protocol          |   `-- (PROTO)
 mtd         0  [ + ]   cfi_flash             |-- flash at 0
 firmware    0  [ + ]   psci                  |-- psci
 sysreset    0  [   ]   psci-sysreset         |   `-- psci-sysreset
 efi_object  0  [   ]   efi_dumb_object       |-- efi_root
 efi_protoc  0  [   ]   efi_protocol          |   |-- (PROTO)
 efi_protoc  1  [   ]   efi_protocol          |   |-- (PROTO)
 efi_protoc  2  [   ]   efi_protocol          |   |-- (PROTO)
 efi_protoc  3  [   ]   efi_protocol          |   `-- (PROTO)
 efi_object  1  [   ]   efi_dumb_object       `-- EFI block driver
 efi_protoc  4  [   ]   efi_protocol              `-- (PROTO)

As you can see, I have only one efi protocol "driver" and so
there's no specific name given for each protocol now.

As I'm still debugging my code, I will re-post my patch
once it gets back working.

Thanks,
-Takahiro Akashi

> Regards,
> Simon

[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[AKASHI Takahiro]

On Wed, Feb 06, 2019 at 12:15:11PM +0900, AKASHI Takahiro wrote:
> On Sat, Feb 02, 2019 at 07:15:53AM -0700, Simon Glass wrote:
> > Hi,
> > 
> > On Thu, 31 Jan 2019 at 22:53, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > Thank you for suggestive comments.
> > > I've got no idea of making DM class for EFI protocol.
> > >
> > > On Wed, Jan 30, 2019 at 06:22:47PM -0700, Simon Glass wrote:
> > > > Hi AKASHI,
> > > >
> > > > On Mon, 28 Jan 2019 at 19:59, AKASHI Takahiro
> > > > <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > efi_disk_create() will initialize efi_disk attributes for each device,
> > > > > either UCLASS_BLK or UCLASS_PARTITION.
> > > > >
> > > > > Currently (temporarily), efi_disk_obj structure is embedded into
> > > > > blk_desc to hold efi-specific attributes.
> > > > >
> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > ---
> > > > >  drivers/block/blk-uclass.c |   9 ++
> > > > >  drivers/core/device.c      |   3 +
> > > > >  include/blk.h              |  24 +++++
> > > > >  include/dm/device.h        |   4 +
> > > > >  lib/efi_loader/efi_disk.c  | 192 ++++++++++++++++++++++++++-----------
> > > > >  5 files changed, 174 insertions(+), 58 deletions(-)
> > > > >
> > > >
> > > > I think the objective should be to drop the EFI data structures.
> > >
> > > More or less so, yes.
> > >
> > > > So your approach of just including them in DM structures seems about
> > > > right to me, as a short-term migration measure. Given the large amount
> > > > of code that has built up I don't think it is possible to do it any
> > > > other way.
> > >
> > > Right.
> > >
> > > > It is very unfortunate though.
> > > >
> > > > So basically migration could be something like this:
> > > >
> > > > 1. Move each of the EFI structs into the DM structs one by one
> > > > 2. Drop struct members that are not needed and can be calculated from DM members
> > > > 3. Update DM to have 1:1 uclasses for each EFI protocol
> > > > 4. Move all the protocol structs into the DM uclasses
> > > > 5. Whittle these down until they are just shells used by the API, with
> > > > everything going through normal DM calls
> > > >
> > > > Or would it be better to just start again and rewrite it with the
> > > > existing code as a base?
> > > >
> > > > Looking at it, efi_object is not very useful in DM. It contains two members:
> > > >
> > > > 1. link - linked list link, which devices already have, although we
> > > > don't have a single list of all them. Instead they are linked into
> > > > separate lists for each uclass
> > > >
> > > > 2. protocols - list of protocols. But DM devices support only one
> > > > protocol. Multiple protocols are handled using child devices. E.g a
> > > > UCLASS_PMIC device that supports UCLASS_GPIO, UCLASS_REGULATOR and
> > > > UCLASS_AUDIO_CODEC would have three children, one for each uclass. So
> > > > perhaps with EFI we should create a separate child for each protocol
> > > > in a similar way?
> > > >
> > > > Which comes back to the idea of creating an EFI child device for every
> > > > DM device. Perhaps instead we create one EFI child for each protocol
> > > > supported by the parent device?
> > >
> > > Well, "child device as a EFI protocol" is really workable, but
> > > I have some concerns:
> > > * Can a DM device be an abstract instance with no real hardware?
> > 
> > Yes we do that quite a bit. Even UCLASS_BLK is like this, if you think about it.
> 
> OK
> 
> > > * There may be two different types of "children" mixed for an EFI object
> > >    - some physical hierarchy, say disk partitions for a raw disk
> > >    - these EFI protocols
> > >   That is, for example, one raw disk has
> > >          - partition 1 has
> > >                  - block io protocol
> > >                  - device path protocol
> > >                  - simple file system protocol
> > >          - partition 2 has
> > >                  - block io protocol
> > >                  - device path protocol
> > >                  - simple file system protocol
> > >          - block io protocol
> > >          - device path protocol
> > > * device path protocol can be annoying as almost all devices (visible
> > >   to UEFI) have some sort of device path, while corresponding u-boot
> > >   notion is, say, "scsi 0:1" which only appears on command interfaces.
> > 
> > Yes. We could use the device path from concatenating device names from
> > all parents, perhaps. I've been thinking about adding that to the
> > command line as an option.
> 
> Device path is kinda device hierarchy of DM, so it's easily confusing.
> Please see an example below.
> 
> > >
> > > I'm not sure if those concerns are acceptable.
> > >
> > > > Another point is that the operations supported by EFI should be in DM
> > > > operations structs. For example I think struct
> > > > efi_simple_text_output_protocol should have methods which call into
> > > > the corresponding uclass operations.
> > >
> > > I have no idea on those "console" devices yet.
> > >
> > > > It is confusing that an EFI disk is in fact a partition. Or do I have
> > > > that wrong?
> > >
> > > IMO, EFI disk is any type of EFI object which supports EFI_BLOCK_IO_PROTOCOL.
> > > So a raw disk(UCLASS_BLK) as well as its partitions(UCLASS_PARTITION) are
> > > EFI disks as well.
> > > Is this an answer to you?
> > 
> > Yes OK, I see.
> > 
> > >
> > > Those said, your suggestion is truly worth considering.
> > 
> > OK, good. Certainly an interesting project.
> 
> Conversion of efi protocols to uclass device(UCLASS_PROTOCOL) was done
> and efi_handle_t is now 'struct udevice *'!

To be clear, what is converted to DM device here is "struct efi_handler,"
not "protocol interface" structure in UEFI world. The latter is
a well-defined data structure in UEFI spec and cannot be treated
as a DM device.

> I could do this almost "systematically," but the code size doesn't decrease
> much. This is no surprise because we rely on udevice only for traversing
> efi handles and protocols. We still need lots of "wrapper" layers
> on top of block devices, file systems and so on due to differences
> of APIs and their semantics.
> (Because of this, I don't have good confidence yet that efi protocol
> should also be a device in DM.)
> 
> Here is a sample operation:
> 
> => efi dev
> Device           Device Path
> ================ ====================
> 000000007eef9590 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> => scsi rescan
> 
> Reset SCSI
> scanning bus for devices...
> Target spinup took 0 ms.
> Target spinup took 0 ms.
> SATA link 2 timeout.
> SATA link 3 timeout.
> SATA link 4 timeout.
> SATA link 5 timeout.
> AHCI 0001.0000 32 slots 6 ports 1.5 Gbps 0x3f impl SATA mode
> flags: 64bit ncq only 
>   Device 0: (0:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+
>             Type: Hard Disk
>             Capacity: 16.0 MB = 0.0 GB (32768 x 512)
>   Device 0: (1:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+
>             Type: Hard Disk
>             Capacity: 256.0 MB = 0.2 GB (524288 x 512)
> => efi devices
> Device           Device Path
> ================ ====================
> 000000007eef9590 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> 000000007ef04c50 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
> 000000007ef02d10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
> 000000007ef05c70 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(335544330,MBR,0x086246ba,0x17ff501a0,0x7eee4770)
> 000000007ef06270 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(335544330,MBR,0x086246ba,0x17ff501a0,0x7eee4770)
> => efi dh
> Handle           Protocols
> ================ ====================
> 000000007eef9590 Device Path, Device Path To Text, Device Path Utilities, Unicode Collation 2
> 000000007ef00970 Driver Binding
> 000000007eef7b80 Simple Text Output, Simple Text Input, Simple Text Input Ex
> 000000007ef04c50 Block IO, Device Path
> 000000007ef02d10 Block IO, Device Path
> 000000007ef05c70 Block IO, Device Path, Simple File System
> 000000007ef06270 Block IO, Device Path, Simple File System
> => dm tree
>  Class    index  Probed  Driver                Name
> -----------------------------------------------------------
>  root        0  [ + ]   root_driver           root_driver
>  simple_bus  0  [   ]   generic_simple_bus    |-- platform at c000000
>  virtio      0  [ + ]   virtio-mmio           |-- virtio_mmio at a000000
> 
>  ...
> 
>  pci         0  [ + ]   pci_generic_ecam      |-- pcie at 10000000
>  pci_generi  0  [   ]   pci_generic_drv       |   |-- pci_0:0.0
>  virtio      32  [   ]   virtio-pci.l          |   |-- virtio-pci.l#0
>  ahci        0  [ + ]   ahci_pci              |   `-- ahci_pci
>  scsi        0  [ + ]   ahci_scsi             |       `-- ahci_scsi
>  blk         0  [   ]   scsi_blk              |           |-- ahci_scsi.id0lun0
>  efi_protoc  8  [   ]   efi_protocol          |           |   |-- (PROTO)
>  efi_protoc  9  [   ]   efi_protocol          |           |   `-- (PROTO)
>  blk         1  [   ]   scsi_blk              |           `-- ahci_scsi.id1lun0
>  efi_protoc  10  [   ]   efi_protocol          |               |-- (PROTO)
>  efi_protoc  11  [   ]   efi_protocol          |               |-- (PROTO)
>  partition   0  [   ]   blk_partition         |               |-- ahci_scsi.id1lun0:1
>  efi_protoc  12  [   ]   efi_protocol          |               |   |-- (PROTO)
>  efi_protoc  13  [   ]   efi_protocol          |               |   |-- (PROTO)
>  efi_protoc  14  [   ]   efi_protocol          |               |   `-- (PROTO)
>  partition   1  [   ]   blk_partition         |               `-- ahci_scsi.id1lun0:2
>  efi_protoc  15  [   ]   efi_protocol          |                   |-- (PROTO)
>  efi_protoc  16  [   ]   efi_protocol          |                   |-- (PROTO)
>  efi_protoc  17  [   ]   efi_protocol          |                   `-- (PROTO)
>  rtc         0  [   ]   rtc-pl031             |-- pl031 at 9010000
>  serial      0  [   ]   serial_pl01x          |-- pl011 at 9050000
>  serial      1  [ + ]   serial_pl01x          |-- pl011 at 9000000
>  efi_protoc  5  [   ]   efi_protocol          |   |-- (PROTO)
>  efi_protoc  6  [   ]   efi_protocol          |   |-- (PROTO)
>  efi_protoc  7  [   ]   efi_protocol          |   `-- (PROTO)
>  mtd         0  [ + ]   cfi_flash             |-- flash at 0
>  firmware    0  [ + ]   psci                  |-- psci
>  sysreset    0  [   ]   psci-sysreset         |   `-- psci-sysreset

I changed this part of output:

>  efi_object  0  [   ]   efi_dumb_object       |-- efi_root
>  efi_protoc  0  [   ]   efi_protocol          |   |-- (PROTO)
>  efi_protoc  1  [   ]   efi_protocol          |   |-- (PROTO)
>  efi_protoc  2  [   ]   efi_protocol          |   |-- (PROTO)
>  efi_protoc  3  [   ]   efi_protocol          |   `-- (PROTO)
>  efi_object  1  [   ]   efi_dumb_object       `-- EFI block driver
>  efi_protoc  4  [   ]   efi_protocol              `-- (PROTO)

to

 efi_object  0  [   ]   efi_dumb_object       `-- efi_root
 efi_protoc  0  [   ]   efi_protocol              |-- (PROTO)
 efi_protoc  1  [   ]   efi_protocol              |-- (PROTO)
 efi_protoc  2  [   ]   efi_protocol              |-- (PROTO)
 efi_protoc  3  [   ]   efi_protocol              |-- (PROTO)
 efi         0  [   ]   EFI block driver          `-- EFI block driver
 efi_protoc  4  [   ]   efi_protocol                  `-- (PROTO)

> As you can see, I have only one efi protocol "driver" and so
> there's no specific name given for each protocol now.

More specifically, there seem to be a few options:
1-1. to keep one uclass, distinguishing protocols with an internal id,
     something like IF_TYPE_* for blk_desc
1-2. to keep one uclass, having one driver for one protocol,
(Either way, there's not common "operation" btw protocols here.)

2. to give each protocol an unique uclass id

Thanks,
-Takahiro Akashi

> As I'm still debugging my code, I will re-post my patch
> once it gets back working.
> 
> Thanks,
> -Takahiro Akashi
> 
> > Regards,
> > Simon

[U-Boot] [RFC 3/3] drivers: align block device drivers with DM-efi integration

[AKASHI Takahiro]

Efi_disk_create() should be hook up at every creation of block device
at each driver. Associated blk_desc must be properly set up before
calling this function.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 common/usb_storage.c              | 27 +++++++++++++++++++++++++--
 drivers/scsi/scsi.c               | 22 ++++++++++++++++++++++
 lib/efi_driver/efi_block_device.c | 30 +++++++++---------------------
 3 files changed, 56 insertions(+), 23 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 8c889bb1a648..ff895c0e4557 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -46,6 +46,10 @@
 #include <part.h>
 #include <usb.h>
 
+/* FIXME */
+extern int efi_disk_create(struct udevice *dev);
+extern int blk_create_partitions(struct udevice *parent);
+
 #undef BBB_COMDAT_TRACE
 #undef BBB_XPORT_TRACE
 
@@ -227,8 +231,27 @@ static int usb_stor_probe_device(struct usb_device *udev)
 
 		ret = usb_stor_get_info(udev, data, blkdev);
 		if (ret == 1) {
-			usb_max_devs++;
-			debug("%s: Found device %p\n", __func__, udev);
+			ret = efi_disk_create(dev);
+			if (ret) {
+				debug("Cannot create efi_disk device\n");
+				ret = device_unbind(dev);
+				if (ret)
+					return ret;
+			} else {
+				usb_max_devs++;
+				ret = blk_create_partitions(dev);
+				if (ret < 0) {
+					debug("Cannot create disk partition device\n");
+					/* TODO: undo create */
+
+					ret = device_unbind(dev);
+					if (ret)
+						return ret;
+				}
+				usb_max_devs += ret;
+				debug("%s: Found device %p, partitions:%d\n",
+				      __func__, udev, ret);
+			}
 		} else {
 			debug("usb_stor_get_info: Invalid device\n");
 			ret = device_unbind(dev);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index df47e2fc78bd..f0f8cbc0bf26 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -11,6 +11,10 @@
 #include <dm/device-internal.h>
 #include <dm/uclass-internal.h>
 
+/* FIXME */
+int efi_disk_create(struct udevice *dev);
+int blk_create_partitions(struct udevice *parent);
+
 #if !defined(CONFIG_DM_SCSI)
 # ifdef CONFIG_SCSI_DEV_LIST
 #  define SCSI_DEV_LIST CONFIG_SCSI_DEV_LIST
@@ -593,9 +597,27 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose)
 	memcpy(&bdesc->product, &bd.product, sizeof(bd.product));
 	memcpy(&bdesc->revision, &bd.revision,	sizeof(bd.revision));
 
+	ret = efi_disk_create(bdev);
+	if (ret) {
+		debug("Can't create efi_disk device\n");
+		ret = device_unbind(bdev);
+
+		return ret;
+	}
+	ret = blk_create_partitions(bdev);
+	if (ret < 0) {
+		debug("Can't create efi_disk partitions\n");
+		/* TODO: undo create */
+
+		ret = device_unbind(bdev);
+
+		return ret;
+	}
+
 	if (verbose) {
 		printf("  Device %d: ", 0);
 		dev_print(bdesc);
+		debug("Found %d partitions\n", ret);
 	}
 	return 0;
 }
diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
index 3f147cf60879..4ab3402d6768 100644
--- a/lib/efi_driver/efi_block_device.c
+++ b/lib/efi_driver/efi_block_device.c
@@ -32,6 +32,10 @@
 #include <dm/device-internal.h>
 #include <dm/root.h>
 
+/* FIXME */
+extern int efi_disk_create(struct udevice *dev);
+extern int blk_create_partitions(struct udevice *parent);
+
 /*
  * EFI attributes of the udevice handled by this driver.
  *
@@ -102,24 +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
- */
-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_platdata(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
  *
@@ -168,15 +154,17 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
 	platdata->handle = handle;
 	platdata->io = interface;
 
+	ret = efi_disk_create(bdev);
+	if (ret)
+		return ret;
+
 	ret = device_probe(bdev);
 	if (ret)
 		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);
+	disks = blk_create_partitions(bdev);
 	EFI_PRINT("Found %d partitions\n", disks);
-
 	return 0;
 }
 
-- 
2.19.1

[U-Boot] [RFC 3/3] drivers: align block device drivers with DM-efi integration

[Alexander Graf]

On 01/29/2019 03:59 AM, AKASHI Takahiro wrote:
> Efi_disk_create() should be hook up at every creation of block device
> at each driver. Associated blk_desc must be properly set up before
> calling this function.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   common/usb_storage.c              | 27 +++++++++++++++++++++++++--
>   drivers/scsi/scsi.c               | 22 ++++++++++++++++++++++
>   lib/efi_driver/efi_block_device.c | 30 +++++++++---------------------
>   3 files changed, 56 insertions(+), 23 deletions(-)
>
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index 8c889bb1a648..ff895c0e4557 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -46,6 +46,10 @@
>   #include <part.h>
>   #include <usb.h>
>   
> +/* FIXME */
> +extern int efi_disk_create(struct udevice *dev);
> +extern int blk_create_partitions(struct udevice *parent);
> +
>   #undef BBB_COMDAT_TRACE
>   #undef BBB_XPORT_TRACE
>   
> @@ -227,8 +231,27 @@ static int usb_stor_probe_device(struct usb_device *udev)
>   
>   		ret = usb_stor_get_info(udev, data, blkdev);
>   		if (ret == 1) {
> -			usb_max_devs++;
> -			debug("%s: Found device %p\n", __func__, udev);
> +			ret = efi_disk_create(dev);
> +			if (ret) {
> +				debug("Cannot create efi_disk device\n");
> +				ret = device_unbind(dev);
> +				if (ret)
> +					return ret;
> +			} else {
> +				usb_max_devs++;
> +				ret = blk_create_partitions(dev);
> +				if (ret < 0) {
> +					debug("Cannot create disk partition device\n");
> +					/* TODO: undo create */
> +
> +					ret = device_unbind(dev);
> +					if (ret)
> +						return ret;
> +				}
> +				usb_max_devs += ret;
> +				debug("%s: Found device %p, partitions:%d\n",
> +				      __func__, udev, ret);
> +			}

Why do we need to do this in device specific code?

Alex


>   		} else {
>   			debug("usb_stor_get_info: Invalid device\n");
>   			ret = device_unbind(dev);
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index df47e2fc78bd..f0f8cbc0bf26 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -11,6 +11,10 @@
>   #include <dm/device-internal.h>
>   #include <dm/uclass-internal.h>
>   
> +/* FIXME */
> +int efi_disk_create(struct udevice *dev);
> +int blk_create_partitions(struct udevice *parent);
> +
>   #if !defined(CONFIG_DM_SCSI)
>   # ifdef CONFIG_SCSI_DEV_LIST
>   #  define SCSI_DEV_LIST CONFIG_SCSI_DEV_LIST
> @@ -593,9 +597,27 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose)
>   	memcpy(&bdesc->product, &bd.product, sizeof(bd.product));
>   	memcpy(&bdesc->revision, &bd.revision,	sizeof(bd.revision));
>   
> +	ret = efi_disk_create(bdev);
> +	if (ret) {
> +		debug("Can't create efi_disk device\n");
> +		ret = device_unbind(bdev);
> +
> +		return ret;
> +	}
> +	ret = blk_create_partitions(bdev);
> +	if (ret < 0) {
> +		debug("Can't create efi_disk partitions\n");
> +		/* TODO: undo create */
> +
> +		ret = device_unbind(bdev);
> +
> +		return ret;
> +	}
> +
>   	if (verbose) {
>   		printf("  Device %d: ", 0);
>   		dev_print(bdesc);
> +		debug("Found %d partitions\n", ret);
>   	}
>   	return 0;
>   }
> diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
> index 3f147cf60879..4ab3402d6768 100644
> --- a/lib/efi_driver/efi_block_device.c
> +++ b/lib/efi_driver/efi_block_device.c
> @@ -32,6 +32,10 @@
>   #include <dm/device-internal.h>
>   #include <dm/root.h>
>   
> +/* FIXME */
> +extern int efi_disk_create(struct udevice *dev);
> +extern int blk_create_partitions(struct udevice *parent);
> +
>   /*
>    * EFI attributes of the udevice handled by this driver.
>    *
> @@ -102,24 +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
> - */
> -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_platdata(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
>    *
> @@ -168,15 +154,17 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
>   	platdata->handle = handle;
>   	platdata->io = interface;
>   
> +	ret = efi_disk_create(bdev);
> +	if (ret)
> +		return ret;
> +
>   	ret = device_probe(bdev);
>   	if (ret)
>   		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);
> +	disks = blk_create_partitions(bdev);
>   	EFI_PRINT("Found %d partitions\n", disks);
> -
>   	return 0;
>   }
>   

[U-Boot] [RFC 3/3] drivers: align block device drivers with DM-efi integration

[AKASHI Takahiro]

Alex,

On Tue, Jan 29, 2019 at 05:19:38PM +0100, Alexander Graf wrote:
> On 01/29/2019 03:59 AM, AKASHI Takahiro wrote:
> >Efi_disk_create() should be hook up at every creation of block device
> >at each driver. Associated blk_desc must be properly set up before
> >calling this function.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  common/usb_storage.c              | 27 +++++++++++++++++++++++++--
> >  drivers/scsi/scsi.c               | 22 ++++++++++++++++++++++
> >  lib/efi_driver/efi_block_device.c | 30 +++++++++---------------------
> >  3 files changed, 56 insertions(+), 23 deletions(-)
> >
> >diff --git a/common/usb_storage.c b/common/usb_storage.c
> >index 8c889bb1a648..ff895c0e4557 100644
> >--- a/common/usb_storage.c
> >+++ b/common/usb_storage.c
> >@@ -46,6 +46,10 @@
> >  #include <part.h>
> >  #include <usb.h>
> >+/* FIXME */
> >+extern int efi_disk_create(struct udevice *dev);
> >+extern int blk_create_partitions(struct udevice *parent);
> >+
> >  #undef BBB_COMDAT_TRACE
> >  #undef BBB_XPORT_TRACE
> >@@ -227,8 +231,27 @@ static int usb_stor_probe_device(struct usb_device *udev)
> >  		ret = usb_stor_get_info(udev, data, blkdev);
> >  		if (ret == 1) {
> >-			usb_max_devs++;
> >-			debug("%s: Found device %p\n", __func__, udev);
> >+			ret = efi_disk_create(dev);
> >+			if (ret) {
> >+				debug("Cannot create efi_disk device\n");
> >+				ret = device_unbind(dev);
> >+				if (ret)
> >+					return ret;
> >+			} else {
> >+				usb_max_devs++;
> >+				ret = blk_create_partitions(dev);
> >+				if (ret < 0) {
> >+					debug("Cannot create disk partition device\n");
> >+					/* TODO: undo create */
> >+
> >+					ret = device_unbind(dev);
> >+					if (ret)
> >+						return ret;
> >+				}
> >+				usb_max_devs += ret;
> >+				debug("%s: Found device %p, partitions:%d\n",
> >+				      __func__, udev, ret);
> >+			}
> 
> Why do we need to do this in device specific code?

Good point.

* efi_disk_create()
As I said in the past, efi_disk_create() will expect that blk_desc has
been initialized before it is called.
(There may be some possibility of removing this assumption.)

Blk_desc is often initialized after blk_create_device() in a driver.
So it won't be able to be placed in blk_create_device().

If, however, we have a "delayed" execution mechanism (like timer event as
you suggested before), we may put this function in a single point.

* blk_create_partions()
I initially thought of putting this function in part_init() which is
to be called at "probe," but was concerned that there might be a chance
that efi API be called before probing.
If this is not the case, we may also place this function in a single point.
(Unfortunately, "scsi rescan" won't kick off a probe hook though.)

Thanks,
-Takahiro Akashi

> Alex
> 
> 
> >  		} else {
> >  			debug("usb_stor_get_info: Invalid device\n");
> >  			ret = device_unbind(dev);
> >diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> >index df47e2fc78bd..f0f8cbc0bf26 100644
> >--- a/drivers/scsi/scsi.c
> >+++ b/drivers/scsi/scsi.c
> >@@ -11,6 +11,10 @@
> >  #include <dm/device-internal.h>
> >  #include <dm/uclass-internal.h>
> >+/* FIXME */
> >+int efi_disk_create(struct udevice *dev);
> >+int blk_create_partitions(struct udevice *parent);
> >+
> >  #if !defined(CONFIG_DM_SCSI)
> >  # ifdef CONFIG_SCSI_DEV_LIST
> >  #  define SCSI_DEV_LIST CONFIG_SCSI_DEV_LIST
> >@@ -593,9 +597,27 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose)
> >  	memcpy(&bdesc->product, &bd.product, sizeof(bd.product));
> >  	memcpy(&bdesc->revision, &bd.revision,	sizeof(bd.revision));
> >+	ret = efi_disk_create(bdev);
> >+	if (ret) {
> >+		debug("Can't create efi_disk device\n");
> >+		ret = device_unbind(bdev);
> >+
> >+		return ret;
> >+	}
> >+	ret = blk_create_partitions(bdev);
> >+	if (ret < 0) {
> >+		debug("Can't create efi_disk partitions\n");
> >+		/* TODO: undo create */
> >+
> >+		ret = device_unbind(bdev);
> >+
> >+		return ret;
> >+	}
> >+
> >  	if (verbose) {
> >  		printf("  Device %d: ", 0);
> >  		dev_print(bdesc);
> >+		debug("Found %d partitions\n", ret);
> >  	}
> >  	return 0;
> >  }
> >diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
> >index 3f147cf60879..4ab3402d6768 100644
> >--- a/lib/efi_driver/efi_block_device.c
> >+++ b/lib/efi_driver/efi_block_device.c
> >@@ -32,6 +32,10 @@
> >  #include <dm/device-internal.h>
> >  #include <dm/root.h>
> >+/* FIXME */
> >+extern int efi_disk_create(struct udevice *dev);
> >+extern int blk_create_partitions(struct udevice *parent);
> >+
> >  /*
> >   * EFI attributes of the udevice handled by this driver.
> >   *
> >@@ -102,24 +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
> >- */
> >-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_platdata(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
> >   *
> >@@ -168,15 +154,17 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
> >  	platdata->handle = handle;
> >  	platdata->io = interface;
> >+	ret = efi_disk_create(bdev);
> >+	if (ret)
> >+		return ret;
> >+
> >  	ret = device_probe(bdev);
> >  	if (ret)
> >  		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);
> >+	disks = blk_create_partitions(bdev);
> >  	EFI_PRINT("Found %d partitions\n", disks);
> >-
> >  	return 0;
> >  }
> 
> 

[U-Boot] [RFC 0/3] dm, efi: integrate efi_disk into DM

[Alexander Graf]

On 01/29/2019 03:59 AM, AKASHI Takahiro wrote:
> This patch set came from the past discussion[1] on my "removable device
> support" patch and is intended to be an attempt to integrate efi_disk
> (more precisely, EFI_BLOCK_IO_PROTOCOL-capable efi object) into u-boot's
> Driver Model as much seamlessly as possible
>
> [1] https://lists.denx.de/pipermail/u-boot/2019-January/354010.html
>
> Basic idea is
> * create UCLASS_PARTITION
> * enumerate all the partitions when a block device is probed
> * hook up a creation of UCLASS_BLK or UCLASS_PARTITION device
>    to efi handler for efi_disk attributes to be properly set up
>
> Since this patch is a prototype (or POC, proof-of-concept), the aim here
> is to discuss further about how in a better shape we will be able to
> merge the two worlds.

I like the approach. It seems pretty clean and gives us a smooth 
transition from the split world to a unified all-dm world. Eventually 
the efi object list will just naturally disappear and we can drop all 
calls to add efi object handles.


Alex

[U-Boot] [RFC 0/3] dm, efi: integrate efi_disk into DM

[Heinrich Schuchardt]

On 1/29/19 5:20 PM, Alexander Graf wrote:
> On 01/29/2019 03:59 AM, AKASHI Takahiro wrote:
>> This patch set came from the past discussion[1] on my "removable device
>> support" patch and is intended to be an attempt to integrate efi_disk
>> (more precisely, EFI_BLOCK_IO_PROTOCOL-capable efi object) into u-boot's
>> Driver Model as much seamlessly as possible
>>
>> [1] https://lists.denx.de/pipermail/u-boot/2019-January/354010.html
>>
>> Basic idea is
>> * create UCLASS_PARTITION
>> * enumerate all the partitions when a block device is probed
>> * hook up a creation of UCLASS_BLK or UCLASS_PARTITION device
>>    to efi handler for efi_disk attributes to be properly set up
>>
>> Since this patch is a prototype (or POC, proof-of-concept), the aim here
>> is to discuss further about how in a better shape we will be able to
>> merge the two worlds.
> 
> I like the approach. It seems pretty clean and gives us a smooth
> transition from the split world to a unified all-dm world. Eventually
> the efi object list will just naturally disappear and we can drop all
> calls to add efi object handles.
> 
> 
> Alex
> 
> 

Thanks Takahiro, it is good to have a reference to work on to bring the
worlds together.

I still have some issues:

The implementation lacks the driver binding protocol to handle block
devices that are not discovered by U-Boot but supplied by an EFI driver
or application. I would prefer if the block uclass would provide this
protocol.

In EFI a handle can always be deleted by first stopping all controllers
and then removing all protocols.

The current implementation of partitions tries to use a struct efi_obj
instead of a handle. This is incompatible to the rest of the EFI subsystem.

Best regards

Heinrich

[U-Boot] [RFC 0/3] dm, efi: integrate efi_disk into DM

[AKASHI Takahiro]

Heinrich,

On Tue, Jan 29, 2019 at 11:48:37PM +0100, Heinrich Schuchardt wrote:
> On 1/29/19 5:20 PM, Alexander Graf wrote:
> > On 01/29/2019 03:59 AM, AKASHI Takahiro wrote:
> >> This patch set came from the past discussion[1] on my "removable device
> >> support" patch and is intended to be an attempt to integrate efi_disk
> >> (more precisely, EFI_BLOCK_IO_PROTOCOL-capable efi object) into u-boot's
> >> Driver Model as much seamlessly as possible
> >>
> >> [1] https://lists.denx.de/pipermail/u-boot/2019-January/354010.html
> >>
> >> Basic idea is
> >> * create UCLASS_PARTITION
> >> * enumerate all the partitions when a block device is probed
> >> * hook up a creation of UCLASS_BLK or UCLASS_PARTITION device
> >>    to efi handler for efi_disk attributes to be properly set up
> >>
> >> Since this patch is a prototype (or POC, proof-of-concept), the aim here
> >> is to discuss further about how in a better shape we will be able to
> >> merge the two worlds.
> > 
> > I like the approach. It seems pretty clean and gives us a smooth
> > transition from the split world to a unified all-dm world. Eventually
> > the efi object list will just naturally disappear and we can drop all
> > calls to add efi object handles.
> > 
> > 
> > Alex
> > 
> > 
> 
> Thanks Takahiro, it is good to have a reference to work on to bring the
> worlds together.
> 
> I still have some issues:
> 
> The implementation lacks the driver binding protocol to handle block
> devices that are not discovered by U-Boot but supplied by an EFI driver
> or application. I would prefer if the block uclass would provide this
> protocol.

I don't yet have deep understandings of DM, but I believe that any
UCLASS_BLK instance should have a backing block device, blk_desc, which is
pointed to by uclass_platdata.
But your efi block device, UCLASS_BLK/IF_TYPE_EFI, does never initialize
this structure, in particular "ops," and so it won't work as a block device
on u-boot side.

I guess that it is one of reasons that EFI block driver doesn't work
with my patch.

If you agree with me, it would be much easier for you to modify
EFI block driver :)

> In EFI a handle can always be deleted by first stopping all controllers
> and then removing all protocols.

What do you mean by "delete?"
Some efi object will directly use efi_add_handle().
Efi object is solely responsible for maintaining a handle.

> The current implementation of partitions tries to use a struct efi_obj
> instead of a handle. This is incompatible to the rest of the EFI subsystem.

How incompatible?
A handle is always a pointer to opaque data outside the implementation.
struct efi_obj in blk_desc is explicitly handled only in efi_disk.c.
What's wrong with that?

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
>