Re: [PATCH RFC 2/5] nvmem: Add UEFI variable NVMEM provider

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 4966 bytes --]

On Wed, Oct 14, 2026 at 04:02:40PM +0300, Abel Vesa wrote:
> +#include <linux/mod_devicetable.h>
> [...]
> +#include <linux/platform_device.h>

<linux/mod_devicetable.h> pulls in a lot of unneeded dependencies and
<linux/platform_device.h> already provides of_device_id. So please drop
the former.

> +static int efivar_nvmem_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const char *name, *guid_str;
> +	struct nvmem_config cfg = {};
> +	struct efivar_nvmem *priv;
> +	struct nvmem_device *nvmem;
> +	efi_char16_t *name16;
> +	efi_guid_t guid;
> +	guid_t g;
> +	unsigned long size;
> +	efi_status_t status;
> +	u32 attr;
> +	int i, len, ret;
> +	void *buf;
> +
> +	/*
> +	 * The EFI variable service may not be a native EFI runtime service;
> +	 * backends such as the Qualcomm uefisecapp driver register the
> +	 * efivars operations asynchronously.  Wait for them.
> +	 */
> +	if (!efivar_is_available())
> +		return -EPROBE_DEFER;
> +
> +	ret = device_property_read_string(dev, "efivar-name", &name);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "missing 'efivar-name'\n");
> +
> +	/* ASCII variable name -> UCS-2 as expected by the EFI service */
> +	len = strlen(name);
> +	name16 = kcalloc(len + 1, sizeof(*name16), GFP_KERNEL);
> +	if (!name16)
> +		return -ENOMEM;
> +	for (i = 0; i < len; i++)
> +		name16[i] = name[i];
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		kfree(name16);
> +		return -ENOMEM;
> +	}
> +
> +	/*
> +	 * The vendor GUID is optional: if it is not provided, look the
> +	 * variable up by name across the whole store.  A variable that has
> +	 * not been provisioned yet is reported as not found and handled
> +	 * below like an erased EEPROM.
> +	 */
> +	if (!device_property_read_string(dev, "efivar-guid", &guid_str)) {
> +		ret = guid_parse(guid_str, &g);
> +		if (ret) {
> +			kfree(name16);
> +			return dev_err_probe(dev, ret, "invalid 'efivar-guid'\n");
> +		}
> +		memcpy(&guid, &g, sizeof(guid));
> +		status = EFI_SUCCESS;
> +	} else {
> +		ret = efivar_nvmem_find_guid(name16, &guid);
> +		if (ret && ret != -ENODEV) {
> +			kfree(name16);
> +			return dev_err_probe(dev, ret,
> +					     "failed to look up variable '%s'\n",
> +					     name);
> +		}
> +		status = ret ? EFI_NOT_FOUND : EFI_SUCCESS;
> +	}
> +
> +	/* First attempt with a modest buffer; grow if the firmware asks. */
> +	size = 512;
> +	buf = kzalloc(size, GFP_KERNEL);
> +	if (!buf) {
> +		kfree(name16);
> +		return -ENOMEM;
> +	}
> +
> +	if (status == EFI_SUCCESS) {
> +		status = efivar_get_variable(name16, &guid, &attr, &size, buf);
> +		if (status == EFI_BUFFER_TOO_SMALL) {
> +			kfree(buf);
> +			buf = kzalloc(size, GFP_KERNEL);
> +			if (!buf) {
> +				kfree(name16);
> +				return -ENOMEM;
> +			}
> +			status = efivar_get_variable(name16, &guid, &attr,
> +						     &size, buf);
> +		}
> +	}
> +	kfree(name16);
> +
> +	if (status == EFI_NOT_FOUND) {
> +		/*
> +		 * Variable not provisioned yet: behave like an erased
> +		 * EEPROM so that consumers still probe and fall back to
> +		 * their defaults.
> +		 */
> +		kfree(buf);
> +		size = efivar_nvmem_cells_extent(dev);
> +		if (!size)
> +			return dev_err_probe(dev, -ENODEV,
> +					     "variable '%s' not found and no cells described\n",
> +					     name);
> +		buf = kzalloc(size, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
> +		dev_info(dev, "variable '%s' not provisioned, reading as zeroes\n",
> +			 name);
> +	} else if (status != EFI_SUCCESS) {
> +		kfree(buf);
> +		return dev_err_probe(dev, -ENODEV,
> +				     "GetVariable('%s') failed: 0x%lx\n",
> +				     name, status);
> +	} else {
> +		dev_dbg(dev, "variable '%s' attr=0x%x size=%lu\n",
> +			name, attr, size);
> +	}
> +
> +	priv->data = devm_kmemdup(dev, buf, size, GFP_KERNEL);
> +	kfree(buf);
> +	if (!priv->data)
> +		return -ENOMEM;
> +	priv->size = size;
> +
> +	cfg.dev			= dev;
> +	cfg.name		= "efivar";
> +	cfg.id			= NVMEM_DEVID_AUTO;
> +	cfg.owner		= THIS_MODULE;
> +	cfg.type		= NVMEM_TYPE_OTP;
> +	cfg.read_only		= true;
> +	cfg.word_size		= 1;
> +	cfg.stride		= 1;
> +	cfg.size		= priv->size;
> +	cfg.reg_read		= efivar_nvmem_reg_read;
> +	cfg.priv		= priv;
> +	cfg.of_node		= dev->of_node;
> +	cfg.add_legacy_fixed_of_cells = true;

having only nearly all of these aligned is really strange. If you ask
me, don't try to align =, just use a single space before it.

You can write this as:

	cfg = (typeof(cfg)) {
		.dev = dev,
		.name = "efivar",
		...
	};

which is a bit less repetitive and easier for the compiler to optimize.

> +	nvmem = devm_nvmem_register(dev, &cfg);

Does nvmem_register only return success or -ENOMEM? If not, a
dev_err_probe() would be nice here.

> +	return PTR_ERR_OR_ZERO(nvmem);
> +}
> +
> +static const struct of_device_id efivar_nvmem_of_match[] = {
> +	{ .compatible = "nvmem-efivar" },
> +	{}

s/{}/{ }/

> +};
> +MODULE_DEVICE_TABLE(of, efivar_nvmem_of_match);

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]