[U-Boot] [PATCH v2 0/3] efi_loader: add removable device support
Under the current implementation, any removable device which is attached to the platform will not be recognized after any efi-related command, in particular bootefi, is once executed.
This patch set resolves this problem by re-scanning and recreating a disk device list for efi operations.
# I didn't run selftest "block device" as I don't know how we can # create a test disk image. # Heinrich, can you provide a script?
-Takahiro Akashi
Changes in v2 (Nov 15, 2018) * efi_init_obj_list() will never return an error once it has succeeded * add "invalid" flag to efi_disk to indicate that a given device is now in stale state * modify block io protocol and file protocol to honor invalid flag * simplify efi simple file system protocol/file procotol by removing details of an u-boot block device * some function were renamed after Heinrich's comment
AKASHI Takahiro (3): efi_loader: export efi_locate_handle() function efi_loader: enumerate disk devices every time efi_loader: remove block device details from efi file
cmd/bootefi.c | 17 ++- include/efi_loader.h | 14 ++- lib/efi_loader/efi_boottime.c | 7 +- lib/efi_loader/efi_disk.c | 229 +++++++++++++++++++++++++++++++++- lib/efi_loader/efi_file.c | 21 ++-- 5 files changed, 266 insertions(+), 22 deletions(-)
This function will be used later to implement efi_disk_update().
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- include/efi_loader.h | 4 ++++ lib/efi_loader/efi_boottime.c | 7 +++---- 2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index ce0f420b5004..5cc3bded03fa 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -318,6 +318,10 @@ efi_status_t efi_create_handle(efi_handle_t *handle); void efi_delete_handle(efi_handle_t obj); /* Call this to validate a handle and find the EFI object for it */ struct efi_object *efi_search_obj(const efi_handle_t handle); +/* locate handles */ +efi_status_t efi_locate_handle(enum efi_locate_search_type search_type, + const efi_guid_t *protocol, void *search_key, + efi_uintn_t *buffer_size, efi_handle_t *buffer); /* Find a protocol on a handle */ efi_status_t efi_search_protocol(const efi_handle_t handle, const efi_guid_t *protocol_guid, diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 8a43a5a84091..a3c56bbab552 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1309,10 +1309,9 @@ static int efi_search(enum efi_locate_search_type search_type, * * Return: status code */ -static efi_status_t efi_locate_handle( - enum efi_locate_search_type search_type, - const efi_guid_t *protocol, void *search_key, - efi_uintn_t *buffer_size, efi_handle_t *buffer) +efi_status_t efi_locate_handle(enum efi_locate_search_type search_type, + const efi_guid_t *protocol, void *search_key, + efi_uintn_t *buffer_size, efi_handle_t *buffer) { struct efi_object *efiobj; efi_uintn_t size = 0;
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 17 +++- include/efi_loader.h | 4 + lib/efi_loader/efi_disk.c | 191 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 210 insertions(+), 2 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3cefb4d0ecaa..82649e211fda 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -56,9 +56,22 @@ efi_status_t efi_init_obj_list(void) */ efi_save_gd();
- /* Initialize once only */ - if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) + if (efi_obj_list_initialized == EFI_SUCCESS) { +#ifdef CONFIG_PARTITIONS + ret = efi_disk_update(); + if (ret != EFI_SUCCESS) + printf("+++ updating disks list failed\n"); + + /* + * It may sound odd, but most part of EFI should + * yet work. + */ +#endif return efi_obj_list_initialized; + } else if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) { + /* Initialize once only */ + return efi_obj_list_initialized; + }
/* Initialize system table */ ret = efi_initialize_system_table(); diff --git a/include/efi_loader.h b/include/efi_loader.h index 5cc3bded03fa..3bae1844befb 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -260,6 +260,10 @@ efi_status_t efi_initialize_system_table(void); 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); +/* Check validity of efi disk */ +bool efi_disk_is_valid(efi_handle_t handle); +/* Called by bootefi to find and update disk storage information */ +efi_status_t efi_disk_update(void); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index c037526ad2d0..0c4d79ee3fc9 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -14,10 +14,14 @@
const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
+#define _EFI_DISK_FLAG_DELETED 0x1 /* to be removed */ +#define _EFI_DISK_FLAG_INVALID 0x80000000 /* in stale state */ + /** * struct efi_disk_obj - EFI disk object * * @header: EFI object header + * @flags: control flags * @ops: EFI disk I/O protocol interface * @ifname: interface name for block device * @dev_index: device index of block device @@ -30,6 +34,7 @@ const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID; */ struct efi_disk_obj { struct efi_object header; + int flags; struct efi_block_io ops; const char *ifname; int dev_index; @@ -41,6 +46,35 @@ struct efi_disk_obj { struct blk_desc *desc; };
+static void efi_disk_mark_deleted(struct efi_disk_obj *disk) +{ + disk->flags |= _EFI_DISK_FLAG_DELETED; +} + +static void efi_disk_clear_deleted(struct efi_disk_obj *disk) +{ + disk->flags &= ~_EFI_DISK_FLAG_DELETED; +} + +static bool efi_disk_deleted_marked(struct efi_disk_obj *disk) +{ + return disk->flags & _EFI_DISK_FLAG_DELETED; +} + +static void efi_disk_mark_invalid(struct efi_disk_obj *disk) +{ + disk->flags |= _EFI_DISK_FLAG_INVALID; +} + +bool efi_disk_is_valid(efi_handle_t handle) +{ + struct efi_disk_obj *disk; + + disk = container_of(handle, struct efi_disk_obj, header); + + return !(disk->flags & _EFI_DISK_FLAG_INVALID); +} + static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this, char extended_verification) { @@ -64,6 +98,9 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, unsigned long n;
diskobj = container_of(this, struct efi_disk_obj, ops); + if (diskobj->flags & _EFI_DISK_FLAG_INVALID) + return EFI_DEVICE_ERROR; + desc = (struct blk_desc *) diskobj->desc; blksz = desc->blksz; blocks = buffer_size / blksz; @@ -440,3 +477,157 @@ efi_status_t efi_disk_register(void)
return EFI_SUCCESS; } + +/* + * Mark all the block devices as "deleted," and return an array of + * handles for later use. It should be freed in a caller. + */ +static efi_status_t efi_disk_mark_deleted_all(efi_handle_t **handlesp) +{ + efi_handle_t *handles = NULL; + efi_uintn_t size = 0; + int num, i; + struct efi_disk_obj *disk; + efi_status_t ret; + + ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL, + &size, handles); + if (ret == EFI_BUFFER_TOO_SMALL) { + handles = calloc(1, size); + if (!handles) + return EFI_OUT_OF_RESOURCES; + + ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL, + &size, handles); + } + if (ret != EFI_SUCCESS) { + free(handles); + return ret; + } + + num = size / sizeof(*handles); + for (i = 0; i < num; i++) { + disk = container_of(handles[i], struct efi_disk_obj, header); + efi_disk_mark_deleted(disk); + } + + *handlesp = handles; + + return num; +} + +/* + * Clear "deleted" flag for a block device which is identified with desc. + * If desc is NULL, clear all devices. + * + * Return a number of disks cleared. + */ +static int efi_disk_clear_deleted_matched(struct blk_desc *desc, + efi_handle_t *handles, int num) +{ + struct efi_disk_obj *disk; + int disks, i; + + for (i = 0, disks = 0; i < num; i++) { + disk = container_of(handles[i], struct efi_disk_obj, header); + + if (!desc || disk->desc == desc) { + efi_disk_clear_deleted(disk); + disks++; + } + } + + return disks; +} + +/* + * Do delete all the block devices marked as "deleted" + */ +static efi_status_t efi_disk_do_delete(efi_handle_t *handles, int num) +{ + struct efi_disk_obj *disk; + int i; + + for (i = 0; i < num; i++) { + disk = container_of(handles[i], struct efi_disk_obj, header); + + if (!efi_disk_deleted_marked(disk)) + continue; + + efi_disk_mark_invalid(disk); + /* + * TODO: + * efi_delete_handle(handles[i]); + */ + } + + return EFI_SUCCESS; +} + +/* + * efi_disk_update - recreate efi disk mappings after initialization + * + * @return efi error code + */ +efi_status_t efi_disk_update(void) +{ +#ifdef CONFIG_BLK + efi_handle_t *handles = NULL; + struct udevice *dev; + struct blk_desc *desc; + const char *if_typename; + struct efi_disk_obj *disk; + int n, disks = 0; + efi_status_t ret; + + /* temporarily mark all the devices "deleted" */ + ret = efi_disk_mark_deleted_all(&handles); + if (ret & EFI_ERROR_MASK) { + printf("ERROR: Failed to rescan block devices.\n"); + return ret; + } + + n = (int)ret; + for (uclass_first_device_check(UCLASS_BLK, &dev); dev; + uclass_next_device_check(&dev)) { + desc = dev_get_uclass_platdata(dev); + if (n && efi_disk_clear_deleted_matched(desc, handles, n)) + /* existing device */ + continue; + + /* new device */ + if_typename = blk_get_if_type_name(desc->if_type); + + /* Add block device for the full device */ + printf("Scanning disk %s...\n", dev->name); + ret = efi_disk_add_dev(NULL, NULL, if_typename, + desc, desc->devnum, 0, 0, &disk); + if (ret == EFI_NOT_READY) { + printf("Disk %s not ready\n", dev->name); + continue; + } + if (ret != EFI_SUCCESS) { + printf("ERROR: failure to add disk device %s, r = %lu\n", + dev->name, ret & ~EFI_ERROR_MASK); + continue; + } + disks++; + + /* Partitions show up as block devices in EFI */ + disks += efi_disk_create_partitions(&disk->header, desc, + if_typename, + desc->devnum, dev->name); + } + + if (n) { + /* do delete "deleted" disks */ + ret = efi_disk_do_delete(handles, n); + + /* undo marking */ + efi_disk_clear_deleted_matched(NULL, handles, n); + + free(handles); + } +#endif + return EFI_SUCCESS; +}
On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
@Marek Why should a 'usb start' command be needed to make a plugged in device available?
Best regards
Heirnich
cmd/bootefi.c | 17 +++- include/efi_loader.h | 4 + lib/efi_loader/efi_disk.c | 191 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 210 insertions(+), 2 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3cefb4d0ecaa..82649e211fda 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -56,9 +56,22 @@ efi_status_t efi_init_obj_list(void) */ efi_save_gd();
- /* Initialize once only */
- if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
- if (efi_obj_list_initialized == EFI_SUCCESS) {
+#ifdef CONFIG_PARTITIONS
ret = efi_disk_update();if (ret != EFI_SUCCESS)printf("+++ updating disks list failed\n");/** It may sound odd, but most part of EFI should* yet work.*/+#endif return efi_obj_list_initialized;
} else if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) {
/* Initialize once only */return efi_obj_list_initialized;}
/* Initialize system table */ ret = efi_initialize_system_table();
diff --git a/include/efi_loader.h b/include/efi_loader.h index 5cc3bded03fa..3bae1844befb 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -260,6 +260,10 @@ efi_status_t efi_initialize_system_table(void); 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); +/* Check validity of efi disk */ +bool efi_disk_is_valid(efi_handle_t handle); +/* Called by bootefi to find and update disk storage information */ +efi_status_t efi_disk_update(void); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index c037526ad2d0..0c4d79ee3fc9 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -14,10 +14,14 @@
const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
+#define _EFI_DISK_FLAG_DELETED 0x1 /* to be removed */ +#define _EFI_DISK_FLAG_INVALID 0x80000000 /* in stale state */
/**
- struct efi_disk_obj - EFI disk object
- @header: EFI object header
- @flags: control flags
- @ops: EFI disk I/O protocol interface
- @ifname: interface name for block device
- @dev_index: device index of block device
@@ -30,6 +34,7 @@ const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID; */ struct efi_disk_obj { struct efi_object header;
- int flags; struct efi_block_io ops; const char *ifname; int dev_index;
@@ -41,6 +46,35 @@ struct efi_disk_obj { struct blk_desc *desc; };
+static void efi_disk_mark_deleted(struct efi_disk_obj *disk) +{
- disk->flags |= _EFI_DISK_FLAG_DELETED;
+}
+static void efi_disk_clear_deleted(struct efi_disk_obj *disk) +{
- disk->flags &= ~_EFI_DISK_FLAG_DELETED;
+}
+static bool efi_disk_deleted_marked(struct efi_disk_obj *disk) +{
- return disk->flags & _EFI_DISK_FLAG_DELETED;
+}
+static void efi_disk_mark_invalid(struct efi_disk_obj *disk) +{
- disk->flags |= _EFI_DISK_FLAG_INVALID;
+}
+bool efi_disk_is_valid(efi_handle_t handle) +{
- struct efi_disk_obj *disk;
- disk = container_of(handle, struct efi_disk_obj, header);
- return !(disk->flags & _EFI_DISK_FLAG_INVALID);
+}
static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this, char extended_verification) { @@ -64,6 +98,9 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, unsigned long n;
diskobj = container_of(this, struct efi_disk_obj, ops);
- if (diskobj->flags & _EFI_DISK_FLAG_INVALID)
return EFI_DEVICE_ERROR;- desc = (struct blk_desc *) diskobj->desc; blksz = desc->blksz; blocks = buffer_size / blksz;
@@ -440,3 +477,157 @@ efi_status_t efi_disk_register(void)
return EFI_SUCCESS; }
+/*
- Mark all the block devices as "deleted," and return an array of
- handles for later use. It should be freed in a caller.
- */
+static efi_status_t efi_disk_mark_deleted_all(efi_handle_t **handlesp) +{
- efi_handle_t *handles = NULL;
- efi_uintn_t size = 0;
- int num, i;
- struct efi_disk_obj *disk;
- efi_status_t ret;
- ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL,
&size, handles);- if (ret == EFI_BUFFER_TOO_SMALL) {
handles = calloc(1, size);if (!handles)return EFI_OUT_OF_RESOURCES;ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL,&size, handles);- }
- if (ret != EFI_SUCCESS) {
free(handles);return ret;- }
- num = size / sizeof(*handles);
- for (i = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);efi_disk_mark_deleted(disk);- }
- *handlesp = handles;
- return num;
+}
+/*
- Clear "deleted" flag for a block device which is identified with desc.
- If desc is NULL, clear all devices.
- Return a number of disks cleared.
- */
+static int efi_disk_clear_deleted_matched(struct blk_desc *desc,
efi_handle_t *handles, int num)+{
- struct efi_disk_obj *disk;
- int disks, i;
- for (i = 0, disks = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);if (!desc || disk->desc == desc) {efi_disk_clear_deleted(disk);disks++;}- }
- return disks;
+}
+/*
- Do delete all the block devices marked as "deleted"
- */
+static efi_status_t efi_disk_do_delete(efi_handle_t *handles, int num) +{
- struct efi_disk_obj *disk;
- int i;
- for (i = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);if (!efi_disk_deleted_marked(disk))continue;efi_disk_mark_invalid(disk);/** TODO:* efi_delete_handle(handles[i]);*/- }
- return EFI_SUCCESS;
+}
+/*
- efi_disk_update - recreate efi disk mappings after initialization
- @return efi error code
- */
+efi_status_t efi_disk_update(void) +{ +#ifdef CONFIG_BLK
- efi_handle_t *handles = NULL;
- struct udevice *dev;
- struct blk_desc *desc;
- const char *if_typename;
- struct efi_disk_obj *disk;
- int n, disks = 0;
- efi_status_t ret;
- /* temporarily mark all the devices "deleted" */
- ret = efi_disk_mark_deleted_all(&handles);
- if (ret & EFI_ERROR_MASK) {
printf("ERROR: Failed to rescan block devices.\n");return ret;- }
- n = (int)ret;
- for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
uclass_next_device_check(&dev)) {desc = dev_get_uclass_platdata(dev);if (n && efi_disk_clear_deleted_matched(desc, handles, n))/* existing device */continue;/* new device */if_typename = blk_get_if_type_name(desc->if_type);/* Add block device for the full device */printf("Scanning disk %s...\n", dev->name);ret = efi_disk_add_dev(NULL, NULL, if_typename,desc, desc->devnum, 0, 0, &disk);if (ret == EFI_NOT_READY) {printf("Disk %s not ready\n", dev->name);continue;}if (ret != EFI_SUCCESS) {printf("ERROR: failure to add disk device %s, r = %lu\n",dev->name, ret & ~EFI_ERROR_MASK);continue;}disks++;/* Partitions show up as block devices in EFI */disks += efi_disk_create_partitions(&disk->header, desc,if_typename,desc->devnum, dev->name);- }
- if (n) {
/* do delete "deleted" disks */ret = efi_disk_do_delete(handles, n);/* undo marking */efi_disk_clear_deleted_matched(NULL, handles, n);free(handles);- }
+#endif
- return EFI_SUCCESS;
+}
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Furthermore, your comment here doesn't match your previous comment[1].
[1] https://lists.denx.de/pipermail/u-boot/2018-November/346860.html
-Takahiro Akashi
@Marek Why should a 'usb start' command be needed to make a plugged in device available?
Best regards
Heirnich
cmd/bootefi.c | 17 +++- include/efi_loader.h | 4 + lib/efi_loader/efi_disk.c | 191 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 210 insertions(+), 2 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3cefb4d0ecaa..82649e211fda 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -56,9 +56,22 @@ efi_status_t efi_init_obj_list(void) */ efi_save_gd();
- /* Initialize once only */
- if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
- if (efi_obj_list_initialized == EFI_SUCCESS) {
+#ifdef CONFIG_PARTITIONS
ret = efi_disk_update();if (ret != EFI_SUCCESS)printf("+++ updating disks list failed\n");/** It may sound odd, but most part of EFI should* yet work.*/+#endif return efi_obj_list_initialized;
} else if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) {
/* Initialize once only */return efi_obj_list_initialized;}
/* Initialize system table */ ret = efi_initialize_system_table();
diff --git a/include/efi_loader.h b/include/efi_loader.h index 5cc3bded03fa..3bae1844befb 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -260,6 +260,10 @@ efi_status_t efi_initialize_system_table(void); 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); +/* Check validity of efi disk */ +bool efi_disk_is_valid(efi_handle_t handle); +/* Called by bootefi to find and update disk storage information */ +efi_status_t efi_disk_update(void); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index c037526ad2d0..0c4d79ee3fc9 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -14,10 +14,14 @@
const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
+#define _EFI_DISK_FLAG_DELETED 0x1 /* to be removed */ +#define _EFI_DISK_FLAG_INVALID 0x80000000 /* in stale state */
/**
- struct efi_disk_obj - EFI disk object
- @header: EFI object header
- @flags: control flags
- @ops: EFI disk I/O protocol interface
- @ifname: interface name for block device
- @dev_index: device index of block device
@@ -30,6 +34,7 @@ const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID; */ struct efi_disk_obj { struct efi_object header;
- int flags; struct efi_block_io ops; const char *ifname; int dev_index;
@@ -41,6 +46,35 @@ struct efi_disk_obj { struct blk_desc *desc; };
+static void efi_disk_mark_deleted(struct efi_disk_obj *disk) +{
- disk->flags |= _EFI_DISK_FLAG_DELETED;
+}
+static void efi_disk_clear_deleted(struct efi_disk_obj *disk) +{
- disk->flags &= ~_EFI_DISK_FLAG_DELETED;
+}
+static bool efi_disk_deleted_marked(struct efi_disk_obj *disk) +{
- return disk->flags & _EFI_DISK_FLAG_DELETED;
+}
+static void efi_disk_mark_invalid(struct efi_disk_obj *disk) +{
- disk->flags |= _EFI_DISK_FLAG_INVALID;
+}
+bool efi_disk_is_valid(efi_handle_t handle) +{
- struct efi_disk_obj *disk;
- disk = container_of(handle, struct efi_disk_obj, header);
- return !(disk->flags & _EFI_DISK_FLAG_INVALID);
+}
static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this, char extended_verification) { @@ -64,6 +98,9 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, unsigned long n;
diskobj = container_of(this, struct efi_disk_obj, ops);
- if (diskobj->flags & _EFI_DISK_FLAG_INVALID)
return EFI_DEVICE_ERROR;- desc = (struct blk_desc *) diskobj->desc; blksz = desc->blksz; blocks = buffer_size / blksz;
@@ -440,3 +477,157 @@ efi_status_t efi_disk_register(void)
return EFI_SUCCESS; }
+/*
- Mark all the block devices as "deleted," and return an array of
- handles for later use. It should be freed in a caller.
- */
+static efi_status_t efi_disk_mark_deleted_all(efi_handle_t **handlesp) +{
- efi_handle_t *handles = NULL;
- efi_uintn_t size = 0;
- int num, i;
- struct efi_disk_obj *disk;
- efi_status_t ret;
- ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL,
&size, handles);- if (ret == EFI_BUFFER_TOO_SMALL) {
handles = calloc(1, size);if (!handles)return EFI_OUT_OF_RESOURCES;ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL,&size, handles);- }
- if (ret != EFI_SUCCESS) {
free(handles);return ret;- }
- num = size / sizeof(*handles);
- for (i = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);efi_disk_mark_deleted(disk);- }
- *handlesp = handles;
- return num;
+}
+/*
- Clear "deleted" flag for a block device which is identified with desc.
- If desc is NULL, clear all devices.
- Return a number of disks cleared.
- */
+static int efi_disk_clear_deleted_matched(struct blk_desc *desc,
efi_handle_t *handles, int num)+{
- struct efi_disk_obj *disk;
- int disks, i;
- for (i = 0, disks = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);if (!desc || disk->desc == desc) {efi_disk_clear_deleted(disk);disks++;}- }
- return disks;
+}
+/*
- Do delete all the block devices marked as "deleted"
- */
+static efi_status_t efi_disk_do_delete(efi_handle_t *handles, int num) +{
- struct efi_disk_obj *disk;
- int i;
- for (i = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);if (!efi_disk_deleted_marked(disk))continue;efi_disk_mark_invalid(disk);/** TODO:* efi_delete_handle(handles[i]);*/- }
- return EFI_SUCCESS;
+}
+/*
- efi_disk_update - recreate efi disk mappings after initialization
- @return efi error code
- */
+efi_status_t efi_disk_update(void) +{ +#ifdef CONFIG_BLK
- efi_handle_t *handles = NULL;
- struct udevice *dev;
- struct blk_desc *desc;
- const char *if_typename;
- struct efi_disk_obj *disk;
- int n, disks = 0;
- efi_status_t ret;
- /* temporarily mark all the devices "deleted" */
- ret = efi_disk_mark_deleted_all(&handles);
- if (ret & EFI_ERROR_MASK) {
printf("ERROR: Failed to rescan block devices.\n");return ret;- }
- n = (int)ret;
- for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
uclass_next_device_check(&dev)) {desc = dev_get_uclass_platdata(dev);if (n && efi_disk_clear_deleted_matched(desc, handles, n))/* existing device */continue;/* new device */if_typename = blk_get_if_type_name(desc->if_type);/* Add block device for the full device */printf("Scanning disk %s...\n", dev->name);ret = efi_disk_add_dev(NULL, NULL, if_typename,desc, desc->devnum, 0, 0, &disk);if (ret == EFI_NOT_READY) {printf("Disk %s not ready\n", dev->name);continue;}if (ret != EFI_SUCCESS) {printf("ERROR: failure to add disk device %s, r = %lu\n",dev->name, ret & ~EFI_ERROR_MASK);continue;}disks++;/* Partitions show up as block devices in EFI */disks += efi_disk_create_partitions(&disk->header, desc,if_typename,desc->devnum, dev->name);- }
- if (n) {
/* do delete "deleted" disks */ret = efi_disk_do_delete(handles, n);/* undo marking */efi_disk_clear_deleted_matched(NULL, handles, n);free(handles);- }
+#endif
- return EFI_SUCCESS;
+}
Heinrich,
Do you have any further comments here? Otherwise, I'd like to send out v3 shortly.
-Takahiro Akashi
On Thu, Dec 13, 2018 at 04:58:29PM +0900, AKASHI Takahiro wrote:
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Furthermore, your comment here doesn't match your previous comment[1].
[1] https://lists.denx.de/pipermail/u-boot/2018-November/346860.html
-Takahiro Akashi
@Marek Why should a 'usb start' command be needed to make a plugged in device available?
Best regards
Heirnich
cmd/bootefi.c | 17 +++- include/efi_loader.h | 4 + lib/efi_loader/efi_disk.c | 191 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 210 insertions(+), 2 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3cefb4d0ecaa..82649e211fda 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -56,9 +56,22 @@ efi_status_t efi_init_obj_list(void) */ efi_save_gd();
- /* Initialize once only */
- if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
- if (efi_obj_list_initialized == EFI_SUCCESS) {
+#ifdef CONFIG_PARTITIONS
ret = efi_disk_update();if (ret != EFI_SUCCESS)printf("+++ updating disks list failed\n");/** It may sound odd, but most part of EFI should* yet work.*/+#endif return efi_obj_list_initialized;
} else if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) {
/* Initialize once only */return efi_obj_list_initialized;}
/* Initialize system table */ ret = efi_initialize_system_table();
diff --git a/include/efi_loader.h b/include/efi_loader.h index 5cc3bded03fa..3bae1844befb 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -260,6 +260,10 @@ efi_status_t efi_initialize_system_table(void); 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); +/* Check validity of efi disk */ +bool efi_disk_is_valid(efi_handle_t handle); +/* Called by bootefi to find and update disk storage information */ +efi_status_t efi_disk_update(void); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index c037526ad2d0..0c4d79ee3fc9 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -14,10 +14,14 @@
const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
+#define _EFI_DISK_FLAG_DELETED 0x1 /* to be removed */ +#define _EFI_DISK_FLAG_INVALID 0x80000000 /* in stale state */
/**
- struct efi_disk_obj - EFI disk object
- @header: EFI object header
- @flags: control flags
- @ops: EFI disk I/O protocol interface
- @ifname: interface name for block device
- @dev_index: device index of block device
@@ -30,6 +34,7 @@ const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID; */ struct efi_disk_obj { struct efi_object header;
- int flags; struct efi_block_io ops; const char *ifname; int dev_index;
@@ -41,6 +46,35 @@ struct efi_disk_obj { struct blk_desc *desc; };
+static void efi_disk_mark_deleted(struct efi_disk_obj *disk) +{
- disk->flags |= _EFI_DISK_FLAG_DELETED;
+}
+static void efi_disk_clear_deleted(struct efi_disk_obj *disk) +{
- disk->flags &= ~_EFI_DISK_FLAG_DELETED;
+}
+static bool efi_disk_deleted_marked(struct efi_disk_obj *disk) +{
- return disk->flags & _EFI_DISK_FLAG_DELETED;
+}
+static void efi_disk_mark_invalid(struct efi_disk_obj *disk) +{
- disk->flags |= _EFI_DISK_FLAG_INVALID;
+}
+bool efi_disk_is_valid(efi_handle_t handle) +{
- struct efi_disk_obj *disk;
- disk = container_of(handle, struct efi_disk_obj, header);
- return !(disk->flags & _EFI_DISK_FLAG_INVALID);
+}
static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this, char extended_verification) { @@ -64,6 +98,9 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, unsigned long n;
diskobj = container_of(this, struct efi_disk_obj, ops);
- if (diskobj->flags & _EFI_DISK_FLAG_INVALID)
return EFI_DEVICE_ERROR;- desc = (struct blk_desc *) diskobj->desc; blksz = desc->blksz; blocks = buffer_size / blksz;
@@ -440,3 +477,157 @@ efi_status_t efi_disk_register(void)
return EFI_SUCCESS; }
+/*
- Mark all the block devices as "deleted," and return an array of
- handles for later use. It should be freed in a caller.
- */
+static efi_status_t efi_disk_mark_deleted_all(efi_handle_t **handlesp) +{
- efi_handle_t *handles = NULL;
- efi_uintn_t size = 0;
- int num, i;
- struct efi_disk_obj *disk;
- efi_status_t ret;
- ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL,
&size, handles);- if (ret == EFI_BUFFER_TOO_SMALL) {
handles = calloc(1, size);if (!handles)return EFI_OUT_OF_RESOURCES;ret = efi_locate_handle(BY_PROTOCOL, &efi_block_io_guid, NULL,&size, handles);- }
- if (ret != EFI_SUCCESS) {
free(handles);return ret;- }
- num = size / sizeof(*handles);
- for (i = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);efi_disk_mark_deleted(disk);- }
- *handlesp = handles;
- return num;
+}
+/*
- Clear "deleted" flag for a block device which is identified with desc.
- If desc is NULL, clear all devices.
- Return a number of disks cleared.
- */
+static int efi_disk_clear_deleted_matched(struct blk_desc *desc,
efi_handle_t *handles, int num)+{
- struct efi_disk_obj *disk;
- int disks, i;
- for (i = 0, disks = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);if (!desc || disk->desc == desc) {efi_disk_clear_deleted(disk);disks++;}- }
- return disks;
+}
+/*
- Do delete all the block devices marked as "deleted"
- */
+static efi_status_t efi_disk_do_delete(efi_handle_t *handles, int num) +{
- struct efi_disk_obj *disk;
- int i;
- for (i = 0; i < num; i++) {
disk = container_of(handles[i], struct efi_disk_obj, header);if (!efi_disk_deleted_marked(disk))continue;efi_disk_mark_invalid(disk);/** TODO:* efi_delete_handle(handles[i]);*/- }
- return EFI_SUCCESS;
+}
+/*
- efi_disk_update - recreate efi disk mappings after initialization
- @return efi error code
- */
+efi_status_t efi_disk_update(void) +{ +#ifdef CONFIG_BLK
- efi_handle_t *handles = NULL;
- struct udevice *dev;
- struct blk_desc *desc;
- const char *if_typename;
- struct efi_disk_obj *disk;
- int n, disks = 0;
- efi_status_t ret;
- /* temporarily mark all the devices "deleted" */
- ret = efi_disk_mark_deleted_all(&handles);
- if (ret & EFI_ERROR_MASK) {
printf("ERROR: Failed to rescan block devices.\n");return ret;- }
- n = (int)ret;
- for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
uclass_next_device_check(&dev)) {desc = dev_get_uclass_platdata(dev);if (n && efi_disk_clear_deleted_matched(desc, handles, n))/* existing device */continue;/* new device */if_typename = blk_get_if_type_name(desc->if_type);/* Add block device for the full device */printf("Scanning disk %s...\n", dev->name);ret = efi_disk_add_dev(NULL, NULL, if_typename,desc, desc->devnum, 0, 0, &disk);if (ret == EFI_NOT_READY) {printf("Disk %s not ready\n", dev->name);continue;}if (ret != EFI_SUCCESS) {printf("ERROR: failure to add disk device %s, r = %lu\n",dev->name, ret & ~EFI_ERROR_MASK);continue;}disks++;/* Partitions show up as block devices in EFI */disks += efi_disk_create_partitions(&disk->header, desc,if_typename,desc->devnum, dev->name);- }
- if (n) {
/* do delete "deleted" disks */ret = efi_disk_do_delete(handles, n);/* undo marking */efi_disk_clear_deleted_matched(NULL, handles, n);free(handles);- }
+#endif
- return EFI_SUCCESS;
+}
On 13.12.18 08:58, AKASHI Takahiro wrote:
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Could we just make this event based for now? Call a hook from the storage dm subsystem when a new u-boot block device gets created to issue a sync of that in the efi subsystem?
That hook would obviously only do something (or get registered?) when the efi object stack is initialized.
The long term goal IMHO should still be though to just merge DM and EFI objects. But we're still waiting on the deprecation of non-DM devices for that.
Alex
Alex,
On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote:
On 13.12.18 08:58, AKASHI Takahiro wrote:
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Could we just make this event based for now? Call a hook from the storage dm subsystem when a new u-boot block device gets created to issue a sync of that in the efi subsystem?
If I correctly understand you, your suggestion here corresponds with my proposal#3 in [1] while my current approach is #2.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
So we will call, say, efi_disk_create(struct udevice *) in blk_create_device() and efi_dsik_delete() in blk_unbind_all(). UEFI handles for disks, however, will *not* be deleted actually because, as Heinrich suggested in the past, no *reference count* for a handle is maintained in the current implementation. Right?
That hook would obviously only do something (or get registered?) when the efi object stack is initialized.
The long term goal IMHO should still be though to just merge DM and EFI objects. But we're still waiting on the deprecation of non-DM devices for that.
Maybe my #4?
-Takahiro Akashi
Alex
On 10.01.19 03:13, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote:
On 13.12.18 08:58, AKASHI Takahiro wrote:
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Could we just make this event based for now? Call a hook from the storage dm subsystem when a new u-boot block device gets created to issue a sync of that in the efi subsystem?
If I correctly understand you, your suggestion here corresponds with my proposal#3 in [1] while my current approach is #2.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Yes, I think so.
So we will call, say, efi_disk_create(struct udevice *) in blk_create_device() and efi_dsik_delete() in blk_unbind_all().
I would prefer if we didn't call them directly, but through an event mechanism. So the efi_disk subsystem registers an event with the dm block subsystem and that will just call all events when block devices get created which will automatically also include the efi disk creation callback. Same for reverse.
UEFI handles for disks, however, will *not* be deleted actually because, as Heinrich suggested in the past, no *reference count* for a handle is maintained in the current implementation. Right?
Sure, but we can have an internal state that just starts failing all callbacks (read, write, etc) when the backing device was destroyed.
That hook would obviously only do something (or get registered?) when the efi object stack is initialized.
The long term goal IMHO should still be though to just merge DM and EFI objects. But we're still waiting on the deprecation of non-DM devices for that.
Maybe my #4?
Not quite.
We would basically get rid of the efi object list altogether. Instead, any DM object would also be an EFI object and expose EFI interfaces if awareness exists.
That way we can merge into the existing object maintenance logic and wouldn't need any callbacks.
Once we're there, we can also start to think about reference counting :).
Alex
Alex,
On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote:
On 10.01.19 03:13, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote:
On 13.12.18 08:58, AKASHI Takahiro wrote:
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Could we just make this event based for now? Call a hook from the storage dm subsystem when a new u-boot block device gets created to issue a sync of that in the efi subsystem?
If I correctly understand you, your suggestion here corresponds with my proposal#3 in [1] while my current approach is #2.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Yes, I think so.
So we will call, say, efi_disk_create(struct udevice *) in blk_create_device() and efi_dsik_delete() in blk_unbind_all().
I would prefer if we didn't call them directly, but through an event mechanism. So the efi_disk subsystem registers an event with the dm block subsystem and that will just call all events when block devices get created which will automatically also include the efi disk creation callback. Same for reverse.
Do you mean efi event by "event?" (I don't think there is any generic event interface on DM side.)
Whatever an "event" is or whether we call efi_disk_create() directly or indirectly via an event, there is one (big?) issue in this approach (while I've almost finished prototyping):
We cannot call efi_disk_create() within blk_create_device() because some data fields of struct blk_desc, which are to be used by efi disk, are initialized *after* blk_create_device() in driver side.
So we need to add a hook at/after every occurrence of blk_create_device() on driver side. For example,
=== drivers/scsi/scsi.c === int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) { ... ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, bd.blksz, bd.lba, &bdev); ... bdesc = dev_get_uclass_platdata(bdev); bdesc->target = id; bdesc->lun = lun; ...
/* * We need have efi_disk_create() called here because bdesc->target * and lun will be used by dp helpers in efi_disk_add_dev(). */ efi_disk_create(bdev); }
int scsi_scan_dev(struct udevice *dev, bool verbose) { for (i = 0; i < uc_plat->max_id; i++) for (lun = 0; lun < uc_plat->max_lun; lun++) do_scsi_scan_one(dev, i, lun, verbose); ... }
int scsi_scan(bool verbose) { ret = uclass_get(UCLASS_SCSI, &uc); ... uclass_foreach_dev(dev, uc) ret = scsi_scan_dev(dev, verbose); ... } === ===
Since scsn_scan() can be directly called by "scsi rescan" command, There seems to be no generic hook, or event, available in order to call efi_disk_create().
Do I miss anything?
UEFI handles for disks, however, will *not* be deleted actually because, as Heinrich suggested in the past, no *reference count* for a handle is maintained in the current implementation. Right?
Sure, but we can have an internal state that just starts failing all callbacks (read, write, etc) when the backing device was destroyed.
Yes, I've already added "flags" to efi_disk_obj.
Thanks, -Takahiro Akashi
That hook would obviously only do something (or get registered?) when the efi object stack is initialized.
The long term goal IMHO should still be though to just merge DM and EFI objects. But we're still waiting on the deprecation of non-DM devices for that.
Maybe my #4?
Not quite.
We would basically get rid of the efi object list altogether. Instead, any DM object would also be an EFI object and expose EFI interfaces if awareness exists.
That way we can merge into the existing object maintenance logic and wouldn't need any callbacks.
Once we're there, we can also start to think about reference counting :).
Alex
On 10.01.19 08:26, AKASHI Takahiro wrote:
Alex,
On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote:
On 10.01.19 03:13, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote:
On 13.12.18 08:58, AKASHI Takahiro wrote:
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
On 11/15/18 5:58 AM, AKASHI Takahiro wrote: > Currently, efi_init_obj_list() scan disk devices only once, and never > change a list of efi disk devices. This will possibly result in failing > to find a removable storage which may be added later on. See [1]. > > In this patch, called is efi_disk_update() which is responsible for > re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. > > For example, > > => efishell devices > Scanning disk pci_mmc.blk... > Found 3 disks > Device Name > ============================================ > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) > => usb start > starting USB... > USB0: USB EHCI 1.00 > scanning bus 0 for devices... 3 USB Device(s) found > scanning usb for storage devices... 1 Storage Device(s) found > => efishell devices > Scanning disk usb_mass_storage.lun0... > Device Name > ============================================ > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) > > Without this patch, the last device, USB mass storage, won't show up. > > [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html > > Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Could we just make this event based for now? Call a hook from the storage dm subsystem when a new u-boot block device gets created to issue a sync of that in the efi subsystem?
If I correctly understand you, your suggestion here corresponds with my proposal#3 in [1] while my current approach is #2.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Yes, I think so.
So we will call, say, efi_disk_create(struct udevice *) in blk_create_device() and efi_dsik_delete() in blk_unbind_all().
I would prefer if we didn't call them directly, but through an event mechanism. So the efi_disk subsystem registers an event with the dm block subsystem and that will just call all events when block devices get created which will automatically also include the efi disk creation callback. Same for reverse.
Do you mean efi event by "event?" (I don't think there is any generic event interface on DM side.)
Whatever an "event" is or whether we call efi_disk_create() directly or indirectly via an event, there is one (big?) issue in this approach (while I've almost finished prototyping):
We cannot call efi_disk_create() within blk_create_device() because some data fields of struct blk_desc, which are to be used by efi disk, are initialized *after* blk_create_device() in driver side.
So we need to add a hook at/after every occurrence of blk_create_device() on driver side. For example,
=== drivers/scsi/scsi.c === int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) { ... ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, bd.blksz, bd.lba, &bdev); ... bdesc = dev_get_uclass_platdata(bdev); bdesc->target = id; bdesc->lun = lun; ...
/* * We need have efi_disk_create() called here because bdesc->target * and lun will be used by dp helpers in efi_disk_add_dev(). */ efi_disk_create(bdev); }
int scsi_scan_dev(struct udevice *dev, bool verbose) { for (i = 0; i < uc_plat->max_id; i++) for (lun = 0; lun < uc_plat->max_lun; lun++) do_scsi_scan_one(dev, i, lun, verbose); ... }
int scsi_scan(bool verbose) { ret = uclass_get(UCLASS_SCSI, &uc); ... uclass_foreach_dev(dev, uc) ret = scsi_scan_dev(dev, verbose); ... } === ===
Since scsn_scan() can be directly called by "scsi rescan" command, There seems to be no generic hook, or event, available in order to call efi_disk_create().
Do I miss anything?
Could the event handler that gets called from somewhere around blk_create_device() just put it into an efi internal "todo list" which we then process using an efi event?
EFI events will only get triggered on the next entry to efi land, so by then we should be safe.
Alex
On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote:
On 10.01.19 08:26, AKASHI Takahiro wrote:
Alex,
On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote:
On 10.01.19 03:13, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote:
On 13.12.18 08:58, AKASHI Takahiro wrote:
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: > On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >> Currently, efi_init_obj_list() scan disk devices only once, and never >> change a list of efi disk devices. This will possibly result in failing >> to find a removable storage which may be added later on. See [1]. >> >> In this patch, called is efi_disk_update() which is responsible for >> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >> >> For example, >> >> => efishell devices >> Scanning disk pci_mmc.blk... >> Found 3 disks >> Device Name >> ============================================ >> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >> => usb start >> starting USB... >> USB0: USB EHCI 1.00 >> scanning bus 0 for devices... 3 USB Device(s) found >> scanning usb for storage devices... 1 Storage Device(s) found >> => efishell devices >> Scanning disk usb_mass_storage.lun0... >> Device Name >> ============================================ >> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >> >> Without this patch, the last device, USB mass storage, won't show up. >> >> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >> >> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org > > Why should we try to fix something in the EFI subsystems that goes wrong > in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Could we just make this event based for now? Call a hook from the storage dm subsystem when a new u-boot block device gets created to issue a sync of that in the efi subsystem?
If I correctly understand you, your suggestion here corresponds with my proposal#3 in [1] while my current approach is #2.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Yes, I think so.
So we will call, say, efi_disk_create(struct udevice *) in blk_create_device() and efi_dsik_delete() in blk_unbind_all().
I would prefer if we didn't call them directly, but through an event mechanism. So the efi_disk subsystem registers an event with the dm block subsystem and that will just call all events when block devices get created which will automatically also include the efi disk creation callback. Same for reverse.
Do you mean efi event by "event?" (I don't think there is any generic event interface on DM side.)
Whatever an "event" is or whether we call efi_disk_create() directly or indirectly via an event, there is one (big?) issue in this approach (while I've almost finished prototyping):
We cannot call efi_disk_create() within blk_create_device() because some data fields of struct blk_desc, which are to be used by efi disk, are initialized *after* blk_create_device() in driver side.
So we need to add a hook at/after every occurrence of blk_create_device() on driver side. For example,
=== drivers/scsi/scsi.c === int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) { ... ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, bd.blksz, bd.lba, &bdev); ... bdesc = dev_get_uclass_platdata(bdev); bdesc->target = id; bdesc->lun = lun; ...
/* * We need have efi_disk_create() called here because bdesc->target * and lun will be used by dp helpers in efi_disk_add_dev(). */ efi_disk_create(bdev); }
int scsi_scan_dev(struct udevice *dev, bool verbose) { for (i = 0; i < uc_plat->max_id; i++) for (lun = 0; lun < uc_plat->max_lun; lun++) do_scsi_scan_one(dev, i, lun, verbose); ... }
int scsi_scan(bool verbose) { ret = uclass_get(UCLASS_SCSI, &uc); ... uclass_foreach_dev(dev, uc) ret = scsi_scan_dev(dev, verbose); ... } === ===
Since scsn_scan() can be directly called by "scsi rescan" command, There seems to be no generic hook, or event, available in order to call efi_disk_create().
Do I miss anything?
Could the event handler that gets called from somewhere around blk_create_device() just put it into an efi internal "todo list" which we then process using an efi event?
EFI events will only get triggered on the next entry to efi land, so by then we should be safe.
I think I now understand your suggestion; we are going to invent a specialized event-queuing mechanism so that we can take any actions later at appropriate time (probably in efi_init_obj_list()?).
But if so, it's not much different from my current approach where a list of efi disks are updated in efi_init_obj_list() :)
-Takahiro Akashi
Alex
Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote:
On 10.01.19 08:26, AKASHI Takahiro wrote: Alex,
On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote:
On 10.01.19 03:13, AKASHI Takahiro wrote: Alex,
On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote:
> On 13.12.18 08:58, AKASHI Takahiro wrote: > Heinrich, > >> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>> Currently, efi_init_obj_list() scan disk devices only once, and never >>> change a list of efi disk devices. This will possibly result in failing >>> to find a removable storage which may be added later on. See [1]. >>> >>> In this patch, called is efi_disk_update() which is responsible for >>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>> >>> For example, >>> >>> => efishell devices >>> Scanning disk pci_mmc.blk... >>> Found 3 disks >>> Device Name >>> ============================================ >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>> => usb start >>> starting USB... >>> USB0: USB EHCI 1.00 >>> scanning bus 0 for devices... 3 USB Device(s) found >>> scanning usb for storage devices... 1 Storage Device(s) found >>> => efishell devices >>> Scanning disk usb_mass_storage.lun0... >>> Device Name >>> ============================================ >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>> >>> Without this patch, the last device, USB mass storage, won't show up. >>> >>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>> >>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >> >> Why should we try to fix something in the EFI subsystems that goes wrong >> in the handling of device enumeration. > > No. > This is a natural result from how efi disks are currently implemented on u-boot. > Do you want to totally re-write/re-implement efi disks?
Could we just make this event based for now? Call a hook from the storage dm subsystem when a new u-boot block device gets created to issue a sync of that in the efi subsystem?
If I correctly understand you, your suggestion here corresponds with my proposal#3 in [1] while my current approach is #2.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Yes, I think so.
So we will call, say, efi_disk_create(struct udevice *) in blk_create_device() and efi_dsik_delete() in blk_unbind_all().
I would prefer if we didn't call them directly, but through an event mechanism. So the efi_disk subsystem registers an event with the dm block subsystem and that will just call all events when block devices get created which will automatically also include the efi disk creation callback. Same for reverse.
Do you mean efi event by "event?" (I don't think there is any generic event interface on DM side.)
Whatever an "event" is or whether we call efi_disk_create() directly or indirectly via an event, there is one (big?) issue in this approach (while I've almost finished prototyping):
We cannot call efi_disk_create() within blk_create_device() because some data fields of struct blk_desc, which are to be used by efi disk, are initialized *after* blk_create_device() in driver side.
So we need to add a hook at/after every occurrence of blk_create_device() on driver side. For example,
=== drivers/scsi/scsi.c === int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) { ... ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, bd.blksz, bd.lba, &bdev); ... bdesc = dev_get_uclass_platdata(bdev); bdesc->target = id; bdesc->lun = lun; ...
/* * We need have efi_disk_create() called here because bdesc->target * and lun will be used by dp helpers in efi_disk_add_dev(). */ efi_disk_create(bdev); }
int scsi_scan_dev(struct udevice *dev, bool verbose) { for (i = 0; i < uc_plat->max_id; i++) for (lun = 0; lun < uc_plat->max_lun; lun++) do_scsi_scan_one(dev, i, lun, verbose); ... }
int scsi_scan(bool verbose) { ret = uclass_get(UCLASS_SCSI, &uc); ... uclass_foreach_dev(dev, uc) ret = scsi_scan_dev(dev, verbose); ... } === ===
Since scsn_scan() can be directly called by "scsi rescan" command, There seems to be no generic hook, or event, available in order to call efi_disk_create().
Do I miss anything?
Could the event handler that gets called from somewhere around blk_create_device() just put it into an efi internal "todo list" which we then process using an efi event?
EFI events will only get triggered on the next entry to efi land, so by then we should be safe.
I think I now understand your suggestion; we are going to invent a specialized event-queuing mechanism so that we can take any actions later at appropriate time (probably in efi_init_obj_list()?).
Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer. That event handler creates a new efi event (like a timer w/ timeout=0). This new event's handler can then create the actual efi block device.
But if so, it's not much different from my current approach where a list of efi disks are updated in efi_init_obj_list() :)
The main difference is that disk logic stays in the disc code scope :).
Alex
-Takahiro Akashi
Alex
On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote:
On 10.01.19 08:26, AKASHI Takahiro wrote: Alex,
On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote:
On 10.01.19 03:13, AKASHI Takahiro wrote: Alex,
> On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote: > > >> On 13.12.18 08:58, AKASHI Takahiro wrote: >> Heinrich, >> >>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>>> Currently, efi_init_obj_list() scan disk devices only once, and never >>>> change a list of efi disk devices. This will possibly result in failing >>>> to find a removable storage which may be added later on. See [1]. >>>> >>>> In this patch, called is efi_disk_update() which is responsible for >>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>>> >>>> For example, >>>> >>>> => efishell devices >>>> Scanning disk pci_mmc.blk... >>>> Found 3 disks >>>> Device Name >>>> ============================================ >>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>> => usb start >>>> starting USB... >>>> USB0: USB EHCI 1.00 >>>> scanning bus 0 for devices... 3 USB Device(s) found >>>> scanning usb for storage devices... 1 Storage Device(s) found >>>> => efishell devices >>>> Scanning disk usb_mass_storage.lun0... >>>> Device Name >>>> ============================================ >>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>>> >>>> Without this patch, the last device, USB mass storage, won't show up. >>>> >>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>> >>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>> >>> Why should we try to fix something in the EFI subsystems that goes wrong >>> in the handling of device enumeration. >> >> No. >> This is a natural result from how efi disks are currently implemented on u-boot. >> Do you want to totally re-write/re-implement efi disks? > > Could we just make this event based for now? Call a hook from the > storage dm subsystem when a new u-boot block device gets created to > issue a sync of that in the efi subsystem?
If I correctly understand you, your suggestion here corresponds with my proposal#3 in [1] while my current approach is #2.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Yes, I think so.
So we will call, say, efi_disk_create(struct udevice *) in blk_create_device() and efi_dsik_delete() in blk_unbind_all().
I would prefer if we didn't call them directly, but through an event mechanism. So the efi_disk subsystem registers an event with the dm block subsystem and that will just call all events when block devices get created which will automatically also include the efi disk creation callback. Same for reverse.
Do you mean efi event by "event?" (I don't think there is any generic event interface on DM side.)
Whatever an "event" is or whether we call efi_disk_create() directly or indirectly via an event, there is one (big?) issue in this approach (while I've almost finished prototyping):
We cannot call efi_disk_create() within blk_create_device() because some data fields of struct blk_desc, which are to be used by efi disk, are initialized *after* blk_create_device() in driver side.
So we need to add a hook at/after every occurrence of blk_create_device() on driver side. For example,
=== drivers/scsi/scsi.c === int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) { ... ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, bd.blksz, bd.lba, &bdev); ... bdesc = dev_get_uclass_platdata(bdev); bdesc->target = id; bdesc->lun = lun; ...
/* * We need have efi_disk_create() called here because bdesc->target * and lun will be used by dp helpers in efi_disk_add_dev(). */ efi_disk_create(bdev); }
int scsi_scan_dev(struct udevice *dev, bool verbose) { for (i = 0; i < uc_plat->max_id; i++) for (lun = 0; lun < uc_plat->max_lun; lun++) do_scsi_scan_one(dev, i, lun, verbose); ... }
int scsi_scan(bool verbose) { ret = uclass_get(UCLASS_SCSI, &uc); ... uclass_foreach_dev(dev, uc) ret = scsi_scan_dev(dev, verbose); ... } === ===
Since scsn_scan() can be directly called by "scsi rescan" command, There seems to be no generic hook, or event, available in order to call efi_disk_create().
Do I miss anything?
Could the event handler that gets called from somewhere around blk_create_device() just put it into an efi internal "todo list" which we then process using an efi event?
EFI events will only get triggered on the next entry to efi land, so by then we should be safe.
I think I now understand your suggestion; we are going to invent a specialized event-queuing mechanism so that we can take any actions later at appropriate time (probably in efi_init_obj_list()?).
Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer.
This is a to-be-invented "specialized event-queuing mechanism" in my language :) as we cannot use efi_create/signal_event() before initializing EFI environment.
This event will be expected to be 'signal'ed at every creation/deletion of UCLASS_BLK device. Right?
That event handler creates a new efi event (like a timer w/ timeout=0).
But when is this event handler fired? I think the only possible timing is at efi_init_obj_list().
This new event's handler can then create the actual efi block device.
I assume that this event handler is fired immediately after efi_signal_event() with timeout=0.
If so, why do we need to create an efi event? To isolate the disk code from the other init code?
(If so, for the same reason, we should re-write efi_init_obj_list() with events for other efi resources as well.)
But if so, it's not much different from my current approach where a list of efi disks are updated in efi_init_obj_list() :)
The main difference is that disk logic stays in the disc code scope :).
My efi_disk_update() (and efi_disk_register()) is the only function visible outside the disk code, isn't it?
Using some kind of events here is smart, but looks to me a bit overdoing because we anyhow have to go through all the UCLASS_BLK devices to mark whether they are still valid or not :)
-Takahiro Akashi
Alex
-Takahiro Akashi
Alex
Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote:
On 10.01.19 08:26, AKASHI Takahiro wrote: Alex,
On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote:
> On 10.01.19 03:13, AKASHI Takahiro wrote: > Alex, > >> On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote: >> >> >>> On 13.12.18 08:58, AKASHI Takahiro wrote: >>> Heinrich, >>> >>>>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>>>> Currently, efi_init_obj_list() scan disk devices only once, and never >>>>> change a list of efi disk devices. This will possibly result in failing >>>>> to find a removable storage which may be added later on. See [1]. >>>>> >>>>> In this patch, called is efi_disk_update() which is responsible for >>>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>>>> >>>>> For example, >>>>> >>>>> => efishell devices >>>>> Scanning disk pci_mmc.blk... >>>>> Found 3 disks >>>>> Device Name >>>>> ============================================ >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>> => usb start >>>>> starting USB... >>>>> USB0: USB EHCI 1.00 >>>>> scanning bus 0 for devices... 3 USB Device(s) found >>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>> => efishell devices >>>>> Scanning disk usb_mass_storage.lun0... >>>>> Device Name >>>>> ============================================ >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>>>> >>>>> Without this patch, the last device, USB mass storage, won't show up. >>>>> >>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>> >>>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>> >>>> Why should we try to fix something in the EFI subsystems that goes wrong >>>> in the handling of device enumeration. >>> >>> No. >>> This is a natural result from how efi disks are currently implemented on u-boot. >>> Do you want to totally re-write/re-implement efi disks? >> >> Could we just make this event based for now? Call a hook from the >> storage dm subsystem when a new u-boot block device gets created to >> issue a sync of that in the efi subsystem? > > If I correctly understand you, your suggestion here corresponds > with my proposal#3 in [1] while my current approach is #2. > > [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Yes, I think so.
> So we will call, say, efi_disk_create(struct udevice *) in > blk_create_device() and efi_dsik_delete() in blk_unbind_all().
I would prefer if we didn't call them directly, but through an event mechanism. So the efi_disk subsystem registers an event with the dm block subsystem and that will just call all events when block devices get created which will automatically also include the efi disk creation callback. Same for reverse.
Do you mean efi event by "event?" (I don't think there is any generic event interface on DM side.)
Whatever an "event" is or whether we call efi_disk_create() directly or indirectly via an event, there is one (big?) issue in this approach (while I've almost finished prototyping):
We cannot call efi_disk_create() within blk_create_device() because some data fields of struct blk_desc, which are to be used by efi disk, are initialized *after* blk_create_device() in driver side.
So we need to add a hook at/after every occurrence of blk_create_device() on driver side. For example,
=== drivers/scsi/scsi.c === int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) { ... ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, bd.blksz, bd.lba, &bdev); ... bdesc = dev_get_uclass_platdata(bdev); bdesc->target = id; bdesc->lun = lun; ...
/*
- We need have efi_disk_create() called here because bdesc->target
- and lun will be used by dp helpers in efi_disk_add_dev().
*/ efi_disk_create(bdev); }
int scsi_scan_dev(struct udevice *dev, bool verbose) { for (i = 0; i < uc_plat->max_id; i++) for (lun = 0; lun < uc_plat->max_lun; lun++) do_scsi_scan_one(dev, i, lun, verbose); ... }
int scsi_scan(bool verbose) { ret = uclass_get(UCLASS_SCSI, &uc); ... uclass_foreach_dev(dev, uc) ret = scsi_scan_dev(dev, verbose); ... } === ===
Since scsn_scan() can be directly called by "scsi rescan" command, There seems to be no generic hook, or event, available in order to call efi_disk_create().
Do I miss anything?
Could the event handler that gets called from somewhere around blk_create_device() just put it into an efi internal "todo list" which we then process using an efi event?
EFI events will only get triggered on the next entry to efi land, so by then we should be safe.
I think I now understand your suggestion; we are going to invent a specialized event-queuing mechanism so that we can take any actions later at appropriate time (probably in efi_init_obj_list()?).
Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer.
This is a to-be-invented "specialized event-queuing mechanism" in my language :) as we cannot use efi_create/signal_event() before initializing EFI environment.
This event will be expected to be 'signal'ed at every creation/deletion of UCLASS_BLK device. Right?
Correct.
That event handler creates a new efi event (like a timer w/ timeout=0).
But when is this event handler fired? I think the only possible timing is at efi_init_obj_list().
We already walk through the event list on any u-boot/efi world switch.
This new event's handler can then create the actual efi block device.
I assume that this event handler is fired immediately after efi_signal_event() with timeout=0.
Right, and that signal_event() will happen the next time we go back into efi land. By that time, the dm blk struct will be complete.
If so, why do we need to create an efi event? To isolate the disk code from the other init code?
I don't think we should call init code during runtime, yes. These are 2 paths.
(If so, for the same reason, we should re-write efi_init_obj_list() with events for other efi resources as well.)
But if so, it's not much different from my current approach where a list of efi disks are updated in efi_init_obj_list() :)
The main difference is that disk logic stays in the disc code scope :).
My efi_disk_update() (and efi_disk_register()) is the only function visible outside the disk code, isn't it?
Using some kind of events here is smart, but looks to me a bit overdoing because we anyhow have to go through all the UCLASS_BLK devices to mark whether they are still valid or not :)
What do you mean?
Alex
On 1/10/19 10:22 AM, Alexander Graf wrote:
Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote:
On 10.01.19 08:26, AKASHI Takahiro wrote: Alex,
> On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote: > > >> On 10.01.19 03:13, AKASHI Takahiro wrote: >> Alex, >> >>> On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote: >>> >>> >>>> On 13.12.18 08:58, AKASHI Takahiro wrote: >>>> Heinrich, >>>> >>>>>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>>>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>>>>> Currently, efi_init_obj_list() scan disk devices only once, and never >>>>>> change a list of efi disk devices. This will possibly result in failing >>>>>> to find a removable storage which may be added later on. See [1]. >>>>>> >>>>>> In this patch, called is efi_disk_update() which is responsible for >>>>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>>>>> >>>>>> For example, >>>>>> >>>>>> => efishell devices >>>>>> Scanning disk pci_mmc.blk... >>>>>> Found 3 disks >>>>>> Device Name >>>>>> ============================================ >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>> => usb start >>>>>> starting USB... >>>>>> USB0: USB EHCI 1.00 >>>>>> scanning bus 0 for devices... 3 USB Device(s) found >>>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>>> => efishell devices >>>>>> Scanning disk usb_mass_storage.lun0... >>>>>> Device Name >>>>>> ============================================ >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>>>>> >>>>>> Without this patch, the last device, USB mass storage, won't show up. >>>>>> >>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>>> >>>>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>>> >>>>> Why should we try to fix something in the EFI subsystems that goes wrong >>>>> in the handling of device enumeration. >>>> >>>> No. >>>> This is a natural result from how efi disks are currently implemented on u-boot. >>>> Do you want to totally re-write/re-implement efi disks? >>> >>> Could we just make this event based for now? Call a hook from the >>> storage dm subsystem when a new u-boot block device gets created to >>> issue a sync of that in the efi subsystem? >> >> If I correctly understand you, your suggestion here corresponds >> with my proposal#3 in [1] while my current approach is #2. >> >> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html > > Yes, I think so. > >> So we will call, say, efi_disk_create(struct udevice *) in >> blk_create_device() and efi_dsik_delete() in blk_unbind_all(). > > I would prefer if we didn't call them directly, but through an event > mechanism. So the efi_disk subsystem registers an event with the dm > block subsystem and that will just call all events when block devices > get created which will automatically also include the efi disk creation > callback. Same for reverse.
Do you mean efi event by "event?" (I don't think there is any generic event interface on DM side.)
Whatever an "event" is or whether we call efi_disk_create() directly or indirectly via an event, there is one (big?) issue in this approach (while I've almost finished prototyping):
We cannot call efi_disk_create() within blk_create_device() because some data fields of struct blk_desc, which are to be used by efi disk, are initialized *after* blk_create_device() in driver side.
So we need to add a hook at/after every occurrence of blk_create_device() on driver side. For example,
=== drivers/scsi/scsi.c === int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) { ... ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, bd.blksz, bd.lba, &bdev); ... bdesc = dev_get_uclass_platdata(bdev); bdesc->target = id; bdesc->lun = lun; ...
/*
- We need have efi_disk_create() called here because bdesc->target
- and lun will be used by dp helpers in efi_disk_add_dev().
*/ efi_disk_create(bdev); }
int scsi_scan_dev(struct udevice *dev, bool verbose) { for (i = 0; i < uc_plat->max_id; i++) for (lun = 0; lun < uc_plat->max_lun; lun++) do_scsi_scan_one(dev, i, lun, verbose); ... }
int scsi_scan(bool verbose) { ret = uclass_get(UCLASS_SCSI, &uc); ... uclass_foreach_dev(dev, uc) ret = scsi_scan_dev(dev, verbose); ... } === ===
Since scsn_scan() can be directly called by "scsi rescan" command, There seems to be no generic hook, or event, available in order to call efi_disk_create().
Do I miss anything?
Could the event handler that gets called from somewhere around blk_create_device() just put it into an efi internal "todo list" which we then process using an efi event?
EFI events will only get triggered on the next entry to efi land, so by then we should be safe.
I think I now understand your suggestion; we are going to invent a specialized event-queuing mechanism so that we can take any actions later at appropriate time (probably in efi_init_obj_list()?).
Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer.
This is a to-be-invented "specialized event-queuing mechanism" in my language :) as we cannot use efi_create/signal_event() before initializing EFI environment.
Why shouldn't we partially initialize the EFI environment when the first block device is created?
I think only the memory part of EFI is needed at this stage.
This event will be expected to be 'signal'ed at every creation/deletion of UCLASS_BLK device. Right?
Correct.
Events are not the EFI way of handling drivers. Drivers are connected by calling ConnectController.
Please, add two new functions in lib/efi_loader/efi_disk.c
* one called after a new block device is created * another called before a device is destroyed
both passing as argument (struct udevice *dev) and the caller being in drivers/block/blk-uclass.c
A separate patch has to add a struct udevice *dev field to struct efi_obj and let efi_block_driver use it to decide if a new device shall be created when binding.
In efi_block_driver.c we have to implement the unbind function.
In a later patch series we will use said functions to create or destroy a handle with the EFI_BLOCK_IO_PROTOCOL and wire up ConnectController and DisconnectController.
Best regards
Heinrich
That event handler creates a new efi event (like a timer w/ timeout=0).
But when is this event handler fired? I think the only possible timing is at efi_init_obj_list().
We already walk through the event list on any u-boot/efi world switch.
This new event's handler can then create the actual efi block device.
I assume that this event handler is fired immediately after efi_signal_event() with timeout=0.
Right, and that signal_event() will happen the next time we go back into efi land. By that time, the dm blk struct will be complete.
If so, why do we need to create an efi event? To isolate the disk code from the other init code?
I don't think we should call init code during runtime, yes. These are 2 paths.
(If so, for the same reason, we should re-write efi_init_obj_list() with events for other efi resources as well.)
But if so, it's not much different from my current approach where a list of efi disks are updated in efi_init_obj_list() :)
The main difference is that disk logic stays in the disc code scope :).
My efi_disk_update() (and efi_disk_register()) is the only function visible outside the disk code, isn't it?
Using some kind of events here is smart, but looks to me a bit overdoing because we anyhow have to go through all the UCLASS_BLK devices to mark whether they are still valid or not :)
What do you mean?
Alex
Heinrich,
On Thu, Jan 10, 2019 at 08:22:25PM +0100, Heinrich Schuchardt wrote:
On 1/10/19 10:22 AM, Alexander Graf wrote:
Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote:
> On 10.01.19 08:26, AKASHI Takahiro wrote: > Alex, > >> On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote: >> >> >>> On 10.01.19 03:13, AKASHI Takahiro wrote: >>> Alex, >>> >>>> On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote: >>>> >>>> >>>>> On 13.12.18 08:58, AKASHI Takahiro wrote: >>>>> Heinrich, >>>>> >>>>>>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>>>>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>>>>>> Currently, efi_init_obj_list() scan disk devices only once, and never >>>>>>> change a list of efi disk devices. This will possibly result in failing >>>>>>> to find a removable storage which may be added later on. See [1]. >>>>>>> >>>>>>> In this patch, called is efi_disk_update() which is responsible for >>>>>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>>>>>> >>>>>>> For example, >>>>>>> >>>>>>> => efishell devices >>>>>>> Scanning disk pci_mmc.blk... >>>>>>> Found 3 disks >>>>>>> Device Name >>>>>>> ============================================ >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>> => usb start >>>>>>> starting USB... >>>>>>> USB0: USB EHCI 1.00 >>>>>>> scanning bus 0 for devices... 3 USB Device(s) found >>>>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>>>> => efishell devices >>>>>>> Scanning disk usb_mass_storage.lun0... >>>>>>> Device Name >>>>>>> ============================================ >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>>>>>> >>>>>>> Without this patch, the last device, USB mass storage, won't show up. >>>>>>> >>>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>>>> >>>>>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>>>> >>>>>> Why should we try to fix something in the EFI subsystems that goes wrong >>>>>> in the handling of device enumeration. >>>>> >>>>> No. >>>>> This is a natural result from how efi disks are currently implemented on u-boot. >>>>> Do you want to totally re-write/re-implement efi disks? >>>> >>>> Could we just make this event based for now? Call a hook from the >>>> storage dm subsystem when a new u-boot block device gets created to >>>> issue a sync of that in the efi subsystem? >>> >>> If I correctly understand you, your suggestion here corresponds >>> with my proposal#3 in [1] while my current approach is #2. >>> >>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >> >> Yes, I think so. >> >>> So we will call, say, efi_disk_create(struct udevice *) in >>> blk_create_device() and efi_dsik_delete() in blk_unbind_all(). >> >> I would prefer if we didn't call them directly, but through an event >> mechanism. So the efi_disk subsystem registers an event with the dm >> block subsystem and that will just call all events when block devices >> get created which will automatically also include the efi disk creation >> callback. Same for reverse. > > Do you mean efi event by "event?" > (I don't think there is any generic event interface on DM side.) > > Whatever an "event" is or whether we call efi_disk_create() directly > or indirectly via an event, there is one (big?) issue in this approach > (while I've almost finished prototyping): > > We cannot call efi_disk_create() within blk_create_device() because > some data fields of struct blk_desc, which are to be used by efi disk, > are initialized *after* blk_create_device() in driver side. > > So we need to add a hook at/after every occurrence of blk_create_device() > on driver side. For example, > > === drivers/scsi/scsi.c === > int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) > { > ... > ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, > bd.blksz, bd.lba, &bdev); > ... > bdesc = dev_get_uclass_platdata(bdev); > bdesc->target = id; > bdesc->lun = lun; > ... > > /* > * We need have efi_disk_create() called here because bdesc->target > * and lun will be used by dp helpers in efi_disk_add_dev(). > */ > efi_disk_create(bdev); > } > > int scsi_scan_dev(struct udevice *dev, bool verbose) > { > for (i = 0; i < uc_plat->max_id; i++) > for (lun = 0; lun < uc_plat->max_lun; lun++) > do_scsi_scan_one(dev, i, lun, verbose); > ... > } > > int scsi_scan(bool verbose) > { > ret = uclass_get(UCLASS_SCSI, &uc); > ... > uclass_foreach_dev(dev, uc) > ret = scsi_scan_dev(dev, verbose); > ... > } > === === > > Since scsn_scan() can be directly called by "scsi rescan" command, > There seems to be no generic hook, or event, available in order to > call efi_disk_create(). > > Do I miss anything?
Could the event handler that gets called from somewhere around blk_create_device() just put it into an efi internal "todo list" which we then process using an efi event?
EFI events will only get triggered on the next entry to efi land, so by then we should be safe.
I think I now understand your suggestion; we are going to invent a specialized event-queuing mechanism so that we can take any actions later at appropriate time (probably in efi_init_obj_list()?).
Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer.
This is a to-be-invented "specialized event-queuing mechanism" in my language :) as we cannot use efi_create/signal_event() before initializing EFI environment.
Why shouldn't we partially initialize the EFI environment when the first block device is created? I think only the memory part of EFI is needed at this stage.
Maybe, but how long we can guarantee this in the future?
This event will be expected to be 'signal'ed at every creation/deletion of UCLASS_BLK device. Right?
Correct.
Events are not the EFI way of handling drivers. Drivers are connected by calling ConnectController.
I didn't get you point very well here, but anyway
Please, add two new functions in lib/efi_loader/efi_disk.c
- one called after a new block device is created
- another called before a device is destroyed
both passing as argument (struct udevice *dev) and the caller being in drivers/block/blk-uclass.c
Those are exactly what I proposed in my reply to Alex; efi_disk_create() and efi_disk_delete().
A separate patch has to add a struct udevice *dev field to struct efi_obj and let efi_block_driver use it to decide if a new device shall be created when binding.
In efi_block_driver.c we have to implement the unbind function.
efi_block_device.c?
The issue I noticed regarding the current implementation of UCLASS_BLK is, as I said in my reply to Simon, that efi disk objects for u-boot block devices are created without going through this (bind) procedure. Yet those objects won't show up as UCLASS_BLK's.
In a later patch series we will use said functions to create or destroy a handle with the EFI_BLOCK_IO_PROTOCOL and wire up ConnectController and DisconnectController.
How do you suggest that we can resolve the issue above?
Thanks, -Takahiro Akashi
Best regards
Heinrich
That event handler creates a new efi event (like a timer w/ timeout=0).
But when is this event handler fired? I think the only possible timing is at efi_init_obj_list().
We already walk through the event list on any u-boot/efi world switch.
This new event's handler can then create the actual efi block device.
I assume that this event handler is fired immediately after efi_signal_event() with timeout=0.
Right, and that signal_event() will happen the next time we go back into efi land. By that time, the dm blk struct will be complete.
If so, why do we need to create an efi event? To isolate the disk code from the other init code?
I don't think we should call init code during runtime, yes. These are 2 paths.
(If so, for the same reason, we should re-write efi_init_obj_list() with events for other efi resources as well.)
But if so, it's not much different from my current approach where a list of efi disks are updated in efi_init_obj_list() :)
The main difference is that disk logic stays in the disc code scope :).
My efi_disk_update() (and efi_disk_register()) is the only function visible outside the disk code, isn't it?
Using some kind of events here is smart, but looks to me a bit overdoing because we anyhow have to go through all the UCLASS_BLK devices to mark whether they are still valid or not :)
What do you mean?
Alex
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
So, first, let me reply to each of your comments. Through this process, I hope we will have better understandings of long-term solution as well as a tentative fix.
On Thu, Jan 10, 2019 at 10:22:04AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote:
On 10.01.19 08:26, AKASHI Takahiro wrote: Alex,
> On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote: > > >> On 10.01.19 03:13, AKASHI Takahiro wrote: >> Alex, >> >>> On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote: >>> >>> >>>> On 13.12.18 08:58, AKASHI Takahiro wrote: >>>> Heinrich, >>>> >>>>>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>>>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>>>>> Currently, efi_init_obj_list() scan disk devices only once, and never >>>>>> change a list of efi disk devices. This will possibly result in failing >>>>>> to find a removable storage which may be added later on. See [1]. >>>>>> >>>>>> In this patch, called is efi_disk_update() which is responsible for >>>>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>>>>> >>>>>> For example, >>>>>> >>>>>> => efishell devices >>>>>> Scanning disk pci_mmc.blk... >>>>>> Found 3 disks >>>>>> Device Name >>>>>> ============================================ >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>> => usb start >>>>>> starting USB... >>>>>> USB0: USB EHCI 1.00 >>>>>> scanning bus 0 for devices... 3 USB Device(s) found >>>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>>> => efishell devices >>>>>> Scanning disk usb_mass_storage.lun0... >>>>>> Device Name >>>>>> ============================================ >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>>>>> >>>>>> Without this patch, the last device, USB mass storage, won't show up. >>>>>> >>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>>> >>>>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>>> >>>>> Why should we try to fix something in the EFI subsystems that goes wrong >>>>> in the handling of device enumeration. >>>> >>>> No. >>>> This is a natural result from how efi disks are currently implemented on u-boot. >>>> Do you want to totally re-write/re-implement efi disks? >>> >>> Could we just make this event based for now? Call a hook from the >>> storage dm subsystem when a new u-boot block device gets created to >>> issue a sync of that in the efi subsystem? >> >> If I correctly understand you, your suggestion here corresponds >> with my proposal#3 in [1] while my current approach is #2. >> >> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html > > Yes, I think so. > >> So we will call, say, efi_disk_create(struct udevice *) in >> blk_create_device() and efi_dsik_delete() in blk_unbind_all(). > > I would prefer if we didn't call them directly, but through an event > mechanism. So the efi_disk subsystem registers an event with the dm > block subsystem and that will just call all events when block devices > get created which will automatically also include the efi disk creation > callback. Same for reverse.
Do you mean efi event by "event?" (I don't think there is any generic event interface on DM side.)
Whatever an "event" is or whether we call efi_disk_create() directly or indirectly via an event, there is one (big?) issue in this approach (while I've almost finished prototyping):
We cannot call efi_disk_create() within blk_create_device() because some data fields of struct blk_desc, which are to be used by efi disk, are initialized *after* blk_create_device() in driver side.
So we need to add a hook at/after every occurrence of blk_create_device() on driver side. For example,
=== drivers/scsi/scsi.c === int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) { ... ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, bd.blksz, bd.lba, &bdev); ... bdesc = dev_get_uclass_platdata(bdev); bdesc->target = id; bdesc->lun = lun; ...
/*
- We need have efi_disk_create() called here because bdesc->target
- and lun will be used by dp helpers in efi_disk_add_dev().
*/ efi_disk_create(bdev); }
int scsi_scan_dev(struct udevice *dev, bool verbose) { for (i = 0; i < uc_plat->max_id; i++) for (lun = 0; lun < uc_plat->max_lun; lun++) do_scsi_scan_one(dev, i, lun, verbose); ... }
int scsi_scan(bool verbose) { ret = uclass_get(UCLASS_SCSI, &uc); ... uclass_foreach_dev(dev, uc) ret = scsi_scan_dev(dev, verbose); ... } === ===
Since scsn_scan() can be directly called by "scsi rescan" command, There seems to be no generic hook, or event, available in order to call efi_disk_create().
Do I miss anything?
Could the event handler that gets called from somewhere around blk_create_device() just put it into an efi internal "todo list" which we then process using an efi event?
EFI events will only get triggered on the next entry to efi land, so by then we should be safe.
I think I now understand your suggestion; we are going to invent a specialized event-queuing mechanism so that we can take any actions later at appropriate time (probably in efi_init_obj_list()?).
Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer.
This is a to-be-invented "specialized event-queuing mechanism" in my language :) as we cannot use efi_create/signal_event() before initializing EFI environment.
This event will be expected to be 'signal'ed at every creation/deletion of UCLASS_BLK device. Right?
Correct.
That event handler creates a new efi event (like a timer w/ timeout=0).
But when is this event handler fired? I think the only possible timing is at efi_init_obj_list().
We already walk through the event list on any u-boot/efi world switch.
? Where is the code?
This new event's handler can then create the actual efi block device.
I assume that this event handler is fired immediately after efi_signal_event() with timeout=0.
Right, and that signal_event() will happen the next time we go back into efi land. By that time, the dm blk struct will be complete.
This will be true.
If so, why do we need to create an efi event? To isolate the disk code from the other init code?
I don't think we should call init code during runtime, yes. These are 2 paths.
(If so, for the same reason, we should re-write efi_init_obj_list() with events for other efi resources as well.)
But if so, it's not much different from my current approach where a list of efi disks are updated in efi_init_obj_list() :)
The main difference is that disk logic stays in the disc code scope :).
My efi_disk_update() (and efi_disk_register()) is the only function visible outside the disk code, isn't it?
Using some kind of events here is smart, but looks to me a bit overdoing because we anyhow have to go through all the UCLASS_BLK devices to mark whether they are still valid or not :)
What do you mean?
"all the UCLASS_BLK deivces" -> all efi_disk_obj's
Let me rephrase it; all efi_disk_obj's will be re-visisted in efi_init_obj_list() to determine its backing u-boot block device is still valid. If not valid, a said efi_disk_obj should be marked so to prevent further accessing in efi world.
-Takahiro Akashi
Alex
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK. Then provide a DM path and have a non-DM fallback still in its own source file that also provides EFI BLK devices. Eventually we just remove the latter.
That way we can then work on getting hotplug working in the DM path, which is the one we want anyway. For non-DM, you simply miss out on that amazing new feature, but we don't regress users.
So, first, let me reply to each of your comments. Through this process, I hope we will have better understandings of long-term solution as well as a tentative fix.
On Thu, Jan 10, 2019 at 10:22:04AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote:
> On 10.01.19 08:26, AKASHI Takahiro wrote: > Alex, > >> On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote: >> >> >>> On 10.01.19 03:13, AKASHI Takahiro wrote: >>> Alex, >>> >>>> On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote: >>>> >>>> >>>>> On 13.12.18 08:58, AKASHI Takahiro wrote: >>>>> Heinrich, >>>>> >>>>>>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>>>>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>>>>>> Currently, efi_init_obj_list() scan disk devices only once, and never >>>>>>> change a list of efi disk devices. This will possibly result in failing >>>>>>> to find a removable storage which may be added later on. See [1]. >>>>>>> >>>>>>> In this patch, called is efi_disk_update() which is responsible for >>>>>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>>>>>> >>>>>>> For example, >>>>>>> >>>>>>> => efishell devices >>>>>>> Scanning disk pci_mmc.blk... >>>>>>> Found 3 disks >>>>>>> Device Name >>>>>>> ============================================ >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>> => usb start >>>>>>> starting USB... >>>>>>> USB0: USB EHCI 1.00 >>>>>>> scanning bus 0 for devices... 3 USB Device(s) found >>>>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>>>> => efishell devices >>>>>>> Scanning disk usb_mass_storage.lun0... >>>>>>> Device Name >>>>>>> ============================================ >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>>>>>> >>>>>>> Without this patch, the last device, USB mass storage, won't show up. >>>>>>> >>>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>>>> >>>>>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>>>> >>>>>> Why should we try to fix something in the EFI subsystems that goes wrong >>>>>> in the handling of device enumeration. >>>>> >>>>> No. >>>>> This is a natural result from how efi disks are currently implemented on u-boot. >>>>> Do you want to totally re-write/re-implement efi disks? >>>> >>>> Could we just make this event based for now? Call a hook from the >>>> storage dm subsystem when a new u-boot block device gets created to >>>> issue a sync of that in the efi subsystem? >>> >>> If I correctly understand you, your suggestion here corresponds >>> with my proposal#3 in [1] while my current approach is #2. >>> >>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >> >> Yes, I think so. >> >>> So we will call, say, efi_disk_create(struct udevice *) in >>> blk_create_device() and efi_dsik_delete() in blk_unbind_all(). >> >> I would prefer if we didn't call them directly, but through an event >> mechanism. So the efi_disk subsystem registers an event with the dm >> block subsystem and that will just call all events when block devices >> get created which will automatically also include the efi disk creation >> callback. Same for reverse. > > Do you mean efi event by "event?" > (I don't think there is any generic event interface on DM side.) > > Whatever an "event" is or whether we call efi_disk_create() directly > or indirectly via an event, there is one (big?) issue in this approach > (while I've almost finished prototyping): > > We cannot call efi_disk_create() within blk_create_device() because > some data fields of struct blk_desc, which are to be used by efi disk, > are initialized *after* blk_create_device() in driver side. > > So we need to add a hook at/after every occurrence of blk_create_device() > on driver side. For example, > > === drivers/scsi/scsi.c === > int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) > { > ... > ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, > bd.blksz, bd.lba, &bdev); > ... > bdesc = dev_get_uclass_platdata(bdev); > bdesc->target = id; > bdesc->lun = lun; > ... > > /* > * We need have efi_disk_create() called here because bdesc->target > * and lun will be used by dp helpers in efi_disk_add_dev(). > */ > efi_disk_create(bdev); > } > > int scsi_scan_dev(struct udevice *dev, bool verbose) > { > for (i = 0; i < uc_plat->max_id; i++) > for (lun = 0; lun < uc_plat->max_lun; lun++) > do_scsi_scan_one(dev, i, lun, verbose); > ... > } > > int scsi_scan(bool verbose) > { > ret = uclass_get(UCLASS_SCSI, &uc); > ... > uclass_foreach_dev(dev, uc) > ret = scsi_scan_dev(dev, verbose); > ... > } > === === > > Since scsn_scan() can be directly called by "scsi rescan" command, > There seems to be no generic hook, or event, available in order to > call efi_disk_create(). > > Do I miss anything?
Could the event handler that gets called from somewhere around blk_create_device() just put it into an efi internal "todo list" which we then process using an efi event?
EFI events will only get triggered on the next entry to efi land, so by then we should be safe.
I think I now understand your suggestion; we are going to invent a specialized event-queuing mechanism so that we can take any actions later at appropriate time (probably in efi_init_obj_list()?).
Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer.
This is a to-be-invented "specialized event-queuing mechanism" in my language :) as we cannot use efi_create/signal_event() before initializing EFI environment.
This event will be expected to be 'signal'ed at every creation/deletion of UCLASS_BLK device. Right?
Correct.
That event handler creates a new efi event (like a timer w/ timeout=0).
But when is this event handler fired? I think the only possible timing is at efi_init_obj_list().
We already walk through the event list on any u-boot/efi world switch.
? Where is the code?
Ah, I must have misremembered, sorry. I think that was one proposed patch a while ago, but we didn't put it in.
But worst case we can just put additional efi_timer_check() calls at strategic places if needed.
This new event's handler can then create the actual efi block device.
I assume that this event handler is fired immediately after efi_signal_event() with timeout=0.
Right, and that signal_event() will happen the next time we go back into efi land. By that time, the dm blk struct will be complete.
This will be true.
If so, why do we need to create an efi event? To isolate the disk code from the other init code?
I don't think we should call init code during runtime, yes. These are 2 paths.
(If so, for the same reason, we should re-write efi_init_obj_list() with events for other efi resources as well.)
But if so, it's not much different from my current approach where a list of efi disks are updated in efi_init_obj_list() :)
The main difference is that disk logic stays in the disc code scope :).
My efi_disk_update() (and efi_disk_register()) is the only function visible outside the disk code, isn't it?
Using some kind of events here is smart, but looks to me a bit overdoing because we anyhow have to go through all the UCLASS_BLK devices to mark whether they are still valid or not :)
What do you mean?
"all the UCLASS_BLK deivces" -> all efi_disk_obj's
Let me rephrase it; all efi_disk_obj's will be re-visisted in efi_init_obj_list() to determine its backing u-boot block device is still valid. If not valid, a said efi_disk_obj should be marked so to prevent further accessing in efi world.
Correct.
Alex
Hi Alex,
On Fri, 11 Jan 2019 at 00:57, Alexander Graf agraf@suse.de wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I don't think that is true anymore. The deadline for patches is effectively 28th January.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK. Then provide a DM path and have a non-DM fallback still in its own source file that also provides EFI BLK devices. Eventually we just remove the latter.
No fallback, please. As above, it is time to remove code that does not use CONFIG_BLK.
That way we can then work on getting hotplug working in the DM path, which is the one we want anyway. For non-DM, you simply miss out on that amazing new feature, but we don't regress users.
Of which there will not be any as of 2019.04. I'm sorry to belabour this point, but I feel quite strongly that we should not be adding new code to old frameworks. We should instead be migrating away from them and deleting them.
Regards, Simon
Am 12.01.2019 um 22:32 schrieb Simon Glass sjg@chromium.org:
Hi Alex,
On Fri, 11 Jan 2019 at 00:57, Alexander Graf agraf@suse.de wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote: Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I don't think that is true anymore. The deadline for patches is effectively 28th January.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK. Then provide a DM path and have a non-DM fallback still in its own source file that also provides EFI BLK devices. Eventually we just remove the latter.
No fallback, please. As above, it is time to remove code that does not use CONFIG_BLK.
That way we can then work on getting hotplug working in the DM path, which is the one we want anyway. For non-DM, you simply miss out on that amazing new feature, but we don't regress users.
Of which there will not be any as of 2019.04. I'm sorry to belabour this point, but I feel quite strongly that we should not be adding new code to old frameworks. We should instead be migrating away from them and deleting them.
I was thinking of an isolated file that we could just remove eventually.
But if we're getting to a dm only world with 2019.04 already, I'll be happy to merge code that merges efi and dm objects for that release.
The one thing I do not want here is any functional regression. If a board worked with efi in 2019.01, it better works at least as well in 2019.04.
Alex
Hi Alex,
On Sat, 12 Jan 2019 at 15:00, Alexander Graf agraf@suse.de wrote:
Am 12.01.2019 um 22:32 schrieb Simon Glass sjg@chromium.org:
Hi Alex,
On Fri, 11 Jan 2019 at 00:57, Alexander Graf agraf@suse.de wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote: Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I don't think that is true anymore. The deadline for patches is effectively 28th January.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK. Then provide a DM path and have a non-DM fallback still in its own source file that also provides EFI BLK devices. Eventually we just remove the latter.
No fallback, please. As above, it is time to remove code that does not use CONFIG_BLK.
That way we can then work on getting hotplug working in the DM path, which is the one we want anyway. For non-DM, you simply miss out on that amazing new feature, but we don't regress users.
Of which there will not be any as of 2019.04. I'm sorry to belabour this point, but I feel quite strongly that we should not be adding new code to old frameworks. We should instead be migrating away from them and deleting them.
I was thinking of an isolated file that we could just remove eventually.
But if we're getting to a dm only world with 2019.04 already, I'll be happy to merge code that merges efi and dm objects for that release.
The one thing I do not want here is any functional regression. If a board worked with efi in 2019.01, it better works at least as well in 2019.04.
So long as it supports BLK, yes. If it doesn't then I suppose it will be removed anyway.
Regards, Simon
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK.
Even in the current implementation, * UEFI disk is implemented using UCLASS_BLK devices (We can ignore !CONFIG_BLK case now as we have agreed.) * UEFI-specific block device can be seen as UCLASS_BLK/IF_TYPE_EFI
So how essentially different is the *ultimate* goal from the current form regarding block devices?
In order to identify UEFI disks with u-boot devices transparently, we will have to have some sort of *hook* (or hotplug in Alex's language?), either in create_block_devices or bind/probe? I don't know, but Simon seems to be in denial about this idea.
Then provide a DM path and have a non-DM fallback still in its own source file that also provides EFI BLK devices. Eventually we just remove the latter.
That way we can then work on getting hotplug working in the DM path, which is the one we want anyway. For non-DM, you simply miss out on that amazing new feature, but we don't regress users.
So, first, let me reply to each of your comments. Through this process, I hope we will have better understandings of long-term solution as well as a tentative fix.
On Thu, Jan 10, 2019 at 10:22:04AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote:
> Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org: > > On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote: > > >> On 10.01.19 08:26, AKASHI Takahiro wrote: >> Alex, >> >>> On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote: >>> >>> >>>> On 10.01.19 03:13, AKASHI Takahiro wrote: >>>> Alex, >>>> >>>>> On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote: >>>>> >>>>> >>>>>> On 13.12.18 08:58, AKASHI Takahiro wrote: >>>>>> Heinrich, >>>>>> >>>>>>>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>>>>>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>>>>>>> Currently, efi_init_obj_list() scan disk devices only once, and never >>>>>>>> change a list of efi disk devices. This will possibly result in failing >>>>>>>> to find a removable storage which may be added later on. See [1]. >>>>>>>> >>>>>>>> In this patch, called is efi_disk_update() which is responsible for >>>>>>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>>>>>>> >>>>>>>> For example, >>>>>>>> >>>>>>>> => efishell devices >>>>>>>> Scanning disk pci_mmc.blk... >>>>>>>> Found 3 disks >>>>>>>> Device Name >>>>>>>> ============================================ >>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>>> => usb start >>>>>>>> starting USB... >>>>>>>> USB0: USB EHCI 1.00 >>>>>>>> scanning bus 0 for devices... 3 USB Device(s) found >>>>>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>>>>> => efishell devices >>>>>>>> Scanning disk usb_mass_storage.lun0... >>>>>>>> Device Name >>>>>>>> ============================================ >>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>>>>>>> >>>>>>>> Without this patch, the last device, USB mass storage, won't show up. >>>>>>>> >>>>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>>>>> >>>>>>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>>>>> >>>>>>> Why should we try to fix something in the EFI subsystems that goes wrong >>>>>>> in the handling of device enumeration. >>>>>> >>>>>> No. >>>>>> This is a natural result from how efi disks are currently implemented on u-boot. >>>>>> Do you want to totally re-write/re-implement efi disks? >>>>> >>>>> Could we just make this event based for now? Call a hook from the >>>>> storage dm subsystem when a new u-boot block device gets created to >>>>> issue a sync of that in the efi subsystem? >>>> >>>> If I correctly understand you, your suggestion here corresponds >>>> with my proposal#3 in [1] while my current approach is #2. >>>> >>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>> >>> Yes, I think so. >>> >>>> So we will call, say, efi_disk_create(struct udevice *) in >>>> blk_create_device() and efi_dsik_delete() in blk_unbind_all(). >>> >>> I would prefer if we didn't call them directly, but through an event >>> mechanism. So the efi_disk subsystem registers an event with the dm >>> block subsystem and that will just call all events when block devices >>> get created which will automatically also include the efi disk creation >>> callback. Same for reverse. >> >> Do you mean efi event by "event?" >> (I don't think there is any generic event interface on DM side.) >> >> Whatever an "event" is or whether we call efi_disk_create() directly >> or indirectly via an event, there is one (big?) issue in this approach >> (while I've almost finished prototyping): >> >> We cannot call efi_disk_create() within blk_create_device() because >> some data fields of struct blk_desc, which are to be used by efi disk, >> are initialized *after* blk_create_device() in driver side. >> >> So we need to add a hook at/after every occurrence of blk_create_device() >> on driver side. For example, >> >> === drivers/scsi/scsi.c === >> int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) >> { >> ... >> ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, >> bd.blksz, bd.lba, &bdev); >> ... >> bdesc = dev_get_uclass_platdata(bdev); >> bdesc->target = id; >> bdesc->lun = lun; >> ... >> >> /* >> * We need have efi_disk_create() called here because bdesc->target >> * and lun will be used by dp helpers in efi_disk_add_dev(). >> */ >> efi_disk_create(bdev); >> } >> >> int scsi_scan_dev(struct udevice *dev, bool verbose) >> { >> for (i = 0; i < uc_plat->max_id; i++) >> for (lun = 0; lun < uc_plat->max_lun; lun++) >> do_scsi_scan_one(dev, i, lun, verbose); >> ... >> } >> >> int scsi_scan(bool verbose) >> { >> ret = uclass_get(UCLASS_SCSI, &uc); >> ... >> uclass_foreach_dev(dev, uc) >> ret = scsi_scan_dev(dev, verbose); >> ... >> } >> === === >> >> Since scsn_scan() can be directly called by "scsi rescan" command, >> There seems to be no generic hook, or event, available in order to >> call efi_disk_create(). >> >> Do I miss anything? > > Could the event handler that gets called from somewhere around > blk_create_device() just put it into an efi internal "todo list" which > we then process using an efi event? > > EFI events will only get triggered on the next entry to efi land, so by > then we should be safe.
I think I now understand your suggestion; we are going to invent a specialized event-queuing mechanism so that we can take any actions later at appropriate time (probably in efi_init_obj_list()?).
Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer.
This is a to-be-invented "specialized event-queuing mechanism" in my language :) as we cannot use efi_create/signal_event() before initializing EFI environment.
This event will be expected to be 'signal'ed at every creation/deletion of UCLASS_BLK device. Right?
Correct.
That event handler creates a new efi event (like a timer w/ timeout=0).
But when is this event handler fired? I think the only possible timing is at efi_init_obj_list().
We already walk through the event list on any u-boot/efi world switch.
? Where is the code?
Ah, I must have misremembered, sorry. I think that was one proposed patch a while ago, but we didn't put it in.
But worst case we can just put additional efi_timer_check() calls at strategic places if needed.
Do you expect this kind of mechanism be implemented in my short-term fix?
Thanks, -Takahiro Akashi
This new event's handler can then create the actual efi block device.
I assume that this event handler is fired immediately after efi_signal_event() with timeout=0.
Right, and that signal_event() will happen the next time we go back into efi land. By that time, the dm blk struct will be complete.
This will be true.
If so, why do we need to create an efi event? To isolate the disk code from the other init code?
I don't think we should call init code during runtime, yes. These are 2 paths.
(If so, for the same reason, we should re-write efi_init_obj_list() with events for other efi resources as well.)
But if so, it's not much different from my current approach where a list of efi disks are updated in efi_init_obj_list() :)
The main difference is that disk logic stays in the disc code scope :).
My efi_disk_update() (and efi_disk_register()) is the only function visible outside the disk code, isn't it?
Using some kind of events here is smart, but looks to me a bit overdoing because we anyhow have to go through all the UCLASS_BLK devices to mark whether they are still valid or not :)
What do you mean?
"all the UCLASS_BLK deivces" -> all efi_disk_obj's
Let me rephrase it; all efi_disk_obj's will be re-visisted in efi_init_obj_list() to determine its backing u-boot block device is still valid. If not valid, a said efi_disk_obj should be marked so to prevent further accessing in efi world.
Correct.
Alex
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK.
Even in the current implementation,
- UEFI disk is implemented using UCLASS_BLK devices (We can ignore !CONFIG_BLK case now as we have agreed.)
- UEFI-specific block device can be seen as UCLASS_BLK/IF_TYPE_EFI
So how essentially different is the *ultimate* goal from the current form regarding block devices?
The ultimate goal is that efi_disk_register() and efi_obj_list disappear.
Functionality wise we should be 100% identical to today, so all test cases would still apply the same way as they do now. This is purely internal rework, nothing visible to UEFI payloads.
In order to identify UEFI disks with u-boot devices transparently, we will have to have some sort of *hook* (or hotplug in Alex's language?), either in create_block_devices or bind/probe? I don't know, but Simon seems to be in denial about this idea.
Well, if a udevice *is* an efi device, we no longer need hooks. The object list would simply change.
We may still need to have event notifications at that stage, but that's a different story.
As transitioning period, we could probably implement 2 efi object roots: efi_obj_list as well as the udevice based one. Every piece of code that iterates through devices then just iterates over both. That way we should be able to slowly move devices from the old object model to the new one.
Then provide a DM path and have a non-DM fallback still in its own source file that also provides EFI BLK devices. Eventually we just remove the latter.
That way we can then work on getting hotplug working in the DM path, which is the one we want anyway. For non-DM, you simply miss out on that amazing new feature, but we don't regress users.
So, first, let me reply to each of your comments. Through this process, I hope we will have better understandings of long-term solution as well as a tentative fix.
On Thu, Jan 10, 2019 at 10:22:04AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote:
>> Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org: >> >> On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote: >> >> >>> On 10.01.19 08:26, AKASHI Takahiro wrote: >>> Alex, >>> >>>> On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote: >>>> >>>> >>>>> On 10.01.19 03:13, AKASHI Takahiro wrote: >>>>> Alex, >>>>> >>>>>> On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote: >>>>>> >>>>>> >>>>>>> On 13.12.18 08:58, AKASHI Takahiro wrote: >>>>>>> Heinrich, >>>>>>> >>>>>>>>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>>>>>>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>>>>>>>> Currently, efi_init_obj_list() scan disk devices only once, and never >>>>>>>>> change a list of efi disk devices. This will possibly result in failing >>>>>>>>> to find a removable storage which may be added later on. See [1]. >>>>>>>>> >>>>>>>>> In this patch, called is efi_disk_update() which is responsible for >>>>>>>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>>>>>>>> >>>>>>>>> For example, >>>>>>>>> >>>>>>>>> => efishell devices >>>>>>>>> Scanning disk pci_mmc.blk... >>>>>>>>> Found 3 disks >>>>>>>>> Device Name >>>>>>>>> ============================================ >>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>>>> => usb start >>>>>>>>> starting USB... >>>>>>>>> USB0: USB EHCI 1.00 >>>>>>>>> scanning bus 0 for devices... 3 USB Device(s) found >>>>>>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>>>>>> => efishell devices >>>>>>>>> Scanning disk usb_mass_storage.lun0... >>>>>>>>> Device Name >>>>>>>>> ============================================ >>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>>>>>>>> >>>>>>>>> Without this patch, the last device, USB mass storage, won't show up. >>>>>>>>> >>>>>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>>>>>> >>>>>>>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>>>>>> >>>>>>>> Why should we try to fix something in the EFI subsystems that goes wrong >>>>>>>> in the handling of device enumeration. >>>>>>> >>>>>>> No. >>>>>>> This is a natural result from how efi disks are currently implemented on u-boot. >>>>>>> Do you want to totally re-write/re-implement efi disks? >>>>>> >>>>>> Could we just make this event based for now? Call a hook from the >>>>>> storage dm subsystem when a new u-boot block device gets created to >>>>>> issue a sync of that in the efi subsystem? >>>>> >>>>> If I correctly understand you, your suggestion here corresponds >>>>> with my proposal#3 in [1] while my current approach is #2. >>>>> >>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>> >>>> Yes, I think so. >>>> >>>>> So we will call, say, efi_disk_create(struct udevice *) in >>>>> blk_create_device() and efi_dsik_delete() in blk_unbind_all(). >>>> >>>> I would prefer if we didn't call them directly, but through an event >>>> mechanism. So the efi_disk subsystem registers an event with the dm >>>> block subsystem and that will just call all events when block devices >>>> get created which will automatically also include the efi disk creation >>>> callback. Same for reverse. >>> >>> Do you mean efi event by "event?" >>> (I don't think there is any generic event interface on DM side.) >>> >>> Whatever an "event" is or whether we call efi_disk_create() directly >>> or indirectly via an event, there is one (big?) issue in this approach >>> (while I've almost finished prototyping): >>> >>> We cannot call efi_disk_create() within blk_create_device() because >>> some data fields of struct blk_desc, which are to be used by efi disk, >>> are initialized *after* blk_create_device() in driver side. >>> >>> So we need to add a hook at/after every occurrence of blk_create_device() >>> on driver side. For example, >>> >>> === drivers/scsi/scsi.c === >>> int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) >>> { >>> ... >>> ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, >>> bd.blksz, bd.lba, &bdev); >>> ... >>> bdesc = dev_get_uclass_platdata(bdev); >>> bdesc->target = id; >>> bdesc->lun = lun; >>> ... >>> >>> /* >>> * We need have efi_disk_create() called here because bdesc->target >>> * and lun will be used by dp helpers in efi_disk_add_dev(). >>> */ >>> efi_disk_create(bdev); >>> } >>> >>> int scsi_scan_dev(struct udevice *dev, bool verbose) >>> { >>> for (i = 0; i < uc_plat->max_id; i++) >>> for (lun = 0; lun < uc_plat->max_lun; lun++) >>> do_scsi_scan_one(dev, i, lun, verbose); >>> ... >>> } >>> >>> int scsi_scan(bool verbose) >>> { >>> ret = uclass_get(UCLASS_SCSI, &uc); >>> ... >>> uclass_foreach_dev(dev, uc) >>> ret = scsi_scan_dev(dev, verbose); >>> ... >>> } >>> === === >>> >>> Since scsn_scan() can be directly called by "scsi rescan" command, >>> There seems to be no generic hook, or event, available in order to >>> call efi_disk_create(). >>> >>> Do I miss anything? >> >> Could the event handler that gets called from somewhere around >> blk_create_device() just put it into an efi internal "todo list" which >> we then process using an efi event? >> >> EFI events will only get triggered on the next entry to efi land, so by >> then we should be safe. > > I think I now understand your suggestion; we are going to invent > a specialized event-queuing mechanism so that we can take any actions > later at appropriate time (probably in efi_init_obj_list()?).
Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer.
This is a to-be-invented "specialized event-queuing mechanism" in my language :) as we cannot use efi_create/signal_event() before initializing EFI environment.
This event will be expected to be 'signal'ed at every creation/deletion of UCLASS_BLK device. Right?
Correct.
That event handler creates a new efi event (like a timer w/ timeout=0).
But when is this event handler fired? I think the only possible timing is at efi_init_obj_list().
We already walk through the event list on any u-boot/efi world switch.
? Where is the code?
Ah, I must have misremembered, sorry. I think that was one proposed patch a while ago, but we didn't put it in.
But worst case we can just put additional efi_timer_check() calls at strategic places if needed.
Do you expect this kind of mechanism be implemented in my short-term fix?
To be completely honest, I don't think your fix is very critical right now, since it touches a case that's known broken today already.
I would prefer if we could concentrate on the real path forward, where everything becomes implicit and we no longer need to sync the two object trees.
Alex
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass - DM uclasses which support EFI would create a child EFI device (e.g. a UCLASS_EFI child of each UCLASS_BLK device) - EFI-uclass devices would thus be bound as needed - Probing an EFI device would causes its parents to be probed - We can use all the existing DM hooks (probe, remove, parent/child data, operations), to implement EFI things
I'm assuming that a small percentage of devices would have EFI children, so that this is more efficient than trying to merge the data structures. It also allows EFI to maintain some separate from the core DM code.
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK.
Even in the current implementation,
- UEFI disk is implemented using UCLASS_BLK devices (We can ignore !CONFIG_BLK case now as we have agreed.)
- UEFI-specific block device can be seen as UCLASS_BLK/IF_TYPE_EFI
So how essentially different is the *ultimate* goal from the current form regarding block devices?
The ultimate goal is that efi_disk_register() and efi_obj_list disappear.
Functionality wise we should be 100% identical to today, so all test cases would still apply the same way as they do now. This is purely internal rework, nothing visible to UEFI payloads.
Yes.
In order to identify UEFI disks with u-boot devices transparently, we will have to have some sort of *hook* (or hotplug in Alex's language?), either in create_block_devices or bind/probe? I don't know, but Simon seems to be in denial about this idea.
Well, if a udevice *is* an efi device, we no longer need hooks. The object list would simply change.
We may still need to have event notifications at that stage, but that's a different story.
Yes, it's something that I think will need to be added to DM. I haven't got to this as I have not run into an important use case yet. Maybe something like:
Controlled by CONFIG_EVENT
- int dev_ev_register(struct udevice *dev, enum event_t type, event_handler_func_t handler, void *userdata)
which calls handler(struct udevice *dev, void *userdata) when an event is fired
- int dev_ev_unregister() to unregister
- int dev_ev_send(struct udevice *dev, enum struct event_info *info)
which sends events to registered listeners.
struct event_info { enum event_t type; union { struct ev_data_probed probed; struct ev_data_removed removed; ... } d; };
As transitioning period, we could probably implement 2 efi object roots: efi_obj_list as well as the udevice based one. Every piece of code that iterates through devices then just iterates over both. That way we should be able to slowly move devices from the old object model to the new one.
Will the uclass I mentioned above you can iterate through UCLASS_EFI and thus you have a list of EFI devices.
[...]
Regards, Simon
On 1/22/19 8:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
- DM uclasses which support EFI would create a child EFI device (e.g.
a UCLASS_EFI child of each UCLASS_BLK device)
- EFI-uclass devices would thus be bound as needed
- Probing an EFI device would causes its parents to be probed
- We can use all the existing DM hooks (probe, remove, parent/child
data, operations), to implement EFI things
I'm assuming that a small percentage of devices would have EFI children, so that this is more efficient than trying to merge the data structures. It also allows EFI to maintain some separate from the core DM code.
Dear Simon,
thanks to your suggestions.
I am not yet convinced that an UCLASS_EFI child device will be helpful. It is not an EFI object.
A DM uclass is the equivalent to an EFI driver, i.e. a handle with the EFI_DRIVER_BINDING_PROTOCOL installed on it [1]. So the natural thing for a uclass supporting EFI would be to provide such a handle.
For the actual devices we also need handles.
In the EFI world partitions are devices. How does this fit into your picture?
[1] https://lists.denx.de/pipermail/u-boot/2019-January/354359.html [RFC] Device model for block devices - integration with EFI subsystem
Best regards
Heinrich
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK.
Even in the current implementation,
- UEFI disk is implemented using UCLASS_BLK devices (We can ignore !CONFIG_BLK case now as we have agreed.)
- UEFI-specific block device can be seen as UCLASS_BLK/IF_TYPE_EFI
So how essentially different is the *ultimate* goal from the current form regarding block devices?
The ultimate goal is that efi_disk_register() and efi_obj_list disappear.
Functionality wise we should be 100% identical to today, so all test cases would still apply the same way as they do now. This is purely internal rework, nothing visible to UEFI payloads.
Yes.
In order to identify UEFI disks with u-boot devices transparently, we will have to have some sort of *hook* (or hotplug in Alex's language?), either in create_block_devices or bind/probe? I don't know, but Simon seems to be in denial about this idea.
Well, if a udevice *is* an efi device, we no longer need hooks. The object list would simply change.
We may still need to have event notifications at that stage, but that's a different story.
Yes, it's something that I think will need to be added to DM. I haven't got to this as I have not run into an important use case yet. Maybe something like:
Controlled by CONFIG_EVENT
- int dev_ev_register(struct udevice *dev, enum event_t type,
event_handler_func_t handler, void *userdata)
which calls handler(struct udevice *dev, void *userdata) when an event is fired
int dev_ev_unregister() to unregister
int dev_ev_send(struct udevice *dev, enum struct event_info *info)
which sends events to registered listeners.
struct event_info { enum event_t type; union { struct ev_data_probed probed; struct ev_data_removed removed; ... } d; };
As transitioning period, we could probably implement 2 efi object roots: efi_obj_list as well as the udevice based one. Every piece of code that iterates through devices then just iterates over both. That way we should be able to slowly move devices from the old object model to the new one.
Will the uclass I mentioned above you can iterate through UCLASS_EFI and thus you have a list of EFI devices.
[...]
Regards, Simon
On Tue, Jan 22, 2019 at 10:04:55PM +0100, Heinrich Schuchardt wrote:
On 1/22/19 8:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
- DM uclasses which support EFI would create a child EFI device (e.g.
a UCLASS_EFI child of each UCLASS_BLK device)
- EFI-uclass devices would thus be bound as needed
- Probing an EFI device would causes its parents to be probed
- We can use all the existing DM hooks (probe, remove, parent/child
data, operations), to implement EFI things
I'm assuming that a small percentage of devices would have EFI children, so that this is more efficient than trying to merge the data structures. It also allows EFI to maintain some separate from the core DM code.
Dear Simon,
thanks to your suggestions.
I am not yet convinced that an UCLASS_EFI child device will be helpful. It is not an EFI object.
A DM uclass is the equivalent to an EFI driver, i.e. a handle with the EFI_DRIVER_BINDING_PROTOCOL installed on it [1]. So the natural thing for a uclass supporting EFI would be to provide such a handle.
For the actual devices we also need handles.
In the EFI world partitions are devices. How does this fit into your picture?
This is one of my concerns, too. The only solution, I can image, in the *ultimate* goal is that all the partitions of block devices be converted to *real* block devices at probe() or any other enumerating processes, say, "mmc rescan" or "usb start."
This, however, requires an extensive change across all the file system types.
-Takahiro Akashi
[1] https://lists.denx.de/pipermail/u-boot/2019-January/354359.html [RFC] Device model for block devices - integration with EFI subsystem
Best regards
Heinrich
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK.
Even in the current implementation,
- UEFI disk is implemented using UCLASS_BLK devices (We can ignore !CONFIG_BLK case now as we have agreed.)
- UEFI-specific block device can be seen as UCLASS_BLK/IF_TYPE_EFI
So how essentially different is the *ultimate* goal from the current form regarding block devices?
The ultimate goal is that efi_disk_register() and efi_obj_list disappear.
Functionality wise we should be 100% identical to today, so all test cases would still apply the same way as they do now. This is purely internal rework, nothing visible to UEFI payloads.
Yes.
In order to identify UEFI disks with u-boot devices transparently, we will have to have some sort of *hook* (or hotplug in Alex's language?), either in create_block_devices or bind/probe? I don't know, but Simon seems to be in denial about this idea.
Well, if a udevice *is* an efi device, we no longer need hooks. The object list would simply change.
We may still need to have event notifications at that stage, but that's a different story.
Yes, it's something that I think will need to be added to DM. I haven't got to this as I have not run into an important use case yet. Maybe something like:
Controlled by CONFIG_EVENT
- int dev_ev_register(struct udevice *dev, enum event_t type,
event_handler_func_t handler, void *userdata)
which calls handler(struct udevice *dev, void *userdata) when an event is fired
int dev_ev_unregister() to unregister
int dev_ev_send(struct udevice *dev, enum struct event_info *info)
which sends events to registered listeners.
struct event_info { enum event_t type; union { struct ev_data_probed probed; struct ev_data_removed removed; ... } d; };
As transitioning period, we could probably implement 2 efi object roots: efi_obj_list as well as the udevice based one. Every piece of code that iterates through devices then just iterates over both. That way we should be able to slowly move devices from the old object model to the new one.
Will the uclass I mentioned above you can iterate through UCLASS_EFI and thus you have a list of EFI devices.
[...]
Regards, Simon
Hi Heinrich,
On Wed, 23 Jan 2019 at 10:05, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 1/22/19 8:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
- DM uclasses which support EFI would create a child EFI device (e.g.
a UCLASS_EFI child of each UCLASS_BLK device)
- EFI-uclass devices would thus be bound as needed
- Probing an EFI device would causes its parents to be probed
- We can use all the existing DM hooks (probe, remove, parent/child
data, operations), to implement EFI things
I'm assuming that a small percentage of devices would have EFI children, so that this is more efficient than trying to merge the data structures. It also allows EFI to maintain some separate from the core DM code.
Dear Simon,
thanks to your suggestions.
I am not yet convinced that an UCLASS_EFI child device will be helpful. It is not an EFI object.
A DM uclass is the equivalent to an EFI driver, i.e. a handle with the EFI_DRIVER_BINDING_PROTOCOL installed on it [1]. So the natural thing for a uclass supporting EFI would be to provide such a handle.
For the actual devices we also need handles.
Yes but I understand why this is problematic?
In the EFI world partitions are devices. How does this fit into your picture?
Perhaps we could consider adding a UCLASS_PARTITION? The rework of the FS layer may not be too much - at least once we drop the non-BLK code (as you say at [1]).
[1] https://lists.denx.de/pipermail/u-boot/2019-January/354359.html [RFC] Device model for block devices - integration with EFI subsystem
Best regards
Heinrich
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK.
Even in the current implementation,
- UEFI disk is implemented using UCLASS_BLK devices (We can ignore !CONFIG_BLK case now as we have agreed.)
- UEFI-specific block device can be seen as UCLASS_BLK/IF_TYPE_EFI
So how essentially different is the *ultimate* goal from the current form regarding block devices?
The ultimate goal is that efi_disk_register() and efi_obj_list disappear.
Functionality wise we should be 100% identical to today, so all test cases would still apply the same way as they do now. This is purely internal rework, nothing visible to UEFI payloads.
Yes.
In order to identify UEFI disks with u-boot devices transparently, we will have to have some sort of *hook* (or hotplug in Alex's language?), either in create_block_devices or bind/probe? I don't know, but Simon seems to be in denial about this idea.
Well, if a udevice *is* an efi device, we no longer need hooks. The object list would simply change.
We may still need to have event notifications at that stage, but that's a different story.
Yes, it's something that I think will need to be added to DM. I haven't got to this as I have not run into an important use case yet. Maybe something like:
Controlled by CONFIG_EVENT
- int dev_ev_register(struct udevice *dev, enum event_t type,
event_handler_func_t handler, void *userdata)
which calls handler(struct udevice *dev, void *userdata) when an event is fired
int dev_ev_unregister() to unregister
int dev_ev_send(struct udevice *dev, enum struct event_info *info)
which sends events to registered listeners.
struct event_info { enum event_t type; union { struct ev_data_probed probed; struct ev_data_removed removed; ... } d; };
As transitioning period, we could probably implement 2 efi object roots: efi_obj_list as well as the udevice based one. Every piece of code that iterates through devices then just iterates over both. That way we should be able to slowly move devices from the old object model to the new one.
Will the uclass I mentioned above you can iterate through UCLASS_EFI and thus you have a list of EFI devices.
[...]
Regards, Simon
Simon,
On Thu, Jan 24, 2019 at 10:58:53AM +1300, Simon Glass wrote:
Hi Heinrich,
On Wed, 23 Jan 2019 at 10:05, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 1/22/19 8:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote: > Alex, Heinrich and Simon, > > Thank you for your comments, they are all valuable but also make me > confused as different people have different requirements :) > I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
- DM uclasses which support EFI would create a child EFI device (e.g.
a UCLASS_EFI child of each UCLASS_BLK device)
- EFI-uclass devices would thus be bound as needed
- Probing an EFI device would causes its parents to be probed
- We can use all the existing DM hooks (probe, remove, parent/child
data, operations), to implement EFI things
I'm assuming that a small percentage of devices would have EFI children, so that this is more efficient than trying to merge the data structures. It also allows EFI to maintain some separate from the core DM code.
Dear Simon,
thanks to your suggestions.
I am not yet convinced that an UCLASS_EFI child device will be helpful. It is not an EFI object.
A DM uclass is the equivalent to an EFI driver, i.e. a handle with the EFI_DRIVER_BINDING_PROTOCOL installed on it [1]. So the natural thing for a uclass supporting EFI would be to provide such a handle.
For the actual devices we also need handles.
Yes but I understand why this is problematic?
In the EFI world partitions are devices. How does this fit into your picture?
Perhaps we could consider adding a UCLASS_PARTITION? The rework of the FS layer may not be too much - at least once we drop the non-BLK code (as you say at [1]).
IMO, instead of considering UCLASS_PARTITION, 1) add IF_TYPE_BLK_PARTITION and set its type_class_id to UCLASS_BLK So any partition has a parent node (I don't know the correct language in DM world) that is a real raw disk device, and yet is seen as a UCLASS_BLK device, or
2) create a block device (blk_desc) for each partition, either in bind/probe or in enumerating devices, as I mentioned in the previous e-mail
What's different between (1) and (2), we may enumerate block devices and identify the given one by scanning a UCLASS_BLK list with (1), while by scanning a blk_desc list with (2) at do_load()/fs_set_blk_dev().
# In any way, we will need a) a bi-directional link between UCLASS_BLK efi_obj or and or blk_desc efi_disk_obj, and b) a event notification mechanism, in your language, as as to maintain (create/delete) those links
If you agree to approach (1) or (2), I will be able to start a prototyping.
-Takahiro Akashi
[1] https://lists.denx.de/pipermail/u-boot/2019-January/354359.html [RFC] Device model for block devices - integration with EFI subsystem
Best regards
Heinrich
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK.
Even in the current implementation,
- UEFI disk is implemented using UCLASS_BLK devices (We can ignore !CONFIG_BLK case now as we have agreed.)
- UEFI-specific block device can be seen as UCLASS_BLK/IF_TYPE_EFI
So how essentially different is the *ultimate* goal from the current form regarding block devices?
The ultimate goal is that efi_disk_register() and efi_obj_list disappear.
Functionality wise we should be 100% identical to today, so all test cases would still apply the same way as they do now. This is purely internal rework, nothing visible to UEFI payloads.
Yes.
In order to identify UEFI disks with u-boot devices transparently, we will have to have some sort of *hook* (or hotplug in Alex's language?), either in create_block_devices or bind/probe? I don't know, but Simon seems to be in denial about this idea.
Well, if a udevice *is* an efi device, we no longer need hooks. The object list would simply change.
We may still need to have event notifications at that stage, but that's a different story.
Yes, it's something that I think will need to be added to DM. I haven't got to this as I have not run into an important use case yet. Maybe something like:
Controlled by CONFIG_EVENT
- int dev_ev_register(struct udevice *dev, enum event_t type,
event_handler_func_t handler, void *userdata)
which calls handler(struct udevice *dev, void *userdata) when an event is fired
int dev_ev_unregister() to unregister
int dev_ev_send(struct udevice *dev, enum struct event_info *info)
which sends events to registered listeners.
struct event_info { enum event_t type; union { struct ev_data_probed probed; struct ev_data_removed removed; ... } d; };
As transitioning period, we could probably implement 2 efi object roots: efi_obj_list as well as the udevice based one. Every piece of code that iterates through devices then just iterates over both. That way we should be able to slowly move devices from the old object model to the new one.
Will the uclass I mentioned above you can iterate through UCLASS_EFI and thus you have a list of EFI devices.
[...]
Regards, Simon
Hi Takahiro,
On Thu, 24 Jan 2019 at 13:53, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Simon,
On Thu, Jan 24, 2019 at 10:58:53AM +1300, Simon Glass wrote:
Hi Heinrich,
On Wed, 23 Jan 2019 at 10:05, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 1/22/19 8:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote: > > > On 11.01.19 05:29, AKASHI Takahiro wrote: >> Alex, Heinrich and Simon, >> >> Thank you for your comments, they are all valuable but also make me >> confused as different people have different requirements :) >> I'm not sure that all of us share the same *ultimate* goal here. > > The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
- DM uclasses which support EFI would create a child EFI device (e.g.
a UCLASS_EFI child of each UCLASS_BLK device)
- EFI-uclass devices would thus be bound as needed
- Probing an EFI device would causes its parents to be probed
- We can use all the existing DM hooks (probe, remove, parent/child
data, operations), to implement EFI things
I'm assuming that a small percentage of devices would have EFI children, so that this is more efficient than trying to merge the data structures. It also allows EFI to maintain some separate from the core DM code.
Dear Simon,
thanks to your suggestions.
I am not yet convinced that an UCLASS_EFI child device will be helpful. It is not an EFI object.
A DM uclass is the equivalent to an EFI driver, i.e. a handle with the EFI_DRIVER_BINDING_PROTOCOL installed on it [1]. So the natural thing for a uclass supporting EFI would be to provide such a handle.
For the actual devices we also need handles.
Yes but I understand why this is problematic?
In the EFI world partitions are devices. How does this fit into your picture?
Perhaps we could consider adding a UCLASS_PARTITION? The rework of the FS layer may not be too much - at least once we drop the non-BLK code (as you say at [1]).
IMO, instead of considering UCLASS_PARTITION,
- add IF_TYPE_BLK_PARTITION and set its type_class_id to UCLASS_BLK So any partition has a parent node (I don't know the correct language in DM world) that is a real raw disk device, and yet is seen as a UCLASS_BLK device, or
Well 'parent node' is used in device tree. For driver model, we say 'parent device'.
In fact enum if_type is not so necessary in the DM world. We could instead use the uclass of the parent. For example, we could use UCLASS_MMC instead of IF_TYPE_MMC.
So I'm not sure that it is right to set the if_type to UCLASS_BLK. There are many U-Boot commands which number the different 'interface types' separately. For example 'mmc' devices create a block device with blk_desc containing devnum values numbered from 0, as do 'sata' devices, etc. So what does it mean to have a generic 'block' interface type?
- create a block device (blk_desc) for each partition, either in bind/probe or in enumerating devices, as I mentioned in the previous e-mail
Here you are really just creating a device of UCLASS_BLK, since there is a 1:1 correspondence between a UCLASS_BLK device and blk_desc.
struct blk_desc *desc = dev_get_uclass_platdata(blk_dev);
What's different between (1) and (2), we may enumerate block devices and identify the given one by scanning a UCLASS_BLK list with (1), while by scanning a blk_desc list with (2) at do_load()/fs_set_blk_dev().
As above these are really the same, in that a blk_desc can only exist as an attachment to a UCLASS_BLK / block device.
# In any way, we will need a) a bi-directional link between UCLASS_BLK efi_obj or and or blk_desc efi_disk_obj, and b) a event notification mechanism, in your language, as as to maintain (create/delete) those links
If you agree to approach (1) or (2), I will be able to start a prototyping.
I think those two are actually the same, but I suspect I misunderstand something.
If you look at how Unix handles devices versus partitions:
/dev/sdb - whole block device /dev/sdb1 - first partition /dev/sdb2 - second partition
So having the partitions as separate devices at least has precedent. I do think it would work pretty well in U-Boot too.
Another thought might be that we make the block devices be siblings of the 'whole' block device instead of children. Then a SATA device may have 4 children, for example, all UCLASS_BLK, with the first one containing the whole device contents and the other three containing subsets of the first.
But it seems to me that having the partitions be children of a parent BLK device makes more sense, since if you write to the parent you are implicitly writing from one of the children. Also, if you write the partition table, you do that with the parent BLK device, but that will cause the children to be deleted and recreated. Also the operations on a partition are not quite the same as those on a whole device: they can only see a window into the whole device, so will likely add their offset and then call the parent-device operations.
[..]
Regards, Simon
On 1/24/19 9:18 PM, Simon Glass wrote:
Hi Takahiro,
On Thu, 24 Jan 2019 at 13:53, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Simon,
On Thu, Jan 24, 2019 at 10:58:53AM +1300, Simon Glass wrote:
Hi Heinrich,
On Wed, 23 Jan 2019 at 10:05, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 1/22/19 8:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote: > Alex, Simon, > > Apologies for my slow response on this matter, > > On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote: >> >> >> On 11.01.19 05:29, AKASHI Takahiro wrote: >>> Alex, Heinrich and Simon, >>> >>> Thank you for your comments, they are all valuable but also make me >>> confused as different people have different requirements :) >>> I'm not sure that all of us share the same *ultimate* goal here. >> >> The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects. > > I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
- DM uclasses which support EFI would create a child EFI device (e.g.
a UCLASS_EFI child of each UCLASS_BLK device)
- EFI-uclass devices would thus be bound as needed
- Probing an EFI device would causes its parents to be probed
- We can use all the existing DM hooks (probe, remove, parent/child
data, operations), to implement EFI things
I'm assuming that a small percentage of devices would have EFI children, so that this is more efficient than trying to merge the data structures. It also allows EFI to maintain some separate from the core DM code.
Dear Simon,
thanks to your suggestions.
I am not yet convinced that an UCLASS_EFI child device will be helpful. It is not an EFI object.
A DM uclass is the equivalent to an EFI driver, i.e. a handle with the EFI_DRIVER_BINDING_PROTOCOL installed on it [1]. So the natural thing for a uclass supporting EFI would be to provide such a handle.
For the actual devices we also need handles.
Yes but I understand why this is problematic?
In the EFI world partitions are devices. How does this fit into your picture?
Perhaps we could consider adding a UCLASS_PARTITION? The rework of the FS layer may not be too much - at least once we drop the non-BLK code (as you say at [1]).
IMO, instead of considering UCLASS_PARTITION,
- add IF_TYPE_BLK_PARTITION and set its type_class_id to UCLASS_BLK So any partition has a parent node (I don't know the correct language in DM world) that is a real raw disk device, and yet is seen as a UCLASS_BLK device, or
Well 'parent node' is used in device tree. For driver model, we say 'parent device'.
In fact enum if_type is not so necessary in the DM world. We could instead use the uclass of the parent. For example, we could use UCLASS_MMC instead of IF_TYPE_MMC.
So I'm not sure that it is right to set the if_type to UCLASS_BLK. There are many U-Boot commands which number the different 'interface types' separately. For example 'mmc' devices create a block device with blk_desc containing devnum values numbered from 0, as do 'sata' devices, etc. So what does it mean to have a generic 'block' interface type?
- create a block device (blk_desc) for each partition, either in bind/probe or in enumerating devices, as I mentioned in the previous e-mail
Here you are really just creating a device of UCLASS_BLK, since there is a 1:1 correspondence between a UCLASS_BLK device and blk_desc.
struct blk_desc *desc = dev_get_uclass_platdata(blk_dev);
What's different between (1) and (2), we may enumerate block devices and identify the given one by scanning a UCLASS_BLK list with (1), while by scanning a blk_desc list with (2) at do_load()/fs_set_blk_dev().
As above these are really the same, in that a blk_desc can only exist as an attachment to a UCLASS_BLK / block device.
# In any way, we will need a) a bi-directional link between UCLASS_BLK efi_obj or and or blk_desc efi_disk_obj, and b) a event notification mechanism, in your language, as as to maintain (create/delete) those links
If you agree to approach (1) or (2), I will be able to start a prototyping.
I think those two are actually the same, but I suspect I misunderstand something.
If you look at how Unix handles devices versus partitions:
/dev/sdb - whole block device /dev/sdb1 - first partition /dev/sdb2 - second partition
So having the partitions as separate devices at least has precedent. I do think it would work pretty well in U-Boot too.
Another thought might be that we make the block devices be siblings of the 'whole' block device instead of children. Then a SATA device may have 4 children, for example, all UCLASS_BLK, with the first one containing the whole device contents and the other three containing subsets of the first.
There are different types of devices nodes in the UEFI spec.
Examples of messaging devices are Sata, NVMe, eMMC, SD, iSCSI, UsbMassStorage, SAS (serial attached SCSI), Scsi, Ata (ATAPI).
Examples of media devices are HD (partition), CDROM, RamDisk.
So a partition belongs into a different catagory of device then a disk drive.
Here is the output of the EFI shell command `devtree` on a 2018 Lenovo computer:
Ctrl[C1] PciRoot(0x0) Ctrl[1E7] NVM Express Controller Ctrl[225] SAMSUNG MZVLW256HEHP Ctrl[233] FAT File System Ctrl[234] PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/NVMe(0x1,..)/HD(2,GPT,..) Ctrl[235] PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/NVMe(0x1,..)/HD(3,GPT,..) Ctrl[236] PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/NVMe(0x1,..)/HD(4,GPT,..) Ctrl[1F7] AHCI Bus Driver Ctrl[228] Crucial_CT525MX300SSD1 Ctrl[229] FAT File System Ctrl[22A] PciRoot(0x0)/Pci(0x8,0x2)/Pci(0x0,0x0)/Sata(0x0,..)/HD(2,GPT,..) Ctrl[22B] PciRoot(0x0)/Pci(0x8,0x2)/Pci(0x0,0x0)/Sata(0x0,..)/HD(3,GPT,..)
The disk Sata(0x0,0x0,0x0) or NVMe(0x1,<DevUID>) is the parent of its respective partitions. There is no pseudo partition device HD(0, ...) for block I/O to the disk.
On U-Boot I see trees like: root_driver |-- pcie@10000000 | |-- pci_0:0.0 | |-- e1000#0 | `-- ahci_pci | `-- ahci_scsi | `-- ahci_scsi.id0lun0
To keep the U-Boot and the UEFI model in sync we should define partitions as child devices of block devices.
Best regards
Heinrich
But it seems to me that having the partitions be children of a parent BLK device makes more sense, since if you write to the parent you are implicitly writing from one of the children. Also, if you write the partition table, you do that with the parent BLK device, but that will cause the children to be deleted and recreated. Also the operations on a partition are not quite the same as those on a whole device: they can only see a window into the whole device, so will likely add their offset and then call the parent-device operations.
[..]
Regards, Simon
Heinrich,
On Thu, Jan 24, 2019 at 10:19:28PM +0100, Heinrich Schuchardt wrote:
On 1/24/19 9:18 PM, Simon Glass wrote:
Hi Takahiro,
On Thu, 24 Jan 2019 at 13:53, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Simon,
On Thu, Jan 24, 2019 at 10:58:53AM +1300, Simon Glass wrote:
Hi Heinrich,
On Wed, 23 Jan 2019 at 10:05, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 1/22/19 8:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote: > > > > On 22.01.19 09:29, AKASHI Takahiro wrote: >> Alex, Simon, >> >> Apologies for my slow response on this matter, >> >> On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote: >>> >>> >>> On 11.01.19 05:29, AKASHI Takahiro wrote: >>>> Alex, Heinrich and Simon, >>>> >>>> Thank you for your comments, they are all valuable but also make me >>>> confused as different people have different requirements :) >>>> I'm not sure that all of us share the same *ultimate* goal here. >>> >>> The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects. >> >> I don't still understand what "merge" means very well. > > It basically means that "struct efi_object" moves into "struct udevice". > Every udevice instance of type UCLASS_BLK would expose the block and > device_path protocols. > > This will be a slightly bigger rework, but eventually allows us to > basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
- DM uclasses which support EFI would create a child EFI device (e.g.
a UCLASS_EFI child of each UCLASS_BLK device)
- EFI-uclass devices would thus be bound as needed
- Probing an EFI device would causes its parents to be probed
- We can use all the existing DM hooks (probe, remove, parent/child
data, operations), to implement EFI things
I'm assuming that a small percentage of devices would have EFI children, so that this is more efficient than trying to merge the data structures. It also allows EFI to maintain some separate from the core DM code.
Dear Simon,
thanks to your suggestions.
I am not yet convinced that an UCLASS_EFI child device will be helpful. It is not an EFI object.
A DM uclass is the equivalent to an EFI driver, i.e. a handle with the EFI_DRIVER_BINDING_PROTOCOL installed on it [1]. So the natural thing for a uclass supporting EFI would be to provide such a handle.
For the actual devices we also need handles.
Yes but I understand why this is problematic?
In the EFI world partitions are devices. How does this fit into your picture?
Perhaps we could consider adding a UCLASS_PARTITION? The rework of the FS layer may not be too much - at least once we drop the non-BLK code (as you say at [1]).
IMO, instead of considering UCLASS_PARTITION,
- add IF_TYPE_BLK_PARTITION and set its type_class_id to UCLASS_BLK So any partition has a parent node (I don't know the correct language in DM world) that is a real raw disk device, and yet is seen as a UCLASS_BLK device, or
Well 'parent node' is used in device tree. For driver model, we say 'parent device'.
In fact enum if_type is not so necessary in the DM world. We could instead use the uclass of the parent. For example, we could use UCLASS_MMC instead of IF_TYPE_MMC.
So I'm not sure that it is right to set the if_type to UCLASS_BLK. There are many U-Boot commands which number the different 'interface types' separately. For example 'mmc' devices create a block device with blk_desc containing devnum values numbered from 0, as do 'sata' devices, etc. So what does it mean to have a generic 'block' interface type?
- create a block device (blk_desc) for each partition, either in bind/probe or in enumerating devices, as I mentioned in the previous e-mail
Here you are really just creating a device of UCLASS_BLK, since there is a 1:1 correspondence between a UCLASS_BLK device and blk_desc.
struct blk_desc *desc = dev_get_uclass_platdata(blk_dev);
What's different between (1) and (2), we may enumerate block devices and identify the given one by scanning a UCLASS_BLK list with (1), while by scanning a blk_desc list with (2) at do_load()/fs_set_blk_dev().
As above these are really the same, in that a blk_desc can only exist as an attachment to a UCLASS_BLK / block device.
# In any way, we will need a) a bi-directional link between UCLASS_BLK efi_obj or and or blk_desc efi_disk_obj, and b) a event notification mechanism, in your language, as as to maintain (create/delete) those links
If you agree to approach (1) or (2), I will be able to start a prototyping.
I think those two are actually the same, but I suspect I misunderstand something.
If you look at how Unix handles devices versus partitions:
/dev/sdb - whole block device /dev/sdb1 - first partition /dev/sdb2 - second partition
So having the partitions as separate devices at least has precedent. I do think it would work pretty well in U-Boot too.
Another thought might be that we make the block devices be siblings of the 'whole' block device instead of children. Then a SATA device may have 4 children, for example, all UCLASS_BLK, with the first one containing the whole device contents and the other three containing subsets of the first.
There are different types of devices nodes in the UEFI spec.
I think that you're talking things in a mixed (and confusing) language here. You seems to use "device" as either "device path (protocol)" and "block io (protocol)" unintentionally.
Examples of messaging devices are Sata, NVMe, eMMC, SD, iSCSI, UsbMassStorage, SAS (serial attached SCSI), Scsi, Ata (ATAPI).
devices -> device paths
Examples of media devices are HD (partition), CDROM, RamDisk.
ditto
So a partition belongs into a different catagory of device then a disk drive.
Wait. Why "so?" What do you mean "disk drive?"
"Messaging device path" is nothing but a location in a specific-type of device interface that will connect the parent (probably "bus") and the attached child (probably a device). For example, Scsi(0,0) means a location of device with PUN=0 and LUN=0.
It will never tell us what device type (block device or whatever else) it is.
Here is the output of the EFI shell command `devtree` on a 2018 Lenovo computer:
Ctrl[C1] PciRoot(0x0) Ctrl[1E7] NVM Express Controller Ctrl[225] SAMSUNG MZVLW256HEHP Ctrl[233] FAT File System Ctrl[234] PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/NVMe(0x1,..)/HD(2,GPT,..) Ctrl[235] PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/NVMe(0x1,..)/HD(3,GPT,..) Ctrl[236] PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/NVMe(0x1,..)/HD(4,GPT,..) Ctrl[1F7] AHCI Bus Driver Ctrl[228] Crucial_CT525MX300SSD1 Ctrl[229] FAT File System Ctrl[22A] PciRoot(0x0)/Pci(0x8,0x2)/Pci(0x0,0x0)/Sata(0x0,..)/HD(2,GPT,..) Ctrl[22B] PciRoot(0x0)/Pci(0x8,0x2)/Pci(0x0,0x0)/Sata(0x0,..)/HD(3,GPT,..)
The disk Sata(0x0,0x0,0x0) or NVMe(0x1,<DevUID>) is the parent of its respective partitions. There is no pseudo partition device HD(0, ...) for block I/O to the disk.
I don't get your point. What do you mean by "pseudo partition device for block I/O?"
Are you saying that a device represented by "sata(0x0,0x0,0x0)" will not be able to be accessed via block io protocol?
I don't think so. UEFI spec v2.7, section 10.4.5 says, "An EFI_BLOCK_IO_PROTOCOL is produced for both raw devices and partitions on devices."
It is also true in u-boot UEFI implementation: (I have two scsi disks here. Scsi(1,0) has two partitions.) => efi devices Scanning disk ahci_scsi.id0lun0... Scanning disk ahci_scsi.id1lun0... Found 4 disks Device Device Path ================ ==================== 000000007ef0afd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) 000000007ef0bb80 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0) 000000007ef0bf50 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0) 000000007ef0c090 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000) 000000007ef0c2c0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => efi dh Handle Protocols ================ ==================== 000000007ef0afd0 Device Path, Device Path To Text, Device Path Utilities, Unicode Collation 2 000000007ef00b50 Driver Binding 000000007ef00bc0 Simple Text Output 000000007ef00c30 Simple Text Input, Simple Text Input Ex 000000007ef0bb80 Block IO, Device Path 000000007ef0bf50 Block IO, Device Path 000000007ef0c090 Block IO, Device Path, Simple File System 000000007ef0c2c0 Block IO, Device Path, Simple File System
On U-Boot I see trees like: root_driver |-- pcie@10000000 | |-- pci_0:0.0 | |-- e1000#0 | `-- ahci_pci | `-- ahci_scsi | `-- ahci_scsi.id0lun0
To keep the U-Boot and the UEFI model in sync we should define partitions as child devices of block devices.
What type of UCLASS do you suggest for a child device here, UCLASS_PARTITION or UCLASS_BLK?
-Takahiro Akashi
Heinrich
But it seems to me that having the partitions be children of a parent BLK device makes more sense, since if you write to the parent you are implicitly writing from one of the children. Also, if you write the partition table, you do that with the parent BLK device, but that will cause the children to be deleted and recreated. Also the operations on a partition are not quite the same as those on a whole device: they can only see a window into the whole device, so will likely add their offset and then call the parent-device operations.
[..]
Regards, Simon
On 01/22/2019 08:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
... and then we need to create our own sub object model around the UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I really see little reason not to just expose every dm device as EFI handle. Things would plug in quite naturally I think.
But either way, someone would need to sit down and prototype things to be sure.
Alex
Hi Alex,
On Wed, 23 Jan 2019 at 22:51, Alexander Graf agraf@suse.de wrote:
On 01/22/2019 08:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
... and then we need to create our own sub object model around the UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I really see little reason not to just expose every dm device as EFI handle. Things would plug in quite naturally I think.
I would like to keep some separation between EFI and DM data structures. We can maintain a list of pointers or whatever is needed for the protocol stuff.
I'm not convinced either way also, and agree that:
But either way, someone would need to sit down and prototype things to be sure.
I think a reasonable starting point would be to create a PARTITION uclass and rework things to deal with that.
Alex
Regards, Simon
Alex,
On Wed, Jan 23, 2019 at 10:51:29AM +0100, Alexander Graf wrote:
On 01/22/2019 08:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
... and then we need to create our own sub object model around the UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I really see little reason not to just expose every dm device as EFI handle. Things would plug in quite naturally I think.
You said that the ultimate goal is to remove all efi_object data. Do you think that all the existing efi_object can be mapped to one of existing u-boot uclass devices?
If so, what would be an real entity of a UEFI handle? struct udevice *?
But Simon seems not to agree to adding any UEFI-specific members in struct udevice.
But either way, someone would need to sit down and prototype things to be sure.
The most simplest prototype would include * event mechanism (just registration and execution of hook/handler) event: udevice creation (and deletion) * efi_disk hook for udevice(UCLASS_BLK) creation * modified block device's enumeration code, say, scsi_scan(), to add an event hook at udevice creation * removing efi_disk_register() from efi_init_obj_list() * Optionally(?) UCLASS_PARTITION (Partition udevices would be created in part_init().)
?
Thanks, -Takahiro Akashi
Alex
On 25.01.19 09:27, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 23, 2019 at 10:51:29AM +0100, Alexander Graf wrote:
On 01/22/2019 08:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote: > Alex, Heinrich and Simon, > > Thank you for your comments, they are all valuable but also make me > confused as different people have different requirements :) > I'm not sure that all of us share the same *ultimate* goal here. The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
... and then we need to create our own sub object model around the UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I really see little reason not to just expose every dm device as EFI handle. Things would plug in quite naturally I think.
You said that the ultimate goal is to remove all efi_object data. Do you think that all the existing efi_object can be mapped to one of existing u-boot uclass devices?
If so, what would be an real entity of a UEFI handle? struct udevice *?
But Simon seems not to agree to adding any UEFI-specific members in struct udevice.
I think we'll have to experiment with both approaches. I personally would like to have struct udevice * be the UEFI handle, yes.
But either way, someone would need to sit down and prototype things to be sure.
The most simplest prototype would include
- event mechanism (just registration and execution of hook/handler) event: udevice creation (and deletion)
- efi_disk hook for udevice(UCLASS_BLK) creation
- modified block device's enumeration code, say, scsi_scan(), to add an event hook at udevice creation
- removing efi_disk_register() from efi_init_obj_list()
- Optionally(?) UCLASS_PARTITION (Partition udevices would be created in part_init().)
Almost.
The simplest prototype would be to add a struct efi_object into struct udevice. Then whenever we're looping over efi_obj_list in the code, we additionally loop over all udevices to find the handle.
Then, we could slowly give the uclasses explicit knowledge of uefi protocols. So most of the logic of efi_disk_register() would move into (or get called by) drivers/block/blk-uclass.c:blk_create_device().
Instead of creating diskobj and adding calling efi_add_handle(), we could then just use existing data structure from the udevice (and its platdata).
Does this make sense? Less events, more implicity :).
Alex
On Fri, Jan 25, 2019 at 09:52:31AM +0100, Alexander Graf wrote:
On 25.01.19 09:27, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 23, 2019 at 10:51:29AM +0100, Alexander Graf wrote:
On 01/22/2019 08:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote: > > On 11.01.19 05:29, AKASHI Takahiro wrote: >> Alex, Heinrich and Simon, >> >> Thank you for your comments, they are all valuable but also make me >> confused as different people have different requirements :) >> I'm not sure that all of us share the same *ultimate* goal here. > The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects. I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
... and then we need to create our own sub object model around the UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I really see little reason not to just expose every dm device as EFI handle. Things would plug in quite naturally I think.
You said that the ultimate goal is to remove all efi_object data. Do you think that all the existing efi_object can be mapped to one of existing u-boot uclass devices?
If so, what would be an real entity of a UEFI handle? struct udevice *?
But Simon seems not to agree to adding any UEFI-specific members in struct udevice.
I think we'll have to experiment with both approaches. I personally would like to have struct udevice * be the UEFI handle, yes.
But either way, someone would need to sit down and prototype things to be sure.
The most simplest prototype would include
- event mechanism (just registration and execution of hook/handler) event: udevice creation (and deletion)
- efi_disk hook for udevice(UCLASS_BLK) creation
- modified block device's enumeration code, say, scsi_scan(), to add an event hook at udevice creation
- removing efi_disk_register() from efi_init_obj_list()
- Optionally(?) UCLASS_PARTITION (Partition udevices would be created in part_init().)
Almost.
The simplest prototype would be to add a struct efi_object into struct udevice. Then whenever we're looping over efi_obj_list in the code, we additionally loop over all udevices to find the handle.
Ah, yes. You're going further :)
Then, we could slowly give the uclasses explicit knowledge of uefi protocols. So most of the logic of efi_disk_register() would move into (or get called by) drivers/block/blk-uclass.c:blk_create_device().
Via event? Otherwise, we cannot decouple u-boot and UEFI world.
Instead of creating diskobj and adding calling efi_add_handle(), we could then just use existing data structure from the udevice (and its platdata).
I don't have good confidence that we can remove struct efi_disk_obj, at least, for the time being as some of its members are quite UEFI-specific.
Does this make sense? Less events, more implicity :).
I'll go for it.
Thanks, -Takahiro Akashi
Alex
On 25.01.19 10:18, AKASHI Takahiro wrote:
On Fri, Jan 25, 2019 at 09:52:31AM +0100, Alexander Graf wrote:
On 25.01.19 09:27, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 23, 2019 at 10:51:29AM +0100, Alexander Graf wrote:
On 01/22/2019 08:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote: > Alex, Simon, > > Apologies for my slow response on this matter, > > On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote: >> >> On 11.01.19 05:29, AKASHI Takahiro wrote: >>> Alex, Heinrich and Simon, >>> >>> Thank you for your comments, they are all valuable but also make me >>> confused as different people have different requirements :) >>> I'm not sure that all of us share the same *ultimate* goal here. >> The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects. > I don't still understand what "merge" means very well. It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
... and then we need to create our own sub object model around the UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I really see little reason not to just expose every dm device as EFI handle. Things would plug in quite naturally I think.
You said that the ultimate goal is to remove all efi_object data. Do you think that all the existing efi_object can be mapped to one of existing u-boot uclass devices?
If so, what would be an real entity of a UEFI handle? struct udevice *?
But Simon seems not to agree to adding any UEFI-specific members in struct udevice.
I think we'll have to experiment with both approaches. I personally would like to have struct udevice * be the UEFI handle, yes.
But either way, someone would need to sit down and prototype things to be sure.
The most simplest prototype would include
- event mechanism (just registration and execution of hook/handler) event: udevice creation (and deletion)
- efi_disk hook for udevice(UCLASS_BLK) creation
- modified block device's enumeration code, say, scsi_scan(), to add an event hook at udevice creation
- removing efi_disk_register() from efi_init_obj_list()
- Optionally(?) UCLASS_PARTITION (Partition udevices would be created in part_init().)
Almost.
The simplest prototype would be to add a struct efi_object into struct udevice. Then whenever we're looping over efi_obj_list in the code, we additionally loop over all udevices to find the handle.
Ah, yes. You're going further :)
Then, we could slowly give the uclasses explicit knowledge of uefi protocols. So most of the logic of efi_disk_register() would move into (or get called by) drivers/block/blk-uclass.c:blk_create_device().
Via event? Otherwise, we cannot decouple u-boot and UEFI world.
For a prototype, just make it explicit and see how far that gets us.
Instead of creating diskobj and adding calling efi_add_handle(), we could then just use existing data structure from the udevice (and its platdata).
I don't have good confidence that we can remove struct efi_disk_obj, at least, for the time being as some of its members are quite UEFI-specific.
Maybe we can move them into struct blk_desc? It's a matter of experimenting I guess.
Does this make sense? Less events, more implicity :).
I'll go for it.
Thanks a lot :). Feel free to pick an easier target for starters too if you prefer.
Alex
On Fri, Jan 25, 2019 at 10:31:20AM +0100, Alexander Graf wrote:
On 25.01.19 10:18, AKASHI Takahiro wrote:
On Fri, Jan 25, 2019 at 09:52:31AM +0100, Alexander Graf wrote:
On 25.01.19 09:27, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 23, 2019 at 10:51:29AM +0100, Alexander Graf wrote:
On 01/22/2019 08:39 PM, Simon Glass wrote:
Hi Alex,
On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote: > > > On 22.01.19 09:29, AKASHI Takahiro wrote: >> Alex, Simon, >> >> Apologies for my slow response on this matter, >> >> On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote: >>> >>> On 11.01.19 05:29, AKASHI Takahiro wrote: >>>> Alex, Heinrich and Simon, >>>> >>>> Thank you for your comments, they are all valuable but also make me >>>> confused as different people have different requirements :) >>>> I'm not sure that all of us share the same *ultimate* goal here. >>> The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects. >> I don't still understand what "merge" means very well. > It basically means that "struct efi_object" moves into "struct udevice". > Every udevice instance of type UCLASS_BLK would expose the block and > device_path protocols. > > This will be a slightly bigger rework, but eventually allows us to > basically get rid of efi_init_obj_list() I think. I envisaged something like:
- EFI objects have their own UCLASS_EFI uclass
... and then we need to create our own sub object model around the UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I really see little reason not to just expose every dm device as EFI handle. Things would plug in quite naturally I think.
You said that the ultimate goal is to remove all efi_object data. Do you think that all the existing efi_object can be mapped to one of existing u-boot uclass devices?
If so, what would be an real entity of a UEFI handle? struct udevice *?
But Simon seems not to agree to adding any UEFI-specific members in struct udevice.
I think we'll have to experiment with both approaches. I personally would like to have struct udevice * be the UEFI handle, yes.
But either way, someone would need to sit down and prototype things to be sure.
The most simplest prototype would include
- event mechanism (just registration and execution of hook/handler) event: udevice creation (and deletion)
- efi_disk hook for udevice(UCLASS_BLK) creation
- modified block device's enumeration code, say, scsi_scan(), to add an event hook at udevice creation
- removing efi_disk_register() from efi_init_obj_list()
- Optionally(?) UCLASS_PARTITION (Partition udevices would be created in part_init().)
Almost.
The simplest prototype would be to add a struct efi_object into struct udevice. Then whenever we're looping over efi_obj_list in the code, we additionally loop over all udevices to find the handle.
Ah, yes. You're going further :)
Then, we could slowly give the uclasses explicit knowledge of uefi protocols. So most of the logic of efi_disk_register() would move into (or get called by) drivers/block/blk-uclass.c:blk_create_device().
Via event? Otherwise, we cannot decouple u-boot and UEFI world.
For a prototype, just make it explicit and see how far that gets us.
Instead of creating diskobj and adding calling efi_add_handle(), we could then just use existing data structure from the udevice (and its platdata).
I don't have good confidence that we can remove struct efi_disk_obj, at least, for the time being as some of its members are quite UEFI-specific.
Maybe we can move them into struct blk_desc? It's a matter of experimenting I guess.
Does this make sense? Less events, more implicity :).
I'll go for it.
Thanks a lot :). Feel free to pick an easier target for starters too if you prefer.
Prototyping is done :) Since it was so easy and simple, now I'm thinking of implementing UCLASS_PARTITION. But it is not so straightforward as I expected, and it won't bring us lots of advantages. (I think that blk_desc should also support a partition in its own.)
Once it gets working, may I send out a patch?
-Takahiro Akashi
Alex
Am 28.01.2019 um 09:56 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Fri, Jan 25, 2019 at 10:31:20AM +0100, Alexander Graf wrote:
On 25.01.19 10:18, AKASHI Takahiro wrote:
On Fri, Jan 25, 2019 at 09:52:31AM +0100, Alexander Graf wrote:
On 25.01.19 09:27, AKASHI Takahiro wrote: Alex,
On Wed, Jan 23, 2019 at 10:51:29AM +0100, Alexander Graf wrote: > On 01/22/2019 08:39 PM, Simon Glass wrote: > Hi Alex, > >> On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote: >> >> >>> On 22.01.19 09:29, AKASHI Takahiro wrote: >>> Alex, Simon, >>> >>> Apologies for my slow response on this matter, >>> >>>> On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote: >>>> >>>>> On 11.01.19 05:29, AKASHI Takahiro wrote: >>>>> Alex, Heinrich and Simon, >>>>> >>>>> Thank you for your comments, they are all valuable but also make me >>>>> confused as different people have different requirements :) >>>>> I'm not sure that all of us share the same *ultimate* goal here. >>>> The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects. >>> I don't still understand what "merge" means very well. >> It basically means that "struct efi_object" moves into "struct udevice". >> Every udevice instance of type UCLASS_BLK would expose the block and >> device_path protocols. >> >> This will be a slightly bigger rework, but eventually allows us to >> basically get rid of efi_init_obj_list() I think. > I envisaged something like: > > - EFI objects have their own UCLASS_EFI uclass
... and then we need to create our own sub object model around the UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I really see little reason not to just expose every dm device as EFI handle. Things would plug in quite naturally I think.
You said that the ultimate goal is to remove all efi_object data. Do you think that all the existing efi_object can be mapped to one of existing u-boot uclass devices?
If so, what would be an real entity of a UEFI handle? struct udevice *?
But Simon seems not to agree to adding any UEFI-specific members in struct udevice.
I think we'll have to experiment with both approaches. I personally would like to have struct udevice * be the UEFI handle, yes.
But either way, someone would need to sit down and prototype things to be sure.
The most simplest prototype would include
- event mechanism (just registration and execution of hook/handler) event: udevice creation (and deletion)
- efi_disk hook for udevice(UCLASS_BLK) creation
- modified block device's enumeration code, say, scsi_scan(),
to add an event hook at udevice creation
- removing efi_disk_register() from efi_init_obj_list()
- Optionally(?) UCLASS_PARTITION
(Partition udevices would be created in part_init().)
Almost.
The simplest prototype would be to add a struct efi_object into struct udevice. Then whenever we're looping over efi_obj_list in the code, we additionally loop over all udevices to find the handle.
Ah, yes. You're going further :)
Then, we could slowly give the uclasses explicit knowledge of uefi protocols. So most of the logic of efi_disk_register() would move into (or get called by) drivers/block/blk-uclass.c:blk_create_device().
Via event? Otherwise, we cannot decouple u-boot and UEFI world.
For a prototype, just make it explicit and see how far that gets us.
Instead of creating diskobj and adding calling efi_add_handle(), we could then just use existing data structure from the udevice (and its platdata).
I don't have good confidence that we can remove struct efi_disk_obj, at least, for the time being as some of its members are quite UEFI-specific.
Maybe we can move them into struct blk_desc? It's a matter of experimenting I guess.
Does this make sense? Less events, more implicity :).
I'll go for it.
Thanks a lot :). Feel free to pick an easier target for starters too if you prefer.
Prototyping is done :) Since it was so easy and simple, now I'm thinking of implementing UCLASS_PARTITION. But it is not so straightforward as I expected, and it won't bring us lots of advantages. (I think that blk_desc should also support a partition in its own.)
Once it gets working, may I send out a patch?
Feel free to even just send a patch of what you have now as RFC, so that we can see if this looks like the right direction. Let's make use of our time zone differences :).
Alex
Hi,
On Mon, 28 Jan 2019 at 01:55, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Jan 25, 2019 at 10:31:20AM +0100, Alexander Graf wrote:
On 25.01.19 10:18, AKASHI Takahiro wrote:
On Fri, Jan 25, 2019 at 09:52:31AM +0100, Alexander Graf wrote:
On 25.01.19 09:27, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 23, 2019 at 10:51:29AM +0100, Alexander Graf wrote:
On 01/22/2019 08:39 PM, Simon Glass wrote: > Hi Alex, > > On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote: >> >> >> On 22.01.19 09:29, AKASHI Takahiro wrote: >>> Alex, Simon, >>> >>> Apologies for my slow response on this matter, >>> >>> On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote: >>>> >>>> On 11.01.19 05:29, AKASHI Takahiro wrote: >>>>> Alex, Heinrich and Simon, >>>>> >>>>> Thank you for your comments, they are all valuable but also make me >>>>> confused as different people have different requirements :) >>>>> I'm not sure that all of us share the same *ultimate* goal here. >>>> The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects. >>> I don't still understand what "merge" means very well. >> It basically means that "struct efi_object" moves into "struct udevice". >> Every udevice instance of type UCLASS_BLK would expose the block and >> device_path protocols. >> >> This will be a slightly bigger rework, but eventually allows us to >> basically get rid of efi_init_obj_list() I think. > I envisaged something like: > > - EFI objects have their own UCLASS_EFI uclass
... and then we need to create our own sub object model around the UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I really see little reason not to just expose every dm device as EFI handle. Things would plug in quite naturally I think.
You said that the ultimate goal is to remove all efi_object data. Do you think that all the existing efi_object can be mapped to one of existing u-boot uclass devices?
If so, what would be an real entity of a UEFI handle? struct udevice *?
But Simon seems not to agree to adding any UEFI-specific members in struct udevice.
I think we'll have to experiment with both approaches. I personally would like to have struct udevice * be the UEFI handle, yes.
But either way, someone would need to sit down and prototype things to be sure.
The most simplest prototype would include
- event mechanism (just registration and execution of hook/handler) event: udevice creation (and deletion)
- efi_disk hook for udevice(UCLASS_BLK) creation
- modified block device's enumeration code, say, scsi_scan(), to add an event hook at udevice creation
- removing efi_disk_register() from efi_init_obj_list()
- Optionally(?) UCLASS_PARTITION (Partition udevices would be created in part_init().)
Almost.
The simplest prototype would be to add a struct efi_object into struct udevice. Then whenever we're looping over efi_obj_list in the code, we additionally loop over all udevices to find the handle.
Ah, yes. You're going further :)
Then, we could slowly give the uclasses explicit knowledge of uefi protocols. So most of the logic of efi_disk_register() would move into (or get called by) drivers/block/blk-uclass.c:blk_create_device().
Via event? Otherwise, we cannot decouple u-boot and UEFI world.
For a prototype, just make it explicit and see how far that gets us.
Instead of creating diskobj and adding calling efi_add_handle(), we could then just use existing data structure from the udevice (and its platdata).
I don't have good confidence that we can remove struct efi_disk_obj, at least, for the time being as some of its members are quite UEFI-specific.
Maybe we can move them into struct blk_desc? It's a matter of experimenting I guess.
Does this make sense? Less events, more implicity :).
I'll go for it.
Thanks a lot :). Feel free to pick an easier target for starters too if you prefer.
Prototyping is done :) Since it was so easy and simple, now I'm thinking of implementing UCLASS_PARTITION. But it is not so straightforward as I expected, and it won't bring us lots of advantages. (I think that blk_desc should also support a partition in its own.)
blk_desc is in UCLASS_BLK. So we already support partitions within blk_desc. Can you expand a bit on what you mean?
Once it gets working, may I send out a patch?
Yes indeed.
Regards, Simon
-Takahiro Akashi
Alex
Simon,
On Mon, Jan 28, 2019 at 05:46:21PM -0700, Simon Glass wrote:
Hi,
On Mon, 28 Jan 2019 at 01:55, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Fri, Jan 25, 2019 at 10:31:20AM +0100, Alexander Graf wrote:
On 25.01.19 10:18, AKASHI Takahiro wrote:
On Fri, Jan 25, 2019 at 09:52:31AM +0100, Alexander Graf wrote:
On 25.01.19 09:27, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 23, 2019 at 10:51:29AM +0100, Alexander Graf wrote: > On 01/22/2019 08:39 PM, Simon Glass wrote: >> Hi Alex, >> >> On Tue, 22 Jan 2019 at 22:08, Alexander Graf agraf@suse.de wrote: >>> >>> >>> On 22.01.19 09:29, AKASHI Takahiro wrote: >>>> Alex, Simon, >>>> >>>> Apologies for my slow response on this matter, >>>> >>>> On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote: >>>>> >>>>> On 11.01.19 05:29, AKASHI Takahiro wrote: >>>>>> Alex, Heinrich and Simon, >>>>>> >>>>>> Thank you for your comments, they are all valuable but also make me >>>>>> confused as different people have different requirements :) >>>>>> I'm not sure that all of us share the same *ultimate* goal here. >>>>> The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects. >>>> I don't still understand what "merge" means very well. >>> It basically means that "struct efi_object" moves into "struct udevice". >>> Every udevice instance of type UCLASS_BLK would expose the block and >>> device_path protocols. >>> >>> This will be a slightly bigger rework, but eventually allows us to >>> basically get rid of efi_init_obj_list() I think. >> I envisaged something like: >> >> - EFI objects have their own UCLASS_EFI uclass > > ... and then we need to create our own sub object model around the > UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I > really see little reason not to just expose every dm device as EFI handle. > Things would plug in quite naturally I think.
You said that the ultimate goal is to remove all efi_object data. Do you think that all the existing efi_object can be mapped to one of existing u-boot uclass devices?
If so, what would be an real entity of a UEFI handle? struct udevice *?
But Simon seems not to agree to adding any UEFI-specific members in struct udevice.
I think we'll have to experiment with both approaches. I personally would like to have struct udevice * be the UEFI handle, yes.
> But either way, someone would need to sit down and prototype things to be > sure. >
The most simplest prototype would include
- event mechanism (just registration and execution of hook/handler) event: udevice creation (and deletion)
- efi_disk hook for udevice(UCLASS_BLK) creation
- modified block device's enumeration code, say, scsi_scan(), to add an event hook at udevice creation
- removing efi_disk_register() from efi_init_obj_list()
- Optionally(?) UCLASS_PARTITION (Partition udevices would be created in part_init().)
Almost.
The simplest prototype would be to add a struct efi_object into struct udevice. Then whenever we're looping over efi_obj_list in the code, we additionally loop over all udevices to find the handle.
Ah, yes. You're going further :)
Then, we could slowly give the uclasses explicit knowledge of uefi protocols. So most of the logic of efi_disk_register() would move into (or get called by) drivers/block/blk-uclass.c:blk_create_device().
Via event? Otherwise, we cannot decouple u-boot and UEFI world.
For a prototype, just make it explicit and see how far that gets us.
Instead of creating diskobj and adding calling efi_add_handle(), we could then just use existing data structure from the udevice (and its platdata).
I don't have good confidence that we can remove struct efi_disk_obj, at least, for the time being as some of its members are quite UEFI-specific.
Maybe we can move them into struct blk_desc? It's a matter of experimenting I guess.
Does this make sense? Less events, more implicity :).
I'll go for it.
Thanks a lot :). Feel free to pick an easier target for starters too if you prefer.
Prototyping is done :) Since it was so easy and simple, now I'm thinking of implementing UCLASS_PARTITION. But it is not so straightforward as I expected, and it won't bring us lots of advantages. (I think that blk_desc should also support a partition in its own.)
blk_desc is in UCLASS_BLK. So we already support partitions within blk_desc. Can you expand a bit on what you mean?
This is partly because "efi_disk_obj" structure is embedded in blk_desc for now. So for UCLASS_PARTITION, we need a dummy blk_desc.
Once it gets working, may I send out a patch?
Yes indeed.
Shortly. We will continue to discuss after my patch is released.
Thanks, -Takahiro Akashi
Regards, Simon
-Takahiro Akashi
Alex
Alex,
On Tue, Jan 22, 2019 at 10:08:53AM +0100, Alexander Graf wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK.
Even in the current implementation,
- UEFI disk is implemented using UCLASS_BLK devices (We can ignore !CONFIG_BLK case now as we have agreed.)
- UEFI-specific block device can be seen as UCLASS_BLK/IF_TYPE_EFI
So how essentially different is the *ultimate* goal from the current form regarding block devices?
The ultimate goal is that efi_disk_register() and efi_obj_list disappear.
Functionality wise we should be 100% identical to today, so all test cases would still apply the same way as they do now. This is purely internal rework, nothing visible to UEFI payloads.
In order to identify UEFI disks with u-boot devices transparently, we will have to have some sort of *hook* (or hotplug in Alex's language?), either in create_block_devices or bind/probe? I don't know, but Simon seems to be in denial about this idea.
Well, if a udevice *is* an efi device, we no longer need hooks. The object list would simply change.
We may still need to have event notifications at that stage, but that's a different story.
As transitioning period, we could probably implement 2 efi object roots: efi_obj_list as well as the udevice based one. Every piece of code that iterates through devices then just iterates over both. That way we should be able to slowly move devices from the old object model to the new one.
Then provide a DM path and have a non-DM fallback still in its own source file that also provides EFI BLK devices. Eventually we just remove the latter.
That way we can then work on getting hotplug working in the DM path, which is the one we want anyway. For non-DM, you simply miss out on that amazing new feature, but we don't regress users.
So, first, let me reply to each of your comments. Through this process, I hope we will have better understandings of long-term solution as well as a tentative fix.
On Thu, Jan 10, 2019 at 10:22:04AM +0100, Alexander Graf wrote:
Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
> On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote: > > >>> Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org: >>> >>> On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote: >>> >>> >>>> On 10.01.19 08:26, AKASHI Takahiro wrote: >>>> Alex, >>>> >>>>> On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote: >>>>> >>>>> >>>>>> On 10.01.19 03:13, AKASHI Takahiro wrote: >>>>>> Alex, >>>>>> >>>>>>> On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote: >>>>>>> >>>>>>> >>>>>>>> On 13.12.18 08:58, AKASHI Takahiro wrote: >>>>>>>> Heinrich, >>>>>>>> >>>>>>>>>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>>>>>>>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>>>>>>>>> Currently, efi_init_obj_list() scan disk devices only once, and never >>>>>>>>>> change a list of efi disk devices. This will possibly result in failing >>>>>>>>>> to find a removable storage which may be added later on. See [1]. >>>>>>>>>> >>>>>>>>>> In this patch, called is efi_disk_update() which is responsible for >>>>>>>>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>>>>>>>>> >>>>>>>>>> For example, >>>>>>>>>> >>>>>>>>>> => efishell devices >>>>>>>>>> Scanning disk pci_mmc.blk... >>>>>>>>>> Found 3 disks >>>>>>>>>> Device Name >>>>>>>>>> ============================================ >>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>>>>> => usb start >>>>>>>>>> starting USB... >>>>>>>>>> USB0: USB EHCI 1.00 >>>>>>>>>> scanning bus 0 for devices... 3 USB Device(s) found >>>>>>>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>>>>>>> => efishell devices >>>>>>>>>> Scanning disk usb_mass_storage.lun0... >>>>>>>>>> Device Name >>>>>>>>>> ============================================ >>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>>>>>>>>> >>>>>>>>>> Without this patch, the last device, USB mass storage, won't show up. >>>>>>>>>> >>>>>>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>>>>>>> >>>>>>>>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>>>>>>> >>>>>>>>> Why should we try to fix something in the EFI subsystems that goes wrong >>>>>>>>> in the handling of device enumeration. >>>>>>>> >>>>>>>> No. >>>>>>>> This is a natural result from how efi disks are currently implemented on u-boot. >>>>>>>> Do you want to totally re-write/re-implement efi disks? >>>>>>> >>>>>>> Could we just make this event based for now? Call a hook from the >>>>>>> storage dm subsystem when a new u-boot block device gets created to >>>>>>> issue a sync of that in the efi subsystem? >>>>>> >>>>>> If I correctly understand you, your suggestion here corresponds >>>>>> with my proposal#3 in [1] while my current approach is #2. >>>>>> >>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>> >>>>> Yes, I think so. >>>>> >>>>>> So we will call, say, efi_disk_create(struct udevice *) in >>>>>> blk_create_device() and efi_dsik_delete() in blk_unbind_all(). >>>>> >>>>> I would prefer if we didn't call them directly, but through an event >>>>> mechanism. So the efi_disk subsystem registers an event with the dm >>>>> block subsystem and that will just call all events when block devices >>>>> get created which will automatically also include the efi disk creation >>>>> callback. Same for reverse. >>>> >>>> Do you mean efi event by "event?" >>>> (I don't think there is any generic event interface on DM side.) >>>> >>>> Whatever an "event" is or whether we call efi_disk_create() directly >>>> or indirectly via an event, there is one (big?) issue in this approach >>>> (while I've almost finished prototyping): >>>> >>>> We cannot call efi_disk_create() within blk_create_device() because >>>> some data fields of struct blk_desc, which are to be used by efi disk, >>>> are initialized *after* blk_create_device() in driver side. >>>> >>>> So we need to add a hook at/after every occurrence of blk_create_device() >>>> on driver side. For example, >>>> >>>> === drivers/scsi/scsi.c === >>>> int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) >>>> { >>>> ... >>>> ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, >>>> bd.blksz, bd.lba, &bdev); >>>> ... >>>> bdesc = dev_get_uclass_platdata(bdev); >>>> bdesc->target = id; >>>> bdesc->lun = lun; >>>> ... >>>> >>>> /* >>>> * We need have efi_disk_create() called here because bdesc->target >>>> * and lun will be used by dp helpers in efi_disk_add_dev(). >>>> */ >>>> efi_disk_create(bdev); >>>> } >>>> >>>> int scsi_scan_dev(struct udevice *dev, bool verbose) >>>> { >>>> for (i = 0; i < uc_plat->max_id; i++) >>>> for (lun = 0; lun < uc_plat->max_lun; lun++) >>>> do_scsi_scan_one(dev, i, lun, verbose); >>>> ... >>>> } >>>> >>>> int scsi_scan(bool verbose) >>>> { >>>> ret = uclass_get(UCLASS_SCSI, &uc); >>>> ... >>>> uclass_foreach_dev(dev, uc) >>>> ret = scsi_scan_dev(dev, verbose); >>>> ... >>>> } >>>> === === >>>> >>>> Since scsn_scan() can be directly called by "scsi rescan" command, >>>> There seems to be no generic hook, or event, available in order to >>>> call efi_disk_create(). >>>> >>>> Do I miss anything? >>> >>> Could the event handler that gets called from somewhere around >>> blk_create_device() just put it into an efi internal "todo list" which >>> we then process using an efi event? >>> >>> EFI events will only get triggered on the next entry to efi land, so by >>> then we should be safe. >> >> I think I now understand your suggestion; we are going to invent >> a specialized event-queuing mechanism so that we can take any actions >> later at appropriate time (probably in efi_init_obj_list()?). > > Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer.
This is a to-be-invented "specialized event-queuing mechanism" in my language :) as we cannot use efi_create/signal_event() before initializing EFI environment.
This event will be expected to be 'signal'ed at every creation/deletion of UCLASS_BLK device. Right?
Correct.
> That event handler creates a new efi event (like a timer w/ timeout=0).
But when is this event handler fired? I think the only possible timing is at efi_init_obj_list().
We already walk through the event list on any u-boot/efi world switch.
? Where is the code?
Ah, I must have misremembered, sorry. I think that was one proposed patch a while ago, but we didn't put it in.
But worst case we can just put additional efi_timer_check() calls at strategic places if needed.
Do you expect this kind of mechanism be implemented in my short-term fix?
To be completely honest, I don't think your fix is very critical right now, since it touches a case that's known broken today already.
So while the issue can be easily reproduced and my fix is quite simple and straightforward (but with a bit inefficiency :), you won't accept it in the current form.
Right? It's totally up to you.
Thanks, -Takahiro Akashi
I would prefer if we could concentrate on the real path forward, where everything becomes implicit and we no longer need to sync the two object trees.
Alex
On 01/23/2019 09:12 AM, AKASHI Takahiro wrote:
Alex,
On Tue, Jan 22, 2019 at 10:08:53AM +0100, Alexander Graf wrote:
On 22.01.19 09:29, AKASHI Takahiro wrote:
Alex, Simon,
Apologies for my slow response on this matter,
On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
On 11.01.19 05:29, AKASHI Takahiro wrote:
Alex, Heinrich and Simon,
Thank you for your comments, they are all valuable but also make me confused as different people have different requirements :) I'm not sure that all of us share the same *ultimate* goal here.
The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
I don't still understand what "merge" means very well.
It basically means that "struct efi_object" moves into "struct udevice". Every udevice instance of type UCLASS_BLK would expose the block and device_path protocols.
This will be a slightly bigger rework, but eventually allows us to basically get rid of efi_init_obj_list() I think.
But we have this annoying interim state where we would lose a few boards because they haven't been converted to DM. That's what keeps us from it.
I think what this discussion boils down to is that someone needs to start prototyping the DM/EFI integration. Start off with a simple subsystem, like BLK.
Even in the current implementation,
- UEFI disk is implemented using UCLASS_BLK devices (We can ignore !CONFIG_BLK case now as we have agreed.)
- UEFI-specific block device can be seen as UCLASS_BLK/IF_TYPE_EFI
So how essentially different is the *ultimate* goal from the current form regarding block devices?
The ultimate goal is that efi_disk_register() and efi_obj_list disappear.
Functionality wise we should be 100% identical to today, so all test cases would still apply the same way as they do now. This is purely internal rework, nothing visible to UEFI payloads.
In order to identify UEFI disks with u-boot devices transparently, we will have to have some sort of *hook* (or hotplug in Alex's language?), either in create_block_devices or bind/probe? I don't know, but Simon seems to be in denial about this idea.
Well, if a udevice *is* an efi device, we no longer need hooks. The object list would simply change.
We may still need to have event notifications at that stage, but that's a different story.
As transitioning period, we could probably implement 2 efi object roots: efi_obj_list as well as the udevice based one. Every piece of code that iterates through devices then just iterates over both. That way we should be able to slowly move devices from the old object model to the new one.
Then provide a DM path and have a non-DM fallback still in its own source file that also provides EFI BLK devices. Eventually we just remove the latter.
That way we can then work on getting hotplug working in the DM path, which is the one we want anyway. For non-DM, you simply miss out on that amazing new feature, but we don't regress users.
So, first, let me reply to each of your comments. Through this process, I hope we will have better understandings of long-term solution as well as a tentative fix.
On Thu, Jan 10, 2019 at 10:22:04AM +0100, Alexander Graf wrote:
> Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro takahiro.akashi@linaro.org: > >> On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote: >> >> >>>> Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro takahiro.akashi@linaro.org: >>>> >>>> On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote: >>>> >>>> >>>>> On 10.01.19 08:26, AKASHI Takahiro wrote: >>>>> Alex, >>>>> >>>>>> On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote: >>>>>> >>>>>> >>>>>>> On 10.01.19 03:13, AKASHI Takahiro wrote: >>>>>>> Alex, >>>>>>> >>>>>>>> On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote: >>>>>>>> >>>>>>>> >>>>>>>>> On 13.12.18 08:58, AKASHI Takahiro wrote: >>>>>>>>> Heinrich, >>>>>>>>> >>>>>>>>>>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote: >>>>>>>>>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote: >>>>>>>>>>> Currently, efi_init_obj_list() scan disk devices only once, and never >>>>>>>>>>> change a list of efi disk devices. This will possibly result in failing >>>>>>>>>>> to find a removable storage which may be added later on. See [1]. >>>>>>>>>>> >>>>>>>>>>> In this patch, called is efi_disk_update() which is responsible for >>>>>>>>>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary. >>>>>>>>>>> >>>>>>>>>>> For example, >>>>>>>>>>> >>>>>>>>>>> => efishell devices >>>>>>>>>>> Scanning disk pci_mmc.blk... >>>>>>>>>>> Found 3 disks >>>>>>>>>>> Device Name >>>>>>>>>>> ============================================ >>>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>>>>>> => usb start >>>>>>>>>>> starting USB... >>>>>>>>>>> USB0: USB EHCI 1.00 >>>>>>>>>>> scanning bus 0 for devices... 3 USB Device(s) found >>>>>>>>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>>>>>>>> => efishell devices >>>>>>>>>>> Scanning disk usb_mass_storage.lun0... >>>>>>>>>>> Device Name >>>>>>>>>>> ============================================ >>>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) >>>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) >>>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) >>>>>>>>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c) >>>>>>>>>>> >>>>>>>>>>> Without this patch, the last device, USB mass storage, won't show up. >>>>>>>>>>> >>>>>>>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>>>>>>>> Why should we try to fix something in the EFI subsystems that goes wrong >>>>>>>>>> in the handling of device enumeration. >>>>>>>>> No. >>>>>>>>> This is a natural result from how efi disks are currently implemented on u-boot. >>>>>>>>> Do you want to totally re-write/re-implement efi disks? >>>>>>>> Could we just make this event based for now? Call a hook from the >>>>>>>> storage dm subsystem when a new u-boot block device gets created to >>>>>>>> issue a sync of that in the efi subsystem? >>>>>>> If I correctly understand you, your suggestion here corresponds >>>>>>> with my proposal#3 in [1] while my current approach is #2. >>>>>>> >>>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html >>>>>> Yes, I think so. >>>>>> >>>>>>> So we will call, say, efi_disk_create(struct udevice *) in >>>>>>> blk_create_device() and efi_dsik_delete() in blk_unbind_all(). >>>>>> I would prefer if we didn't call them directly, but through an event >>>>>> mechanism. So the efi_disk subsystem registers an event with the dm >>>>>> block subsystem and that will just call all events when block devices >>>>>> get created which will automatically also include the efi disk creation >>>>>> callback. Same for reverse. >>>>> Do you mean efi event by "event?" >>>>> (I don't think there is any generic event interface on DM side.) >>>>> >>>>> Whatever an "event" is or whether we call efi_disk_create() directly >>>>> or indirectly via an event, there is one (big?) issue in this approach >>>>> (while I've almost finished prototyping): >>>>> >>>>> We cannot call efi_disk_create() within blk_create_device() because >>>>> some data fields of struct blk_desc, which are to be used by efi disk, >>>>> are initialized *after* blk_create_device() in driver side. >>>>> >>>>> So we need to add a hook at/after every occurrence of blk_create_device() >>>>> on driver side. For example, >>>>> >>>>> === drivers/scsi/scsi.c === >>>>> int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose) >>>>> { >>>>> ... >>>>> ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, >>>>> bd.blksz, bd.lba, &bdev); >>>>> ... >>>>> bdesc = dev_get_uclass_platdata(bdev); >>>>> bdesc->target = id; >>>>> bdesc->lun = lun; >>>>> ... >>>>> >>>>> /* >>>>> * We need have efi_disk_create() called here because bdesc->target >>>>> * and lun will be used by dp helpers in efi_disk_add_dev(). >>>>> */ >>>>> efi_disk_create(bdev); >>>>> } >>>>> >>>>> int scsi_scan_dev(struct udevice *dev, bool verbose) >>>>> { >>>>> for (i = 0; i < uc_plat->max_id; i++) >>>>> for (lun = 0; lun < uc_plat->max_lun; lun++) >>>>> do_scsi_scan_one(dev, i, lun, verbose); >>>>> ... >>>>> } >>>>> >>>>> int scsi_scan(bool verbose) >>>>> { >>>>> ret = uclass_get(UCLASS_SCSI, &uc); >>>>> ... >>>>> uclass_foreach_dev(dev, uc) >>>>> ret = scsi_scan_dev(dev, verbose); >>>>> ... >>>>> } >>>>> === === >>>>> >>>>> Since scsn_scan() can be directly called by "scsi rescan" command, >>>>> There seems to be no generic hook, or event, available in order to >>>>> call efi_disk_create(). >>>>> >>>>> Do I miss anything? >>>> Could the event handler that gets called from somewhere around >>>> blk_create_device() just put it into an efi internal "todo list" which >>>> we then process using an efi event? >>>> >>>> EFI events will only get triggered on the next entry to efi land, so by >>>> then we should be safe. >>> I think I now understand your suggestion; we are going to invent >>> a specialized event-queuing mechanism so that we can take any actions >>> later at appropriate time (probably in efi_init_obj_list()?). >> Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer. > This is a to-be-invented "specialized event-queuing mechanism" > in my language :) as we cannot use efi_create/signal_event() before > initializing EFI environment. > > This event will be expected to be 'signal'ed at every creation/deletion > of UCLASS_BLK device. Right? Correct.
>> That event handler creates a new efi event (like a timer w/ timeout=0). > But when is this event handler fired? > I think the only possible timing is at efi_init_obj_list(). We already walk through the event list on any u-boot/efi world switch.
? Where is the code?
Ah, I must have misremembered, sorry. I think that was one proposed patch a while ago, but we didn't put it in.
But worst case we can just put additional efi_timer_check() calls at strategic places if needed.
Do you expect this kind of mechanism be implemented in my short-term fix?
To be completely honest, I don't think your fix is very critical right now, since it touches a case that's known broken today already.
So while the issue can be easily reproduced and my fix is quite simple and straightforward (but with a bit inefficiency :), you won't accept it in the current form.
Right? It's totally up to you.
I'm afraid it drives us further into a corner that we don't want to be in, yes, sorry :(.
However, I would be thrilled to see the dm object integration move forward :).
Alex
Hi,
On Wed, 9 Jan 2019 at 02:06, Alexander Graf agraf@suse.de wrote:
On 13.12.18 08:58, AKASHI Takahiro wrote:
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Could we just make this event based for now? Call a hook from the storage dm subsystem when a new u-boot block device gets created to issue a sync of that in the efi subsystem?
Please no. We don't want EFI hooks around the place. EFI should use DM, not the other way around.
That hook would obviously only do something (or get registered?) when the efi object stack is initialized.
The long term goal IMHO should still be though to just merge DM and EFI objects. But we're still waiting on the deprecation of non-DM devices for that.
I think think 'merge' is the right word. Perhaps 'create EFI devices in DM' is a better term.
Anyway, let's do that now. As I may have mentioned we should never have enabled EFI on pre-DM boards :-) It has just lead to duplication.
In any case, the migration deadline for DM_MMC (for example) is the upcoming merge window, so the time is now.
As things stand this patch looks reasonable to me. It is a natural consequence of duplicating the DM tables.
Regards, Simon
On Thu, Jan 10, 2019 at 05:57:11AM -0700, Simon Glass wrote:
Hi,
On Wed, 9 Jan 2019 at 02:06, Alexander Graf agraf@suse.de wrote:
On 13.12.18 08:58, AKASHI Takahiro wrote:
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Could we just make this event based for now? Call a hook from the storage dm subsystem when a new u-boot block device gets created to issue a sync of that in the efi subsystem?
Please no. We don't want EFI hooks around the place. EFI should use DM, not the other way around.
Right, but every efi disk is associated with a backing "u-boot" block devices (even more, this is not 1-to-1 mapping but many-to-1 due to partitions). Without any sort of event/hook mechanism, we can know of all block devices only by enumerating them at some point as in my current approach. Do you want to accept this?
(Even in a hacky way. See efi_disk_register(): for (if_type = 0; if_type < IF_TYPE_COUNT; if_type++) ... printf("Scanning disks on %s...\n", if_typename); for (i = 0; i < 4; i++) { <= !!! desc = blk_get_devnum_by_type(if_type, i); ... )
That hook would obviously only do something (or get registered?) when
i
the efi object stack is initialized.
The long term goal IMHO should still be though to just merge DM and EFI objects. But we're still waiting on the deprecation of non-DM devices for that.
I think think 'merge' is the right word. Perhaps 'create EFI devices in DM' is a better term.
How different is your idea from UCLASS_BLK device(efi_block_device.c)?
The current implementation of UCLASS_BLK, in my opinion, is somehow in a half way; an instance of UCLASS_BLK is created only when a efi driver is bound to a controller. In the meantime, efi disks (efi objects which support UEFI_BLOCK_IO_PROTOCOL) is implicitly created in efi_disk_add_dev() without any *binding*.
Anyway, let's do that now. As I may have mentioned we should never have enabled EFI on pre-DM boards :-) It has just lead to duplication.
In any case, the migration deadline for DM_MMC (for example) is the upcoming merge window, so the time is now.
As things stand this patch looks reasonable to me. It is a natural consequence of duplicating the DM tables.
I think so, yes.
-Takahiro Akashi
Regards, Simon
On 11.01.19 05:51, AKASHI Takahiro wrote:
On Thu, Jan 10, 2019 at 05:57:11AM -0700, Simon Glass wrote:
Hi,
On Wed, 9 Jan 2019 at 02:06, Alexander Graf agraf@suse.de wrote:
On 13.12.18 08:58, AKASHI Takahiro wrote:
Heinrich,
On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
Currently, efi_init_obj_list() scan disk devices only once, and never change a list of efi disk devices. This will possibly result in failing to find a removable storage which may be added later on. See [1].
In this patch, called is efi_disk_update() which is responsible for re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
For example,
=> efishell devices Scanning disk pci_mmc.blk... Found 3 disks Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) => usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found => efishell devices Scanning disk usb_mass_storage.lun0... Device Name ============================================ /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800) /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
Without this patch, the last device, USB mass storage, won't show up.
[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Why should we try to fix something in the EFI subsystems that goes wrong in the handling of device enumeration.
No. This is a natural result from how efi disks are currently implemented on u-boot. Do you want to totally re-write/re-implement efi disks?
Could we just make this event based for now? Call a hook from the storage dm subsystem when a new u-boot block device gets created to issue a sync of that in the efi subsystem?
Please no. We don't want EFI hooks around the place. EFI should use DM, not the other way around.
Right, but every efi disk is associated with a backing "u-boot" block devices (even more, this is not 1-to-1 mapping but many-to-1 due to partitions).
In this case we may just want to create DM devices for partitions as well to make things more natural.
Without any sort of event/hook mechanism, we can know of all block devices only by enumerating them at some point as in my current approach. Do you want to accept this?
(Even in a hacky way. See efi_disk_register(): for (if_type = 0; if_type < IF_TYPE_COUNT; if_type++) ... printf("Scanning disks on %s...\n", if_typename); for (i = 0; i < 4; i++) { <= !!! desc = blk_get_devnum_by_type(if_type, i); ... )
That hook would obviously only do something (or get registered?) when
i
the efi object stack is initialized.
The long term goal IMHO should still be though to just merge DM and EFI objects. But we're still waiting on the deprecation of non-DM devices for that.
I think think 'merge' is the right word. Perhaps 'create EFI devices in DM' is a better term.
How different is your idea from UCLASS_BLK device(efi_block_device.c)?
The UCLASS_BLK is the reverse path. It's exposing a DM device from an EFI one.
Alex
The current implementation of UCLASS_BLK, in my opinion, is somehow in a half way; an instance of UCLASS_BLK is created only when a efi driver is bound to a controller. In the meantime, efi disks (efi objects which support UEFI_BLOCK_IO_PROTOCOL) is implicitly created in efi_disk_add_dev() without any *binding*.
Anyway, let's do that now. As I may have mentioned we should never have enabled EFI on pre-DM boards :-) It has just lead to duplication.
In any case, the migration deadline for DM_MMC (for example) is the upcoming merge window, so the time is now.
As things stand this patch looks reasonable to me. It is a natural consequence of duplicating the DM tables.
I think so, yes.
-Takahiro Akashi
Regards, Simon
From: Alexander Graf agraf@suse.de Date: Fri, 11 Jan 2019 09:00:27 +0100
On 11.01.19 05:51, AKASHI Takahiro wrote:
On Thu, Jan 10, 2019 at 05:57:11AM -0700, Simon Glass wrote:
Hi,
Please no. We don't want EFI hooks around the place. EFI should use DM, not the other way around.
Right, but every efi disk is associated with a backing "u-boot" block devices (even more, this is not 1-to-1 mapping but many-to-1 due to partitions).
In this case we may just want to create DM devices for partitions as well to make things more natural.
Be careful here though that this doesn't lead to device paths for partitions that have a different "root" than the disk itself. The OpenBSD bootloader relies on matching the initial EFI device path components to find the entire disk from the partition it was loaded from to find the disk from which to load the kernel.
Cheers,
Mark
Logically, details on u-boot block device used to implement efi file protocol are mostly unnecessary, as well as being duplicated, in efi_file structure. Moreover, a newly introduced flag, _EFI_DISK_FLAG_INVALID, should be honored in any file operations via efi file protocol. These observation suggests that those internal details be set behind efi_disk object.
So rework in this patch addresses all those issues above although there is little change in its functionality.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- include/efi_loader.h | 6 +++++- lib/efi_loader/efi_disk.c | 38 ++++++++++++++++++++++++++++++++++++-- lib/efi_loader/efi_file.c | 21 ++++++++------------- 3 files changed, 49 insertions(+), 16 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3bae1844befb..108ef3fe52ee 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -264,6 +264,10 @@ efi_status_t efi_disk_register(void); bool efi_disk_is_valid(efi_handle_t handle); /* Called by bootefi to find and update disk storage information */ efi_status_t efi_disk_update(void); +/* Called by file protocol to set internal block io device */ +int efi_disk_set_blk_dev(efi_handle_t disk); +/* Called by file protocol to get disk/partition information */ +int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, @@ -355,7 +359,7 @@ void efi_signal_event(struct efi_event *event, bool check_tpl);
/* open file system: */ struct efi_simple_file_system_protocol *efi_simple_file_system( - struct blk_desc *desc, int part, struct efi_device_path *dp); + efi_handle_t disk);
/* open file from device-path: */ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp); diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 0c4d79ee3fc9..180e8e10bb28 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -9,6 +9,7 @@ #include <blk.h> #include <dm.h> #include <efi_loader.h> +#include <fs.h> #include <part.h> #include <malloc.h>
@@ -254,6 +255,40 @@ efi_fs_from_path(struct efi_device_path *full_path) return handler->protocol_interface; }
+/* + * Set block device for later block io's from file system protocol + * + * @disk handle to uefi disk device + * @return 0 for success, -1 for failure + */ +int efi_disk_set_blk_dev(efi_handle_t disk) +{ + struct efi_disk_obj *diskobj; + + diskobj = container_of(disk, struct efi_disk_obj, header); + + return fs_set_blk_dev_with_part(diskobj->desc, diskobj->part); +} + +/* + * Get disk/partition information + * + * @disk handle to uefi disk device + * @part pointer to disk/partition information to be returned + * @return 0 for success, -1 for failure + */ +int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part) +{ + struct efi_disk_obj *diskobj; + + diskobj = container_of(disk, struct efi_disk_obj, header); + + if (diskobj->part >= 1) + return part_get_info(diskobj->desc, diskobj->part, part); + else + return part_get_info_whole_disk(diskobj->desc, part); +} + /* * Create a handle for a partition or disk * @@ -308,8 +343,7 @@ static efi_status_t efi_disk_add_dev( if (ret != EFI_SUCCESS) return ret; if (part >= 1) { - diskobj->volume = efi_simple_file_system(desc, part, - diskobj->dp); + diskobj->volume = efi_simple_file_system(&diskobj->header); ret = efi_add_protocol(&diskobj->header, &efi_simple_file_system_protocol_guid, diskobj->volume); diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c index beb4fba917d8..944383224f30 100644 --- a/lib/efi_loader/efi_file.c +++ b/lib/efi_loader/efi_file.c @@ -17,9 +17,7 @@ const efi_guid_t efi_file_system_info_guid = EFI_FILE_SYSTEM_INFO_GUID;
struct file_system { struct efi_simple_file_system_protocol base; - struct efi_device_path *dp; - struct blk_desc *desc; - int part; + efi_handle_t disk; }; #define to_fs(x) container_of(x, struct file_system, base)
@@ -49,7 +47,10 @@ static char *basename(struct file_handle *fh)
static int set_blk_dev(struct file_handle *fh) { - return fs_set_blk_dev_with_part(fh->fs->desc, fh->fs->part); + if (!efi_disk_is_valid(fh->fs->disk)) + return -1; + + return efi_disk_set_blk_dev(fh->fs->disk); }
/** @@ -570,10 +571,7 @@ static efi_status_t EFIAPI efi_file_getinfo(struct efi_file_handle *file, efi_uintn_t required_size; int r;
- if (fh->fs->part >= 1) - r = part_get_info(fh->fs->desc, fh->fs->part, &part); - else - r = part_get_info_whole_disk(fh->fs->desc, &part); + r = efi_disk_get_info(fh->fs->disk, &part); if (r < 0) { ret = EFI_DEVICE_ERROR; goto error; @@ -694,17 +692,14 @@ efi_open_volume(struct efi_simple_file_system_protocol *this, }
struct efi_simple_file_system_protocol * -efi_simple_file_system(struct blk_desc *desc, int part, - struct efi_device_path *dp) +efi_simple_file_system(efi_handle_t disk) { struct file_system *fs;
fs = calloc(1, sizeof(*fs)); fs->base.rev = EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_REVISION; fs->base.open_volume = efi_open_volume; - fs->desc = desc; - fs->part = part; - fs->dp = dp; + fs->disk = disk;
return &fs->base; }
On 15.11.18 05:58, AKASHI Takahiro wrote:
Logically, details on u-boot block device used to implement efi file protocol are mostly unnecessary, as well as being duplicated, in efi_file structure. Moreover, a newly introduced flag, _EFI_DISK_FLAG_INVALID, should be honored in any file operations via efi file protocol. These observation suggests that those internal details be set behind efi_disk object.
So rework in this patch addresses all those issues above although there is little change in its functionality.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
include/efi_loader.h | 6 +++++- lib/efi_loader/efi_disk.c | 38 ++++++++++++++++++++++++++++++++++++-- lib/efi_loader/efi_file.c | 21 ++++++++------------- 3 files changed, 49 insertions(+), 16 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3bae1844befb..108ef3fe52ee 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -264,6 +264,10 @@ efi_status_t efi_disk_register(void); bool efi_disk_is_valid(efi_handle_t handle); /* Called by bootefi to find and update disk storage information */ efi_status_t efi_disk_update(void); +/* Called by file protocol to set internal block io device */ +int efi_disk_set_blk_dev(efi_handle_t disk); +/* Called by file protocol to get disk/partition information */ +int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, @@ -355,7 +359,7 @@ void efi_signal_event(struct efi_event *event, bool check_tpl);
/* open file system: */ struct efi_simple_file_system_protocol *efi_simple_file_system(
struct blk_desc *desc, int part, struct efi_device_path *dp);
efi_handle_t disk);/* open file from device-path: */ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp); diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 0c4d79ee3fc9..180e8e10bb28 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -9,6 +9,7 @@ #include <blk.h> #include <dm.h> #include <efi_loader.h> +#include <fs.h> #include <part.h> #include <malloc.h>
@@ -254,6 +255,40 @@ efi_fs_from_path(struct efi_device_path *full_path) return handler->protocol_interface; }
+/*
- Set block device for later block io's from file system protocol
- @disk handle to uefi disk device
- @return 0 for success, -1 for failure
- */
+int efi_disk_set_blk_dev(efi_handle_t disk) +{
- struct efi_disk_obj *diskobj;
- diskobj = container_of(disk, struct efi_disk_obj, header);
- return fs_set_blk_dev_with_part(diskobj->desc, diskobj->part);
+}
+/*
- Get disk/partition information
- @disk handle to uefi disk device
- @part pointer to disk/partition information to be returned
- @return 0 for success, -1 for failure
- */
+int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part) +{
- struct efi_disk_obj *diskobj;
- diskobj = container_of(disk, struct efi_disk_obj, header);
- if (diskobj->part >= 1)
return part_get_info(diskobj->desc, diskobj->part, part);- else
return part_get_info_whole_disk(diskobj->desc, part);+}
/*
- Create a handle for a partition or disk
@@ -308,8 +343,7 @@ static efi_status_t efi_disk_add_dev( if (ret != EFI_SUCCESS) return ret; if (part >= 1) {
diskobj->volume = efi_simple_file_system(desc, part,diskobj->dp);
diskobj->volume = efi_simple_file_system(&diskobj->header);diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c index beb4fba917d8..944383224f30 100644 --- a/lib/efi_loader/efi_file.c +++ b/lib/efi_loader/efi_file.c @@ -17,9 +17,7 @@ const efi_guid_t efi_file_system_info_guid = EFI_FILE_SYSTEM_INFO_GUID;
struct file_system { struct efi_simple_file_system_protocol base;
- struct efi_device_path *dp;
- struct blk_desc *desc;
- int part;
- efi_handle_t disk;
Is there a particular reason we can't just make this a struct efi_disk_obj *?
Inside our own code base, we don't need to abstract things away to handles, right? We also want to have the compiler catch the fact early if people pass in non-disk-objects in as parameter.
Alex
Alex,
On Wed, Jan 09, 2019 at 10:18:16AM +0100, Alexander Graf wrote:
On 15.11.18 05:58, AKASHI Takahiro wrote:
Logically, details on u-boot block device used to implement efi file protocol are mostly unnecessary, as well as being duplicated, in efi_file structure. Moreover, a newly introduced flag, _EFI_DISK_FLAG_INVALID, should be honored in any file operations via efi file protocol. These observation suggests that those internal details be set behind efi_disk object.
So rework in this patch addresses all those issues above although there is little change in its functionality.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
include/efi_loader.h | 6 +++++- lib/efi_loader/efi_disk.c | 38 ++++++++++++++++++++++++++++++++++++-- lib/efi_loader/efi_file.c | 21 ++++++++------------- 3 files changed, 49 insertions(+), 16 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3bae1844befb..108ef3fe52ee 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -264,6 +264,10 @@ efi_status_t efi_disk_register(void); bool efi_disk_is_valid(efi_handle_t handle); /* Called by bootefi to find and update disk storage information */ efi_status_t efi_disk_update(void); +/* Called by file protocol to set internal block io device */ +int efi_disk_set_blk_dev(efi_handle_t disk); +/* Called by file protocol to get disk/partition information */ +int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, @@ -355,7 +359,7 @@ void efi_signal_event(struct efi_event *event, bool check_tpl);
/* open file system: */ struct efi_simple_file_system_protocol *efi_simple_file_system(
struct blk_desc *desc, int part, struct efi_device_path *dp);
efi_handle_t disk);/* open file from device-path: */ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp); diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 0c4d79ee3fc9..180e8e10bb28 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -9,6 +9,7 @@ #include <blk.h> #include <dm.h> #include <efi_loader.h> +#include <fs.h> #include <part.h> #include <malloc.h>
@@ -254,6 +255,40 @@ efi_fs_from_path(struct efi_device_path *full_path) return handler->protocol_interface; }
+/*
- Set block device for later block io's from file system protocol
- @disk handle to uefi disk device
- @return 0 for success, -1 for failure
- */
+int efi_disk_set_blk_dev(efi_handle_t disk) +{
- struct efi_disk_obj *diskobj;
- diskobj = container_of(disk, struct efi_disk_obj, header);
- return fs_set_blk_dev_with_part(diskobj->desc, diskobj->part);
+}
+/*
- Get disk/partition information
- @disk handle to uefi disk device
- @part pointer to disk/partition information to be returned
- @return 0 for success, -1 for failure
- */
+int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part) +{
- struct efi_disk_obj *diskobj;
- diskobj = container_of(disk, struct efi_disk_obj, header);
- if (diskobj->part >= 1)
return part_get_info(diskobj->desc, diskobj->part, part);- else
return part_get_info_whole_disk(diskobj->desc, part);+}
/*
- Create a handle for a partition or disk
@@ -308,8 +343,7 @@ static efi_status_t efi_disk_add_dev( if (ret != EFI_SUCCESS) return ret; if (part >= 1) {
diskobj->volume = efi_simple_file_system(desc, part,diskobj->dp);
diskobj->volume = efi_simple_file_system(&diskobj->header);diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c index beb4fba917d8..944383224f30 100644 --- a/lib/efi_loader/efi_file.c +++ b/lib/efi_loader/efi_file.c @@ -17,9 +17,7 @@ const efi_guid_t efi_file_system_info_guid = EFI_FILE_SYSTEM_INFO_GUID;
struct file_system { struct efi_simple_file_system_protocol base;
- struct efi_device_path *dp;
- struct blk_desc *desc;
- int part;
- efi_handle_t disk;
Is there a particular reason we can't just make this a struct efi_disk_obj *?
Just because efi_disk_obj is an internally-defined structure in efi_disk.c.
Inside our own code base, we don't need to abstract things away to handles, right? We also want to have the compiler catch the fact early if people pass in non-disk-objects in as parameter.
If you don't mind efi_disk_obj definition being moved to, say, efi_loader.h, I would follow your suggestion.
Thanks, -Takahiro Akashi
Alex
On 10.01.19 01:37, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 09, 2019 at 10:18:16AM +0100, Alexander Graf wrote:
On 15.11.18 05:58, AKASHI Takahiro wrote:
Logically, details on u-boot block device used to implement efi file protocol are mostly unnecessary, as well as being duplicated, in efi_file structure. Moreover, a newly introduced flag, _EFI_DISK_FLAG_INVALID, should be honored in any file operations via efi file protocol. These observation suggests that those internal details be set behind efi_disk object.
So rework in this patch addresses all those issues above although there is little change in its functionality.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
include/efi_loader.h | 6 +++++- lib/efi_loader/efi_disk.c | 38 ++++++++++++++++++++++++++++++++++++-- lib/efi_loader/efi_file.c | 21 ++++++++------------- 3 files changed, 49 insertions(+), 16 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3bae1844befb..108ef3fe52ee 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -264,6 +264,10 @@ efi_status_t efi_disk_register(void); bool efi_disk_is_valid(efi_handle_t handle); /* Called by bootefi to find and update disk storage information */ efi_status_t efi_disk_update(void); +/* Called by file protocol to set internal block io device */ +int efi_disk_set_blk_dev(efi_handle_t disk); +/* Called by file protocol to get disk/partition information */ +int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, @@ -355,7 +359,7 @@ void efi_signal_event(struct efi_event *event, bool check_tpl);
/* open file system: */ struct efi_simple_file_system_protocol *efi_simple_file_system(
struct blk_desc *desc, int part, struct efi_device_path *dp);
efi_handle_t disk);/* open file from device-path: */ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp); diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 0c4d79ee3fc9..180e8e10bb28 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -9,6 +9,7 @@ #include <blk.h> #include <dm.h> #include <efi_loader.h> +#include <fs.h> #include <part.h> #include <malloc.h>
@@ -254,6 +255,40 @@ efi_fs_from_path(struct efi_device_path *full_path) return handler->protocol_interface; }
+/*
- Set block device for later block io's from file system protocol
- @disk handle to uefi disk device
- @return 0 for success, -1 for failure
- */
+int efi_disk_set_blk_dev(efi_handle_t disk) +{
- struct efi_disk_obj *diskobj;
- diskobj = container_of(disk, struct efi_disk_obj, header);
- return fs_set_blk_dev_with_part(diskobj->desc, diskobj->part);
+}
+/*
- Get disk/partition information
- @disk handle to uefi disk device
- @part pointer to disk/partition information to be returned
- @return 0 for success, -1 for failure
- */
+int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part) +{
- struct efi_disk_obj *diskobj;
- diskobj = container_of(disk, struct efi_disk_obj, header);
- if (diskobj->part >= 1)
return part_get_info(diskobj->desc, diskobj->part, part);- else
return part_get_info_whole_disk(diskobj->desc, part);+}
/*
- Create a handle for a partition or disk
@@ -308,8 +343,7 @@ static efi_status_t efi_disk_add_dev( if (ret != EFI_SUCCESS) return ret; if (part >= 1) {
diskobj->volume = efi_simple_file_system(desc, part,diskobj->dp);
diskobj->volume = efi_simple_file_system(&diskobj->header);diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c index beb4fba917d8..944383224f30 100644 --- a/lib/efi_loader/efi_file.c +++ b/lib/efi_loader/efi_file.c @@ -17,9 +17,7 @@ const efi_guid_t efi_file_system_info_guid = EFI_FILE_SYSTEM_INFO_GUID;
struct file_system { struct efi_simple_file_system_protocol base;
- struct efi_device_path *dp;
- struct blk_desc *desc;
- int part;
- efi_handle_t disk;
Is there a particular reason we can't just make this a struct efi_disk_obj *?
Just because efi_disk_obj is an internally-defined structure in efi_disk.c.
Inside our own code base, we don't need to abstract things away to handles, right? We also want to have the compiler catch the fact early if people pass in non-disk-objects in as parameter.
If you don't mind efi_disk_obj definition being moved to, say, efi_loader.h, I would follow your suggestion.
I don't think we need to move the definition, just the hint that the name exists.
If you add the following to efi_loader.h:
struct efi_disk_obj;
that should be enough to enable other subsystems (and APIs) to use pointers to that struct.
Alex
On Thu, Jan 10, 2019 at 07:22:49AM +0100, Alexander Graf wrote:
On 10.01.19 01:37, AKASHI Takahiro wrote:
Alex,
On Wed, Jan 09, 2019 at 10:18:16AM +0100, Alexander Graf wrote:
On 15.11.18 05:58, AKASHI Takahiro wrote:
Logically, details on u-boot block device used to implement efi file protocol are mostly unnecessary, as well as being duplicated, in efi_file structure. Moreover, a newly introduced flag, _EFI_DISK_FLAG_INVALID, should be honored in any file operations via efi file protocol. These observation suggests that those internal details be set behind efi_disk object.
So rework in this patch addresses all those issues above although there is little change in its functionality.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
include/efi_loader.h | 6 +++++- lib/efi_loader/efi_disk.c | 38 ++++++++++++++++++++++++++++++++++++-- lib/efi_loader/efi_file.c | 21 ++++++++------------- 3 files changed, 49 insertions(+), 16 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3bae1844befb..108ef3fe52ee 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -264,6 +264,10 @@ efi_status_t efi_disk_register(void); bool efi_disk_is_valid(efi_handle_t handle); /* Called by bootefi to find and update disk storage information */ efi_status_t efi_disk_update(void); +/* Called by file protocol to set internal block io device */ +int efi_disk_set_blk_dev(efi_handle_t disk); +/* Called by file protocol to get disk/partition information */ +int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, @@ -355,7 +359,7 @@ void efi_signal_event(struct efi_event *event, bool check_tpl);
/* open file system: */ struct efi_simple_file_system_protocol *efi_simple_file_system(
struct blk_desc *desc, int part, struct efi_device_path *dp);
efi_handle_t disk);/* open file from device-path: */ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp); diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 0c4d79ee3fc9..180e8e10bb28 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -9,6 +9,7 @@ #include <blk.h> #include <dm.h> #include <efi_loader.h> +#include <fs.h> #include <part.h> #include <malloc.h>
@@ -254,6 +255,40 @@ efi_fs_from_path(struct efi_device_path *full_path) return handler->protocol_interface; }
+/*
- Set block device for later block io's from file system protocol
- @disk handle to uefi disk device
- @return 0 for success, -1 for failure
- */
+int efi_disk_set_blk_dev(efi_handle_t disk) +{
- struct efi_disk_obj *diskobj;
- diskobj = container_of(disk, struct efi_disk_obj, header);
- return fs_set_blk_dev_with_part(diskobj->desc, diskobj->part);
+}
+/*
- Get disk/partition information
- @disk handle to uefi disk device
- @part pointer to disk/partition information to be returned
- @return 0 for success, -1 for failure
- */
+int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part) +{
- struct efi_disk_obj *diskobj;
- diskobj = container_of(disk, struct efi_disk_obj, header);
- if (diskobj->part >= 1)
return part_get_info(diskobj->desc, diskobj->part, part);- else
return part_get_info_whole_disk(diskobj->desc, part);+}
/*
- Create a handle for a partition or disk
@@ -308,8 +343,7 @@ static efi_status_t efi_disk_add_dev( if (ret != EFI_SUCCESS) return ret; if (part >= 1) {
diskobj->volume = efi_simple_file_system(desc, part,diskobj->dp);
diskobj->volume = efi_simple_file_system(&diskobj->header);diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c index beb4fba917d8..944383224f30 100644 --- a/lib/efi_loader/efi_file.c +++ b/lib/efi_loader/efi_file.c @@ -17,9 +17,7 @@ const efi_guid_t efi_file_system_info_guid = EFI_FILE_SYSTEM_INFO_GUID;
struct file_system { struct efi_simple_file_system_protocol base;
- struct efi_device_path *dp;
- struct blk_desc *desc;
- int part;
- efi_handle_t disk;
Is there a particular reason we can't just make this a struct efi_disk_obj *?
Just because efi_disk_obj is an internally-defined structure in efi_disk.c.
Inside our own code base, we don't need to abstract things away to handles, right? We also want to have the compiler catch the fact early if people pass in non-disk-objects in as parameter.
If you don't mind efi_disk_obj definition being moved to, say, efi_loader.h, I would follow your suggestion.
I don't think we need to move the definition, just the hint that the name exists.
If you add the following to efi_loader.h:
struct efi_disk_obj;
that should be enough to enable other subsystems (and APIs) to use pointers to that struct.
Ah, right. What we need to have in struct file_system is a *pointer*. So such a declaration is good enough.
Thanks, -Takahiro Akashi
Alex
participants (5)
-
AKASHI Takahiro -
Alexander Graf -
Heinrich Schuchardt -
Mark Kettenis -
Simon Glass