[U-Boot] [PATCH 2/2] efi_loader: enumerate disk devices every time
AKASHI Takahiro
takahiro.akashi at linaro.org
Fri Nov 9 06:22:06 UTC 2018
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 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.
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
> > +}
> >
>
More information about the U-Boot
mailing list