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

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

[Rafał Miłecki]

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%)

diff --git a/Documentation/devicetree/bindings/nvmem/layouts/nvmem-layout.yaml b/Documentation/devicetree/bindings/nvmem/layouts/nvmem-layout.yaml
index 3b40f7880774..382507060651 100644
--- a/Documentation/devicetree/bindings/nvmem/layouts/nvmem-layout.yaml
+++ b/Documentation/devicetree/bindings/nvmem/layouts/nvmem-layout.yaml
@@ -21,6 +21,7 @@ oneOf:
   - $ref: fixed-layout.yaml
   - $ref: kontron,sl28-vpd.yaml
   - $ref: onie,tlv-layout.yaml
+  - $ref: u-boot,env.yaml
 
 properties:
   compatible: true
diff --git a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env.yaml
similarity index 75%
rename from Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
rename to Documentation/devicetree/bindings/nvmem/layouts/u-boot,env.yaml
index 9c36afc7084b..56a8f55d4a09 100644
--- a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
+++ b/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env.yaml
@@ -1,10 +1,10 @@
 # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/nvmem/u-boot,env.yaml#
+$id: http://devicetree.org/schemas/nvmem/layouts/u-boot,env.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: U-Boot environment variables
+title: U-Boot environment variables layout
 
 description: |
   U-Boot uses environment variables to store device parameters and
@@ -21,9 +21,6 @@ description: |
   This binding allows marking storage device (as containing env data) and
   specifying used format.
 
-  Right now only flash partition case is covered but it may be extended to e.g.
-  UBI volumes in the future.
-
   Variables can be defined as NVMEM device subnodes.
 
 maintainers:
@@ -42,6 +39,7 @@ properties:
         const: brcm,env
 
   reg:
+    description: Partition offset and size for env on top of MTD
     maxItems: 1
 
   bootcmd:
@@ -58,6 +56,17 @@ properties:
         description: The first argument is a MAC address offset.
         const: 1
 
+allOf:
+  - if:
+      properties:
+        $nodename:
+          not:
+            contains:
+              pattern: "^partition@[0-9a-f]+$"
+    then:
+      properties:
+        reg: false
+
 additionalProperties: false
 
 examples:
@@ -101,3 +110,23 @@ examples:
             };
         };
     };
+  - |
+    partition@0 {
+        reg = <0x0 0x100000>;
+        label = "ubi";
+        compatible = "linux,ubi";
+
+        volumes {
+            ubi-volume-u-boot-env {
+                volname = "env";
+
+                nvmem-layout {
+                    compatible = "u-boot,env";
+
+                    ethaddr {
+                        #nvmem-cell-cells = <1>;
+                    };
+                };
+            };
+        };
+    };
-- 
2.35.3

[PATCH 2/2] nvmem: layouts: add U-Boot env layout

[Rafał Miłecki]

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 31fb0031a5e8..630d16c480dd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23331,6 +23331,7 @@ U-BOOT ENVIRONMENT VARIABLES
 M:	Rafał Miłecki <rafal@milecki.pl>
 S:	Maintained
 F:	Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
+F:	drivers/nvmem/layouts/u-boot-env.c
 F:	drivers/nvmem/u-boot-env.c
 
 UACCE ACCELERATOR FRAMEWORK
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 283134498fbc..d2c384f58028 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -363,8 +363,7 @@ config NVMEM_SUNXI_SID
 config NVMEM_U_BOOT_ENV
 	tristate "U-Boot environment variables support"
 	depends on OF && MTD
-	select CRC32
-	select GENERIC_NET_UTILS
+	select NVMEM_LAYOUT_U_BOOT_ENV
 	help
 	  U-Boot stores its setup as environment variables. This driver adds
 	  support for verifying & exporting such data. It also exposes variables
diff --git a/drivers/nvmem/layouts/Kconfig b/drivers/nvmem/layouts/Kconfig
index 9c6e672fc350..5e586dfebe47 100644
--- a/drivers/nvmem/layouts/Kconfig
+++ b/drivers/nvmem/layouts/Kconfig
@@ -26,6 +26,17 @@ config NVMEM_LAYOUT_ONIE_TLV
 
 	  If unsure, say N.
 
