diff mbox series

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

Message ID 20181107004434.31491-2-takahiro.akashi@linaro.org
State Changes Requested, archived
Delegated to: Alexander Graf
Headers show
Series [U-Boot,1/2] efi_loader: export efi_locate_handle() function | expand

Commit Message

AKASHI Takahiro Nov. 7, 2018, 12:44 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

Comments

Heinrich Schuchardt Nov. 7, 2018, 7:36 a.m. UTC | #1 
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@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
> +}
>
AKASHI Takahiro Nov. 9, 2018, 6:22 a.m. UTC | #2 
Hi Heinrich,

Thank you for your comments.
First of all, as I said [1], my essential question is whether my approach
here is a right way to go. What do think?

On Wed, Nov 07, 2018 at 08:36:48AM +0100, Heinrich Schuchardt wrote:
> 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@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.

Right.

> > +#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.

Sure.

> None of our other constants starts with an underscore.

This flag name is totally efi_disk.c internal. So an underscore is
added to prevent any potential name collision with UEFI spec.
(unlikely though)

# The name will be renamed to _EFI_DISK_FLAG_DELETE(D)
                                        ^^^^

> In your coding below you keep handles for deleted drives. What will

No, any handles marked "deleted" will be deleted in this patch.

> happen if a binary tries to access a deleted drive?

See below.

> > +
> >  /**
> >   * 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.

OK.

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

I prefer clear_deleted.

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

OK.

> > +{
> > +	return disk->flags & _EFI_DISK_MARK_DELETE;
> > +}
> > +
> 
> 
> Please, provide a comment describing what the function is meant to do.

OK.

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

Panic should be always avoided as any efi-related command would fail anyway,

> > +	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) {

and this function fails in any failure case.

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

Do you know any real case for this situation?

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

I used to think of this option, but we can't exclude any possibility
that an new device path matches with one of old (deleted) ones but
the content in storage may have been changed. In such a case, we will
lose data integrity.

# This is one of reasons behind the question in the top.

Any how, I think the efi_delete_handle() should be able to fail
if a handle is still held by others.

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

Right.

> > +
> > +	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().

Efi_disk_register() does this, too.

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

I'm afraid that you misunderstand here.
The code will try to clear "deleted" marks for recovery in failure case
as we can't do anything other than that here.

# "if" condition will be also reverted for better understanding.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +	}
> > +
> > +	return ret;
> > +#else
> > +	return EFI_SUCCESS;
> > +#endif
> > +}
> >
>
Heinrich Schuchardt Nov. 9, 2018, 6:44 a.m. UTC | #3 
On 11/9/18 7:22 AM, AKASHI Takahiro wrote:
> Hi Heinrich,
> 
> Thank you for your comments.
> First of all, as I said [1], my essential question is whether my approach
> here is a right way to go. What do think?

The approach that you have chosen is surely an improvement. On the other
hand let's look at the target state:

Simon want's to eliminate all block device drivers that are not device
tree based. So we could say an EFI handle for a disk has to be created
when a device tree node is created for a block device. When the node is
removed the EFI handle should be marked as stale.

The UEFI spec foresees applications, boottime drivers and runtime
drivers as binaries. U-Boot cannot decide if a driver keeps a reference
to a handle. So, please, do not deallocate EFI handles for block devices
 unless an application or driver (like iPXE) removes the last protocol.

Instead return "device not ready" if a stale block device handle is used.

Best regards

Heinrich
diff mbox series

Patch

============================================
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
=> usb start
starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 3 USB Device(s) found
       scanning usb for storage devices... 1 Storage Device(s) found
=> efishell devices
Scanning disk usb_mass_storage.lun0...
Device Name
============================================
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)

Without this patch, the last device, USB mass storage, won't show up.

[1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c             |   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) {
+#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
+
 /**
  * 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)
+{
+	disk->flags |= _EFI_DISK_MARK_DELETE;
+}
+
+static void efi_disk_mark_undelete(struct efi_disk_obj *disk)
+{
+	disk->flags &= ~_EFI_DISK_MARK_DELETE;
+}
+
+static bool efi_disk_delete_marked(struct efi_disk_obj *disk)
+{
+	return disk->flags & _EFI_DISK_MARK_DELETE;
+}
+
+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);
+	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]);
+	}
+
+	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;
+
+	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);
+		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);
+	}
+
+	return ret;
+#else
+	return EFI_SUCCESS;
+#endif
+}