[1/2] dt-bindings: nvmem: convert U-Boot env to a layout

From: Rafał Miłecki <rafal@milecki.pl>

U-Boot environment variables can be stored in various data sources. MTD
is just one of available options. Refactor DT binding into a layout so
it can be used with UBI volumes and other NVMEM devices.

Link: https://lore.kernel.org/all/20231221173421.13737-1-zajec5@gmail.com/
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
I'm resending this approved by Rob PATCH in a proper series

 .../bindings/nvmem/layouts/nvmem-layout.yaml  |  1 +
 .../nvmem/{ => layouts}/u-boot,env.yaml       | 39 ++++++++++++++++---
 2 files changed, 35 insertions(+), 5 deletions(-)
 rename Documentation/devicetree/bindings/nvmem/{ => layouts}/u-boot,env.yaml (75%)

On Mon, 15 Jul 2024, at 13:54, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> diff --git a/drivers/nvmem/layouts/u-boot-env.c 
> b/drivers/nvmem/layouts/u-boot-env.c
> new file mode 100644
> index 000000000000..5217dc4a52f8
> --- /dev/null
> +++ b/drivers/nvmem/layouts/u-boot-env.c
> @@ -0,0 +1,203 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 - 2023 Rafał Miłecki <rafal@milecki.pl>
> + */
> +
> +#include <linux/crc32.h>
> +#include <linux/etherdevice.h>
> +#include <linux/export.h>
> +#include <linux/if_ether.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +
> +#include "u-boot-env.h"
> +
> +struct u_boot_env_image_single {
> +	__le32 crc32;
> +	uint8_t data[];
> +} __packed;
> +
> +struct u_boot_env_image_redundant {
> +	__le32 crc32;
> +	u8 mark;
> +	uint8_t data[];
> +} __packed;
> +
> +struct u_boot_env_image_broadcom {
> +	__le32 magic;
> +	__le32 len;
> +	__le32 crc32;
> +	DECLARE_FLEX_ARRAY(uint8_t, data);
> +} __packed;
> +
> +static int u_boot_env_read_post_process_ethaddr(void *context, const 
> char *id, int index,
> +						unsigned int offset, void *buf, size_t bytes)
> +{
> +	u8 mac[ETH_ALEN];
> +
> +	if (bytes != 3 * ETH_ALEN - 1)
> +		return -EINVAL;
> +
> +	if (!mac_pton(buf, mac))
> +		return -EINVAL;
> +
> +	if (index)
> +		eth_addr_add(mac, index);
> +
> +	ether_addr_copy(buf, mac);
> +
> +	return 0;
> +}
> +
> +static int u_boot_env_parse_cells(struct device *dev, struct 
> nvmem_device *nvmem, uint8_t *buf,
> +				  size_t data_offset, size_t data_len)
> +{
> +	char *data = buf + data_offset;
> +	char *var, *value, *eq;
> +
> +	for (var = data;
> +	     var < data + data_len && *var;
> +	     var = value + strlen(value) + 1) {
> +		struct nvmem_cell_info info = {};
> +
> +		eq = strchr(var, '=');
> +		if (!eq)
> +			break;
> +		*eq = '\0';
> +		value = eq + 1;
> +
> +		info.name = devm_kstrdup(dev, var, GFP_KERNEL);
> +		if (!info.name)
> +			return -ENOMEM;
> +		info.offset = data_offset + value - data;
> +		info.bytes = strlen(value);
> +		info.np = of_get_child_by_name(dev->of_node, info.name);
> +		if (!strcmp(var, "ethaddr")) {
> +			info.raw_len = strlen(value);
> +			info.bytes = ETH_ALEN;
> +			info.read_post_process = u_boot_env_read_post_process_ethaddr;
> +		}
> +
> +		nvmem_add_one_cell(nvmem, &info);
> +	}
> +
> +	return 0;
> +}
> +
> +int u_boot_env_parse(struct device *dev, struct nvmem_device *nvmem,
> +		     enum u_boot_env_format format)
> +{
> +	size_t crc32_data_offset;
> +	size_t crc32_data_len;
> +	size_t crc32_offset;
> +	__le32 *crc32_addr;
> +	size_t data_offset;
> +	size_t data_len;
> +	size_t dev_size;
> +	uint32_t crc32;
> +	uint32_t calc;
> +	uint8_t *buf;
> +	int bytes;
> +	int err;
> +
> +	dev_size = nvmem_dev_size(nvmem);
> +
> +	buf = kzalloc(dev_size, GFP_KERNEL);
> +	if (!buf) {
> +		err = -ENOMEM;
> +		goto err_out;
> +	}
> +
> +	bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
> +	if (bytes < 0) {
> +		err = bytes;
> +		goto err_kfree;
> +	} else if (bytes != dev_size) {
> +		err = -EIO;
> +		goto err_kfree;
> +	}
> +
> +	switch (format) {
> +	case U_BOOT_FORMAT_SINGLE:
> +		crc32_offset = offsetof(struct u_boot_env_image_single, crc32);
> +		crc32_data_offset = offsetof(struct u_boot_env_image_single, data);
> +		data_offset = offsetof(struct u_boot_env_image_single, data);
> +		break;
> +	case U_BOOT_FORMAT_REDUNDANT:
> +		crc32_offset = offsetof(struct u_boot_env_image_redundant, crc32);
> +		crc32_data_offset = offsetof(struct u_boot_env_image_redundant, 
> data);
> +		data_offset = offsetof(struct u_boot_env_image_redundant, data);
> +		break;
> +	case U_BOOT_FORMAT_BROADCOM:
> +		crc32_offset = offsetof(struct u_boot_env_image_broadcom, crc32);
> +		crc32_data_offset = offsetof(struct u_boot_env_image_broadcom, data);
> +		data_offset = offsetof(struct u_boot_env_image_broadcom, data);
> +		break;
> +	}

Could we error somewhere if bytes or dev_size is < crc32_data_offset
or alternatively if bytes is zero (above, beside bytes != dev_size)?
Detailed here: https://lore.kernel.org/lkml/20240612031510.14414-1-git@johnthomson.fastmail.com.au/
mtd partitioning can deliver a zero length partition (if misconfigured: Example u-boot partition starts beyond hardware NOR length).

> +	crc32_addr = (__le32 *)(buf + crc32_offset);
> +	crc32 = le32_to_cpu(*crc32_addr);
> +	crc32_data_len = dev_size - crc32_data_offset;
> +	data_len = dev_size - data_offset;
> +
> +	calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L;
> +	if (calc != crc32) {
> +		dev_err(dev, "Invalid calculated CRC32: 0x%08x (expected: 
> 0x%08x)\n", calc, crc32);
> +		err = -EINVAL;
> +		goto err_kfree;
> +	}
> +
> +	buf[dev_size - 1] = '\0';
> +	err = u_boot_env_parse_cells(dev, nvmem, buf, data_offset, data_len);
> +
> +err_kfree:
> +	kfree(buf);
> +err_out:
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(u_boot_env_parse);
> +
> +static int u_boot_env_add_cells(struct nvmem_layout *layout)
> +{
> +	struct device *dev = &layout->dev;
> +	enum u_boot_env_format format;
> +
> +	format = (uintptr_t)device_get_match_data(dev);
> +
> +	return u_boot_env_parse(dev, layout->nvmem, format);
> +}
> +
> +static int u_boot_env_probe(struct nvmem_layout *layout)
> +{
> +	layout->add_cells = u_boot_env_add_cells;
> +
> +	return nvmem_layout_register(layout);
> +}
> +
> +static void u_boot_env_remove(struct nvmem_layout *layout)
> +{
> +	nvmem_layout_unregister(layout);
> +}
> +
> +static const struct of_device_id u_boot_env_of_match_table[] = {
> +	{ .compatible = "u-boot,env", .data = (void *)U_BOOT_FORMAT_SINGLE, },
> +	{ .compatible = "u-boot,env-redundant-bool", .data = (void 
> *)U_BOOT_FORMAT_REDUNDANT, },
> +	{ .compatible = "u-boot,env-redundant-count", .data = (void 
> *)U_BOOT_FORMAT_REDUNDANT, },
> +	{ .compatible = "brcm,env", .data = (void *)U_BOOT_FORMAT_BROADCOM, },
> +	{},
> +};
> +
> +static struct nvmem_layout_driver u_boot_env_layout = {
> +	.driver = {
> +		.name = "u-boot-env-layout",
> +		.of_match_table = u_boot_env_of_match_table,
> +	},
> +	.probe = u_boot_env_probe,
> +	.remove = u_boot_env_remove,
> +};
> +module_nvmem_layout_driver(u_boot_env_layout);
> +
> +MODULE_AUTHOR("Rafał Miłecki");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(of, u_boot_env_of_match_table);

On 7/15/24 06:54, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> U-Boot environment variables are stored in a specific format. Actual
> data can be placed in various storage sources (MTD, UBI volume, EEPROM,
> NVRAM, etc.).
> 
> Move all generic (NVMEM device independent) code from NVMEM device
> driver to an NVMEM layout driver. Then add a simple NVMEM layout code on
> top of it.
> 
> This allows using NVMEM layout for parsing U-Boot env data stored in any
> kind of NVMEM device.
> 
> The old NVMEM glue driver stays in place for handling bindings in the
> MTD context. To avoid code duplication it uses exported layout parsing
> function. Please note that handling MTD & NVMEM layout bindings may be
> refactored in the future.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> This change was originally sent (and approved by Miquel) as a
> [PATCH V3 6/6] nvmem: layouts: add U-Boot env layout
> 
> I just adjusted it to the approved binding and updated commit message.
> I kept Miquel's Reviewed-by tag due to minimal changes.
> 
> I've successfully tested this code using it in both ways: as NVMEM
> device driver & NVMEM layout.
> 
>   MAINTAINERS                        |   1 +
>   drivers/nvmem/Kconfig              |   3 +-
>   drivers/nvmem/layouts/Kconfig      |  11 ++
>   drivers/nvmem/layouts/Makefile     |   1 +
>   drivers/nvmem/layouts/u-boot-env.c | 203 +++++++++++++++++++++++++++++
>   drivers/nvmem/layouts/u-boot-env.h |  15 +++
>   drivers/nvmem/u-boot-env.c         | 158 +---------------------
>   7 files changed, 234 insertions(+), 158 deletions(-)
>   create mode 100644 drivers/nvmem/layouts/u-boot-env.c
>   create mode 100644 drivers/nvmem/layouts/u-boot-env.h
> 
...

> +module_nvmem_layout_driver(u_boot_env_layout);
> +
> +MODULE_AUTHOR("Rafał Miłecki");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(of, u_boot_env_of_match_table);

Is this missing a MODULE_DESCRIPTION()?

Since commit 1fffe7a34c89 ("script: modpost: emit a warning when the 
description is missing") a module without a MODULE_DESCRIPTION() will 
result in a warning with make W=1.

I'll hopefully have all existing warnings fixed in 6.11 so please avoid 
adding new ones :)

/jeff