+config NVMEM_LAYOUT_U_BOOT_ENV
+	tristate "U-Boot environment variables layout"
+	select CRC32
+	select GENERIC_NET_UTILS
+	help
+	  U-Boot stores its setup as environment variables. This driver adds
+	  support for verifying & exporting such data. It also exposes variables
+	  as NVMEM cells so they can be referenced by other drivers.
+
+	  If unsure, say N.
+
 endmenu
 
 endif
diff --git a/drivers/nvmem/layouts/Makefile b/drivers/nvmem/layouts/Makefile
index 2974bd7d33ed..4940c9db0665 100644
--- a/drivers/nvmem/layouts/Makefile
+++ b/drivers/nvmem/layouts/Makefile
@@ -5,3 +5,4 @@
 
 obj-$(CONFIG_NVMEM_LAYOUT_SL28_VPD) += sl28vpd.o
 obj-$(CONFIG_NVMEM_LAYOUT_ONIE_TLV) += onie-tlv.o
+obj-$(CONFIG_NVMEM_LAYOUT_U_BOOT_ENV) += u-boot-env.o
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;
+	}
+	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);
diff --git a/drivers/nvmem/layouts/u-boot-env.h b/drivers/nvmem/layouts/u-boot-env.h
new file mode 100644
index 000000000000..dd5f280ac047
--- /dev/null
+++ b/drivers/nvmem/layouts/u-boot-env.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _LINUX_NVMEM_LAYOUTS_U_BOOT_ENV_H
+#define _LINUX_NVMEM_LAYOUTS_U_BOOT_ENV_H
+
+enum u_boot_env_format {
+	U_BOOT_FORMAT_SINGLE,
+	U_BOOT_FORMAT_REDUNDANT,
+	U_BOOT_FORMAT_BROADCOM,
+};
+
+int u_boot_env_parse(struct device *dev, struct nvmem_device *nvmem,
+		     enum u_boot_env_format format);
+
+#endif  /* ifndef _LINUX_NVMEM_LAYOUTS_U_BOOT_ENV_H */
diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
index 936e39b20b38..ced414fc9e60 100644
--- a/drivers/nvmem/u-boot-env.c
+++ b/drivers/nvmem/u-boot-env.c
@@ -3,23 +3,15 @@
  * Copyright (C) 2022 Rafał Miłecki <rafal@milecki.pl>
  */
 
-#include <linux/crc32.h>
-#include <linux/etherdevice.h>
-#include <linux/if_ether.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/mtd/mtd.h>
-#include <linux/nvmem-consumer.h>
 #include <linux/nvmem-provider.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
-enum u_boot_env_format {
-	U_BOOT_FORMAT_SINGLE,
-	U_BOOT_FORMAT_REDUNDANT,
-	U_BOOT_FORMAT_BROADCOM,
-};
+#include "layouts/u-boot-env.h"
 
 struct u_boot_env {
 	struct device *dev;
@@ -29,24 +21,6 @@ struct u_boot_env {
 	struct mtd_info *mtd;
 };
 
-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(void *context, unsigned int offset, void *val,
 			   size_t bytes)
 {
@@ -69,134 +43,6 @@ static int u_boot_env_read(void *context, unsigned int offset, void *val,
 	return 0;
 }
 
-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_add_cells(struct u_boot_env *priv, uint8_t *buf,
-				size_t data_offset, size_t data_len)
-{
-	struct nvmem_device *nvmem = priv->nvmem;
-	struct device *dev = priv->dev;
-	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;
-}
-
-static int u_boot_env_parse(struct u_boot_env *priv)
-{
-	struct nvmem_device *nvmem = priv->nvmem;
-	struct device *dev = priv->dev;
-	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 (priv->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;
-	}
-	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_add_cells(priv, buf, data_offset, data_len);
-
-err_kfree:
-	kfree(buf);
-err_out:
-	return err;
-}
-
 static int u_boot_env_probe(struct platform_device *pdev)
 {
 	struct nvmem_config config = {
@@ -228,7 +74,7 @@ static int u_boot_env_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->nvmem))
 		return PTR_ERR(priv->nvmem);
 
-	return u_boot_env_parse(priv);
+	return u_boot_env_parse(dev, priv->nvmem, priv->format);
 }
 
 static const struct of_device_id u_boot_env_of_match_table[] = {
-- 
2.35.3

Re: [PATCH 2/2] nvmem: layouts: add U-Boot env layout

[John Thomson]

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);


-- 
  John Thomson

Re: [PATCH 2/2] nvmem: layouts: add U-Boot env layout

[Jeff Johnson]

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