Hi Heinrich,
On Mon, Oct 03, 2022 at 11:44:59AM +0200, Heinrich Schuchardt wrote: > If creating the block device fails, > > * delete all created objects and references > * close the protocol interface on the controller > > Signed-off-by: Heinrich Schuchardt <[email protected]> > --- > include/efi_driver.h | 2 +- > lib/efi_driver/efi_block_device.c | 60 +++++++++++++++++-------------- > lib/efi_driver/efi_uclass.c | 23 ++++++------ > 3 files changed, 46 insertions(+), 39 deletions(-) > > diff --git a/include/efi_driver.h b/include/efi_driver.h > index 2b62219c5b..dc0c1c7ac0 100644 > --- a/include/efi_driver.h > +++ b/include/efi_driver.h > @@ -25,7 +25,7 @@ > struct efi_driver_ops { > const efi_guid_t *protocol; > const efi_guid_t *child_protocol; > - int (*bind)(efi_handle_t handle, void *interface); > + efi_status_t (*bind)(efi_handle_t handle, void *interface); > }; > > /* > diff --git a/lib/efi_driver/efi_block_device.c > b/lib/efi_driver/efi_block_device.c > index 3177ab67b8..9ccc148590 100644 > --- a/lib/efi_driver/efi_block_device.c > +++ b/lib/efi_driver/efi_block_device.c > @@ -112,12 +112,13 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t > blknr, lbaint_t blkcnt, > * > * @handle: handle > * @interface: block io protocol > - * Return: 0 = success > + * Return: status code > */ > -static int efi_bl_bind(efi_handle_t handle, void *interface) > +static efi_status_t efi_bl_bind(efi_handle_t handle, void *interface) > { > - struct udevice *bdev, *parent = dm_root(); > - int ret, devnum; > + struct udevice *bdev = NULL, *parent = dm_root(); > + efi_status_t ret; > + int devnum; > char *name; > struct efi_object *obj = efi_search_obj(handle); > struct efi_block_io *io = interface; > @@ -125,28 +126,28 @@ static int efi_bl_bind(efi_handle_t handle, void > *interface) > > EFI_PRINT("%s: handle %p, interface %p\n", __func__, handle, io); > > - if (!obj) > - return -ENOENT; > + if (!obj || !interface) > + return EFI_INVALID_PARAMETER; > > devnum = blk_find_max_devnum(UCLASS_EFI_LOADER); > if (devnum == -ENODEV) > devnum = 0; > else if (devnum < 0) > - return devnum; > + return EFI_OUT_OF_RESOURCES; Is EFI_NOT_FOUND a better option here? > > name = calloc(1, 18); /* strlen("efiblk#2147483648") + 1 */ > if (!name) > - return -ENOMEM; > + return EFI_OUT_OF_RESOURCES; > sprintf(name, "efiblk#%d", devnum); > > /* Create driver model udevice for the EFI block io device */ > - ret = blk_create_device(parent, "efi_blk", name, UCLASS_EFI_LOADER, > - devnum, io->media->block_size, > - (lbaint_t)io->media->last_block, &bdev); > - if (ret) > - return ret; > - if (!bdev) > - return -ENOENT; > + if (blk_create_device(parent, "efi_blk", name, UCLASS_EFI_LOADER, > + devnum, io->media->block_size, > + (lbaint_t)io->media->last_block, &bdev)) { > + ret = EFI_OUT_OF_RESOURCES; > + free(name); > + goto err; > + } > /* Set the DM_FLAG_NAME_ALLOCED flag to avoid a memory leak */ > device_set_name_alloced(bdev); > > @@ -154,20 +155,25 @@ static int efi_bl_bind(efi_handle_t handle, void > *interface) > plat->handle = handle; > plat->io = interface; > > - /* > - * FIXME: necessary because we won't do almost nothing in > - * efi_disk_create() when called from device_probe(). > - */ > - if (efi_link_dev(handle, bdev)) > - /* FIXME: cleanup for bdev */ > - return ret; > - > - ret = device_probe(bdev); > - if (ret) > - return ret; > + if (efi_link_dev(handle, bdev)) { > + ret = EFI_OUT_OF_RESOURCES; > + goto err; > + } > + > + if (device_probe(bdev)) { > + ret = EFI_DEVICE_ERROR; > + goto err; > + } > EFI_PRINT("%s: block device '%s' created\n", __func__, bdev->name); > > - return 0; > + return EFI_SUCCESS; > + > +err: > + efi_unlink_dev(handle); > + if (bdev) > + device_unbind(bdev); > + > + return ret; The efi_unlink_dev() is definitely needed. Would it also make sense to replace the open coded 'dev_tag_del(dev, DM_TAG_EFI);' instances with it? (there are 2 in efi_disk.c) > } > > /* Block device driver operators */ > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > index 74dd003243..5a285aad89 100644 > --- a/lib/efi_driver/efi_uclass.c > +++ b/lib/efi_driver/efi_uclass.c > @@ -11,7 +11,7 @@ > * The uclass provides the bind, start, and stop entry points for the driver > * binding protocol. > * > - * In bind() and stop() it checks if the controller implements the protocol > + * In supported() and bind() it checks if the controller implements the > protocol > * supported by the EFI driver. In the start() function it calls the bind() > * function of the EFI driver. In the stop() function it destroys the child > * controllers. > @@ -144,18 +144,19 @@ static efi_status_t EFIAPI efi_uc_start( > goto out; > } > ret = check_node_type(controller_handle); > - if (ret != EFI_SUCCESS) { > - r = EFI_CALL(systab.boottime->close_protocol( > - controller_handle, bp->ops->protocol, > - this->driver_binding_handle, > - controller_handle)); > - if (r != EFI_SUCCESS) > - EFI_PRINT("Failure to close handle\n"); > + if (ret != EFI_SUCCESS) > + goto err; > + ret = bp->ops->bind(controller_handle, interface); > + if (ret == EFI_SUCCESS) > goto out; > - } > > - /* TODO: driver specific stuff */ > - bp->ops->bind(controller_handle, interface); > +err: > + r = EFI_CALL(systab.boottime->close_protocol( > + controller_handle, bp->ops->protocol, > + this->driver_binding_handle, > + controller_handle)); > + if (r != EFI_SUCCESS) > + EFI_PRINT("Failure to close handle\n"); > > out: > return EFI_EXIT(ret); > -- > 2.37.2 > Thanks /Ilias
