[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk
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
---
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
+#include
#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
#include
+#include
#include
#include
#include
@@ -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)
{
@@ -6
Re: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk
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 > 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 > > --- > > 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 mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk
Hi, On Thu, 31 Jan 2019 at 22:53, AKASHI Takahiro 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 > > 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 > > > --- > > > 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 mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk
On Sat, Feb 02, 2019 at 07:15:53AM -0700, Simon Glass wrote: > Hi, > > On Thu, 31 Jan 2019 at 22:53, AKASHI Takahiro > 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 > > > 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 > > > > --- > > > > 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
Re: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk
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 > > 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 > > > > 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 > > > > > --- > > > > > 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. >
Re: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk
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
> ---
> 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
> +#include
>
> #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
> #include
> +#include
> #include
> #include
> #include
> @@ -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:
Re: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk
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
> > ---
> > 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
> > +#include
> >
> > #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
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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 c037526ad
Re: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk
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
>>> ---
>>> 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
>>> +#include
>>>
>>> #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
>>> #include
>>> +#include
>>> #include
>>> #include
>>> #include
>>> @@ -146,6 +147,9 @@ struct udevice {
>>> #ifdef CONFIG_DEVRES
>>> struct list_he
Re: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk
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
> >>> ---
> >>> 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
> >>> +#include
> >>>
> >>> #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
Re: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk
Hi AKASHI, On Mon, 28 Jan 2019 at 19:59, 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 > --- > 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 mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
