Re: [PATCH] media: atomisp_gmin_platform: stop abusing efivar API

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

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>