[U-Boot] [PATCH 2/2] efi_loader: enumerate disk devices every time

[Heinrich Schuchardt]

On 11/7/18 1:44 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 at linaro.org>
> ---
>  cmd/bootefi.c             |   8 +-
>  include/efi_loader.h      |   2 +
>  lib/efi_loader/efi_disk.c | 150 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 159 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3cefb4d0ecaa..493022a09482 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -57,8 +57,14 @@ 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 != OBJ_LIST_NOT_INITIALIZED) {

If efi_obj_list_initialized is neither OBJ_LIST_NOT_INITIALIZED nor
EFI_SUCCESS, return efi_obj_list_initialized.

> +#ifdef CONFIG_PARTITIONS
> +		ret = efi_disk_update();
> +		if (ret != EFI_SUCCESS)
> +			printf("+++ updating disks failed\n");
> +#endif
>  		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..e5a080281dba 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -260,6 +260,8 @@ 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);
> +/* 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..e1d47f34049b 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -14,10 +14,13 @@
>
>  const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
>
> +#define _EFI_DISK_MARK_DELETE 0x1

Plese, add a comment to the code explaining what this constant is used
for. None of our other constants starts with an underscore.

In your coding below you keep handles for deleted drives. What will
happen if a binary tries to access a deleted drive?

> +
>  /**
>   * 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 +33,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;
> @@ -440,3 +444,149 @@ efi_status_t efi_disk_register(void)
>
>  	return EFI_SUCCESS;
>  }
> +
> +static void efi_disk_mark_delete(struct efi_disk_obj *disk)

efi_disk_mark_deleted.

> +{
> +	disk->flags |= _EFI_DISK_MARK_DELETE;
> +}
> +
> +static void efi_disk_mark_undelete(struct efi_disk_obj *disk)

 efi_disk_mark_not_deleted.

> +{
> +	disk->flags &= ~_EFI_DISK_MARK_DELETE;
> +}
> +
> +static bool efi_disk_delete_marked(struct efi_disk_obj *disk)

efi_disk_marked_deleted.

> +{
> +	return disk->flags & _EFI_DISK_MARK_DELETE;
> +}
> +


Please, provide a comment describing what the function is meant to do.
> +static efi_status_t efi_disk_mark_delete_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);

Please, panic() or error out if ret != EFI_BUFFER_TOO_SMALL.

> +	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_delete(disk);
> +	}
> +
> +	*handlesp = handles;
> +
> +	return num;
> +}
> +
> +static int efi_disk_mark_undelete_handles(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_mark_undelete(disk);
> +			disks++;
> +		}
> +	}
> +
> +	return disks;
> +}
> +
> +static efi_status_t efi_disk_delete_all(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_delete_marked(disk))
> +			continue;
> +
> +		efi_delete_handle(handles[i]);

what will happen if an EFI runtime or boottime driver still has a
reference to a handle which will be deleted here?

Isn't the whole idea of the deleted flag that you always keep the handle
and reuse it when rescanning finds the same disk?

Please, do not delete any handle to block devices but reuse existing by
comparing device paths.

> +	}
> +
> +	return EFI_SUCCESS;
> +}
> +
> +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;
> +
> +	ret = efi_disk_mark_delete_all(&handles);
> +	if (ret & EFI_ERROR_MASK)
> +		return ret;

Why should it be an error not to have any block device?
Please, return EFI_SUCEESS if ret = EFI_NOT_FOUND.

> +
> +	n = (int)ret;
> +	ret = EFI_SUCCESS;
> +	for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
> +	     uclass_next_device_check(&dev)) {
> +		desc = dev_get_uclass_platdata(dev);
> +		if (n && efi_disk_mark_undelete_handles(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);

Please, use debug().

> +		ret = efi_disk_add_dev(NULL, NULL, if_typename,
> +				       desc, desc->devnum, 0, 0, &disk);
> +		if (ret == EFI_NOT_READY) {
> +			printf("Disk %s not ready\n", dev->name);
> +			continue;
> +		}
> +		if (ret) {
> +			printf("ERROR: failure to add disk device %s, r = %lu\n",
> +			       dev->name, ret & ~EFI_ERROR_MASK);
> +			break;
> +		}
> +		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) {
> +		if (ret != EFI_SUCCESS)
> +			efi_disk_mark_undelete_handles(NULL, handles, n);
> +		else
> +			ret = efi_disk_delete_all(handles, n);
> +		free(handles);

Why would you mark all disks deleted if you had a problem with one of them?

Best regards

Heinrich

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