[PATCH 2/2] efi_driver: fix error handling

[Heinrich Schuchardt]

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