[PATCH 0/4] efivar: remove inappropriate uses of the efivar API

[PATCH 0/4] efivar: remove inappropriate uses of the efivar API

[Ard Biesheuvel]

The efivar layer is a caching non-volatile variable store abstraction
that is normally backed by EFI, but in some cases, might be backed by
Google SMI firmware interfaces instead.

It is mainly used by efivarfs and EFI pstore, both of which actually
need the caching and abstraction properties. However, there are a few
other occurrences where efivar is not necessary, or used in an invalid
way. So let's fix this up, and remove some impediments to refactoring
and cleaning up the efivars layer in the future.

Assuming there are no objections to these changes, I intend to queue
them up in the EFI tree fairly soon, so that ongoing work depending on
these changes can continue as well.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Arend van Spriel <aspriel@gmail.com>
Cc: Franky Lin <franky.lin@broadcom.com>
Cc: Hante Meuleman <hante.meuleman@broadcom.com>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Gregory Greenman <gregory.greenman@intel.com>
Cc: linux-input@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: brcm80211-dev-list.pdl@broadcom.com

Ard Biesheuvel (4):
  efi: avoid efivars layer when loading SSDTs from variables
  Input: applespi - avoid efivars API and invoke EFI services directly
  iwlwifi: Switch to proper EFI variable store interface
  brcmfmac: Switch to appropriate helper to load EFI variable contents

 drivers/firmware/efi/efi.c                                  | 103 ++++++++------------
 drivers/input/keyboard/applespi.c                           |  42 +++-----
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c |  25 ++---
 drivers/net/wireless/intel/iwlwifi/fw/uefi.c                |  96 ++++++------------
 4 files changed, 95 insertions(+), 171 deletions(-)

-- 
2.35.1

[PATCH 1/4] efi: avoid efivars layer when loading SSDTs from variables

[Ard Biesheuvel]

The efivars intermediate variable access layer provides an abstraction
that permits the EFI variable store to be replaced by something else
that implements a compatible interface, and caches all variables in the
variable store for fast access via the efivarfs pseudo-filesystem.

The SSDT override feature does not take advantage of either feature, as
it is only used when the generic EFI implementation of efivars is used,
and it traverses all variables only once to find the ones it is
interested in, and frees all data structures that the efivars layer
keeps right after.

So in this case, let's just call EFI's code directly, using the function
pointers in struct efi.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/efi.c | 103 ++++++++------------
 1 file changed, 41 insertions(+), 62 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 860534bcfdac..630c18618f6a 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -202,7 +202,7 @@ static void generic_ops_unregister(void)
 }
 
 #ifdef CONFIG_EFI_CUSTOM_SSDT_OVERLAYS
-#define EFIVAR_SSDT_NAME_MAX	16
+#define EFIVAR_SSDT_NAME_MAX	16UL
 static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata;
 static int __init efivar_ssdt_setup(char *str)
 {
@@ -219,83 +219,62 @@ static int __init efivar_ssdt_setup(char *str)
 }
 __setup("efivar_ssdt=", efivar_ssdt_setup);
 
