[v2,2/3] efi_loader: enumerate disk devices every time

Message ID	20181115045810.28736-3-takahiro.akashi@linaro.org
State	New
Headers	show Delivered-To: patch@linaro.org Received-SPF: pass (google.com: best guess record for domain of u-boot-bounces@lists.denx.de designates 81.169.180.215 as permitted sender) client-ip=81.169.180.215; From: AKASHI Takahiro <takahiro.akashi@linaro.org> To: trini@konsulko.com, agraf@suse.de Date: Thu, 15 Nov 2018 13:58:09 +0900 Message-Id: <20181115045810.28736-3-takahiro.akashi@linaro.org> In-Reply-To: <20181115045810.28736-1-takahiro.akashi@linaro.org> References: <20181115045810.28736-1-takahiro.akashi@linaro.org> MIME-Version: 1.0 Cc: xypron.glpk@gmx.de, u-boot@lists.denx.de Subject: [U-Boot] [PATCH v2 2/3] efi_loader: enumerate disk devices every time Precedence: list Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" <u-boot-bounces@lists.denx.de>
Series	efi_loader: add removable device support \| expand [v2,0/3] efi_loader: add removable device support [v2,1/3] efi_loader: export efi_locate_handle() function [v2,2/3] efi_loader: enumerate disk devices every time [v2,3/3] efi_loader: remove block device details from efi file

AKASHI Takahiro Nov. 15, 2018, 4:58 a.m. UTC

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

Heinrich Schuchardt Dec. 11, 2018, 7:55 p.m. UTC | #1

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;
> +}
>

AKASHI Takahiro Dec. 13, 2018, 7:58 a.m. UTC | #2

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;
> > +}
> > 
>

AKASHI Takahiro Jan. 9, 2019, 1:05 a.m. UTC | #3

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;
> > > +}
> > > 
> >

Alexander Graf Jan. 9, 2019, 9:06 a.m. UTC | #4

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

AKASHI Takahiro Jan. 10, 2019, 2:13 a.m. UTC | #5

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

Alexander Graf Jan. 10, 2019, 6:21 a.m. UTC | #6

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

AKASHI Takahiro Jan. 10, 2019, 7:26 a.m. UTC | #7

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

Alexander Graf Jan. 10, 2019, 7:30 a.m. UTC | #8

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

AKASHI Takahiro Jan. 10, 2019, 8:02 a.m. UTC | #9

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

Alexander Graf Jan. 10, 2019, 8:15 a.m. UTC | #10

> 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

AKASHI Takahiro Jan. 10, 2019, 9:16 a.m. UTC | #11

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
>

Alexander Graf Jan. 10, 2019, 9:22 a.m. UTC | #12

> 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

Simon Glass Jan. 10, 2019, 12:57 p.m. UTC | #13

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

Heinrich Schuchardt Jan. 10, 2019, 7:22 p.m. UTC | #14

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
> 
> 
>

AKASHI Takahiro Jan. 11, 2019, 4:29 a.m. UTC | #15

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
> 
>

AKASHI Takahiro Jan. 11, 2019, 4:51 a.m. UTC | #16

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

AKASHI Takahiro Jan. 11, 2019, 5:08 a.m. UTC | #17

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
> > 
> > 
> > 
>

Alexander Graf Jan. 11, 2019, 7:57 a.m. UTC | #18

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

Alexander Graf Jan. 11, 2019, 8 a.m. UTC | #19

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

Mark Kettenis Jan. 11, 2019, 1:03 p.m. UTC | #20

> 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

Simon Glass Jan. 12, 2019, 9:32 p.m. UTC | #21

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

Alexander Graf Jan. 12, 2019, 10 p.m. UTC | #22

> 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

Simon Glass Jan. 16, 2019, 9:34 p.m. UTC | #23

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

AKASHI Takahiro Jan. 22, 2019, 8:29 a.m. UTC | #24

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

Alexander Graf Jan. 22, 2019, 9:08 a.m. UTC | #25

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

[v2,2/3] efi_loader: enumerate disk devices every time

Commit Message

Comments

Patch