Re: [PATCH 1/2] efi: Export boot-services code and data as debugfs-blobs

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sat, Mar 31, 2018 at 02:19:43PM +0200, Hans de Goede wrote:
> Sometimes it is useful to be able to dump the efi boot-services code and
> data. This commit adds these as debugfs-blobs to /sys/kernel/debug/efi,
> but only if efi=debug is passed on the kernel-commandline as this requires
> not freeing those memory-regions, which costs 20+ MB of RAM.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  arch/x86/platform/efi/quirks.c |  4 +++
>  drivers/firmware/efi/efi.c     | 57 ++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 5b513ccffde4..0f968c7bcfec 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -374,6 +374,10 @@ void __init efi_free_boot_services(void)
>  	int num_entries = 0;
>  	void *new, *new_md;
>  
> +	/* Keep all regions for /sys/kernel/debug/efi */
> +	if (efi_enabled(EFI_DBG))
> +		return;
> +
>  	for_each_efi_memory_desc(md) {
>  		unsigned long long start = md->phys_addr;
>  		unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index cd42f66a7c85..fddc5f706fd2 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -18,6 +18,7 @@
>  #include <linux/kobject.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> +#include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/efi.h>
>  #include <linux/of.h>
> @@ -316,6 +317,59 @@ static __init int efivar_ssdt_load(void)
>  static inline int efivar_ssdt_load(void) { return 0; }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_FS
> +
> +#define EFI_DEBUGFS_MAX_BLOBS 32
> +
> +struct debugfs_blob_wrapper debugfs_blob[EFI_DEBUGFS_MAX_BLOBS];
> +
> +static void __init efi_debugfs_init(void)
> +{
> +	struct dentry *efi_debugfs;
> +	efi_memory_desc_t *md;
> +	char name[32];
> +	int type_count[EFI_BOOT_SERVICES_DATA + 1] = {};
> +	int i = 0;
> +
> +	efi_debugfs = debugfs_create_dir("efi", NULL);
> +	if (IS_ERR_OR_NULL(efi_debugfs)) {
> +		pr_warn("Could not create efi debugfs entry\n");
> +		return;
> +	}

{sigh}

No, don't warn, or complain, or do anything else if a debugfs call
fails.  Just keep on moving, you can always use the return value
properly in any future call if you need it, and no code flow should ever
care if a debugfs call succeeded or failed.


>  /*
>   * We register the efi subsystem with the firmware subsystem and the
>   * efivars subsystem with the efi subsystem, if the system was booted with
> @@ -360,6 +414,9 @@ static int __init efisubsys_init(void)
>  		goto err_remove_group;
>  	}
>  
> +	if (efi_enabled(EFI_DBG))
> +		efi_debugfs_init();

You never remove the directory?

thanks,

greg k-h