-static __init int efivar_ssdt_iter(efi_char16_t *name, efi_guid_t vendor,
-				   unsigned long name_size, void *data)
-{
-	struct efivar_entry *entry;
-	struct list_head *list = data;
-	char utf8_name[EFIVAR_SSDT_NAME_MAX];
-	int limit = min_t(unsigned long, EFIVAR_SSDT_NAME_MAX, name_size);
-
-	ucs2_as_utf8(utf8_name, name, limit - 1);
-	if (strncmp(utf8_name, efivar_ssdt, limit) != 0)
-		return 0;
-
-	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
-	if (!entry)
-		return 0;
-
-	memcpy(entry->var.VariableName, name, name_size);
-	memcpy(&entry->var.VendorGuid, &vendor, sizeof(efi_guid_t));
-
-	efivar_entry_add(entry, list);
-
-	return 0;
-}
-
 static __init int efivar_ssdt_load(void)
 {
-	LIST_HEAD(entries);
-	struct efivar_entry *entry, *aux;
-	unsigned long size;
-	void *data;
-	int ret;
+	unsigned long name_size = 256;
+	efi_char16_t *name = NULL;
+	efi_status_t status;
+	efi_guid_t guid;
 
 	if (!efivar_ssdt[0])
 		return 0;
 
-	ret = efivar_init(efivar_ssdt_iter, &entries, true, &entries);
-
-	list_for_each_entry_safe(entry, aux, &entries, list) {
-		pr_info("loading SSDT from variable %s-%pUl\n", efivar_ssdt,
-			&entry->var.VendorGuid);
+	name = kzalloc(name_size, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
 
-		list_del(&entry->list);
+	for (;;) {
+		char utf8_name[EFIVAR_SSDT_NAME_MAX];
+		unsigned long data_size = 0;
+		void *data;
+		int limit;
 
-		ret = efivar_entry_size(entry, &size);
-		if (ret) {
-			pr_err("failed to get var size\n");
-			goto free_entry;
+		status = efi.get_next_variable(&name_size, name, &guid);
+		if (status == EFI_NOT_FOUND) {
+			break;
+		} else if (status == EFI_BUFFER_TOO_SMALL) {
+			name = krealloc(name, name_size, GFP_KERNEL);
+			if (!name)
+				return -ENOMEM;
+			continue;
 		}
 
-		data = kmalloc(size, GFP_KERNEL);
-		if (!data) {
-			ret = -ENOMEM;
-			goto free_entry;
-		}
+		limit = min(EFIVAR_SSDT_NAME_MAX, name_size);
+		ucs2_as_utf8(utf8_name, name, limit - 1);
+		if (strncmp(utf8_name, efivar_ssdt, limit) != 0)
+			continue;
 
-		ret = efivar_entry_get(entry, NULL, &size, data);
-		if (ret) {
-			pr_err("failed to get var data\n");
-			goto free_data;
-		}
+		pr_info("loading SSDT from variable %s-%pUl\n", efivar_ssdt, &guid);
 
-		ret = acpi_load_table(data, NULL);
-		if (ret) {
-			pr_err("failed to load table: %d\n", ret);
-			goto free_data;
-		}
+		status = efi.get_variable(name, &guid, NULL, &data_size, NULL);
+		if (status != EFI_BUFFER_TOO_SMALL || !data_size)
+			return -EIO;
 
-		goto free_entry;
+		data = kmalloc(data_size, GFP_KERNEL);
+		if (!data)
+			return -ENOMEM;
 
-free_data:
+		status = efi.get_variable(name, &guid, NULL, &data_size, data);
+		if (status == EFI_SUCCESS) {
+			acpi_status ret = acpi_load_table(data, NULL);
+			if (ret)
+				pr_err("failed to load table: %u\n", ret);
+		} else {
+			pr_err("failed to get var data: 0x%lx\n", status);
+		}
 		kfree(data);
-
-free_entry:
-		kfree(entry);
 	}
-
-	return ret;
+	return 0;
 }
 #else
 static inline int efivar_ssdt_load(void) { return 0; }
-- 
2.35.1

[PATCH 2/4] Input: applespi - avoid efivars API and invoke EFI services directly

[Ard Biesheuvel]

This driver abuses the efivar API, by using a few of its helpers on
entries that were not instantiated by the API itself. This is a problem
as future cleanup work on efivars is complicated by this.

So let's just switch to the get/set variable runtime wrappers directly.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/input/keyboard/applespi.c | 42 +++++++-------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index d1f5354d5ea2..cbc6c0d4670a 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -1597,52 +1597,38 @@ static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context)
 
 static int applespi_get_saved_bl_level(struct applespi_data *applespi)
 {
-	struct efivar_entry *efivar_entry;
+	efi_status_t sts = EFI_NOT_FOUND;
 	u16 efi_data = 0;
-	unsigned long efi_data_len;
-	int sts;
-
-	efivar_entry = kmalloc(sizeof(*efivar_entry), GFP_KERNEL);
-	if (!efivar_entry)
-		return -ENOMEM;
-
-	memcpy(efivar_entry->var.VariableName, EFI_BL_LEVEL_NAME,
-	       sizeof(EFI_BL_LEVEL_NAME));
-	efivar_entry->var.VendorGuid = EFI_BL_LEVEL_GUID;
-	efi_data_len = sizeof(efi_data);
+	unsigned long efi_data_len = sizeof(efi_data);
 
-	sts = efivar_entry_get(efivar_entry, NULL, &efi_data_len, &efi_data);
-	if (sts && sts != -ENOENT)
+	if (efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
+		sts = efi.get_variable(EFI_BL_LEVEL_NAME, &EFI_BL_LEVEL_GUID,
+				       NULL, &efi_data_len, &efi_data);
+	if (sts != EFI_SUCCESS && sts != EFI_NOT_FOUND)
 		dev_warn(&applespi->spi->dev,
-			 "Error getting backlight level from EFI vars: %d\n",
+			 "Error getting backlight level from EFI vars: 0x%lx\n",
 			 sts);
 
-	kfree(efivar_entry);
-
-	return sts ? sts : efi_data;
+	return sts != EFI_SUCCESS ? -ENODEV : efi_data;
 }
 
 static void applespi_save_bl_level(struct applespi_data *applespi,
 				   unsigned int level)
 {
-	efi_guid_t efi_guid;
+	efi_status_t sts = EFI_UNSUPPORTED;
 	u32 efi_attr;
-	unsigned long efi_data_len;
 	u16 efi_data;
-	int sts;
 
-	/* Save keyboard backlight level */
-	efi_guid = EFI_BL_LEVEL_GUID;
 	efi_data = (u16)level;
-	efi_data_len = sizeof(efi_data);
 	efi_attr = EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
 		   EFI_VARIABLE_RUNTIME_ACCESS;
 
-	sts = efivar_entry_set_safe((efi_char16_t *)EFI_BL_LEVEL_NAME, efi_guid,
-				    efi_attr, true, efi_data_len, &efi_data);
-	if (sts)
+	if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE))
+		sts = efi.set_variable(EFI_BL_LEVEL_NAME, &EFI_BL_LEVEL_GUID,
+				       efi_attr, sizeof(efi_data), &efi_data);
+	if (sts != EFI_SUCCESS)
 		dev_warn(&applespi->spi->dev,
-			 "Error saving backlight level to EFI vars: %d\n", sts);
+			 "Error saving backlight level to EFI vars: 0x%lx\n", sts);
 }
 
 static int applespi_probe(struct spi_device *spi)
-- 
2.35.1

Re: [PATCH 2/4] Input: applespi - avoid efivars API and invoke EFI services directly

[Ard Biesheuvel]

On Fri, 17 Jun 2022 at 19:49, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> This driver abuses the efivar API, by using a few of its helpers on
> entries that were not instantiated by the API itself. This is a problem
> as future cleanup work on efivars is complicated by this.
>
> So let's just switch to the get/set variable runtime wrappers directly.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Unless anyone minds, I will queue this up in efi/next

> ---
>  drivers/input/keyboard/applespi.c | 42 +++++++-------------
>  1 file changed, 14 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index d1f5354d5ea2..cbc6c0d4670a 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1597,52 +1597,38 @@ static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context)
>
>  static int applespi_get_saved_bl_level(struct applespi_data *applespi)
>  {
> -       struct efivar_entry *efivar_entry;
> +       efi_status_t sts = EFI_NOT_FOUND;
>         u16 efi_data = 0;
> -       unsigned long efi_data_len;
> -       int sts;
> -
> -       efivar_entry = kmalloc(sizeof(*efivar_entry), GFP_KERNEL);
> -       if (!efivar_entry)
> -               return -ENOMEM;
> -
> -       memcpy(efivar_entry->var.VariableName, EFI_BL_LEVEL_NAME,
> -              sizeof(EFI_BL_LEVEL_NAME));
> -       efivar_entry->var.VendorGuid = EFI_BL_LEVEL_GUID;
> -       efi_data_len = sizeof(efi_data);
> +       unsigned long efi_data_len = sizeof(efi_data);
>
> -       sts = efivar_entry_get(efivar_entry, NULL, &efi_data_len, &efi_data);
> -       if (sts && sts != -ENOENT)
> +       if (efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
> +               sts = efi.get_variable(EFI_BL_LEVEL_NAME, &EFI_BL_LEVEL_GUID,
> +                                      NULL, &efi_data_len, &efi_data);
> +       if (sts != EFI_SUCCESS && sts != EFI_NOT_FOUND)
>                 dev_warn(&applespi->spi->dev,
> -                        "Error getting backlight level from EFI vars: %d\n",
> +                        "Error getting backlight level from EFI vars: 0x%lx\n",
>                          sts);
>
> -       kfree(efivar_entry);
> -
> -       return sts ? sts : efi_data;
> +       return sts != EFI_SUCCESS ? -ENODEV : efi_data;
>  }
>
>  static void applespi_save_bl_level(struct applespi_data *applespi,
>                                    unsigned int level)
>  {
> -       efi_guid_t efi_guid;
> +       efi_status_t sts = EFI_UNSUPPORTED;
>         u32 efi_attr;
> -       unsigned long efi_data_len;
>         u16 efi_data;
> -       int sts;
>
> -       /* Save keyboard backlight level */
> -       efi_guid = EFI_BL_LEVEL_GUID;
>         efi_data = (u16)level;
> -       efi_data_len = sizeof(efi_data);
>         efi_attr = EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
>                    EFI_VARIABLE_RUNTIME_ACCESS;
>
> -       sts = efivar_entry_set_safe((efi_char16_t *)EFI_BL_LEVEL_NAME, efi_guid,
> -                                   efi_attr, true, efi_data_len, &efi_data);
> -       if (sts)
> +       if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE))
> +               sts = efi.set_variable(EFI_BL_LEVEL_NAME, &EFI_BL_LEVEL_GUID,
> +                                      efi_attr, sizeof(efi_data), &efi_data);
> +       if (sts != EFI_SUCCESS)
>                 dev_warn(&applespi->spi->dev,
> -                        "Error saving backlight level to EFI vars: %d\n", sts);
> +                        "Error saving backlight level to EFI vars: 0x%lx\n", sts);
>  }
>
>  static int applespi_probe(struct spi_device *spi)
> --
> 2.35.1
>

