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

AKASHI Takahiro takahiro.akashi at linaro.org
Wed Jan 9 01:05:33 UTC 2019


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


More information about the U-Boot mailing list