| Message ID | 20221003094459.107553-3-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_driver: fix error handling | expand |
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 <heinrich.schuchardt@canonical.com> > --- > 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
On 10/3/22 12:18, Ilias Apalodimas wrote: > 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 <heinrich.schuchardt@canonical.com> >> --- >> 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? We reach this point if either uclass UCLASS_EFI_LOADER does not exist or a device for the uclass has a negative value of devnum > -ENODEV. The caller did not reference a UEFI object involved that we could not find. And EFI_NOT_FOUND is not a return value foreseen by the UEFI specification. EFI_OUT_OF_RESOURCES reflects that there is a problem that the caller cannot fix. > >> >> 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) Yes, they should be moved to efi_delete_handle(). But that is beyond the scope of this patch. Best regards Heinrich > >> } >> >> /* 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
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; 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; } /* 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);
If creating the block device fails, * delete all created objects and references * close the protocol interface on the controller Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- 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(-)