[PATCH 3/4] iwlwifi: Switch to proper EFI variable store interface

[Ard Biesheuvel]

Using half of the efivar API with locally baked efivar_entry instances
is not the right way to use this API, and these uses impede planned work
on the efivar layer itself.

So switch to direct EFI variable store accesses: we don't need the
efivar layer anyway.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/net/wireless/intel/iwlwifi/fw/uefi.c | 96 +++++++-------------
 1 file changed, 32 insertions(+), 64 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/fw/uefi.c b/drivers/net/wireless/intel/iwlwifi/fw/uefi.c
index 23b1d689ba7b..9854466c21d9 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/uefi.c
+++ b/drivers/net/wireless/intel/iwlwifi/fw/uefi.c
@@ -19,20 +19,14 @@
 
 void *iwl_uefi_get_pnvm(struct iwl_trans *trans, size_t *len)
 {
-	struct efivar_entry *pnvm_efivar;
 	void *data;
 	unsigned long package_size;
-	int err;
+	efi_status_t status;
 
 	*len = 0;
 
-	pnvm_efivar = kzalloc(sizeof(*pnvm_efivar), GFP_KERNEL);
-	if (!pnvm_efivar)
-		return ERR_PTR(-ENOMEM);
-
-	memcpy(&pnvm_efivar->var.VariableName, IWL_UEFI_OEM_PNVM_NAME,
-	       sizeof(IWL_UEFI_OEM_PNVM_NAME));
-	pnvm_efivar->var.VendorGuid = IWL_EFI_VAR_GUID;
+	if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
+		return ERR_PTR(-ENODEV);
 
 	/*
 	 * TODO: we hardcode a maximum length here, because reading
@@ -42,27 +36,22 @@ void *iwl_uefi_get_pnvm(struct iwl_trans *trans, size_t *len)
 	package_size = IWL_HARDCODED_PNVM_SIZE;
 
 	data = kmalloc(package_size, GFP_KERNEL);
-	if (!data) {
-		data = ERR_PTR(-ENOMEM);
-		goto out;
-	}
+	if (!data)
+		return ERR_PTR(-ENOMEM);
 
-	err = efivar_entry_get(pnvm_efivar, NULL, &package_size, data);
-	if (err) {
+	status = efi.get_variable(IWL_UEFI_OEM_PNVM_NAME, &IWL_EFI_VAR_GUID,
+				  NULL, &package_size, data);
+	if (status != EFI_SUCCESS) {
 		IWL_DEBUG_FW(trans,
-			     "PNVM UEFI variable not found %d (len %lu)\n",
-			     err, package_size);
+			     "PNVM UEFI variable not found 0x%lx (len %lu)\n",
+			     status, package_size);
 		kfree(data);
-		data = ERR_PTR(err);
-		goto out;
+		return ERR_PTR(efi_status_to_err(status));
 	}
 
 	IWL_DEBUG_FW(trans, "Read PNVM from UEFI with size %lu\n", package_size);
 	*len = package_size;
 
-out:
-	kfree(pnvm_efivar);
-
 	return data;
 }
 
@@ -211,21 +200,15 @@ static void *iwl_uefi_reduce_power_parse(struct iwl_trans *trans,
 
 void *iwl_uefi_get_reduced_power(struct iwl_trans *trans, size_t *len)
 {
-	struct efivar_entry *reduce_power_efivar;
 	struct pnvm_sku_package *package;
 	void *data = NULL;
 	unsigned long package_size;
-	int err;
+	efi_status_t status;
 
 	*len = 0;
 
-	reduce_power_efivar = kzalloc(sizeof(*reduce_power_efivar), GFP_KERNEL);
-	if (!reduce_power_efivar)
-		return ERR_PTR(-ENOMEM);
-
-	memcpy(&reduce_power_efivar->var.VariableName, IWL_UEFI_REDUCED_POWER_NAME,
-	       sizeof(IWL_UEFI_REDUCED_POWER_NAME));
-	reduce_power_efivar->var.VendorGuid = IWL_EFI_VAR_GUID;
+	if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
+		return ERR_PTR(-ENODEV);
 
 	/*
 	 * TODO: we hardcode a maximum length here, because reading
@@ -235,19 +218,17 @@ void *iwl_uefi_get_reduced_power(struct iwl_trans *trans, size_t *len)
 	package_size = IWL_HARDCODED_REDUCE_POWER_SIZE;
 
 	package = kmalloc(package_size, GFP_KERNEL);
-	if (!package) {
-		package = ERR_PTR(-ENOMEM);
-		goto out;
-	}
+	if (!package)
+		return ERR_PTR(-ENOMEM);
 
-	err = efivar_entry_get(reduce_power_efivar, NULL, &package_size, package);
-	if (err) {
+	status = efi.get_variable(IWL_UEFI_REDUCED_POWER_NAME, &IWL_EFI_VAR_GUID,
+				  NULL, &package_size, data);
+	if (status != EFI_SUCCESS) {
 		IWL_DEBUG_FW(trans,
-			     "Reduced Power UEFI variable not found %d (len %lu)\n",
-			     err, package_size);
+			     "Reduced Power UEFI variable not found 0x%lx (len %lu)\n",
+			     status, package_size);
 		kfree(package);
-		data = ERR_PTR(err);
-		goto out;
+		return ERR_PTR(efi_status_to_err(status));
 	}
 
 	IWL_DEBUG_FW(trans, "Read reduced power from UEFI with size %lu\n",
@@ -262,9 +243,6 @@ void *iwl_uefi_get_reduced_power(struct iwl_trans *trans, size_t *len)
 
 	kfree(package);
 
-out:
-	kfree(reduce_power_efivar);
-
 	return data;
 }
 
@@ -304,22 +282,15 @@ static int iwl_uefi_sgom_parse(struct uefi_cnv_wlan_sgom_data *sgom_data,
 void iwl_uefi_get_sgom_table(struct iwl_trans *trans,
 			     struct iwl_fw_runtime *fwrt)
 {
-	struct efivar_entry *sgom_efivar;
 	struct uefi_cnv_wlan_sgom_data *data;
 	unsigned long package_size;
-	int err, ret;
-
-	if (!fwrt->geo_enabled)
-		return;
+	efi_status_t status;
+	int ret;
 
-	sgom_efivar = kzalloc(sizeof(*sgom_efivar), GFP_KERNEL);
-	if (!sgom_efivar)
+	if (!fwrt->geo_enabled ||
+	    !efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
 		return;
 
-	memcpy(&sgom_efivar->var.VariableName, IWL_UEFI_SGOM_NAME,
-	       sizeof(IWL_UEFI_SGOM_NAME));
-	sgom_efivar->var.VendorGuid = IWL_EFI_VAR_GUID;
-
 	/* TODO: we hardcode a maximum length here, because reading
 	 * from the UEFI is not working.  To implement this properly,
 	 * we have to call efivar_entry_size().
@@ -327,15 +298,14 @@ void iwl_uefi_get_sgom_table(struct iwl_trans *trans,
 	package_size = IWL_HARDCODED_SGOM_SIZE;
 
 	data = kmalloc(package_size, GFP_KERNEL);
-	if (!data) {
-		data = ERR_PTR(-ENOMEM);
-		goto out;
-	}
+	if (!data)
+		return;
 
-	err = efivar_entry_get(sgom_efivar, NULL, &package_size, data);
-	if (err) {
+	status = efi.get_variable(IWL_UEFI_SGOM_NAME, &IWL_EFI_VAR_GUID,
+				  NULL, &package_size, data);
+	if (status != EFI_SUCCESS) {
 		IWL_DEBUG_FW(trans,
-			     "SGOM UEFI variable not found %d\n", err);
+			     "SGOM UEFI variable not found 0x%lx\n", status);
 		goto out_free;
 	}
 
@@ -349,8 +319,6 @@ void iwl_uefi_get_sgom_table(struct iwl_trans *trans,
 out_free:
 	kfree(data);
 
-out:
-	kfree(sgom_efivar);
 }
 IWL_EXPORT_SYMBOL(iwl_uefi_get_sgom_table);
 #endif /* CONFIG_ACPI */
-- 
2.35.1

[PATCH 4/4] brcmfmac: Switch to appropriate helper to load EFI variable contents

[Ard Biesheuvel]

Avoid abusing the efivar layer by invoking it with locally constructed
efivar_entry instances, and instead, just call the EFI routines directly
if available.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 25 +++++++-------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index dcbe55b56e43..b8379e4034a4 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -459,43 +459,34 @@ static void brcmf_fw_fix_efi_nvram_ccode(char *data, unsigned long data_len)
 
 static u8 *brcmf_fw_nvram_from_efi(size_t *data_len_ret)
 {
-	const u16 name[] = { 'n', 'v', 'r', 'a', 'm', 0 };
-	struct efivar_entry *nvram_efivar;
+	efi_guid_t guid = EFI_GUID(0x74b00bd9, 0x805a, 0x4d61, 0xb5, 0x1f,
+				   0x43, 0x26, 0x81, 0x23, 0xd1, 0x13);
 	unsigned long data_len = 0;
+	efi_status_t status;
 	u8 *data = NULL;
-	int err;
 
-	nvram_efivar = kzalloc(sizeof(*nvram_efivar), GFP_KERNEL);
-	if (!nvram_efivar)
+	if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
 		return NULL;
 
-	memcpy(&nvram_efivar->var.VariableName, name, sizeof(name));
-	nvram_efivar->var.VendorGuid = EFI_GUID(0x74b00bd9, 0x805a, 0x4d61,
-						0xb5, 0x1f, 0x43, 0x26,
-						0x81, 0x23, 0xd1, 0x13);
-
-	err = efivar_entry_size(nvram_efivar, &data_len);
-	if (err)
+	status = efi.get_variable(L"nvram", &guid, NULL, &data_len, NULL);
+	if (status != EFI_BUFFER_TOO_SMALL)
 		goto fail;
 
 	data = kmalloc(data_len, GFP_KERNEL);
 	if (!data)
 		goto fail;
 
-	err = efivar_entry_get(nvram_efivar, NULL, &data_len, data);
-	if (err)
+	status = efi.get_variable(L"nvram", &guid, NULL, &data_len, data);
+	if (status != EFI_SUCCESS)
 		goto fail;
 
 	brcmf_fw_fix_efi_nvram_ccode(data, data_len);
 	brcmf_info("Using nvram EFI variable\n");
 
-	kfree(nvram_efivar);
 	*data_len_ret = data_len;
 	return data;
-
 fail:
 	kfree(data);
-	kfree(nvram_efivar);
 	return NULL;
 }
 #else
-- 
2.35.1

Re: [PATCH 0/4] efivar: remove inappropriate uses of the efivar API

[Kalle Valo]

Ard Biesheuvel <ardb@kernel.org> writes:

> The efivar layer is a caching non-volatile variable store abstraction
> that is normally backed by EFI, but in some cases, might be backed by
> Google SMI firmware interfaces instead.
>
> It is mainly used by efivarfs and EFI pstore, both of which actually
> need the caching and abstraction properties. However, there are a few
> other occurrences where efivar is not necessary, or used in an invalid
> way. So let's fix this up, and remove some impediments to refactoring
> and cleaning up the efivars layer in the future.
>
> Assuming there are no objections to these changes, I intend to queue
> them up in the EFI tree fairly soon, so that ongoing work depending on
> these changes can continue as well.
>

[...]

>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c |  25 ++---
>  drivers/net/wireless/intel/iwlwifi/fw/uefi.c                |  96 ++++++------------

Feel free to take the wireless patches via your tree:

Acked-by: Kalle Valo <kvalo@kernel.org>

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 0/4] efivar: remove inappropriate uses of the efivar API

[Ard Biesheuvel]

On Mon, 20 Jun 2022 at 11:00, Kalle Valo <kvalo@kernel.org> wrote:
>
> Ard Biesheuvel <ardb@kernel.org> writes:
>
> > The efivar layer is a caching non-volatile variable store abstraction
> > that is normally backed by EFI, but in some cases, might be backed by
> > Google SMI firmware interfaces instead.
> >
> > It is mainly used by efivarfs and EFI pstore, both of which actually
> > need the caching and abstraction properties. However, there are a few
> > other occurrences where efivar is not necessary, or used in an invalid
> > way. So let's fix this up, and remove some impediments to refactoring
> > and cleaning up the efivars layer in the future.
> >
> > Assuming there are no objections to these changes, I intend to queue
> > them up in the EFI tree fairly soon, so that ongoing work depending on
> > these changes can continue as well.
> >
>
> [...]
>
> >  drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c |  25 ++---
> >  drivers/net/wireless/intel/iwlwifi/fw/uefi.c                |  96 ++++++------------
>
> Feel free to take the wireless patches via your tree:
>
> Acked-by: Kalle Valo <kvalo@kernel.org>
>

Thanks, I've queued these up.