[PATCH v2 22/71] efi: Improve logging in efi_disk

[Heinrich Schuchardt]

On 1/8/23 03:49, Simon Glass wrote:
> When this fails it can be time-consuming to debug. Add some debugging
> to help with this. Also try to return error codes instead of just using
> -1.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> (no changes since v1)
>
>   lib/efi_loader/efi_disk.c | 30 +++++++++++++++++++++---------
>   1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 7ea0334083f..37123dd2474 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -421,13 +421,16 @@ static efi_status_t efi_disk_add_dev(
>
>   		if (!node) {
>   			ret = EFI_OUT_OF_RESOURCES;
> +			log_debug("no node\n");

Please, provide a descriptive message. I would not know what "no node"
might mean if I were to read it.

There is a reason why we set ret = EFI_OUT_OF_RESOURCES?

The caller should know what this means.

>   			goto error;
>   		}
>
>   		/* Parent must expose EFI_BLOCK_IO_PROTOCOL */
>   		ret = efi_search_protocol(parent, &efi_block_io_guid, &handler);
> -		if (ret != EFI_SUCCESS)
> +		if (ret != EFI_SUCCESS) {
> +			log_debug("search failed\n");

This message text is not descriptive enough.

>   			goto error;
> +		}
>
>   		/*
>   		 * Link the partition (child controller) to the block device
> @@ -436,8 +439,10 @@ static efi_status_t efi_disk_add_dev(
>   		ret = efi_protocol_open(handler, &protocol_interface, NULL,
>   					&diskobj->header,
>   					EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER);
> -		if (ret != EFI_SUCCESS)
> -				goto error;
> +		if (ret != EFI_SUCCESS) {
> +			log_debug("prot open failed\n");

ditto

> +			goto error;
> +		}
>
>   		diskobj->dp = efi_dp_append_node(dp_parent, node);
>   		efi_free_pool(node);
> @@ -468,8 +473,10 @@ static efi_status_t efi_disk_add_dev(
>   					 */
>   					esp_guid, NULL,
>   					NULL);
> -	if (ret != EFI_SUCCESS)
> +	if (ret != EFI_SUCCESS) {
> +		log_debug("install failed %lx\n", ret);

ditto

>   		goto error;
> +	}
>
>   	/*
>   	 * On partitions or whole disks without partitions install the
> @@ -482,8 +489,10 @@ static efi_status_t efi_disk_add_dev(
>   		ret = efi_add_protocol(&diskobj->header,
>   				       &efi_simple_file_system_protocol_guid,
>   				       diskobj->volume);
> -		if (ret != EFI_SUCCESS)
> +		if (ret != EFI_SUCCESS) {
> +			log_debug("simple FS failed\n");

It is not the the simple file system protocol that failed.

Assuming that you don't provide invalid parameters the error can only
occur if we are out of resources and ret will convey that message.

Did you really ever miss any of these messages?
All of these relate to internal errors and I would not find out what is
going wrong without debugging.

>   			return ret;
> +		}
>   	}
>   	diskobj->ops = block_io_disk_template;
>   	diskobj->dev_index = dev_index;
> @@ -552,18 +561,21 @@ static int efi_disk_create_raw(struct udevice *dev)
>   	ret = efi_disk_add_dev(NULL, NULL, desc,
>   			       diskid, NULL, 0, &disk);
>   	if (ret != EFI_SUCCESS) {
> -		if (ret == EFI_NOT_READY)
> +		if (ret == EFI_NOT_READY) {
>   			log_notice("Disk %s not ready\n", dev->name);
> -		else
> +			ret = -EBUSY;

Don't reuse variable ret which is of type efi_status_t. Use a separate
variable of type int instead.

> +		} else {
>   			log_err("Adding disk for %s failed (err=%ld/%#lx)\n", dev->name, ret, ret);
> +			ret = -ENOENT;

ditto

> +		}
>
> -		return -1;
> +		return ret;

ditto

>   	}
>   	if (efi_link_dev(&disk->header, dev)) {
>   		efi_free_pool(disk->dp);
>   		efi_delete_handle(&disk->header);
>
> -		return -1;
> +		return -EINVAL;

The caller will convert any of these return values to -1.
So this change seems superfluous.

Best regards

Heinrich

>   	}
>
>   	return 0;