diff mbox series

media: atomisp_gmin_platform: stop abusing efivar API

Message ID 20220620100819.1682995-1-ardb@kernel.org
State Accepted
Commit 38d4f74bc14847491d07bd745dc4a2c274f4987d
Headers show
Series media: atomisp_gmin_platform: stop abusing efivar API | expand

Commit Message

Ard Biesheuvel June 20, 2022, 10:08 a.m. UTC
As the code comment already suggests, using the efivar API in this way
is not how it is intended, and so let's switch to the right one, which
is simply to call efi.get_variable() directly after checking whether or
not the GetVariable() runtime service is supported.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---

If I can please get an ack, I'd like to take this via the EFI tree for
the next cycle.

 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 27 +++++---------------
 1 file changed, 6 insertions(+), 21 deletions(-)

Comments

Greg KH June 20, 2022, 10:26 a.m. UTC | #1
On Mon, Jun 20, 2022 at 12:08:19PM +0200, Ard Biesheuvel wrote:
> As the code comment already suggests, using the efivar API in this way
> is not how it is intended, and so let's switch to the right one, which
> is simply to call efi.get_variable() directly after checking whether or
> not the GetVariable() runtime service is supported.
> 
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> 
> If I can please get an ack, I'd like to take this via the EFI tree for
> the next cycle.
> 
>  drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 27 +++++---------------
>  1 file changed, 6 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index 7e47db82de07..bf527b366ab3 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -1284,7 +1284,7 @@ static int gmin_get_config_var(struct device *maindev,
>  	const struct dmi_system_id *id;
>  	struct device *dev = maindev;
>  	char var8[CFG_VAR_NAME_MAX];
> -	struct efivar_entry *ev;
> +	efi_status_t status;
>  	int i, ret;
>  
>  	/* For sensors, try first to use the _DSM table */
> @@ -1326,24 +1326,11 @@ static int gmin_get_config_var(struct device *maindev,
>  	for (i = 0; i < sizeof(var8) && var8[i]; i++)
>  		var16[i] = var8[i];
>  
> -	/* Not sure this API usage is kosher; efivar_entry_get()'s
> -	 * implementation simply uses VariableName and VendorGuid from
> -	 * the struct and ignores the rest, but it seems like there
> -	 * ought to be an "official" efivar_entry registered
> -	 * somewhere?
> -	 */
> -	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
> -	if (!ev)
> -		return -ENOMEM;
> -	memcpy(&ev->var.VariableName, var16, sizeof(var16));
> -	ev->var.VendorGuid = GMIN_CFG_VAR_EFI_GUID;
> -	ev->var.DataSize = *out_len;
> -
> -	ret = efivar_entry_get(ev, &ev->var.Attributes,
> -			       &ev->var.DataSize, ev->var.Data);
> -	if (ret == 0) {
> -		memcpy(out, ev->var.Data, ev->var.DataSize);
> -		*out_len = ev->var.DataSize;
> +	status = EFI_UNSUPPORTED;
> +	if (efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
> +		status = efi.get_variable(var16, &GMIN_CFG_VAR_EFI_GUID, NULL,
> +					  (unsigned long *)out_len, out);
> +	if (status == EFI_SUCCESS) {
>  		dev_info(maindev, "found EFI entry for '%s'\n", var8);
>  	} else if (is_gmin) {
>  		dev_info(maindev, "Failed to find EFI gmin variable %s\n", var8);
> @@ -1351,8 +1338,6 @@ static int gmin_get_config_var(struct device *maindev,
>  		dev_info(maindev, "Failed to find EFI variable %s\n", var8);
>  	}
>  
> -	kfree(ev);
> -
>  	return ret;
>  }
>  
> -- 
> 2.35.1
> 
> 

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff mbox series

Patch

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 7e47db82de07..bf527b366ab3 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -1284,7 +1284,7 @@  static int gmin_get_config_var(struct device *maindev,
 	const struct dmi_system_id *id;
 	struct device *dev = maindev;
 	char var8[CFG_VAR_NAME_MAX];
-	struct efivar_entry *ev;
+	efi_status_t status;
 	int i, ret;
 
 	/* For sensors, try first to use the _DSM table */
@@ -1326,24 +1326,11 @@  static int gmin_get_config_var(struct device *maindev,
 	for (i = 0; i < sizeof(var8) && var8[i]; i++)
 		var16[i] = var8[i];
 
-	/* Not sure this API usage is kosher; efivar_entry_get()'s
-	 * implementation simply uses VariableName and VendorGuid from
-	 * the struct and ignores the rest, but it seems like there
-	 * ought to be an "official" efivar_entry registered
-	 * somewhere?
-	 */
-	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
-	if (!ev)
-		return -ENOMEM;
-	memcpy(&ev->var.VariableName, var16, sizeof(var16));
-	ev->var.VendorGuid = GMIN_CFG_VAR_EFI_GUID;
-	ev->var.DataSize = *out_len;
-
-	ret = efivar_entry_get(ev, &ev->var.Attributes,
-			       &ev->var.DataSize, ev->var.Data);
-	if (ret == 0) {
-		memcpy(out, ev->var.Data, ev->var.DataSize);
-		*out_len = ev->var.DataSize;
+	status = EFI_UNSUPPORTED;
+	if (efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
+		status = efi.get_variable(var16, &GMIN_CFG_VAR_EFI_GUID, NULL,
+					  (unsigned long *)out_len, out);
+	if (status == EFI_SUCCESS) {
 		dev_info(maindev, "found EFI entry for '%s'\n", var8);
 	} else if (is_gmin) {
 		dev_info(maindev, "Failed to find EFI gmin variable %s\n", var8);
@@ -1351,8 +1338,6 @@  static int gmin_get_config_var(struct device *maindev,
 		dev_info(maindev, "Failed to find EFI variable %s\n", var8);
 	}
 
-	kfree(ev);
-
 	return ret;
 }