[PATCH v2 00/20] efi_loader: more tightly integrate UEFI disks to driver model
Background: =========== The purpose of this patch is to reignite the discussion about how UEFI subystem would best be integrated into U-Boot driver model. In the past, I proposed a couple of patch series, the latest one[1], while Heinrich revealed his idea[2], and the approach taken here is something between them, with a focus on block device handlings.
Disks in UEFI world: ==================== In general in UEFI world, accessing to any device is performed through a 'protocol' interface which are installed to (or associated with) the device's UEFI handle (or an opaque pointer to UEFI object data). Protocols are implemented by either the UEFI system itself or UEFI drivers.
For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk hereafter). Currently, every efi_disk may have one of two origins:
a.U-Boot's block devices or related partitions (lib/efi_loader/efi_disk.c) b.UEFI objects which are implemented as a block device by UEFI drivers. (lib/efi_driver/efi_block_device.c)
All the efi_diskss as (a) will be enumerated and created only once at UEFI subsystem initialization (efi_disk_register()), which is triggered by first executing one of UEFI-related U-Boot commands, like "bootefi", "setenv -e" or "efidebug". EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) in the corresponding udevice(UCLASS_BLK).
On the other hand, efi_disk as (b) will be created each time UEFI boot services' connect_controller() is executed in UEFI app which, as a (device) controller, gives the method to access the device's data, ie. EFI_BLOCK_IO_PROTOCOL.
more details >>>
Internally, connect_controller() search for UEFI driver that can support this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, then calls the driver's 'bind' interface, which eventually installs the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for * creating additional partitions efi_disk's, and * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it. <<< <<<
Issues: ======= 1. While an efi_disk represents a device equally for either a whole disk or a partition in UEFI world, the driver model treats only a whole disk as a real block device or udevice(UCLASS_BLK).
2. efi_disk holds and makes use of "blk_desc" data even though blk_desc in plat_data is supposed to be private and not to be accessed outside the driver model. # This issue, though, exists for all the implementation of U-Boot # file systems as well.
For efi_disk(a), 3. A block device can be enumerated dynamically by 'scanning' a device bus in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly. For examples, => scsi rescan; efidebug devices => usb start; efidebug devices ... (A) (A) doesn't show any usb devices detected.
=> scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ... => scsi rescan ... (B) => bootefi bootmgr ... (C) (C) may de-reference a bogus blk_desc pointer which has been freed by (B). (Please note that "scsi rescan" removes all udevices/blk_desc and then re-create them even if nothing is changed on a bus.)
For efi_disk(b), 4. A "controller (handle)", combined with efi_block driver, has no corresponding udevice as a parent of efi_disks in DM tree, unlike, say, a scsi controller, even though it provides methods for block io operations. 5. There is no way supported to remove efi_disk's even after disconnect_controller() is called.
My approach: ============ Due to functional differences in semantics, it would be difficult to identify "udevice" structure as a handle in UEFI world. Instead, we will have to somehow maintain a relationship between a udevice and a handle.
1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions Currently, the uclass for partitions is not a UCLASS_BLK. It can be possible to define partitions as UCLASS_BLK (with IF_TYPE_PARTION?), but I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK) is tightly coupled with 'struct blk_desc' data which is still used as a "structure to a whole disk" in a lot of interfaces. (I hope that you understand what it means.)
In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent: For instance, UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION (IF_TYPE_SCSI) | +- struct blk_desc +- struct disk_part +- scsi_blk_ops +- blk_part_ops
1-2. create partition udevices in the context of device_probe() part_init() is already called in blk_post_probe(). See the commit d0851c893706 ("blk: Call part_init() in the post_probe() method"). Why not enumerate partitions as well in there.
2. add new block access interfaces, which takes a *udevice* as a target device, in U-Boot and use those functions to implement efi_disk operations (i.e. EFI_BLOCK_IO_PROTOCOL).
3-1. maintain a bi-directional link between a udevice and an efi_disk by adding - a UEFI handle pointer as a tag for a udevice - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
3-2. synchronize the lifetime of efi_disk objects in UEFI world with the driver model using - event notification associated with device's probe/remove.
4. I have no solution to issue(4) and (5) yet.
<<<Example DM tree on qemu-arm64>>> => dm tree Class Driver Name -------------------------------------------- root root_driver root_driver ... pci pci_generic_ecam |-- pcie@10000000 pci_generi pci_generic_drv | |-- pci_0:0.0 virtio virtio-pci.l | |-- virtio-pci.l#0 ethernet virtio-net | | `-- virtio-net#32 ahci ahci_pci | |-- ahci_pci scsi ahci_scsi | | `-- ahci_scsi blk scsi_blk | | |-- ahci_scsi.id0lun0 partition blk_partition | | | |-- ahci_scsi.id0lun0:1 partition blk_partition | | | `-- ahci_scsi.id0lun0:2 blk scsi_blk | | `-- ahci_scsi.id1lun0 partition blk_partition | | |-- ahci_scsi.id1lun0:1 partition blk_partition | | `-- ahci_scsi.id1lun0:2 usb xhci_pci | `-- xhci_pci usb_hub usb_hub | `-- usb_hub usb_dev_ge usb_dev_generic_drv | |-- generic_bus_0_dev_2 usb_mass_s usb_mass_storage | `-- usb_mass_storage blk usb_storage_blk | `-- usb_mass_storage.lun0 partition blk_partition | |-- usb_mass_storage.lun0:1 partition blk_partition | `-- usb_mass_storage.lun0:2 ... => efi devices Device Device Path ================ ==================== 000000013eeea8d0 /VenHw() 000000013eeed810 /VenHw()/MAC(525252525252,1) 000000013eefc460 /VenHw()/Scsi(0,0) 000000013eefc5a0 /VenHw()/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eefe320 /VenHw()/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013eeff210 /VenHw()/Scsi(1,0) 000000013eeff390 /VenHw()/Scsi(1,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eeff7d0 /VenHw()/Scsi(1,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013ef04c20 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0) 000000013ef04da0 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(1,0x01,0,0x0,0x99800) 000000013ef04f70 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(2,0x01,0,0x99800,0x1800)
Patchs: ======= For easy understandings, patches may be categorized into separate groups of changes.
Patch#1-#7: DM: add device_probe() for later use of events Patch#8-#11: DM: add new features (tag and event notification) Patch#12-#16: UEFI: dynamically create/remove efi_disk's for a raw disk and its partitions For removal case, we may need more consideration since removing handles unconditionally may end up breaking integrity of handles (as some may still be held and referenced to by a UEFI app). Patch#17-#18: UEFI: use udevice read/write interfaces Patch#19-#20: UEFI: fix-up efi_driver, aligning with changes in DM integration
[1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html
Change history: =============== v2 (Feb 10, 2022) * add/revise an error message if device_probe() fails (patch#3,#5) * fix a build error in sandbox_spl_defconfig (patch#8) * fix warnings in 'make htmldocs' (patch#8,#9,#18) * new commit: split efi_init_obj_list() (patch#14)
v1 (Feb 2, 2022) * rebased on 2022.04-rc1 * drop patches that have already been merged * modify a tag-range check with "tag >= DM_TAG_COUNT" (patch#9) * move dmtag_list to GD (global data) (patch#9) * add function descriptions and a document about DM tag feature (patch#9,10) * add tests for DM tag support (patch#11) * change 'depends on EVENT' to 'select EVENT' for EFI_LOADER (patch#14) * migrate IF_TYPE_EFI to IF_TYPE_EFI_LOADER (patch#18)
RFCv2 (Dec 10, 2021) * rebased on 2022-rc3 * re-order and merge some related commits into ones * call device_probe() in MMC (not bind, but) probe hook (patch#5) * fix a wrong name of variable (patch#7) * add patch#9 * invoke device_probe() for virtio devices (patch#10) * add DM event notification (from Simon) (patch#11) * add DM tag support (patch#12) * move UCLASS_PARTITION driver under disk/ (patch#13) * create partition's dp using its parent's. This change is necessary in particular for 'efi_blk' efi_disk (patch#13) * modify the code so that we will use new features like tags and event notification (patch#13,15,16,20) * rename new functions from blk_read/write() to dev_read/write() (patch#17,18) * isolate changes in efi_driver from the rest (in efi_loader) (patch#19) * drop the previous patch#22 ("efi_selftest: block device: adjust dp for a test") due to the fix in patch#13
RFC (Nov 16, 2021) * initial RFC
AKASHI Takahiro (19): scsi: call device_probe() after scanning usb: storage: call device_probe() after scanning mmc: call device_probe() after scanning nvme: call device_probe() after scanning sata: call device_probe() after scanning block: ide: call device_probe() after scanning virtio: call device_probe() in scanning dm: add tag support dm: tag: add some document test: dm: add tests for tag support dm: disk: add UCLASS_PARTITION dm: blk: add a device-probe hook for scanning disk partitions efi_loader: split efi_init_obj_list() into two stages efi_loader: disk: a helper function to create efi_disk objects from udevice efi_loader: disk: a helper function to delete efi_disk objects dm: disk: add read/write interfaces with udevice efi_loader: disk: use udevice instead of blk_desc efi_loader: disk: not create BLK device for BLK(IF_TYPE_EFI_LOADER) devices efi_driver: align with efi_disk-dm integration
Simon Glass (1): dm: add event notification
cmd/virtio.c | 21 +- common/Kconfig | 11 + common/Makefile | 2 + common/board_f.c | 2 + common/board_r.c | 2 +- common/event.c | 103 +++++++++ common/log.c | 1 + common/main.c | 7 +- common/usb_storage.c | 4 + disk/Makefile | 3 + disk/disk-uclass.c | 247 +++++++++++++++++++++ doc/develop/driver-model/design.rst | 20 ++ drivers/ata/dwc_ahsata.c | 5 + drivers/ata/fsl_sata.c | 11 + drivers/ata/sata_mv.c | 5 + drivers/ata/sata_sil.c | 12 + drivers/block/blk-uclass.c | 4 + drivers/block/ide.c | 4 + drivers/core/Makefile | 2 +- drivers/core/device-remove.c | 9 + drivers/core/device.c | 9 + drivers/core/root.c | 2 + drivers/core/tag.c | 139 ++++++++++++ drivers/mmc/mmc-uclass.c | 12 + drivers/nvme/nvme.c | 4 + drivers/scsi/scsi.c | 5 + include/asm-generic/global_data.h | 10 + include/dm/device-internal.h | 10 + include/dm/tag.h | 110 +++++++++ include/dm/uclass-id.h | 1 + include/efi_loader.h | 6 +- include/event.h | 105 +++++++++ include/event_internal.h | 34 +++ include/log.h | 2 + include/part.h | 18 ++ lib/efi_driver/efi_block_device.c | 34 +-- lib/efi_loader/Kconfig | 2 + lib/efi_loader/efi_disk.c | 331 ++++++++++++++++++++-------- lib/efi_loader/efi_setup.c | 62 +++++- test/common/Makefile | 1 + test/common/event.c | 87 ++++++++ test/dm/Makefile | 1 + test/dm/tag.c | 80 +++++++ test/test-main.c | 7 + 44 files changed, 1416 insertions(+), 131 deletions(-) create mode 100644 common/event.c create mode 100644 disk/disk-uclass.c create mode 100644 drivers/core/tag.c create mode 100644 include/dm/tag.h create mode 100644 include/event.h create mode 100644 include/event_internal.h create mode 100644 test/common/event.c create mode 100644 test/dm/tag.c
Every time a scsi bus/port is scanned and a new block device is detected, we want to call device_probe() as it will give us a chance to run additional post-processings for some purposes.
In particular, support for creating partitions on a device will be added.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- drivers/scsi/scsi.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index d93d24192853..9a763ea78bbd 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -595,6 +595,11 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) ata_swap_buf_le16((u16 *)&bdesc->revision, sizeof(bd.revision) / 2); }
+ ret = blk_probe_or_unbind(bdev); + if (ret < 0) + /* TODO: undo create */ + return ret; + if (verbose) { printf(" Device %d: ", bdesc->devnum); dev_print(bdesc);
Every time a usb bus/port is scanned and a new device is detected, we want to call device_probe() as it will give us a chance to run additional post-processings for some purposes.
In particular, support for creating partitions on a device will be added.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- common/usb_storage.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/common/usb_storage.c b/common/usb_storage.c index c9e2d7343ce2..291728f37e0a 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -239,6 +239,10 @@ static int usb_stor_probe_device(struct usb_device *udev) if (ret) return ret; } + + ret = blk_probe_or_unbind(dev); + if (ret) + return ret; } #else /* We don't have space to even probe if we hit the maximum */
Every time a mmc bus/port is scanned and a new device is detected, we want to call device_probe() as it will give us a chance to run additional post-processings for some purposes.
In particular, support for creating partitions on a device will be added.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- drivers/mmc/mmc-uclass.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index b80e838066ca..aa2ab5d8c753 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -467,6 +467,18 @@ static int mmc_blk_probe(struct udevice *dev) return ret; }
+ ret = device_probe(dev); + if (ret) { + debug("Probing %s failed (err=%d)\n", dev->name, ret); + + if (IS_ENABLED(CONFIG_MMC_UHS_SUPPORT) || + IS_ENABLED(CONFIG_MMC_HS200_SUPPORT) || + IS_ENABLED(CONFIG_MMC_HS400_SUPPORT)) + mmc_deinit(mmc); + + return ret; + } + return 0; }
On 2/10/22 17:11, AKASHI Takahiro wrote:
Every time a mmc bus/port is scanned and a new device is detected, we want to call device_probe() as it will give us a chance to run additional post-processings for some purposes.
In particular, support for creating partitions on a device will be added.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Reviewed-by: Jaehoon Chung jh80.chung@samsung.com
Best Regards, Jaehoon Chung
drivers/mmc/mmc-uclass.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index b80e838066ca..aa2ab5d8c753 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -467,6 +467,18 @@ static int mmc_blk_probe(struct udevice *dev) return ret; }
- ret = device_probe(dev);
- if (ret) {
debug("Probing %s failed (err=%d)\n", dev->name, ret);if (IS_ENABLED(CONFIG_MMC_UHS_SUPPORT) ||IS_ENABLED(CONFIG_MMC_HS200_SUPPORT) ||IS_ENABLED(CONFIG_MMC_HS400_SUPPORT))mmc_deinit(mmc);return ret;- }
- return 0;
}
Every time a nvme bus/port is scanned and a new device is detected, we want to call device_probe() as it will give us a chance to run additional post-processings for some purposes.
In particular, support for creating partitions on a device will be added.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- drivers/nvme/nvme.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 3c529a2fce22..120a0f05a591 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -910,6 +910,10 @@ static int nvme_probe(struct udevice *udev) -1, 512, 0, &ns_udev); if (ret) goto free_id; + + ret = blk_probe_or_unbind(ns_udev); + if (ret) + goto free_id; }
free(id);
Every time a sata bus/port is scanned and a new device is detected, we want to call device_probe() as it will give us a chance to run additional post-processings for some purposes.
In particular, support for creating partitions on a device will be added.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- drivers/ata/dwc_ahsata.c | 5 +++++ drivers/ata/fsl_sata.c | 11 +++++++++++ drivers/ata/sata_mv.c | 5 +++++ drivers/ata/sata_sil.c | 12 ++++++++++++ 4 files changed, 33 insertions(+)
diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c index 6d42548087b3..d9fd850c6fae 100644 --- a/drivers/ata/dwc_ahsata.c +++ b/drivers/ata/dwc_ahsata.c @@ -1026,6 +1026,11 @@ int dwc_ahsata_scan(struct udevice *dev) return ret; }
+ ret = blk_probe_or_unbind(dev); + if (ret < 0) + /* TODO: undo create */ + return ret; + return 0; }
diff --git a/drivers/ata/fsl_sata.c b/drivers/ata/fsl_sata.c index e44db0a37458..7b2d62d7581b 100644 --- a/drivers/ata/fsl_sata.c +++ b/drivers/ata/fsl_sata.c @@ -982,6 +982,17 @@ static int fsl_ata_probe(struct udevice *dev) failed_number++; continue; } + + ret = device_probe(dev); + if (ret < 0) { + debug("Probing $s failed (%d)\n", dev->name, ret); + ret = fsl_unbind_device(blk); + if (ret) + return ret; + + failed_number++; + continue; + } }
if (failed_number == nr_ports) diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c index 003222d47be6..a187796dfcdf 100644 --- a/drivers/ata/sata_mv.c +++ b/drivers/ata/sata_mv.c @@ -1099,6 +1099,11 @@ static int sata_mv_probe(struct udevice *dev) continue; }
+ ret = blk_probe_or_unbind(dev); + if (ret < 0) + /* TODO: undo create */ + continue; + /* If we got here, the current SATA port was probed * successfully, so set the probe status to successful. */ diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c index dda712f42cb2..cffeb01561b1 100644 --- a/drivers/ata/sata_sil.c +++ b/drivers/ata/sata_sil.c @@ -864,6 +864,18 @@ static int sil_pci_probe(struct udevice *dev) failed_number++; continue; } + + ret = device_probe(dev); + if (ret < 0) { + debug("Probing %s failed (%d)\n", dev, ret); + ret = sil_unbind_device(blk); + device_unbind(dev); + if (ret) + return ret; + + failed_number++; + continue; + } }
if (failed_number == sata_info.maxport)
Every time an ide bus/port is scanned and a new device is detected, we want to call device_probe() as it will give us a chance to run additional post-processings for some purposes.
In particular, support for creating partitions on a device will be added.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- drivers/block/ide.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/block/ide.c b/drivers/block/ide.c index 085aa356feef..a96df4f0a3c7 100644 --- a/drivers/block/ide.c +++ b/drivers/block/ide.c @@ -1145,6 +1145,10 @@ static int ide_probe(struct udevice *udev) blksz, size, &blk_dev); if (ret) return ret; + + ret = blk_probe_or_unbind(blk_dev); + if (ret) + return ret; } }
virtio_init() enumerates all the peripherals that are to be materialised with udevices(UCLASS_VIRIO) and creates particular device instances (UCLASS_BlK or whatever else) as children. On the other hand, device_probe() won't be invoked against those resultant udevices unlike other ordinary device drivers do in the driver model.
This is particularly inconvenient when we want to add "event notification" callback so that we will be able to automatically create all efi_disk objects in a later patch.
With this patch applied, "virtio scan" will work in a similar way to "scsi rescan", "usb start" or others in term of 'probe' semantics.
I didn't add this change to virtio_init() itself because this function may be called in board_init_r() (indirectly in board_late_init()) before UEFI subsustem is initialized.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- cmd/virtio.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/cmd/virtio.c b/cmd/virtio.c index 3dace5344f7e..ea3ed2e631e4 100644 --- a/cmd/virtio.c +++ b/cmd/virtio.c @@ -17,8 +17,25 @@ static int do_virtio(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { if (argc == 2 && !strcmp(argv[1], "scan")) { - /* make sure all virtio devices are enumerated */ - virtio_init(); + /* + * make sure all virtio devices are enumerated. + * Do the same as virtio_init(), but also call + * device_probe() for children (i.e. virtio devices) + */ + struct udevice *bus, *child; + int ret; + + ret = uclass_first_device(UCLASS_VIRTIO, &bus); + if (ret) + return CMD_RET_FAILURE; + + while (bus) { + device_foreach_child_probe(child, bus) + ; + ret = uclass_next_device(&bus); + if (ret) + break; + }
return CMD_RET_SUCCESS; }
From: Simon Glass sjg@chromium.org
This is a draft implementation of event notification mechanism from Simon. Under this scheme, any U-Boot subsystem can register some kind of callback function to a particular event (more event types will be added later) and that function will be invoked once the event is fired.
As a first user, UEFI subsystem makes use of PROBE and REMOVE events so that we will be able to automatically create/remove efi_disk objects relying on associated block devices (UCLASS_BLK).
To run the test: ./u-boot -T -c "ut common test_event_probe"
Signed-off-by: Simon Glass sjg@chromium.org [add REMOVE event; fix checkpatch warnings] Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- common/Kconfig | 11 ++++ common/Makefile | 2 + common/board_f.c | 2 + common/event.c | 103 +++++++++++++++++++++++++++++ common/log.c | 1 + drivers/core/device-remove.c | 9 +++ drivers/core/device.c | 9 +++ include/asm-generic/global_data.h | 6 ++ include/dm/device-internal.h | 10 +++ include/event.h | 105 ++++++++++++++++++++++++++++++ include/event_internal.h | 34 ++++++++++ include/log.h | 2 + test/common/Makefile | 1 + test/common/event.c | 87 +++++++++++++++++++++++++ test/test-main.c | 7 ++ 15 files changed, 389 insertions(+) create mode 100644 common/event.c create mode 100644 include/event.h create mode 100644 include/event_internal.h create mode 100644 test/common/event.c
diff --git a/common/Kconfig b/common/Kconfig index 82cd864baf93..d411e5bf7a50 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -492,6 +492,17 @@ config DISPLAY_BOARDINFO_LATE
menu "Start-up hooks"
+config EVENT + bool "General-purpose event-handling mechanism" + default y if SANDBOX + help + This enables sending and processing of events, to allow interested + parties to be alerted when something happens. This is an attempt to + step the flow of weak functions, hooks, functions in board_f.c + and board_r.c and the Kconfig options below. + + See doc/develop/event.rst for more information. + config ARCH_EARLY_INIT_R bool "Call arch-specific init soon after relocation" help diff --git a/common/Makefile b/common/Makefile index 3eff71960160..cc2ba30c631f 100644 --- a/common/Makefile +++ b/common/Makefile @@ -89,6 +89,8 @@ obj-y += malloc_simple.o endif endif
+obj-$(CONFIG_$(SPL_TPL_)EVENT) += event.o + obj-$(CONFIG_$(SPL_TPL_)HASH) += hash.o obj-$(CONFIG_IO_TRACE) += iotrace.o obj-y += memsize.o diff --git a/common/board_f.c b/common/board_f.c index a68760092ac1..e36bdbc988fa 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -19,6 +19,7 @@ #include <dm.h> #include <env.h> #include <env_internal.h> +#include <event.h> #include <fdtdec.h> #include <fs.h> #include <hang.h> @@ -828,6 +829,7 @@ static const init_fnc_t init_sequence_f[] = { initf_malloc, log_init, initf_bootstage, /* uses its own timer, so does not need DM */ + event_init, #ifdef CONFIG_BLOBLIST bloblist_init, #endif diff --git a/common/event.c b/common/event.c new file mode 100644 index 000000000000..428628da44d6 --- /dev/null +++ b/common/event.c @@ -0,0 +1,103 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Events provide a general-purpose way to react to / subscribe to changes + * within U-Boot + * + * Copyright 2021 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#define LOG_CATEGORY LOGC_EVENT + +#include <common.h> +#include <event.h> +#include <event_internal.h> +#include <log.h> +#include <malloc.h> +#include <asm/global_data.h> +#include <linux/list.h> + +DECLARE_GLOBAL_DATA_PTR; + +static void spy_free(struct event_spy *spy) +{ + list_del(&spy->sibling_node); +} + +int event_register(const char *id, enum event_t type, event_handler_t func, void *ctx) +{ + struct event_state *state = gd->event_state; + struct event_spy *spy; + + spy = malloc(sizeof(*spy)); + if (!spy) + return log_msg_ret("alloc", -ENOMEM); + + spy->id = id; + spy->type = type; + spy->func = func; + spy->ctx = ctx; + list_add_tail(&spy->sibling_node, &state->spy_head); + + return 0; +} + +int event_notify(enum event_t type, void *data, int size) +{ + struct event_state *state = gd->event_state; + struct event_spy *spy, *next; + struct event event; + + event.type = type; + if (size > sizeof(event.data)) + return log_msg_ret("size", -E2BIG); + memcpy(&event.data, data, size); + list_for_each_entry_safe(spy, next, &state->spy_head, sibling_node) { + if (spy->type == type) { + int ret; + + log_debug("Sending event %x to spy '%s'\n", type, + spy->id); + ret = spy->func(spy->ctx, &event); + + /* + * TODO: Handle various return codes to + * + * - claim an event (no others will see it) + * - return an error from the event + */ + if (ret) + return log_msg_ret("spy", ret); + } + } + + return 0; +} + +int event_uninit(void) +{ + struct event_state *state = gd->event_state; + struct event_spy *spy, *next; + + if (!state) + return 0; + list_for_each_entry_safe(spy, next, &state->spy_head, sibling_node) + spy_free(spy); + + return 0; +} + +int event_init(void) +{ + struct event_state *state; + + state = malloc(sizeof(struct event_state)); + if (!state) + return log_msg_ret("alloc", -ENOMEM); + + INIT_LIST_HEAD(&state->spy_head); + + gd->event_state = state; + + return 0; +} diff --git a/common/log.c b/common/log.c index f7e0c0fbf556..7254aa70bfdf 100644 --- a/common/log.c +++ b/common/log.c @@ -28,6 +28,7 @@ static const char *const log_cat_name[] = { "devres", "acpi", "boot", + "event", };
_Static_assert(ARRAY_SIZE(log_cat_name) == LOGC_COUNT - LOGC_NONE, diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index e6ec6ff42121..c3f8ed3e5721 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -12,6 +12,7 @@
#include <common.h> #include <errno.h> +#include <event.h> #include <log.h> #include <malloc.h> #include <dm/device.h> @@ -207,6 +208,10 @@ int device_remove(struct udevice *dev, uint flags) if (!(dev_get_flags(dev) & DM_FLAG_ACTIVATED)) return 0;
+ ret = device_notify(dev, EVT_DM_PRE_REMOVE); + if (ret) + return ret; + /* * If the child returns EKEYREJECTED, continue. It just means that it * didn't match the flags. @@ -256,6 +261,10 @@ int device_remove(struct udevice *dev, uint flags)
dev_bic_flags(dev, DM_FLAG_ACTIVATED);
+ ret = device_notify(dev, EVT_DM_POST_REMOVE); + if (ret) + goto err_remove; + return 0;
err_remove: diff --git a/drivers/core/device.c b/drivers/core/device.c index 901c1e2f7db3..6f69b234a182 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -10,6 +10,7 @@
#include <common.h> #include <cpu_func.h> +#include <event.h> #include <log.h> #include <asm/global_data.h> #include <asm/io.h> @@ -493,6 +494,10 @@ int device_probe(struct udevice *dev) if (dev_get_flags(dev) & DM_FLAG_ACTIVATED) return 0;
+ ret = device_notify(dev, EVT_DM_PRE_PROBE); + if (ret) + return ret; + drv = dev->driver; assert(drv);
@@ -597,6 +602,10 @@ int device_probe(struct udevice *dev) dev->name, ret, errno_str(ret)); }
+ ret = device_notify(dev, EVT_DM_POST_PROBE); + if (ret) + goto fail; + return 0; fail_uclass: if (device_remove(dev, DM_REMOVE_NORMAL)) { diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index c2f8fad1cb92..a65a85de3ea5 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -467,6 +467,12 @@ struct global_data { */ char *smbios_version; #endif +#if CONFIG_IS_ENABLED(EVENT) + /** + * @event_state: List of event notifications + */ + struct event_state *event_state; +#endif }; #ifndef DO_DEPS_ONLY static_assert(sizeof(struct global_data) == GD_SIZE); diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h index 02002acb787c..c33bbf3b29b9 100644 --- a/include/dm/device-internal.h +++ b/include/dm/device-internal.h @@ -10,6 +10,7 @@ #ifndef _DM_DEVICE_INTERNAL_H #define _DM_DEVICE_INTERNAL_H
+#include <event.h> #include <linker_lists.h> #include <dm/ofnode.h>
@@ -426,4 +427,13 @@ static inline void devres_release_all(struct udevice *dev) }
#endif /* ! CONFIG_DEVRES */ + +static inline int device_notify(const struct udevice *dev, enum event_t type) +{ +#if CONFIG_IS_ENABLED(EVENT) + return event_notify(type, &dev, sizeof(dev)); +#else + return 0; +#endif +} #endif diff --git a/include/event.h b/include/event.h new file mode 100644 index 000000000000..e2b74e6e62f0 --- /dev/null +++ b/include/event.h @@ -0,0 +1,105 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Events provide a general-purpose way to react to / subscribe to changes + * within U-Boot + * + * Copyright 2021 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#ifndef __event_h +#define __event_h + +/** + * enum event_t - Types of events supported by U-Boot + * + * @EVT_DM_PRE_PROBE: Device is about to be probed + */ +enum event_t { + EVT_NONE, + EVT_TEST, + + /* Events related to driver model */ + EVT_DM_PRE_PROBE, + EVT_DM_POST_PROBE, + EVT_DM_PRE_REMOVE, + EVT_DM_POST_REMOVE, + + EVT_COUNT +}; + +union event_data { + /** + * struct event_data_test - test data + * + * @signal: A value to update the state with + */ + struct event_data_test { + int signal; + } test; + + /** + * struct event_dm - driver model event + * + * @dev: Device this event relates to + */ + struct event_dm { + struct udevice *dev; + } dm; +}; + +/** + * struct event - an event that can be sent and received + * + * @type: Event type + * @data: Data for this particular event + */ +struct event { + enum event_t type; + union event_data data; +}; + +/** Function type for event handlers */ +typedef int (*event_handler_t)(void *ctx, struct event *event); + +/** + * event_register - register a new spy + * + * @id: Spy ID + * @type: Event type to subscribe to + * @func: Function to call when the event is sent + * @ctx: Context to pass to the function + * @return 0 if OK, -ve on erropr + */ +int event_register(const char *id, enum event_t type, event_handler_t func, + void *ctx); + +/** + * event_notify() - notify spies about an event + * + * It is possible to pass in union event_data here but that may not be + * convenient if the data is elsewhere, or is one of the members of the union. + * So this uses a void * for @data, with a separate @size. + * + * @type: Event type + * @data: Event data to be sent (e.g. union_event_data) + * @size: Size of data in bytes + */ +int event_notify(enum event_t type, void *data, int size); + +#if CONFIG_IS_ENABLED(EVENT) +int event_uninit(void); +int event_init(void); +#else +static inline int event_uninit(void) +{ + return 0; +} + +static inline int event_init(void) +{ + return 0; +} +#endif + +#endif diff --git a/include/event_internal.h b/include/event_internal.h new file mode 100644 index 000000000000..19308453f7b2 --- /dev/null +++ b/include/event_internal.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Internal definitions for events + * + * Copyright 2021 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#ifndef __event_internal_h +#define __event_internal_h + +#include <linux/list.h> + +/** + * struct event_spy - a spy that watches for an event of a particular type + * + * @id: Spy ID + * @type: Event type to subscribe to + * @func: Function to call when the event is sent + * @ctx: Context to pass to the function + */ +struct event_spy { + struct list_head sibling_node; + const char *id; + enum event_t type; + event_handler_t func; + void *ctx; +}; + +struct event_state { + struct list_head spy_head; +}; + +#endif diff --git a/include/log.h b/include/log.h index ce48d51446f5..8f35c10abb5e 100644 --- a/include/log.h +++ b/include/log.h @@ -98,6 +98,8 @@ enum log_category_t { LOGC_ACPI, /** @LOGC_BOOT: Related to boot process / boot image processing */ LOGC_BOOT, + /** @LOGC_EVENT: Related to event and event handling */ + LOGC_EVENT, /** @LOGC_COUNT: Number of log categories */ LOGC_COUNT, /** @LOGC_END: Sentinel value for lists of log categories */ diff --git a/test/common/Makefile b/test/common/Makefile index 24c9145dccc8..9087788ba6a8 100644 --- a/test/common/Makefile +++ b/test/common/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0+ obj-y += cmd_ut_common.o obj-$(CONFIG_AUTOBOOT) += test_autoboot.o +obj-$(CONFIG_EVENT) += event.o diff --git a/test/common/event.c b/test/common/event.c new file mode 100644 index 000000000000..ddce7400f269 --- /dev/null +++ b/test/common/event.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Unit tests for event handling + * + * Copyright 2021 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#include <common.h> +#include <dm.h> +#include <event.h> +#include <test/common.h> +#include <test/test.h> +#include <test/ut.h> + +struct test_state { + struct udevice *dev; + int val; +}; + +static int h_adder(void *ctx, struct event *event) +{ + struct event_data_test *data = &event->data.test; + struct test_state *test_state = ctx; + + test_state->val += data->signal; + + return 0; +} + +static int test_event_base(struct unit_test_state *uts) +{ + struct test_state state; + int signal; + + state.val = 12; + ut_assertok(event_register("wibble", EVT_TEST, h_adder, &state)); + + signal = 17; + + /* Check that the handler is called */ + ut_assertok(event_notify(EVT_TEST, &signal, sizeof(signal))); + ut_asserteq(12 + 17, state.val); + + return 0; +} + +COMMON_TEST(test_event_base, 0); + +static int h_probe(void *ctx, struct event *event) +{ + struct test_state *test_state = ctx; + + test_state->dev = event->data.dm.dev; + switch (event->type) { + case EVT_DM_PRE_PROBE: + test_state->val |= 1; + break; + case EVT_DM_POST_PROBE: + test_state->val |= 2; + break; + default: + break; + } + + return 0; +} + +static int test_event_probe(struct unit_test_state *uts) +{ + struct test_state state; + struct udevice *dev; + + state.val = 0; + ut_assertok(event_register("pre", EVT_DM_PRE_PROBE, h_probe, &state)); + ut_assertok(event_register("post", EVT_DM_POST_PROBE, h_probe, &state)); + + /* Probe a device */ + ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev)); + + /* Check that the handler is called */ + ut_asserteq(3, state.val); + + return 0; +} + +COMMON_TEST(test_event_probe, UT_TESTF_DM | UT_TESTF_SCAN_FDT); diff --git a/test/test-main.c b/test/test-main.c index 8fcb02ecea5c..dedfd0f81dae 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -7,6 +7,7 @@ #include <common.h> #include <console.h> #include <dm.h> +#include <event.h> #include <dm/root.h> #include <dm/test.h> #include <dm/uclass-internal.h> @@ -218,6 +219,11 @@ static int dm_test_restore(struct device_node *of_root) */ static int test_pre_run(struct unit_test_state *uts, struct unit_test *test) { +#if CONFIG_IS_ENABLED(EVENT) + gd->event_state = NULL; +#endif + ut_assertok(event_init()); + if (test->flags & UT_TESTF_DM) ut_assertok(dm_test_pre_run(uts));
@@ -260,6 +266,7 @@ static int test_post_run(struct unit_test_state *uts, struct unit_test *test) ut_unsilence_console(uts); if (test->flags & UT_TESTF_DM) ut_assertok(dm_test_post_run(uts)); + ut_assertok(event_uninit());
return 0; }
With dm-tag feature, any U-Boot subsystem is allowed to associate arbitrary number of data with a particular udevice. This can been see as expanding "struct udevice" without modifying the definition.
As a first user, UEFI subsystem makes use of tags to associate an efi_disk object with a block device.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- drivers/core/Makefile | 2 +- drivers/core/root.c | 2 + drivers/core/tag.c | 139 ++++++++++++++++++++++++++++++ include/asm-generic/global_data.h | 4 + include/dm/tag.h | 110 +++++++++++++++++++++++ 5 files changed, 256 insertions(+), 1 deletion(-) create mode 100644 drivers/core/tag.c create mode 100644 include/dm/tag.h
diff --git a/drivers/core/Makefile b/drivers/core/Makefile index 5edd4e413576..3742e7574525 100644 --- a/drivers/core/Makefile +++ b/drivers/core/Makefile @@ -2,7 +2,7 @@ # # Copyright (c) 2013 Google, Inc
-obj-y += device.o fdtaddr.o lists.o root.o uclass.o util.o +obj-y += device.o fdtaddr.o lists.o root.o uclass.o util.o tag.o obj-$(CONFIG_$(SPL_TPL_)ACPIGEN) += acpi.o obj-$(CONFIG_DEVRES) += devres.o obj-$(CONFIG_$(SPL_)DM_DEVICE_REMOVE) += device-remove.o diff --git a/drivers/core/root.c b/drivers/core/root.c index e3f87956d866..1aa4819ceb6c 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -199,6 +199,8 @@ int dm_init(bool of_live) return ret; }
+ INIT_LIST_HEAD((struct list_head *)&gd->dmtag_list); + return 0; }
diff --git a/drivers/core/tag.c b/drivers/core/tag.c new file mode 100644 index 000000000000..6829bcd8806c --- /dev/null +++ b/drivers/core/tag.c @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2021 Linaro Limited + * Author: AKASHI Takahiro + */ + +#include <malloc.h> +#include <asm/global_data.h> +#include <dm/tag.h> +#include <linux/err.h> +#include <linux/list.h> +#include <linux/types.h> + +struct udevice; + +DECLARE_GLOBAL_DATA_PTR; + +int dev_tag_set_ptr(struct udevice *dev, enum dm_tag_t tag, void *ptr) +{ + struct dmtag_node *node; + + if (!dev || tag >= DM_TAG_COUNT) + return -EINVAL; + + list_for_each_entry(node, &gd->dmtag_list, sibling) { + if (node->dev == dev && node->tag == tag) + return -EEXIST; + } + + node = calloc(sizeof(*node), 1); + if (!node) + return -ENOSPC; + + node->dev = dev; + node->tag = tag; + node->ptr = ptr; + list_add_tail(&node->sibling, (struct list_head *)&gd->dmtag_list); + + return 0; +} + +int dev_tag_set_val(struct udevice *dev, enum dm_tag_t tag, ulong val) +{ + struct dmtag_node *node; + + if (!dev || tag >= DM_TAG_COUNT) + return -EINVAL; + + list_for_each_entry(node, &gd->dmtag_list, sibling) { + if (node->dev == dev && node->tag == tag) + return -EEXIST; + } + + node = calloc(sizeof(*node), 1); + if (!node) + return -ENOSPC; + + node->dev = dev; + node->tag = tag; + node->val = val; + list_add_tail(&node->sibling, (struct list_head *)&gd->dmtag_list); + + return 0; +} + +int dev_tag_get_ptr(struct udevice *dev, enum dm_tag_t tag, void **ptrp) +{ + struct dmtag_node *node; + + if (!dev || tag >= DM_TAG_COUNT) + return -EINVAL; + + list_for_each_entry(node, &gd->dmtag_list, sibling) { + if (node->dev == dev && node->tag == tag) { + *ptrp = node->ptr; + return 0; + } + } + + return -ENOENT; +} + +int dev_tag_get_val(struct udevice *dev, enum dm_tag_t tag, ulong *valp) +{ + struct dmtag_node *node; + + if (!dev || tag >= DM_TAG_COUNT) + return -EINVAL; + + list_for_each_entry(node, &gd->dmtag_list, sibling) { + if (node->dev == dev && node->tag == tag) { + *valp = node->val; + return 0; + } + } + + return -ENOENT; +} + +int dev_tag_del(struct udevice *dev, enum dm_tag_t tag) +{ + struct dmtag_node *node, *tmp; + + if (!dev || tag >= DM_TAG_COUNT) + return -EINVAL; + + list_for_each_entry_safe(node, tmp, &gd->dmtag_list, sibling) { + if (node->dev == dev && node->tag == tag) { + list_del(&node->sibling); + free(node); + + return 0; + } + } + + return -ENOENT; +} + +int dev_tag_del_all(struct udevice *dev) +{ + struct dmtag_node *node, *tmp; + bool found = false; + + if (!dev) + return -EINVAL; + + list_for_each_entry_safe(node, tmp, &gd->dmtag_list, sibling) { + if (node->dev == dev) { + list_del(&node->sibling); + free(node); + found = true; + } + } + + if (found) + return 0; + + return -ENOENT; +} diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index a65a85de3ea5..5b599a274847 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -473,6 +473,10 @@ struct global_data { */ struct event_state *event_state; #endif + /** + * @dmtag_list: List of DM tags + */ + struct list_head dmtag_list; }; #ifndef DO_DEPS_ONLY static_assert(sizeof(struct global_data) == GD_SIZE); diff --git a/include/dm/tag.h b/include/dm/tag.h new file mode 100644 index 000000000000..54fc31eb1539 --- /dev/null +++ b/include/dm/tag.h @@ -0,0 +1,110 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright (c) 2021 Linaro Limited + * Author: AKASHI Takahiro + */ + +#ifndef _DM_TAG_H +#define _DM_TAG_H + +#include <linux/list.h> +#include <linux/types.h> + +struct udevice; + +enum dm_tag_t { + /* EFI_LOADER */ + DM_TAG_EFI = 0, + + DM_TAG_COUNT, +}; + +/** + * dmtag_node + * + * @sibling: List of dm-tag nodes + * @dev: Associated udevice + * @tag: Tag type + * @ptr: Pointer as a value + * @val: Value + */ +struct dmtag_node { + struct list_head sibling; + struct udevice *dev; + enum dm_tag_t tag; + union { + void *ptr; + ulong val; + }; +}; + +/** + * dev_tag_set_ptr() - set a tag's value as a pointer + * @dev: Device to operate + * @tag: Tag type + * @ptr: Pointer to set + * + * Set the value, @ptr, as of @tag associated with the device, @dev + * + * Return: 0 on success, -ve on error + */ +int dev_tag_set_ptr(struct udevice *dev, enum dm_tag_t tag, void *ptr); + +/** + * dev_tag_set_val() set a tag's value as an integer + * @dev: Device to operate + * @tag: Tag type + * @val: Value to set + * + * Set the value, @val, as of @tag associated with the device, @dev + * + * Return: on success, -ve on error + */ +int dev_tag_set_val(struct udevice *dev, enum dm_tag_t tag, ulong val); + +/** + * dev_tag_get_ptr() - get a tag's value as a pointer + * @dev: Device to operate + * @tag: Tag type + * @ptrp: Pointer to tag's value (pointer) + * + * Get a tag's value as a pointer + * + * Return: on success, -ve on error + */ +int dev_tag_get_ptr(struct udevice *dev, enum dm_tag_t tag, void **ptrp); + +/** + * dev_tag_get_val() - get a tag's value as an integer + * @dev: Device to operate + * @tag: Tag type + * @valp: Pointer to tag's value (ulong) + * + * Get a tag's value as an integer + * + * Return: 0 on success, -ve on error + */ +int dev_tag_get_val(struct udevice *dev, enum dm_tag_t tag, ulong *valp); + +/** + * dev_tag_del() - delete a tag + * @dev: Device to operate + * @tag: Tag type + * + * Delete a tag of @tag associated with the device, @dev + * + * Return: 0 on success, -ve on error + */ +int dev_tag_del(struct udevice *dev, enum dm_tag_t tag); + +/** + * dev_tag_del_all() - delete all tags + * @dev: Device to operate + * + * Delete all the tags associated with the device, @dev + * + * Return: 0 on success, -ve on error + */ +int dev_tag_del_all(struct udevice *dev); + +#endif /* _DM_TAG_H */
On Thu, 10 Feb 2022 at 01:12, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
With dm-tag feature, any U-Boot subsystem is allowed to associate arbitrary number of data with a particular udevice. This can been see as expanding "struct udevice" without modifying the definition.
As a first user, UEFI subsystem makes use of tags to associate an efi_disk object with a block device.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
drivers/core/Makefile | 2 +- drivers/core/root.c | 2 + drivers/core/tag.c | 139 ++++++++++++++++++++++++++++++ include/asm-generic/global_data.h | 4 + include/dm/tag.h | 110 +++++++++++++++++++++++ 5 files changed, 256 insertions(+), 1 deletion(-) create mode 100644 drivers/core/tag.c create mode 100644 include/dm/tag.h
Reviewed-by: Simon Glass sjg@chromium.org
Some basic stuff about tag support is explained under doc/devlop/driver-model.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- doc/develop/driver-model/design.rst | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/doc/develop/driver-model/design.rst b/doc/develop/driver-model/design.rst index b0e6337030a1..3e88dc40e6fd 100644 --- a/doc/develop/driver-model/design.rst +++ b/doc/develop/driver-model/design.rst @@ -1042,6 +1042,26 @@ data structure might be worthwhile in some rare cases, once we understand what the bottlenecks are.
+Tag Support +----------- + +It is sometimes useful for a subsystem to associate its own private +data (or object) to a DM device, i.e. struct udevice, to support +additional features. + +Tag support in driver model will give us the ability to do so dynamically +instead of modifying "udevice" data structure. In the initial release, we +will support two type of attributes: +- a pointer with dm_tag_set_ptr(), and +- an unsigned long with dm_tag_set_val() + +For example, UEFI subsystem utilizes the feature to maintain efi_disk +objects depending on linked udevice's lifecycle. + +While the current implementation is quite simple, it will get evolved +as the feature is more extensively used in U-Boot subsystems. + + Changes since v1 ----------------
On Thu, 10 Feb 2022 at 01:12, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Some basic stuff about tag support is explained under doc/devlop/driver-model.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
doc/develop/driver-model/design.rst | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
The new test covers all tag-related interfaces.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- test/dm/Makefile | 1 + test/dm/tag.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 test/dm/tag.c
diff --git a/test/dm/Makefile b/test/dm/Makefile index d46552fbf320..dc3177dbb7f4 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -102,6 +102,7 @@ obj-y += syscon.o obj-$(CONFIG_RESET_SYSCON) += syscon-reset.o obj-$(CONFIG_SYSINFO) += sysinfo.o obj-$(CONFIG_SYSINFO_GPIO) += sysinfo-gpio.o +obj-$(CONFIG_UT_DM) += tag.o obj-$(CONFIG_TEE) += tee.o obj-$(CONFIG_TIMER) += timer.o obj-$(CONFIG_DM_USB) += usb.o diff --git a/test/dm/tag.c b/test/dm/tag.c new file mode 100644 index 000000000000..8599fdc242d8 --- /dev/null +++ b/test/dm/tag.c @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * DM tag test + * + * Copyright (c) 2021 Linaro Limited + * Author: AKASHI Takahiro + */ + +#include <common.h> +#include <dm/tag.h> +#include <dm/test.h> /* DM_TEST() */ +#include <test/test.h> /* struct unit_test_state */ +#include <test/ut.h> /* assertions */ + +/* + * Test dm_tag_ptr() API + */ +static int dm_test_tag_ptr(struct unit_test_state *uts) +{ + ulong val; + void *ptr = NULL; + + ut_assertok(dev_tag_set_ptr(uts->root, DM_TAG_EFI, &val)); + + ut_assertok(dev_tag_get_ptr(uts->root, DM_TAG_EFI, &ptr)); + + ut_asserteq_ptr(&val, ptr); + + ut_assertok(dev_tag_del(uts->root, DM_TAG_EFI)); + + return 0; +} +DM_TEST(dm_test_tag_ptr, 0); + +/* + * Test dm_tag_val() API + */ +static int dm_test_tag_val(struct unit_test_state *uts) +{ + ulong val1 = 0x12345678, val2 = 0; + + ut_assertok(dev_tag_set_val(uts->root, DM_TAG_EFI, val1)); + + ut_assertok(dev_tag_get_val(uts->root, DM_TAG_EFI, &val2)); + + ut_asserteq_64(val1, val2); + + ut_assertok(dev_tag_del(uts->root, DM_TAG_EFI)); + + return 0; +} +DM_TEST(dm_test_tag_val, 0); + +/* + * Test against an invalid tag + */ +static int dm_test_tag_inval(struct unit_test_state *uts) +{ + ulong val; + + ut_asserteq(-EINVAL, dev_tag_set_ptr(uts->root, DM_TAG_COUNT, &val)); + + return 0; +} +DM_TEST(dm_test_tag_inval, 0); + +/* + * Test dm_tag_del_all() AP: + */ +static int dm_test_tag_del_all(struct unit_test_state *uts) +{ + ulong val; + + ut_assertok(dev_tag_set_ptr(uts->root, DM_TAG_EFI, &val)); + + ut_assertok(dev_tag_del_all(uts->root)); + + return 0; +} +DM_TEST(dm_test_tag_del_all, 0);
On Thu, 10 Feb 2022 at 01:12, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
The new test covers all tag-related interfaces.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
test/dm/Makefile | 1 + test/dm/tag.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 test/dm/tag.c
Reviewed-by: Simon Glass sjg@chromium.org
With this new function, UCLASS_PARTITION devices will be created as child nodes of UCLASS_BLK device.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- disk/Makefile | 3 + disk/disk-uclass.c | 153 +++++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/part.h | 11 +++ 4 files changed, 168 insertions(+) create mode 100644 disk/disk-uclass.c
diff --git a/disk/Makefile b/disk/Makefile index 6ce5a687b36c..ec37b74f5f40 100644 --- a/disk/Makefile +++ b/disk/Makefile @@ -6,6 +6,9 @@ #ccflags-y += -DET_DEBUG -DDEBUG
obj-$(CONFIG_PARTITIONS) += part.o +ifdef CONFIG_$(SPL_)BLK +obj-$(CONFIG_PARTITIONS) += disk-uclass.o +endif obj-$(CONFIG_$(SPL_)MAC_PARTITION) += part_mac.o obj-$(CONFIG_$(SPL_)DOS_PARTITION) += part_dos.o obj-$(CONFIG_$(SPL_)ISO_PARTITION) += part_iso.o diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c new file mode 100644 index 000000000000..4918a2f72d1e --- /dev/null +++ b/disk/disk-uclass.c @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Software partition device (UCLASS_PARTITION) + * + * Copyright (c) 2021 Linaro Limited + * Author: AKASHI Takahiro + */ + +#define LOG_CATEGORY UCLASS_PARTITION + +#include <blk.h> +#include <dm.h> +#include <log.h> +#include <part.h> +#include <vsprintf.h> +#include <dm/device-internal.h> +#include <dm/lists.h> + +int part_create_block_devices(struct udevice *blk_dev) +{ + int part, count; + struct blk_desc *desc = dev_get_uclass_plat(blk_dev); + struct disk_partition info; + struct disk_part *part_data; + char devname[32]; + struct udevice *dev; + int ret; + + if (!CONFIG_IS_ENABLED(PARTITIONS) || + !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE)) + return 0; + + if (device_get_uclass_id(blk_dev) != UCLASS_BLK) + return 0; + + /* Add devices for each partition */ + for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { + if (part_get_info(desc, part, &info)) + continue; + snprintf(devname, sizeof(devname), "%s:%d", blk_dev->name, + part); + + ret = device_bind_driver(blk_dev, "blk_partition", + strdup(devname), &dev); + if (ret) + return ret; + + part_data = dev_get_uclass_plat(dev); + part_data->partnum = part; + part_data->gpt_part_info = info; + count++; + + ret = device_probe(dev); + if (ret) { + debug("Can't probe\n"); + count--; + device_unbind(dev); + + continue; + } + } + debug("%s: %d partitions found in %s\n", __func__, count, + blk_dev->name); + + return 0; +} + +static ulong blk_part_read(struct udevice *dev, lbaint_t start, + lbaint_t blkcnt, void *buffer) +{ + struct udevice *parent; + struct disk_part *part; + const struct blk_ops *ops; + + parent = dev_get_parent(dev); + ops = blk_get_ops(parent); + if (!ops->read) + return -ENOSYS; + + part = dev_get_uclass_plat(dev); + if (start >= part->gpt_part_info.size) + return 0; + + if ((start + blkcnt) > part->gpt_part_info.size) + blkcnt = part->gpt_part_info.size - start; + start += part->gpt_part_info.start; + + return ops->read(parent, start, blkcnt, buffer); +} + +static ulong blk_part_write(struct udevice *dev, lbaint_t start, + lbaint_t blkcnt, const void *buffer) +{ + struct udevice *parent; + struct disk_part *part; + const struct blk_ops *ops; + + parent = dev_get_parent(dev); + ops = blk_get_ops(parent); + if (!ops->write) + return -ENOSYS; + + part = dev_get_uclass_plat(dev); + if (start >= part->gpt_part_info.size) + return 0; + + if ((start + blkcnt) > part->gpt_part_info.size) + blkcnt = part->gpt_part_info.size - start; + start += part->gpt_part_info.start; + + return ops->write(parent, start, blkcnt, buffer); +} + +static ulong blk_part_erase(struct udevice *dev, lbaint_t start, + lbaint_t blkcnt) +{ + struct udevice *parent; + struct disk_part *part; + const struct blk_ops *ops; + + parent = dev_get_parent(dev); + ops = blk_get_ops(parent); + if (!ops->erase) + return -ENOSYS; + + part = dev_get_uclass_plat(dev); + if (start >= part->gpt_part_info.size) + return 0; + + if ((start + blkcnt) > part->gpt_part_info.size) + blkcnt = part->gpt_part_info.size - start; + start += part->gpt_part_info.start; + + return ops->erase(parent, start, blkcnt); +} + +static const struct blk_ops blk_part_ops = { + .read = blk_part_read, + .write = blk_part_write, + .erase = blk_part_erase, +}; + +U_BOOT_DRIVER(blk_partition) = { + .name = "blk_partition", + .id = UCLASS_PARTITION, + .ops = &blk_part_ops, +}; + +UCLASS_DRIVER(partition) = { + .id = UCLASS_PARTITION, + .per_device_plat_auto = sizeof(struct disk_part), + .name = "partition", +}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 0e26e1d13824..230b1ea528cf 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -83,6 +83,7 @@ enum uclass_id { UCLASS_P2SB, /* (x86) Primary-to-Sideband Bus */ UCLASS_PANEL, /* Display panel, such as an LCD */ UCLASS_PANEL_BACKLIGHT, /* Backlight controller for panel */ + UCLASS_PARTITION, /* Logical disk partition device */ UCLASS_PCH, /* x86 platform controller hub */ UCLASS_PCI, /* PCI bus */ UCLASS_PCI_EP, /* PCI endpoint device */ diff --git a/include/part.h b/include/part.h index 53cfbdd87671..95e30e60af10 100644 --- a/include/part.h +++ b/include/part.h @@ -253,6 +253,17 @@ void part_set_generic_name(const struct blk_desc *dev_desc, int part_num, char *name);
extern const struct block_drvr block_drvr[]; + +struct udevice; +/** + * part_create_block_devices - Create block devices for disk partitions + * + * Create UCLASS_PARTITION udevices for each of disk partitions in @parent + * + * @blk_dev: Whole disk device + */ +int part_create_block_devices(struct udevice *blk_dev); + #else static inline struct blk_desc *blk_get_dev(const char *ifname, int dev) { return NULL; }
Now that all the block device drivers have enable a probe hook, we will call part_create_block_devices() to enumerate all the partitions and create associated udevices when a block device is detected.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- drivers/block/blk-uclass.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c index bee1cd6f0d80..58dc74e71f1e 100644 --- a/drivers/block/blk-uclass.c +++ b/drivers/block/blk-uclass.c @@ -717,6 +717,10 @@ static int blk_post_probe(struct udevice *dev) struct blk_desc *desc = dev_get_uclass_plat(dev);
part_init(desc); + + if (desc->part_type != PART_TYPE_UNKNOWN && + part_create_block_devices(dev)) + debug("*** creating partitions failed\n"); }
return 0;
In the next commit, CONFIG_EFI_SETUP_EARLY will become mandated in order to support dynamic enumeration of efi_disk objects.
This can, however, be problematic particularly in case of file-based variable storage (efi_variable.c, default). Non-volatile variables are to be restored from EFI system partition by efi_init_variables() in efi_init_obj_list(). When efi_init_obj_list() is called in board_init_r(), we don't know yet what disk devices we have since none of device probing commands (say, scsi rescan) has not been executed at that stage.
So in this commit, a preparatory change is made; efi_init_obj_list() is broken into the two functions; * efi_init_early(), and * new efi_init_obj_list()
Only efi_init_early() will be called in board_init_r(), which allows us to execute any of device probing commands, either though "preboot" variable or normal command line, before calling efi_init_obj_list() which is to be invoked at the first execution of an efi-related command (or at efi_launch_capsules()) as used to be.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- common/board_r.c | 2 +- common/main.c | 7 +++-- include/efi_loader.h | 2 ++ lib/efi_loader/efi_setup.c | 58 ++++++++++++++++++++++++++++++++------ 4 files changed, 57 insertions(+), 12 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index c24d9b4e220b..7b004b8ae9af 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -789,7 +789,7 @@ static init_fnc_t init_sequence_r[] = { initr_mem, #endif #ifdef CONFIG_EFI_SETUP_EARLY - (init_fnc_t)efi_init_obj_list, + efi_init_early, #endif run_main_loop, }; diff --git a/common/main.c b/common/main.c index 3f5214fd44b8..682f3359ea38 100644 --- a/common/main.c +++ b/common/main.c @@ -54,8 +54,11 @@ void main_loop(void) if (IS_ENABLED(CONFIG_UPDATE_TFTP)) update_tftp(0UL, NULL, NULL);
- if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY)) - efi_launch_capsules(); + if (IS_ENABLED(CONFIG_EFI_CAPSULE_ON_DISK_EARLY)) { + /* efi_init_early() already called */ + if (efi_init_obj_list() == EFI_SUCCESS) + efi_launch_capsules(); + }
s = bootdelay_process(); if (cli_process_fdt(&s)) diff --git a/include/efi_loader.h b/include/efi_loader.h index 4e50f2d0c368..58b661d5c4c6 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -491,6 +491,8 @@ struct efi_register_notify_event { /* List of all events registered by RegisterProtocolNotify() */ extern struct list_head efi_register_notify_events;
+/* called at pre-initialization */ +int efi_init_early(void); /* Initialize efi execution environment */ efi_status_t efi_init_obj_list(void); /* Install device tree */ diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index 380adc15c886..74b4d2623f88 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -174,20 +174,18 @@ static efi_status_t efi_init_os_indications(void) &os_indications_supported, false); }
- /** - * efi_init_obj_list() - Initialize and populate EFI object list + * __efi_init_early() - handle initialization at early stage + * + * This function is called in efi_init_obj_list() only if + * !CONFIG_EFI_SETUP_EARLY. * * Return: status code */ -efi_status_t efi_init_obj_list(void) +static efi_status_t __efi_init_early(void) { efi_status_t ret = EFI_SUCCESS;
- /* Initialize once only */ - if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) - return efi_obj_list_initialized; - /* Allow unaligned memory access */ allow_unaligned();
@@ -202,9 +200,51 @@ efi_status_t efi_init_obj_list(void)
#ifdef CONFIG_PARTITIONS ret = efi_disk_register(); - if (ret != EFI_SUCCESS) - goto out; #endif +out: + return ret; +} + +/** + * efi_init_early() - handle initialization at early stage + * + * external version of __efi_init_early(); expected to be called in + * board_init_r(). + * + * Return: status code + */ +int efi_init_early(void) +{ + efi_status_t ret; + + ret = __efi_init_early(); + if (ret != EFI_SUCCESS) { + /* never re-init UEFI subsystem */ + efi_obj_list_initialized = ret; + return -1; + } + return 0; +} + +/** + * efi_init_obj_list() - Initialize and populate EFI object list + * + * Return: status code + */ +efi_status_t efi_init_obj_list(void) +{ + efi_status_t ret = EFI_SUCCESS; + + /* Initialize once only */ + if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) + return efi_obj_list_initialized; + + if (!IS_ENABLED(CONFIG_EFI_SETUP_EARLY)) { + ret = __efi_init_early(); + if (ret != EFI_SUCCESS) + goto out; + } + if (IS_ENABLED(CONFIG_EFI_RNG_PROTOCOL)) { ret = efi_rng_register(); if (ret != EFI_SUCCESS)
Add efi_disk_probe() function. This function creates an efi_disk object for a raw disk device (UCLASS_BLK) and additional objects for related partitions (UCLASS_PARTITION).
So this function is expected to be called through driver model's "probe" interface every time one raw disk device is detected and activated. We assume that partition devices (UCLASS_PARTITION) have been created when this function is invoked.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- include/efi_loader.h | 4 +- lib/efi_loader/Kconfig | 2 + lib/efi_loader/efi_disk.c | 204 +++++++++++++++++++++++-------------- lib/efi_loader/efi_setup.c | 4 +- 4 files changed, 135 insertions(+), 79 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 58b661d5c4c6..ed9d4f4ffa96 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -523,8 +523,8 @@ void efi_carve_out_dt_rsv(void *fdt); void efi_try_purge_kaslr_seed(void *fdt); /* Called by bootefi to make console interface available */ efi_status_t efi_console_register(void); -/* Called by bootefi to make all disk storage accessible as EFI objects */ -efi_status_t efi_disk_register(void); +/* Called by efi_init_obj_list() to initialize efi_disks */ +efi_status_t efi_disk_init(void); /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */ efi_status_t efi_rng_register(void); /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index e5e35fe51f65..57a417a7eb23 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -14,6 +14,7 @@ config EFI_LOADER depends on DM_ETH || !NET depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 + select EVENT select LIB_UUID select PARTITION_UUIDS select HAVE_BLOCK_DEVICE @@ -41,6 +42,7 @@ config CMD_BOOTEFI_BOOTMGR
config EFI_SETUP_EARLY bool + default y
choice prompt "Store for non-volatile UEFI variables" diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 45127d176869..243cd0e215d4 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -10,6 +10,9 @@ #include <common.h> #include <blk.h> #include <dm.h> +#include <dm/device-internal.h> +#include <dm/tag.h> +#include <event.h> #include <efi_loader.h> #include <fs.h> #include <log.h> @@ -487,103 +490,156 @@ error: return ret; }
-/** - * efi_disk_create_partitions() - create handles and protocols for partitions +/* + * Create a handle for a whole raw disk * - * Create handles and protocols for the partitions of a block device. + * @dev uclass device (UCLASS_BLK) * - * @parent: handle of the parent disk - * @desc: block device - * @if_typename: interface type - * @diskid: device number - * @pdevname: device name - * Return: number of partitions created + * Create an efi_disk object which is associated with @dev. + * The type of @dev must be UCLASS_BLK. + * + * @return 0 on success, -1 otherwise */ -int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, - const char *if_typename, int diskid, - const char *pdevname) +static int efi_disk_create_raw(struct udevice *dev) { - int disks = 0; - char devname[32] = { 0 }; /* dp->str is u16[32] long */ - int part; - struct efi_device_path *dp = NULL; + struct efi_disk_obj *disk; + struct blk_desc *desc; + const char *if_typename; + int diskid; efi_status_t ret; - struct efi_handler *handler;
- /* Get the device path of the parent */ - ret = efi_search_protocol(parent, &efi_guid_device_path, &handler); - if (ret == EFI_SUCCESS) - dp = handler->protocol_interface; - - /* Add devices for each partition */ - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { - struct disk_partition info; - - if (part_get_info(desc, part, &info)) - continue; - snprintf(devname, sizeof(devname), "%s:%x", pdevname, - part); - ret = efi_disk_add_dev(parent, dp, if_typename, desc, diskid, - &info, part, NULL); - if (ret != EFI_SUCCESS) { - log_err("Adding partition %s failed\n", pdevname); - continue; - } - disks++; + desc = dev_get_uclass_plat(dev); + if_typename = blk_get_if_type_name(desc->if_type); + diskid = desc->devnum; + + ret = efi_disk_add_dev(NULL, NULL, if_typename, desc, + diskid, NULL, 0, &disk); + if (ret != EFI_SUCCESS) { + if (ret == EFI_NOT_READY) + log_notice("Disk %s not ready\n", dev->name); + else + log_err("Adding disk for %s failed\n", dev->name); + + return -1; + } + if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { + efi_free_pool(disk->dp); + efi_delete_handle(&disk->header); + + return -1; }
- return disks; + return 0; }
-/** - * efi_disk_register() - register block devices - * - * U-Boot doesn't have a list of all online disk devices. So when running our - * EFI payload, we scan through all of the potentially available ones and - * store them in our object pool. +/* + * Create a handle for a disk partition * - * This function is called in efi_init_obj_list(). + * @dev uclass device (UCLASS_PARTITION) * - * TODO(sjg@chromium.org): Actually with CONFIG_BLK, U-Boot does have this. - * Consider converting the code to look up devices as needed. The EFI device - * could be a child of the UCLASS_BLK block device, perhaps. + * Create an efi_disk object which is associated with @dev. + * The type of @dev must be UCLASS_PARTITION. * - * Return: status code + * @return 0 on success, -1 otherwise */ -efi_status_t efi_disk_register(void) +static int efi_disk_create_part(struct udevice *dev) { + efi_handle_t parent; + struct blk_desc *desc; + const char *if_typename; + struct disk_part *part_data; + struct disk_partition *info; + unsigned int part; + int diskid; + struct efi_handler *handler; + struct efi_device_path *dp_parent; struct efi_disk_obj *disk; - int disks = 0; efi_status_t ret; + + if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent)) + return -1; + + desc = dev_get_uclass_plat(dev_get_parent(dev)); + if_typename = blk_get_if_type_name(desc->if_type); + diskid = desc->devnum; + + part_data = dev_get_uclass_plat(dev); + part = part_data->partnum; + info = &part_data->gpt_part_info; + + ret = efi_search_protocol(parent, &efi_guid_device_path, &handler); + if (ret != EFI_SUCCESS) + return -1; + dp_parent = (struct efi_device_path *)handler->protocol_interface; + + ret = efi_disk_add_dev(parent, dp_parent, if_typename, desc, diskid, + info, part, &disk); + if (ret != EFI_SUCCESS) { + log_err("Adding partition for %s failed\n", dev->name); + return -1; + } + if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { + efi_free_pool(disk->dp); + efi_delete_handle(&disk->header); + + return -1; + } + + return 0; +} + +/* + * Create efi_disk objects for a block device + * + * @dev uclass device (UCLASS_BLK) + * + * Create efi_disk objects for partitions as well as a raw disk + * which is associated with @dev. + * The type of @dev must be UCLASS_BLK. + * This function is expected to be called at EV_PM_POST_PROBE. + * + * @return 0 on success, -1 otherwise + */ +static int efi_disk_probe(void *ctx, struct event *event) +{ struct udevice *dev; + enum uclass_id id; + struct udevice *child; + int ret;
- for (uclass_first_device_check(UCLASS_BLK, &dev); dev; - uclass_next_device_check(&dev)) { - struct blk_desc *desc = dev_get_uclass_plat(dev); - const char *if_typename = blk_get_if_type_name(desc->if_type); + dev = event->data.dm.dev; + id = device_get_uclass_id(dev);
- /* Add block device for the full device */ - log_info("Scanning disk %s...\n", dev->name); - ret = efi_disk_add_dev(NULL, NULL, if_typename, - desc, desc->devnum, NULL, 0, &disk); - if (ret == EFI_NOT_READY) { - log_notice("Disk %s not ready\n", dev->name); - continue; - } - if (ret) { - log_err("ERROR: failure to add disk device %s, r = %lu\n", - dev->name, ret & ~EFI_ERROR_MASK); - continue; - } - disks++; + /* TODO: We won't support partitions in a partition */ + if (id != UCLASS_BLK) { + if (id != UCLASS_PARTITION) + log_info("Not a block device: %s\n", dev->name); + return 0; + } + + ret = efi_disk_create_raw(dev); + if (ret) + return -1;
- /* Partitions show up as block devices in EFI */ - disks += efi_disk_create_partitions( - &disk->header, desc, if_typename, - desc->devnum, dev->name); + device_foreach_child(child, dev) { + ret = efi_disk_create_part(child); + if (ret) + return -1; }
- log_info("Found %d disks\n", disks); + return 0; +} + +efi_status_t efi_disk_init(void) +{ + int ret; + + ret = event_register("efi_disk add", EVT_DM_POST_PROBE, + efi_disk_probe, NULL); + if (ret) { + log_err("Event registration for efi_disk add failed\n"); + return EFI_OUT_OF_RESOURCES; + }
return EFI_SUCCESS; } diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index 74b4d2623f88..b9b36a5f6707 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -198,9 +198,7 @@ static efi_status_t __efi_init_early(void) if (ret != EFI_SUCCESS) goto out;
-#ifdef CONFIG_PARTITIONS - ret = efi_disk_register(); -#endif + ret = efi_disk_init(); out: return ret; }
This function is expected to be called, in particular from dm's pre_remove hook, when associated block devices no longer exist.
Add efi_disk_remove() function. This function removes an efi_disk object for a raw disk device (UCLASS_BLK) and related objects for its partitions (UCLASS_PARTITION).
So this function is expected to be called through driver model's "remove" interface every time a raw disk device is to be disconnected.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- lib/efi_loader/efi_disk.c | 88 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 243cd0e215d4..7abe5d3a6bd2 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -630,6 +630,87 @@ static int efi_disk_probe(void *ctx, struct event *event) return 0; }
+/* + * Delete an efi_disk object for a whole raw disk + * + * @dev uclass device (UCLASS_BLK) + * + * Delete an efi_disk object which is associated with @dev. + * The type of @dev must be UCLASS_BLK. + * + * @return 0 on success, -1 otherwise + */ +static int efi_disk_delete_raw(struct udevice *dev) +{ + efi_handle_t handle; + struct efi_disk_obj *diskobj; + + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) + return -1; + + diskobj = container_of(handle, struct efi_disk_obj, header); + efi_free_pool(diskobj->dp); + + efi_delete_handle(handle); + dev_tag_del(dev, DM_TAG_EFI); + + return 0; +} + +/* + * Delete an efi_disk object for a disk partition + * + * @dev uclass device (UCLASS_PARTITION) + * + * Delete an efi_disk object which is associated with @dev. + * The type of @dev must be UCLASS_PARTITION. + * + * @return 0 on success, -1 otherwise + */ +static int efi_disk_delete_part(struct udevice *dev) +{ + efi_handle_t handle; + struct efi_disk_obj *diskobj; + + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) + return -1; + + diskobj = container_of(handle, struct efi_disk_obj, header); + + efi_free_pool(diskobj->dp); + efi_delete_handle(handle); + dev_tag_del(dev, DM_TAG_EFI); + + return 0; +} + +/* + * Delete an efi_disk object for a block device + * + * @dev uclass device (UCLASS_BLK or UCLASS_PARTITION) + * + * Delete an efi_disk object which is associated with @dev. + * The type of @dev must be either UCLASS_BLK or UCLASS_PARTITION. + * This function is expected to be called at EV_PM_PRE_REMOVE. + * + * @return 0 on success, -1 otherwise + */ +static int efi_disk_remove(void *ctx, struct event *event) +{ + enum uclass_id id; + struct udevice *dev; + + dev = event->data.dm.dev; + id = device_get_uclass_id(dev); + + if (id == UCLASS_BLK) + return efi_disk_delete_raw(dev); + else if (id == UCLASS_PARTITION) + return efi_disk_delete_part(dev); + else + return 0; +} + efi_status_t efi_disk_init(void) { int ret; @@ -641,6 +722,13 @@ efi_status_t efi_disk_init(void) return EFI_OUT_OF_RESOURCES; }
+ ret = event_register("efi_disk del", EVT_DM_PRE_REMOVE, + efi_disk_remove, NULL); + if (ret) { + log_err("Event registration for efi_disk del failed\n"); + return EFI_OUT_OF_RESOURCES; + } + return EFI_SUCCESS; }
In include/blk.h, Simon suggested: ===> /* * These functions should take struct udevice instead of struct blk_desc, * but this is convenient for migration to driver model. Add a 'd' prefix * to the function operations, so that blk_read(), etc. can be reserved for * functions with the correct arguments. */ unsigned long blk_dread(struct blk_desc *block_dev, lbaint_t start, lbaint_t blkcnt, void *buffer); unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t start, lbaint_t blkcnt, const void *buffer); unsigned long blk_derase(struct blk_desc *block_dev, lbaint_t start, lbaint_t blkcnt); <===
So new interfaces are provided with this patch.
They are expected to be used everywhere in U-Boot at the end. The exceptions are block device drivers, partition drivers and efi_disk which should know details of blk_desc structure.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- disk/disk-uclass.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++ include/part.h | 7 ++++ 2 files changed, 101 insertions(+)
diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c index 4918a2f72d1e..72ff62ebf581 100644 --- a/disk/disk-uclass.c +++ b/disk/disk-uclass.c @@ -146,6 +146,100 @@ U_BOOT_DRIVER(blk_partition) = { .ops = &blk_part_ops, };
+/* + * BLOCK IO APIs + */ +static struct blk_desc *dev_get_blk(struct udevice *dev) +{ + struct blk_desc *block_dev; + + switch (device_get_uclass_id(dev)) { + /* + * We won't support UCLASS_BLK with dev_* interfaces. + */ + case UCLASS_PARTITION: + block_dev = dev_get_uclass_plat(dev_get_parent(dev)); + break; + default: + block_dev = NULL; + break; + } + + return block_dev; +} + +unsigned long dev_read(struct udevice *dev, lbaint_t start, + lbaint_t blkcnt, void *buffer) +{ + struct blk_desc *block_dev; + const struct blk_ops *ops; + struct disk_part *part; + lbaint_t start_in_disk; + ulong blks_read; + + block_dev = dev_get_blk(dev); + if (!block_dev) + return -ENOSYS; + + ops = blk_get_ops(dev); + if (!ops->read) + return -ENOSYS; + + start_in_disk = start; + if (device_get_uclass_id(dev) == UCLASS_PARTITION) { + part = dev_get_uclass_plat(dev); + start_in_disk += part->gpt_part_info.start; + } + + if (blkcache_read(block_dev->if_type, block_dev->devnum, + start_in_disk, blkcnt, block_dev->blksz, buffer)) + return blkcnt; + blks_read = ops->read(dev, start, blkcnt, buffer); + if (blks_read == blkcnt) + blkcache_fill(block_dev->if_type, block_dev->devnum, + start_in_disk, blkcnt, block_dev->blksz, buffer); + + return blks_read; +} + +unsigned long dev_write(struct udevice *dev, lbaint_t start, + lbaint_t blkcnt, const void *buffer) +{ + struct blk_desc *block_dev; + const struct blk_ops *ops; + + block_dev = dev_get_blk(dev); + if (!block_dev) + return -ENOSYS; + + ops = blk_get_ops(dev); + if (!ops->write) + return -ENOSYS; + + blkcache_invalidate(block_dev->if_type, block_dev->devnum); + + return ops->write(dev, start, blkcnt, buffer); +} + +unsigned long dev_erase(struct udevice *dev, lbaint_t start, + lbaint_t blkcnt) +{ + struct blk_desc *block_dev; + const struct blk_ops *ops; + + block_dev = dev_get_blk(dev); + if (!block_dev) + return -ENOSYS; + + ops = blk_get_ops(dev); + if (!ops->erase) + return -ENOSYS; + + blkcache_invalidate(block_dev->if_type, block_dev->devnum); + + return ops->erase(dev, start, blkcnt); +} + UCLASS_DRIVER(partition) = { .id = UCLASS_PARTITION, .per_device_plat_auto = sizeof(struct disk_part), diff --git a/include/part.h b/include/part.h index 95e30e60af10..d4e5cd921db1 100644 --- a/include/part.h +++ b/include/part.h @@ -264,6 +264,13 @@ struct udevice; */ int part_create_block_devices(struct udevice *blk_dev);
+unsigned long dev_read(struct udevice *dev, lbaint_t start, + lbaint_t blkcnt, void *buffer); +unsigned long dev_write(struct udevice *dev, lbaint_t start, + lbaint_t blkcnt, const void *buffer); +unsigned long dev_erase(struct udevice *dev, lbaint_t start, + lbaint_t blkcnt); + #else static inline struct blk_desc *blk_get_dev(const char *ifname, int dev) { return NULL; }
In most of all cases, we can avoid using blk_desc which is expected to be private to udevice(UCLASS_BLK), that is, the data should not be manipulated outside the device driver unless really needed.
Now efi_disk's internally use dev_read/write() interfaces.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- lib/efi_loader/efi_disk.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 7abe5d3a6bd2..a8cac0a002c9 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -36,7 +36,7 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID; * @part: partition * @volume: simple file system protocol of the partition * @offset: offset into disk for simple partition - * @desc: internal block device descriptor + * @dev: associated DM device */ struct efi_disk_obj { struct efi_object header; @@ -48,7 +48,7 @@ struct efi_disk_obj { unsigned int part; struct efi_simple_file_system_protocol *volume; lbaint_t offset; - struct blk_desc *desc; + struct udevice *dev; /* TODO: move it to efi_object */ };
/** @@ -83,14 +83,12 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, void *buffer, enum efi_disk_direction direction) { struct efi_disk_obj *diskobj; - struct blk_desc *desc; int blksz; int blocks; unsigned long n;
diskobj = container_of(this, struct efi_disk_obj, ops); - desc = (struct blk_desc *) diskobj->desc; - blksz = desc->blksz; + blksz = diskobj->media.block_size; blocks = buffer_size / blksz; lba += diskobj->offset;
@@ -102,9 +100,9 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, return EFI_BAD_BUFFER_SIZE;
if (direction == EFI_DISK_READ) - n = blk_dread(desc, lba, blocks, buffer); + n = dev_read(diskobj->dev, lba, blocks, buffer); else - n = blk_dwrite(desc, lba, blocks, buffer); + n = dev_write(diskobj->dev, lba, blocks, buffer);
/* We don't do interrupts, so check for timers cooperatively */ efi_timer_check(); @@ -446,7 +444,6 @@ static efi_status_t efi_disk_add_dev( diskobj->ops = block_io_disk_template; diskobj->ifname = if_typename; diskobj->dev_index = dev_index; - diskobj->desc = desc;
/* Fill in EFI IO Media info (for read/write callbacks) */ diskobj->media.removable_media = desc->removable; @@ -522,6 +519,7 @@ static int efi_disk_create_raw(struct udevice *dev)
return -1; } + disk->dev = dev; if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { efi_free_pool(disk->dp); efi_delete_handle(&disk->header); @@ -578,6 +576,7 @@ static int efi_disk_create_part(struct udevice *dev) log_err("Adding partition for %s failed\n", dev->name); return -1; } + disk->dev = dev; if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { efi_free_pool(disk->dp); efi_delete_handle(&disk->header); @@ -743,20 +742,22 @@ bool efi_disk_is_system_part(efi_handle_t handle) { struct efi_handler *handler; struct efi_disk_obj *diskobj; - struct disk_partition info; + struct udevice *dev; + struct disk_part *part; efi_status_t ret; - int r;
/* check if this is a block device */ ret = efi_search_protocol(handle, &efi_block_io_guid, &handler); if (ret != EFI_SUCCESS) return false;
+ /* find a partition udevice */ diskobj = container_of(handle, struct efi_disk_obj, header); - - r = part_get_info(diskobj->desc, diskobj->part, &info); - if (r) + dev = diskobj->dev; + if (!dev || dev->driver->id != UCLASS_PARTITION) return false;
- return !!(info.bootable & PART_EFI_SYSTEM_PARTITION); + part = dev_get_uclass_plat(dev); + + return !!(part->gpt_part_info.bootable & PART_EFI_SYSTEM_PARTITION); }
When we create an efi_disk device with an UEFI application using driver binding protocol, the 'efi_driver' framework tries to create a corresponding block device(UCLASS_BLK/IF_TYPE_EFI). This will lead to calling a PROBE callback, efi_disk_probe(). In this case, however, we don't need to create another "efi_disk" device as we already have this device instance.
So we should avoid recursively invoke further processing in the callback function.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- lib/efi_loader/efi_disk.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index a8cac0a002c9..5474e867533b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -603,6 +603,7 @@ static int efi_disk_probe(void *ctx, struct event *event) { struct udevice *dev; enum uclass_id id; + struct blk_desc *desc; struct udevice *child; int ret;
@@ -616,9 +617,16 @@ static int efi_disk_probe(void *ctx, struct event *event) return 0; }
- ret = efi_disk_create_raw(dev); - if (ret) - return -1; + /* + * avoid creating duplicated objects now that efi_driver + * has already created an efi_disk at this moment. + */ + desc = dev_get_uclass_plat(dev); + if (desc->if_type != IF_TYPE_EFI_LOADER) { + ret = efi_disk_create_raw(dev); + if (ret) + return -1; + }
device_foreach_child(child, dev) { ret = efi_disk_create_part(child); @@ -642,13 +650,17 @@ static int efi_disk_probe(void *ctx, struct event *event) static int efi_disk_delete_raw(struct udevice *dev) { efi_handle_t handle; + struct blk_desc *desc; struct efi_disk_obj *diskobj;
if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) return -1;
- diskobj = container_of(handle, struct efi_disk_obj, header); - efi_free_pool(diskobj->dp); + desc = dev_get_uclass_plat(dev); + if (desc->if_type != IF_TYPE_EFI_LOADER) { + diskobj = container_of(handle, struct efi_disk_obj, header); + efi_free_pool(diskobj->dp); + }
efi_delete_handle(handle); dev_tag_del(dev, DM_TAG_EFI);
With DM-efi_disk integration, we don't need to explicitly call efi_disk_create_partitions().
The only thing to do is to associate an efi_disk object to the corresponding udevice as we skip most of processing in efi_disk_probe() by the previous commit ("efi_loader: disk: not create BLK device for BLK(IF_TYPE_EFI) devices").
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Simon Glass sjg@chromium.org --- lib/efi_driver/efi_block_device.c | 34 +++++++++---------------------- 1 file changed, 10 insertions(+), 24 deletions(-)
diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c index 04cb3ef0d4e5..5baa6f87a375 100644 --- a/lib/efi_driver/efi_block_device.c +++ b/lib/efi_driver/efi_block_device.c @@ -35,6 +35,7 @@ #include <malloc.h> #include <dm/device-internal.h> #include <dm/root.h> +#include <dm/tag.h>
/* * EFI attributes of the udevice handled by this driver. @@ -106,25 +107,6 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, return blkcnt; }
-/** - * Create partions for the block device. - * - * @handle: EFI handle of the block device - * @dev: udevice of the block device - * Return: number of partitions created - */ -static int efi_bl_bind_partitions(efi_handle_t handle, struct udevice *dev) -{ - struct blk_desc *desc; - const char *if_typename; - - desc = dev_get_uclass_plat(dev); - if_typename = blk_get_if_type_name(desc->if_type); - - return efi_disk_create_partitions(handle, desc, if_typename, - desc->devnum, dev->name); -} - /** * Create a block device for a handle * @@ -139,7 +121,6 @@ static int efi_bl_bind(efi_handle_t handle, void *interface) char *name; struct efi_object *obj = efi_search_obj(handle); struct efi_block_io *io = interface; - int disks; struct efi_blk_plat *plat;
EFI_PRINT("%s: handle %p, interface %p\n", __func__, handle, io); @@ -173,15 +154,20 @@ static int efi_bl_bind(efi_handle_t handle, void *interface) plat->handle = handle; plat->io = interface;
+ /* + * FIXME: necessary because we won't do almost nothing in + * efi_disk_create() when called from device_probe(). + */ + ret = dev_tag_set_ptr(bdev, DM_TAG_EFI, handle); + if (ret) + /* FIXME: cleanup for bdev */ + 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); - EFI_PRINT("Found %d partitions\n", disks); - return 0; }
On 2/10/22 09:11, AKASHI Takahiro wrote:
Background:
The purpose of this patch is to reignite the discussion about how UEFI subystem would best be integrated into U-Boot driver model. In the past, I proposed a couple of patch series, the latest one[1], while Heinrich revealed his idea[2], and the approach taken here is something between them, with a focus on block device handlings.
Disks in UEFI world:
In general in UEFI world, accessing to any device is performed through a 'protocol' interface which are installed to (or associated with) the device's UEFI handle (or an opaque pointer to UEFI object data). Protocols are implemented by either the UEFI system itself or UEFI drivers.
For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk hereafter). Currently, every efi_disk may have one of two origins:
a.U-Boot's block devices or related partitions (lib/efi_loader/efi_disk.c) b.UEFI objects which are implemented as a block device by UEFI drivers. (lib/efi_driver/efi_block_device.c)
All the efi_diskss as (a) will be enumerated and created only once at UEFI subsystem initialization (efi_disk_register()), which is triggered by first executing one of UEFI-related U-Boot commands, like "bootefi", "setenv -e" or "efidebug". EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) in the corresponding udevice(UCLASS_BLK).
On the other hand, efi_disk as (b) will be created each time UEFI boot services' connect_controller() is executed in UEFI app which, as a (device) controller, gives the method to access the device's data, ie. EFI_BLOCK_IO_PROTOCOL.
more details >>>
Internally, connect_controller() search for UEFI driver that can support this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, then calls the driver's 'bind' interface, which eventually installs the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for
- creating additional partitions efi_disk's, and
- supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it.
<<< <<<
Issues:
While an efi_disk represents a device equally for either a whole disk or a partition in UEFI world, the driver model treats only a whole disk as a real block device or udevice(UCLASS_BLK).
efi_disk holds and makes use of "blk_desc" data even though blk_desc in plat_data is supposed to be private and not to be accessed outside the driver model. # This issue, though, exists for all the implementation of U-Boot # file systems as well.
For efi_disk(a), 3. A block device can be enumerated dynamically by 'scanning' a device bus in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly. For examples, => scsi rescan; efidebug devices => usb start; efidebug devices ... (A) (A) doesn't show any usb devices detected.
=> scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ... => scsi rescan ... (B) => bootefi bootmgr ... (C) (C) may de-reference a bogus blk_desc pointer which has been freed by (B). (Please note that "scsi rescan" removes all udevices/blk_desc and then re-create them even if nothing is changed on a bus.)For efi_disk(b), 4. A "controller (handle)", combined with efi_block driver, has no corresponding udevice as a parent of efi_disks in DM tree, unlike, say, a scsi controller, even though it provides methods for block io operations. 5. There is no way supported to remove efi_disk's even after disconnect_controller() is called.
My approach:
Due to functional differences in semantics, it would be difficult to identify "udevice" structure as a handle in UEFI world. Instead, we will have to somehow maintain a relationship between a udevice and a handle.
1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions Currently, the uclass for partitions is not a UCLASS_BLK. It can be possible to define partitions as UCLASS_BLK (with IF_TYPE_PARTION?), but I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK) is tightly coupled with 'struct blk_desc' data which is still used as a "structure to a whole disk" in a lot of interfaces. (I hope that you understand what it means.)
In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent: For instance, UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION (IF_TYPE_SCSI) | +- struct blk_desc +- struct disk_part +- scsi_blk_ops +- blk_part_ops1-2. create partition udevices in the context of device_probe() part_init() is already called in blk_post_probe(). See the commit d0851c893706 ("blk: Call part_init() in the post_probe() method"). Why not enumerate partitions as well in there.
- add new block access interfaces, which takes a *udevice* as a target device, in U-Boot and use those functions to implement efi_disk operations (i.e. EFI_BLOCK_IO_PROTOCOL).
3-1. maintain a bi-directional link between a udevice and an efi_disk by adding - a UEFI handle pointer as a tag for a udevice - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
3-2. synchronize the lifetime of efi_disk objects in UEFI world with the driver model using - event notification associated with device's probe/remove.
- I have no solution to issue(4) and (5) yet.
<<<Example DM tree on qemu-arm64>>> => dm tree Class Driver Name
root root_driver root_driver ... pci pci_generic_ecam |-- pcie@10000000 pci_generi pci_generic_drv | |-- pci_0:0.0 virtio virtio-pci.l | |-- virtio-pci.l#0 ethernet virtio-net | | `-- virtio-net#32 ahci ahci_pci | |-- ahci_pci scsi ahci_scsi | | `-- ahci_scsi blk scsi_blk | | |-- ahci_scsi.id0lun0 partition blk_partition | | | |-- ahci_scsi.id0lun0:1 partition blk_partition | | | `-- ahci_scsi.id0lun0:2 blk scsi_blk | | `-- ahci_scsi.id1lun0 partition blk_partition | | |-- ahci_scsi.id1lun0:1 partition blk_partition | | `-- ahci_scsi.id1lun0:2 usb xhci_pci | `-- xhci_pci usb_hub usb_hub | `-- usb_hub usb_dev_ge usb_dev_generic_drv | |-- generic_bus_0_dev_2 usb_mass_s usb_mass_storage | `-- usb_mass_storage blk usb_storage_blk | `-- usb_mass_storage.lun0 partition blk_partition | |-- usb_mass_storage.lun0:1 partition blk_partition | `-- usb_mass_storage.lun0:2 ... => efi devices Device Device Path ================ ==================== 000000013eeea8d0 /VenHw() 000000013eeed810 /VenHw()/MAC(525252525252,1) 000000013eefc460 /VenHw()/Scsi(0,0) 000000013eefc5a0 /VenHw()/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eefe320 /VenHw()/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013eeff210 /VenHw()/Scsi(1,0) 000000013eeff390 /VenHw()/Scsi(1,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eeff7d0 /VenHw()/Scsi(1,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013ef04c20 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0) 000000013ef04da0 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(1,0x01,0,0x0,0x99800) 000000013ef04f70 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(2,0x01,0,0x99800,0x1800)
Patchs:
For easy understandings, patches may be categorized into separate groups of changes.
Patch#1-#7: DM: add device_probe() for later use of events Patch#8-#11: DM: add new features (tag and event notification) Patch#12-#16: UEFI: dynamically create/remove efi_disk's for a raw disk and its partitions For removal case, we may need more consideration since removing handles unconditionally may end up breaking integrity of handles (as some may still be held and referenced to by a UEFI app). Patch#17-#18: UEFI: use udevice read/write interfaces Patch#19-#20: UEFI: fix-up efi_driver, aligning with changes in DM integration
[1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html
This series does not pass Gitlab CI:
See https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391030 https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391031
I will set the whole series to "changes requested"
Please, run 'make tests' before resubmitting.
Best regards
Heinrich
=================================== FAILURES =================================== ________________________________ test_gpt_write ________________________________ test/py/tests/test_gpt.py:169: in test_gpt_write assert 'Writing GPT: success!' in output E AssertionError: assert 'Writing GPT: success!' in 'Writing GPT: Not a block device: rng\r\r\nsuccess!' ----------------------------- Captured stdout call ----------------------------- => host bind 0 /tmp/sandbox/test_gpt_disk_image.bin
=> => gpt write host 0 "name=all,size=0"
Writing GPT: Not a block device: rng
success!
=> ___________________ test_ut[ut_dm_dm_test_video_comp_bmp32] ____________________ test/py/tests/test_ut.py:43: in test_ut assert output.endswith('Failures: 0') E AssertionError: assert False E + where False = <built-in method endswith of str object at 0x7fd72d2fc800>('Failures: 0') E + where <built-in method endswith of str object at 0x7fd72d2fc800> = 'Test: dm_test_video_comp_bmp32: video.c\r\r\nSDL renderer does not exist\r\r\ntest/dm/video.c:88, compress_frame_buff..._test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)\r\r\nFailures: 2'.endswith ----------------------------- Captured stdout call ----------------------------- => ut dm dm_test_video_comp_bmp32
Test: dm_test_video_comp_bmp32: video.c
SDL renderer does not exist
test/dm/video.c:88, compress_frame_buffer(): !memcmp(uc_priv->fb, uc_priv->copy_fb, uc_priv->fb_size): Copy framebuffer does not match fb
test/dm/video.c:484, dm_test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)
Failures: 2
=> _______________________________ test_avb_read_rb _______________________________ test/py/tests/test_android/test_avb.py:83: in test_avb_read_rb assert response == 'Rollback index: 0' E AssertionError: assert 'Not a block ...back index: 0' == 'Rollback index: 0' E - Not a block device: sandbox_tee E - E Rollback index: 0 ----------------------------- Captured stdout call ----------------------------- => avb init 1
=> => avb read_rb 1
Not a block device: sandbox_tee
Rollback index: 0
=> _____________________________ test_avb_is_unlocked _____________________________ test/py/tests/test_android/test_avb.py:95: in test_avb_is_unlocked assert response == 'Unlocked = 1' E AssertionError: assert 'Not a block ...nUnlocked = 1' == 'Unlocked = 1' E - Not a block device: sandbox_tee E - E Unlocked = 1 ---------------------------- Captured stdout setup ----------------------------- /u-boot
U-Boot 2022.04-rc1-00209-g173fff8119 (Feb 10 2022 - 14:59:41 +0000)
Model: sandbox
DRAM: 128 MiB
Core: 248 devices, 90 uclasses, devicetree: board
WDT: Not starting gpio-wdt
WDT: Not starting wdt@0
MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from nowhere... OK
In: cros-ec-keyb
Out: vidconsole
Err: vidconsole
Model: sandbox
SCSI:
Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1
7[r[999;999H[6n8Not a block device: pinmux_i2c0_pins
Not a block device: i2c@0
Not a block device: rtc@61
Not a block device: bootcount@0
Not a block device: emul
Not a block device: emull
Hit any key to stop autoboot: 2 0
=> ----------------------------- Captured stdout call ----------------------------- => avb init 1
=> => avb is_unlocked
Not a block device: sandbox_tee
Unlocked = 1
=> __________________________ test_avb_persistent_values __________________________ test/py/tests/test_android/test_avb.py:134: in test_avb_persistent_values assert response == 'Wrote 12 bytes' E AssertionError: assert 'Not a block ...rote 12 bytes' == 'Wrote 12 bytes' E - Not a block device: sandbox_tee E - E Wrote 12 bytes ---------------------------- Captured stdout setup ----------------------------- /u-boot
U-Boot 2022.04-rc1-00209-g173fff8119 (Feb 10 2022 - 14:59:41 +0000)
Model: sandbox
DRAM: 128 MiB
Core: 248 devices, 90 uclasses, devicetree: board
WDT: Not starting gpio-wdt
WDT: Not starting wdt@0
MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from nowhere... OK
In: cros-ec-keyb
Out: vidconsole
Err: vidconsole
Model: sandbox
SCSI:
Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1
7[r[999;999H[6n8Not a block device: pinmux_i2c0_pins
Not a block device: i2c@0
Not a block device: rtc@61
Not a block device: bootcount@0
Not a block device: emul
Not a block device: emull
Hit any key to stop autoboot: 2 0
=> ----------------------------- Captured stdout call ----------------------------- => avb init 1
=> => avb write_pvalue test value_value
Not a block device: sandbox_tee
Wrote 12 bytes
=>
Change history:
v2 (Feb 10, 2022)
- add/revise an error message if device_probe() fails (patch#3,#5)
- fix a build error in sandbox_spl_defconfig (patch#8)
- fix warnings in 'make htmldocs' (patch#8,#9,#18)
- new commit: split efi_init_obj_list() (patch#14)
v1 (Feb 2, 2022)
- rebased on 2022.04-rc1
- drop patches that have already been merged
- modify a tag-range check with "tag >= DM_TAG_COUNT" (patch#9)
- move dmtag_list to GD (global data) (patch#9)
- add function descriptions and a document about DM tag feature (patch#9,10)
- add tests for DM tag support (patch#11)
- change 'depends on EVENT' to 'select EVENT' for EFI_LOADER (patch#14)
- migrate IF_TYPE_EFI to IF_TYPE_EFI_LOADER (patch#18)
RFCv2 (Dec 10, 2021)
- rebased on 2022-rc3
- re-order and merge some related commits into ones
- call device_probe() in MMC (not bind, but) probe hook (patch#5)
- fix a wrong name of variable (patch#7)
- add patch#9
- invoke device_probe() for virtio devices (patch#10)
- add DM event notification (from Simon) (patch#11)
- add DM tag support (patch#12)
- move UCLASS_PARTITION driver under disk/ (patch#13)
- create partition's dp using its parent's. This change is necessary in particular for 'efi_blk' efi_disk (patch#13)
- modify the code so that we will use new features like tags and event notification (patch#13,15,16,20)
- rename new functions from blk_read/write() to dev_read/write() (patch#17,18)
- isolate changes in efi_driver from the rest (in efi_loader) (patch#19)
- drop the previous patch#22 ("efi_selftest: block device: adjust dp for a test") due to the fix in patch#13
RFC (Nov 16, 2021)
- initial RFC
AKASHI Takahiro (19): scsi: call device_probe() after scanning usb: storage: call device_probe() after scanning mmc: call device_probe() after scanning nvme: call device_probe() after scanning sata: call device_probe() after scanning block: ide: call device_probe() after scanning virtio: call device_probe() in scanning dm: add tag support dm: tag: add some document test: dm: add tests for tag support dm: disk: add UCLASS_PARTITION dm: blk: add a device-probe hook for scanning disk partitions efi_loader: split efi_init_obj_list() into two stages efi_loader: disk: a helper function to create efi_disk objects from udevice efi_loader: disk: a helper function to delete efi_disk objects dm: disk: add read/write interfaces with udevice efi_loader: disk: use udevice instead of blk_desc efi_loader: disk: not create BLK device for BLK(IF_TYPE_EFI_LOADER) devices efi_driver: align with efi_disk-dm integration
Simon Glass (1): dm: add event notification
cmd/virtio.c | 21 +- common/Kconfig | 11 + common/Makefile | 2 + common/board_f.c | 2 + common/board_r.c | 2 +- common/event.c | 103 +++++++++ common/log.c | 1 + common/main.c | 7 +- common/usb_storage.c | 4 + disk/Makefile | 3 + disk/disk-uclass.c | 247 +++++++++++++++++++++ doc/develop/driver-model/design.rst | 20 ++ drivers/ata/dwc_ahsata.c | 5 + drivers/ata/fsl_sata.c | 11 + drivers/ata/sata_mv.c | 5 + drivers/ata/sata_sil.c | 12 + drivers/block/blk-uclass.c | 4 + drivers/block/ide.c | 4 + drivers/core/Makefile | 2 +- drivers/core/device-remove.c | 9 + drivers/core/device.c | 9 + drivers/core/root.c | 2 + drivers/core/tag.c | 139 ++++++++++++ drivers/mmc/mmc-uclass.c | 12 + drivers/nvme/nvme.c | 4 + drivers/scsi/scsi.c | 5 + include/asm-generic/global_data.h | 10 + include/dm/device-internal.h | 10 + include/dm/tag.h | 110 +++++++++ include/dm/uclass-id.h | 1 + include/efi_loader.h | 6 +- include/event.h | 105 +++++++++ include/event_internal.h | 34 +++ include/log.h | 2 + include/part.h | 18 ++ lib/efi_driver/efi_block_device.c | 34 +-- lib/efi_loader/Kconfig | 2 + lib/efi_loader/efi_disk.c | 331 ++++++++++++++++++++-------- lib/efi_loader/efi_setup.c | 62 +++++- test/common/Makefile | 1 + test/common/event.c | 87 ++++++++ test/dm/Makefile | 1 + test/dm/tag.c | 80 +++++++ test/test-main.c | 7 + 44 files changed, 1416 insertions(+), 131 deletions(-) create mode 100644 common/event.c create mode 100644 disk/disk-uclass.c create mode 100644 drivers/core/tag.c create mode 100644 include/dm/tag.h create mode 100644 include/event.h create mode 100644 include/event_internal.h create mode 100644 test/common/event.c create mode 100644 test/dm/tag.c
Heinrich,
On Thu, Feb 10, 2022 at 04:20:11PM +0100, Heinrich Schuchardt wrote:
On 2/10/22 09:11, AKASHI Takahiro wrote:
Background:
The purpose of this patch is to reignite the discussion about how UEFI subystem would best be integrated into U-Boot driver model. In the past, I proposed a couple of patch series, the latest one[1], while Heinrich revealed his idea[2], and the approach taken here is something between them, with a focus on block device handlings.
Disks in UEFI world:
In general in UEFI world, accessing to any device is performed through a 'protocol' interface which are installed to (or associated with) the device's UEFI handle (or an opaque pointer to UEFI object data). Protocols are implemented by either the UEFI system itself or UEFI drivers.
For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk hereafter). Currently, every efi_disk may have one of two origins:
a.U-Boot's block devices or related partitions (lib/efi_loader/efi_disk.c) b.UEFI objects which are implemented as a block device by UEFI drivers. (lib/efi_driver/efi_block_device.c)
All the efi_diskss as (a) will be enumerated and created only once at UEFI subsystem initialization (efi_disk_register()), which is triggered by first executing one of UEFI-related U-Boot commands, like "bootefi", "setenv -e" or "efidebug". EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) in the corresponding udevice(UCLASS_BLK).
On the other hand, efi_disk as (b) will be created each time UEFI boot services' connect_controller() is executed in UEFI app which, as a (device) controller, gives the method to access the device's data, ie. EFI_BLOCK_IO_PROTOCOL.
more details >>>
Internally, connect_controller() search for UEFI driver that can support this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, then calls the driver's 'bind' interface, which eventually installs the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for
- creating additional partitions efi_disk's, and
- supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it.
<<< <<<
Issues:
While an efi_disk represents a device equally for either a whole disk or a partition in UEFI world, the driver model treats only a whole disk as a real block device or udevice(UCLASS_BLK).
efi_disk holds and makes use of "blk_desc" data even though blk_desc in plat_data is supposed to be private and not to be accessed outside the driver model. # This issue, though, exists for all the implementation of U-Boot # file systems as well.
For efi_disk(a), 3. A block device can be enumerated dynamically by 'scanning' a device bus in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly. For examples, => scsi rescan; efidebug devices => usb start; efidebug devices ... (A) (A) doesn't show any usb devices detected.
=> scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ... => scsi rescan ... (B) => bootefi bootmgr ... (C) (C) may de-reference a bogus blk_desc pointer which has been freed by (B). (Please note that "scsi rescan" removes all udevices/blk_desc and then re-create them even if nothing is changed on a bus.)For efi_disk(b), 4. A "controller (handle)", combined with efi_block driver, has no corresponding udevice as a parent of efi_disks in DM tree, unlike, say, a scsi controller, even though it provides methods for block io operations. 5. There is no way supported to remove efi_disk's even after disconnect_controller() is called.
My approach:
Due to functional differences in semantics, it would be difficult to identify "udevice" structure as a handle in UEFI world. Instead, we will have to somehow maintain a relationship between a udevice and a handle.
1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions Currently, the uclass for partitions is not a UCLASS_BLK. It can be possible to define partitions as UCLASS_BLK (with IF_TYPE_PARTION?), but I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK) is tightly coupled with 'struct blk_desc' data which is still used as a "structure to a whole disk" in a lot of interfaces. (I hope that you understand what it means.)
In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent: For instance, UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION (IF_TYPE_SCSI) | +- struct blk_desc +- struct disk_part +- scsi_blk_ops +- blk_part_ops1-2. create partition udevices in the context of device_probe() part_init() is already called in blk_post_probe(). See the commit d0851c893706 ("blk: Call part_init() in the post_probe() method"). Why not enumerate partitions as well in there.
- add new block access interfaces, which takes a *udevice* as a target device, in U-Boot and use those functions to implement efi_disk operations (i.e. EFI_BLOCK_IO_PROTOCOL).
3-1. maintain a bi-directional link between a udevice and an efi_disk by adding - a UEFI handle pointer as a tag for a udevice - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
3-2. synchronize the lifetime of efi_disk objects in UEFI world with the driver model using - event notification associated with device's probe/remove.
- I have no solution to issue(4) and (5) yet.
<<<Example DM tree on qemu-arm64>>> => dm tree Class Driver Name
root root_driver root_driver ... pci pci_generic_ecam |-- pcie@10000000 pci_generi pci_generic_drv | |-- pci_0:0.0 virtio virtio-pci.l | |-- virtio-pci.l#0 ethernet virtio-net | | `-- virtio-net#32 ahci ahci_pci | |-- ahci_pci scsi ahci_scsi | | `-- ahci_scsi blk scsi_blk | | |-- ahci_scsi.id0lun0 partition blk_partition | | | |-- ahci_scsi.id0lun0:1 partition blk_partition | | | `-- ahci_scsi.id0lun0:2 blk scsi_blk | | `-- ahci_scsi.id1lun0 partition blk_partition | | |-- ahci_scsi.id1lun0:1 partition blk_partition | | `-- ahci_scsi.id1lun0:2 usb xhci_pci | `-- xhci_pci usb_hub usb_hub | `-- usb_hub usb_dev_ge usb_dev_generic_drv | |-- generic_bus_0_dev_2 usb_mass_s usb_mass_storage | `-- usb_mass_storage blk usb_storage_blk | `-- usb_mass_storage.lun0 partition blk_partition | |-- usb_mass_storage.lun0:1 partition blk_partition | `-- usb_mass_storage.lun0:2 ... => efi devices Device Device Path ================ ==================== 000000013eeea8d0 /VenHw() 000000013eeed810 /VenHw()/MAC(525252525252,1) 000000013eefc460 /VenHw()/Scsi(0,0) 000000013eefc5a0 /VenHw()/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eefe320 /VenHw()/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013eeff210 /VenHw()/Scsi(1,0) 000000013eeff390 /VenHw()/Scsi(1,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eeff7d0 /VenHw()/Scsi(1,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013ef04c20 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0) 000000013ef04da0 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(1,0x01,0,0x0,0x99800) 000000013ef04f70 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(2,0x01,0,0x99800,0x1800)
Patchs:
For easy understandings, patches may be categorized into separate groups of changes.
Patch#1-#7: DM: add device_probe() for later use of events Patch#8-#11: DM: add new features (tag and event notification) Patch#12-#16: UEFI: dynamically create/remove efi_disk's for a raw disk and its partitions For removal case, we may need more consideration since removing handles unconditionally may end up breaking integrity of handles (as some may still be held and referenced to by a UEFI app). Patch#17-#18: UEFI: use udevice read/write interfaces Patch#19-#20: UEFI: fix-up efi_driver, aligning with changes in DM integration
[1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html
This series does not pass Gitlab CI:
See https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391030 https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391031
I have noticed those errors but I didn't think that they were related to my patch set initially as I didn't touch any code in gpt driver, android/avb nor video driver.
I will set the whole series to "changes requested"
Please, run 'make tests' before resubmitting.
Best regards
Heinrich
=================================== FAILURES
________________________________ test_gpt_write ________________________________ test/py/tests/test_gpt.py:169: in test_gpt_write assert 'Writing GPT: success!' in output E AssertionError: assert 'Writing GPT: success!' in 'Writing GPT: Not a block device: rng\r\r\nsuccess!'
The reason of assertion failure here is that some log message was inserted in a output message although the test itself was finished successfully: "Writing GPT: success!" <== a correct output message ^ "Not a block device: rng"
Adding efi_disk_probe() as a callback to EVT_DM_POST_PROBE created this *log_info* message in dm_rng_read() <- get_rand_uuid() <- gen_rand_uuid_str() in "gpt write" command.
We can fix this type of failure by the hack: ===8<=== --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -612,8 +612,6 @@ static int efi_disk_probe(void *ctx, struct event *event)
/* TODO: We won't support partitions in a partition */ if (id != UCLASS_BLK) { - if (id != UCLASS_PARTITION) - log_info("Not a block device: %s\n", dev->name); return 0; } ===>8===
I don't think, however, that it is a good thing that test results depend on console outputs, especially *log* messages.
Furthermore, I don't know why we see *info*-level messages even under CONFIG_LOGLEVEL=4 (warning).
----------------------------- Captured stdout call
=> host bind 0 /tmp/sandbox/test_gpt_disk_image.bin
=> => gpt write host 0 "name=all,size=0"
Writing GPT: Not a block device: rng
success!
=> ___________________ test_ut[ut_dm_dm_test_video_comp_bmp32] ____________________ test/py/tests/test_ut.py:43: in test_ut assert output.endswith('Failures: 0') E AssertionError: assert False E + where False = <built-in method endswith of str object at 0x7fd72d2fc800>('Failures: 0') E + where <built-in method endswith of str object at 0x7fd72d2fc800> = 'Test: dm_test_video_comp_bmp32: video.c\r\r\nSDL renderer does not exist\r\r\ntest/dm/video.c:88, compress_frame_buff..._test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)\r\r\nFailures: 2'.endswith
----------------------------- Captured stdout call
=> ut dm dm_test_video_comp_bmp32
Test: dm_test_video_comp_bmp32: video.c
SDL renderer does not exist
test/dm/video.c:88, compress_frame_buffer(): !memcmp(uc_priv->fb, uc_priv->copy_fb, uc_priv->fb_size): Copy framebuffer does not match fb
test/dm/video.c:484, dm_test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)
Failures: 2
I don't know yet why this happened.
=> _______________________________ test_avb_read_rb _______________________________ test/py/tests/test_android/test_avb.py:83: in test_avb_read_rb assert response == 'Rollback index: 0' E AssertionError: assert 'Not a block ...back index: 0' == 'Rollback index: 0' E - Not a block device: sandbox_tee E - E Rollback index: 0
----------------------------- Captured stdout call
=> avb init 1
=> => avb read_rb 1
Not a block device: sandbox_tee
The same error as mentioned above.
-Takahiro Akashi
Rollback index: 0
=> _____________________________ test_avb_is_unlocked _____________________________ test/py/tests/test_android/test_avb.py:95: in test_avb_is_unlocked assert response == 'Unlocked = 1' E AssertionError: assert 'Not a block ...nUnlocked = 1' == 'Unlocked = 1' E - Not a block device: sandbox_tee E - E Unlocked = 1
---------------------------- Captured stdout setup
/u-boot
U-Boot 2022.04-rc1-00209-g173fff8119 (Feb 10 2022 - 14:59:41 +0000)
Model: sandbox
DRAM: 128 MiB
Core: 248 devices, 90 uclasses, devicetree: board
WDT: Not starting gpio-wdt
WDT: Not starting wdt@0
MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from nowhere... OK
In: cros-ec-keyb
Out: vidconsole
Err: vidconsole
Model: sandbox
SCSI:
Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1
7[r[999;999H[6n8Not a block device: pinmux_i2c0_pins
Not a block device: i2c@0
Not a block device: rtc@61
Not a block device: bootcount@0
Not a block device: emul
Not a block device: emull
Hit any key to stop autoboot: 2 0
=>
----------------------------- Captured stdout call
=> avb init 1
=> => avb is_unlocked
Not a block device: sandbox_tee
Unlocked = 1
=> __________________________ test_avb_persistent_values __________________________ test/py/tests/test_android/test_avb.py:134: in test_avb_persistent_values assert response == 'Wrote 12 bytes' E AssertionError: assert 'Not a block ...rote 12 bytes' == 'Wrote 12 bytes' E - Not a block device: sandbox_tee E - E Wrote 12 bytes
---------------------------- Captured stdout setup
/u-boot
U-Boot 2022.04-rc1-00209-g173fff8119 (Feb 10 2022 - 14:59:41 +0000)
Model: sandbox
DRAM: 128 MiB
Core: 248 devices, 90 uclasses, devicetree: board
WDT: Not starting gpio-wdt
WDT: Not starting wdt@0
MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from nowhere... OK
In: cros-ec-keyb
Out: vidconsole
Err: vidconsole
Model: sandbox
SCSI:
Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1
7[r[999;999H[6n8Not a block device: pinmux_i2c0_pins
Not a block device: i2c@0
Not a block device: rtc@61
Not a block device: bootcount@0
Not a block device: emul
Not a block device: emull
Hit any key to stop autoboot: 2 0
=>
----------------------------- Captured stdout call
=> avb init 1
=> => avb write_pvalue test value_value
Not a block device: sandbox_tee
Wrote 12 bytes
=>
Change history:
v2 (Feb 10, 2022)
- add/revise an error message if device_probe() fails (patch#3,#5)
- fix a build error in sandbox_spl_defconfig (patch#8)
- fix warnings in 'make htmldocs' (patch#8,#9,#18)
- new commit: split efi_init_obj_list() (patch#14)
v1 (Feb 2, 2022)
- rebased on 2022.04-rc1
- drop patches that have already been merged
- modify a tag-range check with "tag >= DM_TAG_COUNT" (patch#9)
- move dmtag_list to GD (global data) (patch#9)
- add function descriptions and a document about DM tag feature (patch#9,10)
- add tests for DM tag support (patch#11)
- change 'depends on EVENT' to 'select EVENT' for EFI_LOADER (patch#14)
- migrate IF_TYPE_EFI to IF_TYPE_EFI_LOADER (patch#18)
RFCv2 (Dec 10, 2021)
- rebased on 2022-rc3
- re-order and merge some related commits into ones
- call device_probe() in MMC (not bind, but) probe hook (patch#5)
- fix a wrong name of variable (patch#7)
- add patch#9
- invoke device_probe() for virtio devices (patch#10)
- add DM event notification (from Simon) (patch#11)
- add DM tag support (patch#12)
- move UCLASS_PARTITION driver under disk/ (patch#13)
- create partition's dp using its parent's. This change is necessary in particular for 'efi_blk' efi_disk (patch#13)
- modify the code so that we will use new features like tags and event notification (patch#13,15,16,20)
- rename new functions from blk_read/write() to dev_read/write() (patch#17,18)
- isolate changes in efi_driver from the rest (in efi_loader) (patch#19)
- drop the previous patch#22 ("efi_selftest: block device: adjust dp for a test") due to the fix in patch#13
RFC (Nov 16, 2021)
- initial RFC
AKASHI Takahiro (19): scsi: call device_probe() after scanning usb: storage: call device_probe() after scanning mmc: call device_probe() after scanning nvme: call device_probe() after scanning sata: call device_probe() after scanning block: ide: call device_probe() after scanning virtio: call device_probe() in scanning dm: add tag support dm: tag: add some document test: dm: add tests for tag support dm: disk: add UCLASS_PARTITION dm: blk: add a device-probe hook for scanning disk partitions efi_loader: split efi_init_obj_list() into two stages efi_loader: disk: a helper function to create efi_disk objects from udevice efi_loader: disk: a helper function to delete efi_disk objects dm: disk: add read/write interfaces with udevice efi_loader: disk: use udevice instead of blk_desc efi_loader: disk: not create BLK device for BLK(IF_TYPE_EFI_LOADER) devices efi_driver: align with efi_disk-dm integration
Simon Glass (1): dm: add event notification
cmd/virtio.c | 21 +- common/Kconfig | 11 + common/Makefile | 2 + common/board_f.c | 2 + common/board_r.c | 2 +- common/event.c | 103 +++++++++ common/log.c | 1 + common/main.c | 7 +- common/usb_storage.c | 4 + disk/Makefile | 3 + disk/disk-uclass.c | 247 +++++++++++++++++++++ doc/develop/driver-model/design.rst | 20 ++ drivers/ata/dwc_ahsata.c | 5 + drivers/ata/fsl_sata.c | 11 + drivers/ata/sata_mv.c | 5 + drivers/ata/sata_sil.c | 12 + drivers/block/blk-uclass.c | 4 + drivers/block/ide.c | 4 + drivers/core/Makefile | 2 +- drivers/core/device-remove.c | 9 + drivers/core/device.c | 9 + drivers/core/root.c | 2 + drivers/core/tag.c | 139 ++++++++++++ drivers/mmc/mmc-uclass.c | 12 + drivers/nvme/nvme.c | 4 + drivers/scsi/scsi.c | 5 + include/asm-generic/global_data.h | 10 + include/dm/device-internal.h | 10 + include/dm/tag.h | 110 +++++++++ include/dm/uclass-id.h | 1 + include/efi_loader.h | 6 +- include/event.h | 105 +++++++++ include/event_internal.h | 34 +++ include/log.h | 2 + include/part.h | 18 ++ lib/efi_driver/efi_block_device.c | 34 +-- lib/efi_loader/Kconfig | 2 + lib/efi_loader/efi_disk.c | 331 ++++++++++++++++++++-------- lib/efi_loader/efi_setup.c | 62 +++++- test/common/Makefile | 1 + test/common/event.c | 87 ++++++++ test/dm/Makefile | 1 + test/dm/tag.c | 80 +++++++ test/test-main.c | 7 + 44 files changed, 1416 insertions(+), 131 deletions(-) create mode 100644 common/event.c create mode 100644 disk/disk-uclass.c create mode 100644 drivers/core/tag.c create mode 100644 include/dm/tag.h create mode 100644 include/event.h create mode 100644 include/event_internal.h create mode 100644 test/common/event.c create mode 100644 test/dm/tag.c
Hi Simon,
On Mon, Feb 14, 2022 at 11:35:06AM +0900, AKASHI Takahiro wrote:
Heinrich,
On Thu, Feb 10, 2022 at 04:20:11PM +0100, Heinrich Schuchardt wrote:
On 2/10/22 09:11, AKASHI Takahiro wrote:
Background:
The purpose of this patch is to reignite the discussion about how UEFI subystem would best be integrated into U-Boot driver model. In the past, I proposed a couple of patch series, the latest one[1], while Heinrich revealed his idea[2], and the approach taken here is something between them, with a focus on block device handlings.
Disks in UEFI world:
In general in UEFI world, accessing to any device is performed through a 'protocol' interface which are installed to (or associated with) the device's UEFI handle (or an opaque pointer to UEFI object data). Protocols are implemented by either the UEFI system itself or UEFI drivers.
For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk hereafter). Currently, every efi_disk may have one of two origins:
a.U-Boot's block devices or related partitions (lib/efi_loader/efi_disk.c) b.UEFI objects which are implemented as a block device by UEFI drivers. (lib/efi_driver/efi_block_device.c)
All the efi_diskss as (a) will be enumerated and created only once at UEFI subsystem initialization (efi_disk_register()), which is triggered by first executing one of UEFI-related U-Boot commands, like "bootefi", "setenv -e" or "efidebug". EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) in the corresponding udevice(UCLASS_BLK).
On the other hand, efi_disk as (b) will be created each time UEFI boot services' connect_controller() is executed in UEFI app which, as a (device) controller, gives the method to access the device's data, ie. EFI_BLOCK_IO_PROTOCOL.
more details >>>
Internally, connect_controller() search for UEFI driver that can support this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, then calls the driver's 'bind' interface, which eventually installs the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for
- creating additional partitions efi_disk's, and
- supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it.
<<< <<<
Issues:
While an efi_disk represents a device equally for either a whole disk or a partition in UEFI world, the driver model treats only a whole disk as a real block device or udevice(UCLASS_BLK).
efi_disk holds and makes use of "blk_desc" data even though blk_desc in plat_data is supposed to be private and not to be accessed outside the driver model. # This issue, though, exists for all the implementation of U-Boot # file systems as well.
For efi_disk(a), 3. A block device can be enumerated dynamically by 'scanning' a device bus in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly. For examples, => scsi rescan; efidebug devices => usb start; efidebug devices ... (A) (A) doesn't show any usb devices detected.
=> scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ... => scsi rescan ... (B) => bootefi bootmgr ... (C) (C) may de-reference a bogus blk_desc pointer which has been freed by (B). (Please note that "scsi rescan" removes all udevices/blk_desc and then re-create them even if nothing is changed on a bus.)For efi_disk(b), 4. A "controller (handle)", combined with efi_block driver, has no corresponding udevice as a parent of efi_disks in DM tree, unlike, say, a scsi controller, even though it provides methods for block io operations. 5. There is no way supported to remove efi_disk's even after disconnect_controller() is called.
My approach:
Due to functional differences in semantics, it would be difficult to identify "udevice" structure as a handle in UEFI world. Instead, we will have to somehow maintain a relationship between a udevice and a handle.
1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions Currently, the uclass for partitions is not a UCLASS_BLK. It can be possible to define partitions as UCLASS_BLK (with IF_TYPE_PARTION?), but I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK) is tightly coupled with 'struct blk_desc' data which is still used as a "structure to a whole disk" in a lot of interfaces. (I hope that you understand what it means.)
In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent: For instance, UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION (IF_TYPE_SCSI) | +- struct blk_desc +- struct disk_part +- scsi_blk_ops +- blk_part_ops1-2. create partition udevices in the context of device_probe() part_init() is already called in blk_post_probe(). See the commit d0851c893706 ("blk: Call part_init() in the post_probe() method"). Why not enumerate partitions as well in there.
- add new block access interfaces, which takes a *udevice* as a target device, in U-Boot and use those functions to implement efi_disk operations (i.e. EFI_BLOCK_IO_PROTOCOL).
3-1. maintain a bi-directional link between a udevice and an efi_disk by adding - a UEFI handle pointer as a tag for a udevice - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
3-2. synchronize the lifetime of efi_disk objects in UEFI world with the driver model using - event notification associated with device's probe/remove.
- I have no solution to issue(4) and (5) yet.
<<<Example DM tree on qemu-arm64>>> => dm tree Class Driver Name
root root_driver root_driver ... pci pci_generic_ecam |-- pcie@10000000 pci_generi pci_generic_drv | |-- pci_0:0.0 virtio virtio-pci.l | |-- virtio-pci.l#0 ethernet virtio-net | | `-- virtio-net#32 ahci ahci_pci | |-- ahci_pci scsi ahci_scsi | | `-- ahci_scsi blk scsi_blk | | |-- ahci_scsi.id0lun0 partition blk_partition | | | |-- ahci_scsi.id0lun0:1 partition blk_partition | | | `-- ahci_scsi.id0lun0:2 blk scsi_blk | | `-- ahci_scsi.id1lun0 partition blk_partition | | |-- ahci_scsi.id1lun0:1 partition blk_partition | | `-- ahci_scsi.id1lun0:2 usb xhci_pci | `-- xhci_pci usb_hub usb_hub | `-- usb_hub usb_dev_ge usb_dev_generic_drv | |-- generic_bus_0_dev_2 usb_mass_s usb_mass_storage | `-- usb_mass_storage blk usb_storage_blk | `-- usb_mass_storage.lun0 partition blk_partition | |-- usb_mass_storage.lun0:1 partition blk_partition | `-- usb_mass_storage.lun0:2 ... => efi devices Device Device Path ================ ==================== 000000013eeea8d0 /VenHw() 000000013eeed810 /VenHw()/MAC(525252525252,1) 000000013eefc460 /VenHw()/Scsi(0,0) 000000013eefc5a0 /VenHw()/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eefe320 /VenHw()/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013eeff210 /VenHw()/Scsi(1,0) 000000013eeff390 /VenHw()/Scsi(1,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eeff7d0 /VenHw()/Scsi(1,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013ef04c20 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0) 000000013ef04da0 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(1,0x01,0,0x0,0x99800) 000000013ef04f70 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(2,0x01,0,0x99800,0x1800)
Patchs:
For easy understandings, patches may be categorized into separate groups of changes.
Patch#1-#7: DM: add device_probe() for later use of events Patch#8-#11: DM: add new features (tag and event notification) Patch#12-#16: UEFI: dynamically create/remove efi_disk's for a raw disk and its partitions For removal case, we may need more consideration since removing handles unconditionally may end up breaking integrity of handles (as some may still be held and referenced to by a UEFI app). Patch#17-#18: UEFI: use udevice read/write interfaces Patch#19-#20: UEFI: fix-up efi_driver, aligning with changes in DM integration
[1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html
This series does not pass Gitlab CI:
See https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391030 https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391031
I have noticed those errors but I didn't think that they were related to my patch set initially as I didn't touch any code in gpt driver, android/avb nor video driver.
I will set the whole series to "changes requested"
Please, run 'make tests' before resubmitting.
Best regards
Heinrich
=================================== FAILURES
________________________________ test_gpt_write ________________________________ test/py/tests/test_gpt.py:169: in test_gpt_write assert 'Writing GPT: success!' in output E AssertionError: assert 'Writing GPT: success!' in 'Writing GPT: Not a block device: rng\r\r\nsuccess!'
The reason of assertion failure here is that some log message was inserted in a output message although the test itself was finished successfully: "Writing GPT: success!" <== a correct output message ^ "Not a block device: rng"
Adding efi_disk_probe() as a callback to EVT_DM_POST_PROBE created this *log_info* message in dm_rng_read() <- get_rand_uuid() <- gen_rand_uuid_str() in "gpt write" command.
We can fix this type of failure by the hack: ===8<=== --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -612,8 +612,6 @@ static int efi_disk_probe(void *ctx, struct event *event)
/* TODO: We won't support partitions in a partition */ if (id != UCLASS_BLK) {
if (id != UCLASS_PARTITION)log_info("Not a block device: %s\n", dev->name); return 0; }===>8===
I don't think, however, that it is a good thing that test results depend on console outputs, especially *log* messages.
Furthermore, I don't know why we see *info*-level messages even under CONFIG_LOGLEVEL=4 (warning).
----------------------------- Captured stdout call
=> host bind 0 /tmp/sandbox/test_gpt_disk_image.bin
=> => gpt write host 0 "name=all,size=0"
Writing GPT: Not a block device: rng
success!
=> ___________________ test_ut[ut_dm_dm_test_video_comp_bmp32] ____________________ test/py/tests/test_ut.py:43: in test_ut assert output.endswith('Failures: 0') E AssertionError: assert False E + where False = <built-in method endswith of str object at 0x7fd72d2fc800>('Failures: 0') E + where <built-in method endswith of str object at 0x7fd72d2fc800> = 'Test: dm_test_video_comp_bmp32: video.c\r\r\nSDL renderer does not exist\r\r\ntest/dm/video.c:88, compress_frame_buff..._test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)\r\r\nFailures: 2'.endswith
----------------------------- Captured stdout call
=> ut dm dm_test_video_comp_bmp32
Test: dm_test_video_comp_bmp32: video.c
SDL renderer does not exist
test/dm/video.c:88, compress_frame_buffer(): !memcmp(uc_priv->fb, uc_priv->copy_fb, uc_priv->fb_size): Copy framebuffer does not match fb
test/dm/video.c:484, dm_test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)
Failures: 2
I don't know yet why this happened.
It seems that this error happened simply because more ut DM tests were added. Added here are DM tag tests (in my patch#14 of 20).
But what type of test is added doesn't matter. When a total number of ut DM tests is increased (and exceeds some limit?), one of tests (either video or another) may unexpectedly fail. For instance, I randomly picked up one test from test/dm/gpio.c and commented it out, and then I didn't see any error in test_ut.py.
So I suspect there may be some problem with pytest framework.
Do you have any clue, Simon?
-Takahiro Akashi
=> _______________________________ test_avb_read_rb _______________________________ test/py/tests/test_android/test_avb.py:83: in test_avb_read_rb assert response == 'Rollback index: 0' E AssertionError: assert 'Not a block ...back index: 0' == 'Rollback index: 0' E - Not a block device: sandbox_tee E - E Rollback index: 0
----------------------------- Captured stdout call
=> avb init 1
=> => avb read_rb 1
Not a block device: sandbox_tee
The same error as mentioned above.
-Takahiro Akashi
Rollback index: 0
=> _____________________________ test_avb_is_unlocked _____________________________ test/py/tests/test_android/test_avb.py:95: in test_avb_is_unlocked assert response == 'Unlocked = 1' E AssertionError: assert 'Not a block ...nUnlocked = 1' == 'Unlocked = 1' E - Not a block device: sandbox_tee E - E Unlocked = 1
---------------------------- Captured stdout setup
/u-boot
U-Boot 2022.04-rc1-00209-g173fff8119 (Feb 10 2022 - 14:59:41 +0000)
Model: sandbox
DRAM: 128 MiB
Core: 248 devices, 90 uclasses, devicetree: board
WDT: Not starting gpio-wdt
WDT: Not starting wdt@0
MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from nowhere... OK
In: cros-ec-keyb
Out: vidconsole
Err: vidconsole
Model: sandbox
SCSI:
Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1
7[r[999;999H[6n8Not a block device: pinmux_i2c0_pins
Not a block device: i2c@0
Not a block device: rtc@61
Not a block device: bootcount@0
Not a block device: emul
Not a block device: emull
Hit any key to stop autoboot: 2 0
=>
----------------------------- Captured stdout call
=> avb init 1
=> => avb is_unlocked
Not a block device: sandbox_tee
Unlocked = 1
=> __________________________ test_avb_persistent_values __________________________ test/py/tests/test_android/test_avb.py:134: in test_avb_persistent_values assert response == 'Wrote 12 bytes' E AssertionError: assert 'Not a block ...rote 12 bytes' == 'Wrote 12 bytes' E - Not a block device: sandbox_tee E - E Wrote 12 bytes
---------------------------- Captured stdout setup
/u-boot
U-Boot 2022.04-rc1-00209-g173fff8119 (Feb 10 2022 - 14:59:41 +0000)
Model: sandbox
DRAM: 128 MiB
Core: 248 devices, 90 uclasses, devicetree: board
WDT: Not starting gpio-wdt
WDT: Not starting wdt@0
MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from nowhere... OK
In: cros-ec-keyb
Out: vidconsole
Err: vidconsole
Model: sandbox
SCSI:
Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1
7[r[999;999H[6n8Not a block device: pinmux_i2c0_pins
Not a block device: i2c@0
Not a block device: rtc@61
Not a block device: bootcount@0
Not a block device: emul
Not a block device: emull
Hit any key to stop autoboot: 2 0
=>
----------------------------- Captured stdout call
=> avb init 1
=> => avb write_pvalue test value_value
Not a block device: sandbox_tee
Wrote 12 bytes
=>
Change history:
v2 (Feb 10, 2022)
- add/revise an error message if device_probe() fails (patch#3,#5)
- fix a build error in sandbox_spl_defconfig (patch#8)
- fix warnings in 'make htmldocs' (patch#8,#9,#18)
- new commit: split efi_init_obj_list() (patch#14)
v1 (Feb 2, 2022)
- rebased on 2022.04-rc1
- drop patches that have already been merged
- modify a tag-range check with "tag >= DM_TAG_COUNT" (patch#9)
- move dmtag_list to GD (global data) (patch#9)
- add function descriptions and a document about DM tag feature (patch#9,10)
- add tests for DM tag support (patch#11)
- change 'depends on EVENT' to 'select EVENT' for EFI_LOADER (patch#14)
- migrate IF_TYPE_EFI to IF_TYPE_EFI_LOADER (patch#18)
RFCv2 (Dec 10, 2021)
- rebased on 2022-rc3
- re-order and merge some related commits into ones
- call device_probe() in MMC (not bind, but) probe hook (patch#5)
- fix a wrong name of variable (patch#7)
- add patch#9
- invoke device_probe() for virtio devices (patch#10)
- add DM event notification (from Simon) (patch#11)
- add DM tag support (patch#12)
- move UCLASS_PARTITION driver under disk/ (patch#13)
- create partition's dp using its parent's. This change is necessary in particular for 'efi_blk' efi_disk (patch#13)
- modify the code so that we will use new features like tags and event notification (patch#13,15,16,20)
- rename new functions from blk_read/write() to dev_read/write() (patch#17,18)
- isolate changes in efi_driver from the rest (in efi_loader) (patch#19)
- drop the previous patch#22 ("efi_selftest: block device: adjust dp for a test") due to the fix in patch#13
RFC (Nov 16, 2021)
- initial RFC
AKASHI Takahiro (19): scsi: call device_probe() after scanning usb: storage: call device_probe() after scanning mmc: call device_probe() after scanning nvme: call device_probe() after scanning sata: call device_probe() after scanning block: ide: call device_probe() after scanning virtio: call device_probe() in scanning dm: add tag support dm: tag: add some document test: dm: add tests for tag support dm: disk: add UCLASS_PARTITION dm: blk: add a device-probe hook for scanning disk partitions efi_loader: split efi_init_obj_list() into two stages efi_loader: disk: a helper function to create efi_disk objects from udevice efi_loader: disk: a helper function to delete efi_disk objects dm: disk: add read/write interfaces with udevice efi_loader: disk: use udevice instead of blk_desc efi_loader: disk: not create BLK device for BLK(IF_TYPE_EFI_LOADER) devices efi_driver: align with efi_disk-dm integration
Simon Glass (1): dm: add event notification
cmd/virtio.c | 21 +- common/Kconfig | 11 + common/Makefile | 2 + common/board_f.c | 2 + common/board_r.c | 2 +- common/event.c | 103 +++++++++ common/log.c | 1 + common/main.c | 7 +- common/usb_storage.c | 4 + disk/Makefile | 3 + disk/disk-uclass.c | 247 +++++++++++++++++++++ doc/develop/driver-model/design.rst | 20 ++ drivers/ata/dwc_ahsata.c | 5 + drivers/ata/fsl_sata.c | 11 + drivers/ata/sata_mv.c | 5 + drivers/ata/sata_sil.c | 12 + drivers/block/blk-uclass.c | 4 + drivers/block/ide.c | 4 + drivers/core/Makefile | 2 +- drivers/core/device-remove.c | 9 + drivers/core/device.c | 9 + drivers/core/root.c | 2 + drivers/core/tag.c | 139 ++++++++++++ drivers/mmc/mmc-uclass.c | 12 + drivers/nvme/nvme.c | 4 + drivers/scsi/scsi.c | 5 + include/asm-generic/global_data.h | 10 + include/dm/device-internal.h | 10 + include/dm/tag.h | 110 +++++++++ include/dm/uclass-id.h | 1 + include/efi_loader.h | 6 +- include/event.h | 105 +++++++++ include/event_internal.h | 34 +++ include/log.h | 2 + include/part.h | 18 ++ lib/efi_driver/efi_block_device.c | 34 +-- lib/efi_loader/Kconfig | 2 + lib/efi_loader/efi_disk.c | 331 ++++++++++++++++++++-------- lib/efi_loader/efi_setup.c | 62 +++++- test/common/Makefile | 1 + test/common/event.c | 87 ++++++++ test/dm/Makefile | 1 + test/dm/tag.c | 80 +++++++ test/test-main.c | 7 + 44 files changed, 1416 insertions(+), 131 deletions(-) create mode 100644 common/event.c create mode 100644 disk/disk-uclass.c create mode 100644 drivers/core/tag.c create mode 100644 include/dm/tag.h create mode 100644 include/event.h create mode 100644 include/event_internal.h create mode 100644 test/common/event.c create mode 100644 test/dm/tag.c
On 2/16/22 09:31, AKASHI Takahiro wrote:
Hi Simon,
On Mon, Feb 14, 2022 at 11:35:06AM +0900, AKASHI Takahiro wrote:
Heinrich,
On Thu, Feb 10, 2022 at 04:20:11PM +0100, Heinrich Schuchardt wrote:
On 2/10/22 09:11, AKASHI Takahiro wrote:
Background:
The purpose of this patch is to reignite the discussion about how UEFI subystem would best be integrated into U-Boot driver model. In the past, I proposed a couple of patch series, the latest one[1], while Heinrich revealed his idea[2], and the approach taken here is something between them, with a focus on block device handlings.
Disks in UEFI world:
In general in UEFI world, accessing to any device is performed through a 'protocol' interface which are installed to (or associated with) the device's UEFI handle (or an opaque pointer to UEFI object data). Protocols are implemented by either the UEFI system itself or UEFI drivers.
For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk hereafter). Currently, every efi_disk may have one of two origins:
a.U-Boot's block devices or related partitions (lib/efi_loader/efi_disk.c) b.UEFI objects which are implemented as a block device by UEFI drivers. (lib/efi_driver/efi_block_device.c)
All the efi_diskss as (a) will be enumerated and created only once at UEFI subsystem initialization (efi_disk_register()), which is triggered by first executing one of UEFI-related U-Boot commands, like "bootefi", "setenv -e" or "efidebug". EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) in the corresponding udevice(UCLASS_BLK).
On the other hand, efi_disk as (b) will be created each time UEFI boot services' connect_controller() is executed in UEFI app which, as a (device) controller, gives the method to access the device's data, ie. EFI_BLOCK_IO_PROTOCOL.
> more details >>>
Internally, connect_controller() search for UEFI driver that can support this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, then calls the driver's 'bind' interface, which eventually installs the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for * creating additional partitions efi_disk's, and * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it. <<< <<<
Issues:
While an efi_disk represents a device equally for either a whole disk or a partition in UEFI world, the driver model treats only a whole disk as a real block device or udevice(UCLASS_BLK).
efi_disk holds and makes use of "blk_desc" data even though blk_desc in plat_data is supposed to be private and not to be accessed outside the driver model. # This issue, though, exists for all the implementation of U-Boot # file systems as well.
For efi_disk(a), 3. A block device can be enumerated dynamically by 'scanning' a device bus in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly. For examples, => scsi rescan; efidebug devices => usb start; efidebug devices ... (A) (A) doesn't show any usb devices detected.
=> scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ... => scsi rescan ... (B) => bootefi bootmgr ... (C) (C) may de-reference a bogus blk_desc pointer which has been freed by (B). (Please note that "scsi rescan" removes all udevices/blk_desc and then re-create them even if nothing is changed on a bus.)For efi_disk(b), 4. A "controller (handle)", combined with efi_block driver, has no corresponding udevice as a parent of efi_disks in DM tree, unlike, say, a scsi controller, even though it provides methods for block io operations. 5. There is no way supported to remove efi_disk's even after disconnect_controller() is called.
My approach:
Due to functional differences in semantics, it would be difficult to identify "udevice" structure as a handle in UEFI world. Instead, we will have to somehow maintain a relationship between a udevice and a handle.
1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions Currently, the uclass for partitions is not a UCLASS_BLK. It can be possible to define partitions as UCLASS_BLK (with IF_TYPE_PARTION?), but I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK) is tightly coupled with 'struct blk_desc' data which is still used as a "structure to a whole disk" in a lot of interfaces. (I hope that you understand what it means.)
In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent: For instance, UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION (IF_TYPE_SCSI) | +- struct blk_desc +- struct disk_part +- scsi_blk_ops +- blk_part_ops1-2. create partition udevices in the context of device_probe() part_init() is already called in blk_post_probe(). See the commit d0851c893706 ("blk: Call part_init() in the post_probe() method"). Why not enumerate partitions as well in there.
- add new block access interfaces, which takes a *udevice* as a target device, in U-Boot and use those functions to implement efi_disk operations (i.e. EFI_BLOCK_IO_PROTOCOL).
3-1. maintain a bi-directional link between a udevice and an efi_disk by adding - a UEFI handle pointer as a tag for a udevice - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
3-2. synchronize the lifetime of efi_disk objects in UEFI world with the driver model using - event notification associated with device's probe/remove.
- I have no solution to issue(4) and (5) yet.
<<<Example DM tree on qemu-arm64>>> => dm tree Class Driver Name
root root_driver root_driver ... pci pci_generic_ecam |-- pcie@10000000 pci_generi pci_generic_drv | |-- pci_0:0.0 virtio virtio-pci.l | |-- virtio-pci.l#0 ethernet virtio-net | | `-- virtio-net#32 ahci ahci_pci | |-- ahci_pci scsi ahci_scsi | | `-- ahci_scsi blk scsi_blk | | |-- ahci_scsi.id0lun0 partition blk_partition | | | |-- ahci_scsi.id0lun0:1 partition blk_partition | | | `-- ahci_scsi.id0lun0:2 blk scsi_blk | | `-- ahci_scsi.id1lun0 partition blk_partition | | |-- ahci_scsi.id1lun0:1 partition blk_partition | | `-- ahci_scsi.id1lun0:2 usb xhci_pci | `-- xhci_pci usb_hub usb_hub | `-- usb_hub usb_dev_ge usb_dev_generic_drv | |-- generic_bus_0_dev_2 usb_mass_s usb_mass_storage | `-- usb_mass_storage blk usb_storage_blk | `-- usb_mass_storage.lun0 partition blk_partition | |-- usb_mass_storage.lun0:1 partition blk_partition | `-- usb_mass_storage.lun0:2 ... => efi devices Device Device Path ================ ==================== 000000013eeea8d0 /VenHw() 000000013eeed810 /VenHw()/MAC(525252525252,1) 000000013eefc460 /VenHw()/Scsi(0,0) 000000013eefc5a0 /VenHw()/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eefe320 /VenHw()/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013eeff210 /VenHw()/Scsi(1,0) 000000013eeff390 /VenHw()/Scsi(1,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eeff7d0 /VenHw()/Scsi(1,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013ef04c20 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0) 000000013ef04da0 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(1,0x01,0,0x0,0x99800) 000000013ef04f70 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(2,0x01,0,0x99800,0x1800)
Patchs:
For easy understandings, patches may be categorized into separate groups of changes.
Patch#1-#7: DM: add device_probe() for later use of events Patch#8-#11: DM: add new features (tag and event notification) Patch#12-#16: UEFI: dynamically create/remove efi_disk's for a raw disk and its partitions For removal case, we may need more consideration since removing handles unconditionally may end up breaking integrity of handles (as some may still be held and referenced to by a UEFI app). Patch#17-#18: UEFI: use udevice read/write interfaces Patch#19-#20: UEFI: fix-up efi_driver, aligning with changes in DM integration
[1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html
This series does not pass Gitlab CI:
See https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391030 https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391031
I have noticed those errors but I didn't think that they were related to my patch set initially as I didn't touch any code in gpt driver, android/avb nor video driver.
I will set the whole series to "changes requested"
Please, run 'make tests' before resubmitting.
Best regards
Heinrich
=================================== FAILURES
________________________________ test_gpt_write ________________________________ test/py/tests/test_gpt.py:169: in test_gpt_write assert 'Writing GPT: success!' in output E AssertionError: assert 'Writing GPT: success!' in 'Writing GPT: Not a block device: rng\r\r\nsuccess!'
The reason of assertion failure here is that some log message was inserted in a output message although the test itself was finished successfully: "Writing GPT: success!" <== a correct output message ^ "Not a block device: rng"
You could adjust the assert() statement to allow for additional messages.
Adding efi_disk_probe() as a callback to EVT_DM_POST_PROBE created this *log_info* message in dm_rng_read() <- get_rand_uuid() <- gen_rand_uuid_str() in "gpt write" command.
We can fix this type of failure by the hack: ===8<=== --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -612,8 +612,6 @@ static int efi_disk_probe(void *ctx, struct event *event)
/* TODO: We won't support partitions in a partition */ if (id != UCLASS_BLK) {
if (id != UCLASS_PARTITION)log_info("Not a block device: %s\n", dev->name); return 0; }===>8===
I don't think, however, that it is a good thing that test results depend on console outputs, especially *log* messages.
Python tests check the user view of the system. If you want to test library functions, you will use a C based unit test.
Furthermore, I don't know why we see *info*-level messages even under CONFIG_LOGLEVEL=4 (warning).
Why are you calling efi_disk_probe() for a rng device? This makes no sense. It should be the block uclass that calls efi_disk_probe() for its children.
Best regards
Heinrich
----------------------------- Captured stdout call
=> host bind 0 /tmp/sandbox/test_gpt_disk_image.bin
=> => gpt write host 0 "name=all,size=0"
Writing GPT: Not a block device: rng
success!
=> ___________________ test_ut[ut_dm_dm_test_video_comp_bmp32] ____________________ test/py/tests/test_ut.py:43: in test_ut assert output.endswith('Failures: 0') E AssertionError: assert False E + where False = <built-in method endswith of str object at 0x7fd72d2fc800>('Failures: 0') E + where <built-in method endswith of str object at 0x7fd72d2fc800> = 'Test: dm_test_video_comp_bmp32: video.c\r\r\nSDL renderer does not exist\r\r\ntest/dm/video.c:88, compress_frame_buff..._test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)\r\r\nFailures: 2'.endswith
----------------------------- Captured stdout call
=> ut dm dm_test_video_comp_bmp32
Test: dm_test_video_comp_bmp32: video.c
SDL renderer does not exist
test/dm/video.c:88, compress_frame_buffer(): !memcmp(uc_priv->fb, uc_priv->copy_fb, uc_priv->fb_size): Copy framebuffer does not match fb
test/dm/video.c:484, dm_test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)
Failures: 2
I don't know yet why this happened.
It seems that this error happened simply because more ut DM tests were added. Added here are DM tag tests (in my patch#14 of 20).
But what type of test is added doesn't matter. When a total number of ut DM tests is increased (and exceeds some limit?), one of tests (either video or another) may unexpectedly fail. For instance, I randomly picked up one test from test/dm/gpio.c and commented it out, and then I didn't see any error in test_ut.py.
So I suspect there may be some problem with pytest framework.
Do you have any clue, Simon?
-Takahiro Akashi
=> _______________________________ test_avb_read_rb _______________________________ test/py/tests/test_android/test_avb.py:83: in test_avb_read_rb assert response == 'Rollback index: 0' E AssertionError: assert 'Not a block ...back index: 0' == 'Rollback index: 0' E - Not a block device: sandbox_tee E - E Rollback index: 0
----------------------------- Captured stdout call
=> avb init 1
=> => avb read_rb 1
Not a block device: sandbox_tee
The same error as mentioned above.
-Takahiro Akashi
Rollback index: 0
=> _____________________________ test_avb_is_unlocked _____________________________ test/py/tests/test_android/test_avb.py:95: in test_avb_is_unlocked assert response == 'Unlocked = 1' E AssertionError: assert 'Not a block ...nUnlocked = 1' == 'Unlocked = 1' E - Not a block device: sandbox_tee E - E Unlocked = 1
---------------------------- Captured stdout setup
/u-boot
U-Boot 2022.04-rc1-00209-g173fff8119 (Feb 10 2022 - 14:59:41 +0000)
Model: sandbox
DRAM: 128 MiB
Core: 248 devices, 90 uclasses, devicetree: board
WDT: Not starting gpio-wdt
WDT: Not starting wdt@0
MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from nowhere... OK
In: cros-ec-keyb
Out: vidconsole
Err: vidconsole
Model: sandbox
SCSI:
Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1
7[r[999;999H[6n8Not a block device: pinmux_i2c0_pins
Not a block device: i2c@0
Not a block device: rtc@61
Not a block device: bootcount@0
Not a block device: emul
Not a block device: emull
Hit any key to stop autoboot: 2 0
=>
----------------------------- Captured stdout call
=> avb init 1
=> => avb is_unlocked
Not a block device: sandbox_tee
Unlocked = 1
=> __________________________ test_avb_persistent_values __________________________ test/py/tests/test_android/test_avb.py:134: in test_avb_persistent_values assert response == 'Wrote 12 bytes' E AssertionError: assert 'Not a block ...rote 12 bytes' == 'Wrote 12 bytes' E - Not a block device: sandbox_tee E - E Wrote 12 bytes
---------------------------- Captured stdout setup
/u-boot
U-Boot 2022.04-rc1-00209-g173fff8119 (Feb 10 2022 - 14:59:41 +0000)
Model: sandbox
DRAM: 128 MiB
Core: 248 devices, 90 uclasses, devicetree: board
WDT: Not starting gpio-wdt
WDT: Not starting wdt@0
MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from nowhere... OK
In: cros-ec-keyb
Out: vidconsole
Err: vidconsole
Model: sandbox
SCSI:
Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1
7[r[999;999H[6n8Not a block device: pinmux_i2c0_pins
Not a block device: i2c@0
Not a block device: rtc@61
Not a block device: bootcount@0
Not a block device: emul
Not a block device: emull
Hit any key to stop autoboot: 2 0
=>
----------------------------- Captured stdout call
=> avb init 1
=> => avb write_pvalue test value_value
Not a block device: sandbox_tee
Wrote 12 bytes
=>
Change history:
v2 (Feb 10, 2022)
- add/revise an error message if device_probe() fails (patch#3,#5)
- fix a build error in sandbox_spl_defconfig (patch#8)
- fix warnings in 'make htmldocs' (patch#8,#9,#18)
- new commit: split efi_init_obj_list() (patch#14)
v1 (Feb 2, 2022)
- rebased on 2022.04-rc1
- drop patches that have already been merged
- modify a tag-range check with "tag >= DM_TAG_COUNT" (patch#9)
- move dmtag_list to GD (global data) (patch#9)
- add function descriptions and a document about DM tag feature (patch#9,10)
- add tests for DM tag support (patch#11)
- change 'depends on EVENT' to 'select EVENT' for EFI_LOADER (patch#14)
- migrate IF_TYPE_EFI to IF_TYPE_EFI_LOADER (patch#18)
RFCv2 (Dec 10, 2021)
- rebased on 2022-rc3
- re-order and merge some related commits into ones
- call device_probe() in MMC (not bind, but) probe hook (patch#5)
- fix a wrong name of variable (patch#7)
- add patch#9
- invoke device_probe() for virtio devices (patch#10)
- add DM event notification (from Simon) (patch#11)
- add DM tag support (patch#12)
- move UCLASS_PARTITION driver under disk/ (patch#13)
- create partition's dp using its parent's. This change is necessary in particular for 'efi_blk' efi_disk (patch#13)
- modify the code so that we will use new features like tags and event notification (patch#13,15,16,20)
- rename new functions from blk_read/write() to dev_read/write() (patch#17,18)
- isolate changes in efi_driver from the rest (in efi_loader) (patch#19)
- drop the previous patch#22 ("efi_selftest: block device: adjust dp for a test") due to the fix in patch#13
RFC (Nov 16, 2021)
- initial RFC
AKASHI Takahiro (19): scsi: call device_probe() after scanning usb: storage: call device_probe() after scanning mmc: call device_probe() after scanning nvme: call device_probe() after scanning sata: call device_probe() after scanning block: ide: call device_probe() after scanning virtio: call device_probe() in scanning dm: add tag support dm: tag: add some document test: dm: add tests for tag support dm: disk: add UCLASS_PARTITION dm: blk: add a device-probe hook for scanning disk partitions efi_loader: split efi_init_obj_list() into two stages efi_loader: disk: a helper function to create efi_disk objects from udevice efi_loader: disk: a helper function to delete efi_disk objects dm: disk: add read/write interfaces with udevice efi_loader: disk: use udevice instead of blk_desc efi_loader: disk: not create BLK device for BLK(IF_TYPE_EFI_LOADER) devices efi_driver: align with efi_disk-dm integration
Simon Glass (1): dm: add event notification
cmd/virtio.c | 21 +- common/Kconfig | 11 + common/Makefile | 2 + common/board_f.c | 2 + common/board_r.c | 2 +- common/event.c | 103 +++++++++ common/log.c | 1 + common/main.c | 7 +- common/usb_storage.c | 4 + disk/Makefile | 3 + disk/disk-uclass.c | 247 +++++++++++++++++++++ doc/develop/driver-model/design.rst | 20 ++ drivers/ata/dwc_ahsata.c | 5 + drivers/ata/fsl_sata.c | 11 + drivers/ata/sata_mv.c | 5 + drivers/ata/sata_sil.c | 12 + drivers/block/blk-uclass.c | 4 + drivers/block/ide.c | 4 + drivers/core/Makefile | 2 +- drivers/core/device-remove.c | 9 + drivers/core/device.c | 9 + drivers/core/root.c | 2 + drivers/core/tag.c | 139 ++++++++++++ drivers/mmc/mmc-uclass.c | 12 + drivers/nvme/nvme.c | 4 + drivers/scsi/scsi.c | 5 + include/asm-generic/global_data.h | 10 + include/dm/device-internal.h | 10 + include/dm/tag.h | 110 +++++++++ include/dm/uclass-id.h | 1 + include/efi_loader.h | 6 +- include/event.h | 105 +++++++++ include/event_internal.h | 34 +++ include/log.h | 2 + include/part.h | 18 ++ lib/efi_driver/efi_block_device.c | 34 +-- lib/efi_loader/Kconfig | 2 + lib/efi_loader/efi_disk.c | 331 ++++++++++++++++++++-------- lib/efi_loader/efi_setup.c | 62 +++++- test/common/Makefile | 1 + test/common/event.c | 87 ++++++++ test/dm/Makefile | 1 + test/dm/tag.c | 80 +++++++ test/test-main.c | 7 + 44 files changed, 1416 insertions(+), 131 deletions(-) create mode 100644 common/event.c create mode 100644 disk/disk-uclass.c create mode 100644 drivers/core/tag.c create mode 100644 include/dm/tag.h create mode 100644 include/event.h create mode 100644 include/event_internal.h create mode 100644 test/common/event.c create mode 100644 test/dm/tag.c
Hi Takahiro,
On Wed, 16 Feb 2022 at 01:31, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Mon, Feb 14, 2022 at 11:35:06AM +0900, AKASHI Takahiro wrote:
Heinrich,
On Thu, Feb 10, 2022 at 04:20:11PM +0100, Heinrich Schuchardt wrote:
On 2/10/22 09:11, AKASHI Takahiro wrote:
Background:
The purpose of this patch is to reignite the discussion about how UEFI subystem would best be integrated into U-Boot driver model. In the past, I proposed a couple of patch series, the latest one[1], while Heinrich revealed his idea[2], and the approach taken here is something between them, with a focus on block device handlings.
Disks in UEFI world:
In general in UEFI world, accessing to any device is performed through a 'protocol' interface which are installed to (or associated with) the device's UEFI handle (or an opaque pointer to UEFI object data). Protocols are implemented by either the UEFI system itself or UEFI drivers.
For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk hereafter). Currently, every efi_disk may have one of two origins:
a.U-Boot's block devices or related partitions (lib/efi_loader/efi_disk.c) b.UEFI objects which are implemented as a block device by UEFI drivers. (lib/efi_driver/efi_block_device.c)
All the efi_diskss as (a) will be enumerated and created only once at UEFI subsystem initialization (efi_disk_register()), which is triggered by first executing one of UEFI-related U-Boot commands, like "bootefi", "setenv -e" or "efidebug". EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) in the corresponding udevice(UCLASS_BLK).
On the other hand, efi_disk as (b) will be created each time UEFI boot services' connect_controller() is executed in UEFI app which, as a (device) controller, gives the method to access the device's data, ie. EFI_BLOCK_IO_PROTOCOL.
> more details >>>
Internally, connect_controller() search for UEFI driver that can support this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, then calls the driver's 'bind' interface, which eventually installs the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for
- creating additional partitions efi_disk's, and
- supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it.
<<< <<<
Issues:
While an efi_disk represents a device equally for either a whole disk or a partition in UEFI world, the driver model treats only a whole disk as a real block device or udevice(UCLASS_BLK).
efi_disk holds and makes use of "blk_desc" data even though blk_desc in plat_data is supposed to be private and not to be accessed outside the driver model. # This issue, though, exists for all the implementation of U-Boot # file systems as well.
For efi_disk(a), 3. A block device can be enumerated dynamically by 'scanning' a device bus in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly. For examples, => scsi rescan; efidebug devices => usb start; efidebug devices ... (A) (A) doesn't show any usb devices detected.
=> scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ... => scsi rescan ... (B) => bootefi bootmgr ... (C) (C) may de-reference a bogus blk_desc pointer which has been freed by (B). (Please note that "scsi rescan" removes all udevices/blk_desc and then re-create them even if nothing is changed on a bus.)For efi_disk(b), 4. A "controller (handle)", combined with efi_block driver, has no corresponding udevice as a parent of efi_disks in DM tree, unlike, say, a scsi controller, even though it provides methods for block io operations. 5. There is no way supported to remove efi_disk's even after disconnect_controller() is called.
My approach:
Due to functional differences in semantics, it would be difficult to identify "udevice" structure as a handle in UEFI world. Instead, we will have to somehow maintain a relationship between a udevice and a handle.
1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions Currently, the uclass for partitions is not a UCLASS_BLK. It can be possible to define partitions as UCLASS_BLK (with IF_TYPE_PARTION?), but I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK) is tightly coupled with 'struct blk_desc' data which is still used as a "structure to a whole disk" in a lot of interfaces. (I hope that you understand what it means.)
In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent: For instance, UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION (IF_TYPE_SCSI) | +- struct blk_desc +- struct disk_part +- scsi_blk_ops +- blk_part_ops1-2. create partition udevices in the context of device_probe() part_init() is already called in blk_post_probe(). See the commit d0851c893706 ("blk: Call part_init() in the post_probe() method"). Why not enumerate partitions as well in there.
- add new block access interfaces, which takes a *udevice* as a target device, in U-Boot and use those functions to implement efi_disk operations (i.e. EFI_BLOCK_IO_PROTOCOL).
3-1. maintain a bi-directional link between a udevice and an efi_disk by adding - a UEFI handle pointer as a tag for a udevice - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
3-2. synchronize the lifetime of efi_disk objects in UEFI world with the driver model using - event notification associated with device's probe/remove.
- I have no solution to issue(4) and (5) yet.
<<<Example DM tree on qemu-arm64>>> => dm tree Class Driver Name
root root_driver root_driver ... pci pci_generic_ecam |-- pcie@10000000 pci_generi pci_generic_drv | |-- pci_0:0.0 virtio virtio-pci.l | |-- virtio-pci.l#0 ethernet virtio-net | | `-- virtio-net#32 ahci ahci_pci | |-- ahci_pci scsi ahci_scsi | | `-- ahci_scsi blk scsi_blk | | |-- ahci_scsi.id0lun0 partition blk_partition | | | |-- ahci_scsi.id0lun0:1 partition blk_partition | | | `-- ahci_scsi.id0lun0:2 blk scsi_blk | | `-- ahci_scsi.id1lun0 partition blk_partition | | |-- ahci_scsi.id1lun0:1 partition blk_partition | | `-- ahci_scsi.id1lun0:2 usb xhci_pci | `-- xhci_pci usb_hub usb_hub | `-- usb_hub usb_dev_ge usb_dev_generic_drv | |-- generic_bus_0_dev_2 usb_mass_s usb_mass_storage | `-- usb_mass_storage blk usb_storage_blk | `-- usb_mass_storage.lun0 partition blk_partition | |-- usb_mass_storage.lun0:1 partition blk_partition | `-- usb_mass_storage.lun0:2 ... => efi devices Device Device Path ================ ==================== 000000013eeea8d0 /VenHw() 000000013eeed810 /VenHw()/MAC(525252525252,1) 000000013eefc460 /VenHw()/Scsi(0,0) 000000013eefc5a0 /VenHw()/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eefe320 /VenHw()/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013eeff210 /VenHw()/Scsi(1,0) 000000013eeff390 /VenHw()/Scsi(1,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eeff7d0 /VenHw()/Scsi(1,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013ef04c20 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0) 000000013ef04da0 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(1,0x01,0,0x0,0x99800) 000000013ef04f70 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(2,0x01,0,0x99800,0x1800)
Patchs:
For easy understandings, patches may be categorized into separate groups of changes.
Patch#1-#7: DM: add device_probe() for later use of events Patch#8-#11: DM: add new features (tag and event notification) Patch#12-#16: UEFI: dynamically create/remove efi_disk's for a raw disk and its partitions For removal case, we may need more consideration since removing handles unconditionally may end up breaking integrity of handles (as some may still be held and referenced to by a UEFI app). Patch#17-#18: UEFI: use udevice read/write interfaces Patch#19-#20: UEFI: fix-up efi_driver, aligning with changes in DM integration
[1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html
This series does not pass Gitlab CI:
See https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391030 https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391031
I have noticed those errors but I didn't think that they were related to my patch set initially as I didn't touch any code in gpt driver, android/avb nor video driver.
I will set the whole series to "changes requested"
Please, run 'make tests' before resubmitting.
Best regards
Heinrich
=================================== FAILURES
________________________________ test_gpt_write ________________________________ test/py/tests/test_gpt.py:169: in test_gpt_write assert 'Writing GPT: success!' in output E AssertionError: assert 'Writing GPT: success!' in 'Writing GPT: Not a block device: rng\r\r\nsuccess!'
The reason of assertion failure here is that some log message was inserted in a output message although the test itself was finished successfully: "Writing GPT: success!" <== a correct output message ^ "Not a block device: rng"
Adding efi_disk_probe() as a callback to EVT_DM_POST_PROBE created this *log_info* message in dm_rng_read() <- get_rand_uuid() <- gen_rand_uuid_str() in "gpt write" command.
We can fix this type of failure by the hack: ===8<=== --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -612,8 +612,6 @@ static int efi_disk_probe(void *ctx, struct event *event)
/* TODO: We won't support partitions in a partition */ if (id != UCLASS_BLK) {
if (id != UCLASS_PARTITION)log_info("Not a block device: %s\n", dev->name); return 0; }===>8===
I don't think, however, that it is a good thing that test results depend on console outputs, especially *log* messages.
Furthermore, I don't know why we see *info*-level messages even under CONFIG_LOGLEVEL=4 (warning).
----------------------------- Captured stdout call
=> host bind 0 /tmp/sandbox/test_gpt_disk_image.bin
=> => gpt write host 0 "name=all,size=0"
Writing GPT: Not a block device: rng
success!
=> ___________________ test_ut[ut_dm_dm_test_video_comp_bmp32] ____________________ test/py/tests/test_ut.py:43: in test_ut assert output.endswith('Failures: 0') E AssertionError: assert False E + where False = <built-in method endswith of str object at 0x7fd72d2fc800>('Failures: 0') E + where <built-in method endswith of str object at 0x7fd72d2fc800> = 'Test: dm_test_video_comp_bmp32: video.c\r\r\nSDL renderer does not exist\r\r\ntest/dm/video.c:88, compress_frame_buff..._test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)\r\r\nFailures: 2'.endswith
----------------------------- Captured stdout call
=> ut dm dm_test_video_comp_bmp32
Test: dm_test_video_comp_bmp32: video.c
SDL renderer does not exist
test/dm/video.c:88, compress_frame_buffer(): !memcmp(uc_priv->fb, uc_priv->copy_fb, uc_priv->fb_size): Copy framebuffer does not match fb
test/dm/video.c:484, dm_test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)
Failures: 2
I don't know yet why this happened.
It seems that this error happened simply because more ut DM tests were added. Added here are DM tag tests (in my patch#14 of 20).
But what type of test is added doesn't matter. When a total number of ut DM tests is increased (and exceeds some limit?), one of tests (either video or another) may unexpectedly fail. For instance, I randomly picked up one test from test/dm/gpio.c and commented it out, and then I didn't see any error in test_ut.py.
So I suspect there may be some problem with pytest framework.
Do you have any clue, Simon?
Yes I believe it is a problem with memory allocation. Perhaps we run out of memory, or something else goes wrong. The value:
#define top (av_[2])
seems to get corrupted. I did spent some time trying to figure out what it was but have not found it yet.
Regards, Simon
Hi Simon,
On Wed, Feb 16, 2022 at 12:00:10PM -0700, Simon Glass wrote:
Hi Takahiro,
On Wed, 16 Feb 2022 at 01:31, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Mon, Feb 14, 2022 at 11:35:06AM +0900, AKASHI Takahiro wrote:
Heinrich,
On Thu, Feb 10, 2022 at 04:20:11PM +0100, Heinrich Schuchardt wrote:
On 2/10/22 09:11, AKASHI Takahiro wrote:
Background:
The purpose of this patch is to reignite the discussion about how UEFI subystem would best be integrated into U-Boot driver model. In the past, I proposed a couple of patch series, the latest one[1], while Heinrich revealed his idea[2], and the approach taken here is something between them, with a focus on block device handlings.
Disks in UEFI world:
In general in UEFI world, accessing to any device is performed through a 'protocol' interface which are installed to (or associated with) the device's UEFI handle (or an opaque pointer to UEFI object data). Protocols are implemented by either the UEFI system itself or UEFI drivers.
For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk hereafter). Currently, every efi_disk may have one of two origins:
a.U-Boot's block devices or related partitions (lib/efi_loader/efi_disk.c) b.UEFI objects which are implemented as a block device by UEFI drivers. (lib/efi_driver/efi_block_device.c)
All the efi_diskss as (a) will be enumerated and created only once at UEFI subsystem initialization (efi_disk_register()), which is triggered by first executing one of UEFI-related U-Boot commands, like "bootefi", "setenv -e" or "efidebug". EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) in the corresponding udevice(UCLASS_BLK).
On the other hand, efi_disk as (b) will be created each time UEFI boot services' connect_controller() is executed in UEFI app which, as a (device) controller, gives the method to access the device's data, ie. EFI_BLOCK_IO_PROTOCOL.
> > more details >>>
Internally, connect_controller() search for UEFI driver that can support this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, then calls the driver's 'bind' interface, which eventually installs the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for
- creating additional partitions efi_disk's, and
- supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it.
<<< <<<
Issues:
While an efi_disk represents a device equally for either a whole disk or a partition in UEFI world, the driver model treats only a whole disk as a real block device or udevice(UCLASS_BLK).
efi_disk holds and makes use of "blk_desc" data even though blk_desc in plat_data is supposed to be private and not to be accessed outside the driver model. # This issue, though, exists for all the implementation of U-Boot # file systems as well.
For efi_disk(a), 3. A block device can be enumerated dynamically by 'scanning' a device bus in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly. For examples, => scsi rescan; efidebug devices => usb start; efidebug devices ... (A) (A) doesn't show any usb devices detected.
=> scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ... => scsi rescan ... (B) => bootefi bootmgr ... (C) (C) may de-reference a bogus blk_desc pointer which has been freed by (B). (Please note that "scsi rescan" removes all udevices/blk_desc and then re-create them even if nothing is changed on a bus.)For efi_disk(b), 4. A "controller (handle)", combined with efi_block driver, has no corresponding udevice as a parent of efi_disks in DM tree, unlike, say, a scsi controller, even though it provides methods for block io operations. 5. There is no way supported to remove efi_disk's even after disconnect_controller() is called.
My approach:
Due to functional differences in semantics, it would be difficult to identify "udevice" structure as a handle in UEFI world. Instead, we will have to somehow maintain a relationship between a udevice and a handle.
1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions Currently, the uclass for partitions is not a UCLASS_BLK. It can be possible to define partitions as UCLASS_BLK (with IF_TYPE_PARTION?), but I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK) is tightly coupled with 'struct blk_desc' data which is still used as a "structure to a whole disk" in a lot of interfaces. (I hope that you understand what it means.)
In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent: For instance, UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION (IF_TYPE_SCSI) | +- struct blk_desc +- struct disk_part +- scsi_blk_ops +- blk_part_ops1-2. create partition udevices in the context of device_probe() part_init() is already called in blk_post_probe(). See the commit d0851c893706 ("blk: Call part_init() in the post_probe() method"). Why not enumerate partitions as well in there.
- add new block access interfaces, which takes a *udevice* as a target device, in U-Boot and use those functions to implement efi_disk operations (i.e. EFI_BLOCK_IO_PROTOCOL).
3-1. maintain a bi-directional link between a udevice and an efi_disk by adding - a UEFI handle pointer as a tag for a udevice - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
3-2. synchronize the lifetime of efi_disk objects in UEFI world with the driver model using - event notification associated with device's probe/remove.
- I have no solution to issue(4) and (5) yet.
<<<Example DM tree on qemu-arm64>>> => dm tree Class Driver Name
root root_driver root_driver ... pci pci_generic_ecam |-- pcie@10000000 pci_generi pci_generic_drv | |-- pci_0:0.0 virtio virtio-pci.l | |-- virtio-pci.l#0 ethernet virtio-net | | `-- virtio-net#32 ahci ahci_pci | |-- ahci_pci scsi ahci_scsi | | `-- ahci_scsi blk scsi_blk | | |-- ahci_scsi.id0lun0 partition blk_partition | | | |-- ahci_scsi.id0lun0:1 partition blk_partition | | | `-- ahci_scsi.id0lun0:2 blk scsi_blk | | `-- ahci_scsi.id1lun0 partition blk_partition | | |-- ahci_scsi.id1lun0:1 partition blk_partition | | `-- ahci_scsi.id1lun0:2 usb xhci_pci | `-- xhci_pci usb_hub usb_hub | `-- usb_hub usb_dev_ge usb_dev_generic_drv | |-- generic_bus_0_dev_2 usb_mass_s usb_mass_storage | `-- usb_mass_storage blk usb_storage_blk | `-- usb_mass_storage.lun0 partition blk_partition | |-- usb_mass_storage.lun0:1 partition blk_partition | `-- usb_mass_storage.lun0:2 ... => efi devices Device Device Path ================ ==================== 000000013eeea8d0 /VenHw() 000000013eeed810 /VenHw()/MAC(525252525252,1) 000000013eefc460 /VenHw()/Scsi(0,0) 000000013eefc5a0 /VenHw()/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eefe320 /VenHw()/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013eeff210 /VenHw()/Scsi(1,0) 000000013eeff390 /VenHw()/Scsi(1,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) 000000013eeff7d0 /VenHw()/Scsi(1,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) 000000013ef04c20 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0) 000000013ef04da0 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(1,0x01,0,0x0,0x99800) 000000013ef04f70 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(2,0x01,0,0x99800,0x1800)
Patchs:
For easy understandings, patches may be categorized into separate groups of changes.
Patch#1-#7: DM: add device_probe() for later use of events Patch#8-#11: DM: add new features (tag and event notification) Patch#12-#16: UEFI: dynamically create/remove efi_disk's for a raw disk and its partitions For removal case, we may need more consideration since removing handles unconditionally may end up breaking integrity of handles (as some may still be held and referenced to by a UEFI app). Patch#17-#18: UEFI: use udevice read/write interfaces Patch#19-#20: UEFI: fix-up efi_driver, aligning with changes in DM integration
[1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html
This series does not pass Gitlab CI:
See https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391030 https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391031
I have noticed those errors but I didn't think that they were related to my patch set initially as I didn't touch any code in gpt driver, android/avb nor video driver.
I will set the whole series to "changes requested"
Please, run 'make tests' before resubmitting.
Best regards
Heinrich
=================================== FAILURES
________________________________ test_gpt_write ________________________________ test/py/tests/test_gpt.py:169: in test_gpt_write assert 'Writing GPT: success!' in output E AssertionError: assert 'Writing GPT: success!' in 'Writing GPT: Not a block device: rng\r\r\nsuccess!'
The reason of assertion failure here is that some log message was inserted in a output message although the test itself was finished successfully: "Writing GPT: success!" <== a correct output message ^ "Not a block device: rng"
Adding efi_disk_probe() as a callback to EVT_DM_POST_PROBE created this *log_info* message in dm_rng_read() <- get_rand_uuid() <- gen_rand_uuid_str() in "gpt write" command.
We can fix this type of failure by the hack: ===8<=== --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -612,8 +612,6 @@ static int efi_disk_probe(void *ctx, struct event *event)
/* TODO: We won't support partitions in a partition */ if (id != UCLASS_BLK) {
if (id != UCLASS_PARTITION)log_info("Not a block device: %s\n", dev->name); return 0; }===>8===
I don't think, however, that it is a good thing that test results depend on console outputs, especially *log* messages.
Furthermore, I don't know why we see *info*-level messages even under CONFIG_LOGLEVEL=4 (warning).
----------------------------- Captured stdout call
=> host bind 0 /tmp/sandbox/test_gpt_disk_image.bin
=> => gpt write host 0 "name=all,size=0"
Writing GPT: Not a block device: rng
success!
=> ___________________ test_ut[ut_dm_dm_test_video_comp_bmp32] ____________________ test/py/tests/test_ut.py:43: in test_ut assert output.endswith('Failures: 0') E AssertionError: assert False E + where False = <built-in method endswith of str object at 0x7fd72d2fc800>('Failures: 0') E + where <built-in method endswith of str object at 0x7fd72d2fc800> = 'Test: dm_test_video_comp_bmp32: video.c\r\r\nSDL renderer does not exist\r\r\ntest/dm/video.c:88, compress_frame_buff..._test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)\r\r\nFailures: 2'.endswith
----------------------------- Captured stdout call
=> ut dm dm_test_video_comp_bmp32
Test: dm_test_video_comp_bmp32: video.c
SDL renderer does not exist
test/dm/video.c:88, compress_frame_buffer(): !memcmp(uc_priv->fb, uc_priv->copy_fb, uc_priv->fb_size): Copy framebuffer does not match fb
test/dm/video.c:484, dm_test_video_comp_bmp32(): 2024 == compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)
Failures: 2
I don't know yet why this happened.
It seems that this error happened simply because more ut DM tests were added. Added here are DM tag tests (in my patch#14 of 20).
But what type of test is added doesn't matter. When a total number of ut DM tests is increased (and exceeds some limit?), one of tests (either video or another) may unexpectedly fail. For instance, I randomly picked up one test from test/dm/gpio.c and commented it out, and then I didn't see any error in test_ut.py.
So I suspect there may be some problem with pytest framework.
Do you have any clue, Simon?
Yes I believe it is a problem with memory allocation. Perhaps we run out of memory, or something else goes wrong. The value:
#define top (av_[2])
seems to get corrupted. I did spent some time trying to figure out what it was but have not found it yet.
Do you have any new insight here?
I still see the same problem even after rebasing my DM-integration patches to v2022.07 branch and I want to fix the issue.
-Takahiro Akashi
Regards, Simon
participants (4)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Jaehoon Chung -
Simon Glass