2023-12-18 13:37:53

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 1/4] dt-bindings: nvmem: layouts: add U-Boot environment variables layout

From: Rafał Miłecki <[email protected]>

U-Boot env data is a way of storing firmware variables. It's a format
that can be used of top of various storage devices. Its binding should
be an NVMEM layout instead of a standalone device.

This patch adds layout binding which allows using it on top of MTD NVMEM
device as well as any other. At the same time it deprecates the old
combined binding.

Signed-off-by: Rafał Miłecki <[email protected]>
---
.../bindings/nvmem/layouts/u-boot,env.yaml | 55 +++++++++++++++++++
.../devicetree/bindings/nvmem/u-boot,env.yaml | 6 ++
2 files changed, 61 insertions(+)
create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/u-boot,env.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env.yaml
new file mode 100644
index 000000000000..3a7ec02b3535
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/layouts/u-boot,env.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NVMEM layout of U-Boot environment variables
+
+maintainers:
+ - Rafał Miłecki <[email protected]>
+
+description:
+ U-Boot uses environment variables to store device parameters and
+ configuration. They may be used for booting process, setup or keeping end user
+ info.
+
+ Data is stored using U-Boot specific formats (variant specific header and NUL
+ separated key-value pairs).
+
+properties:
+ compatible:
+ oneOf:
+ - description: A standalone env data block
+ const: u-boot,env
+ - description: Two redundant blocks with active one flagged
+ const: u-boot,env-redundant-bool
+ - description: Two redundant blocks with active having higher counter
+ const: u-boot,env-redundant-count
+ - description: Broadcom's variant with custom header
+ const: brcm,env
+
+additionalProperties: false
+
+examples:
+ - |
+ partitions {
+ compatible = "fixed-partitions";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ partition@0 {
+ reg = <0x0 0x40000>;
+ label = "u-boot";
+ read-only;
+ };
+
+ partition@40000 {
+ reg = <0x40000 0x10000>;
+ label = "u-boot-env";
+
+ nvmem-layout {
+ compatible = "u-boot,env";
+ };
+ };
+ };
diff --git a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
index 68214b96f5c9..fd95e611322d 100644
--- a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
+++ b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
@@ -26,9 +26,15 @@ description: |

Variables can be defined as NVMEM device subnodes.

+ Introduction of NVMEM layouts exposed a limitation of this binding design.
+ Description of NVMEM data content should be separated from NVMEM devices.
+ Since the introduction of U-Boot env data layout this binding is deprecated.
+
maintainers:
- Rafał Miłecki <[email protected]>

+deprecated: true
+
properties:
compatible:
oneOf:
--
2.35.3



2023-12-18 13:39:06

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 2/4] nvmem: core: add nvmem_dev_size() helper

From: Rafał Miłecki <[email protected]>

This is required by layouts that need to read whole NVMEM content. It's
especially useful for NVMEM devices without hardcoded layout (like
U-Boot environment data block).

Signed-off-by: Rafał Miłecki <[email protected]>
---
drivers/nvmem/core.c | 13 +++++++++++++
include/linux/nvmem-consumer.h | 1 +
2 files changed, 14 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index ba559e81f77f..1b08bb072f86 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -2176,6 +2176,19 @@ const char *nvmem_dev_name(struct nvmem_device *nvmem)
}
EXPORT_SYMBOL_GPL(nvmem_dev_name);

+/**
+ * nvmem_dev_size() - Get the size of a given nvmem device.
+ *
+ * @nvmem: nvmem device.
+ *
+ * Return: size of the nvmem device.
+ */
+size_t nvmem_dev_size(struct nvmem_device *nvmem)
+{
+ return nvmem->size;
+}
+EXPORT_SYMBOL_GPL(nvmem_dev_size);
+
static int __init nvmem_init(void)
{
int ret;
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 2d306fa13b1a..34c0e58dfa26 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -81,6 +81,7 @@ int nvmem_device_cell_write(struct nvmem_device *nvmem,
struct nvmem_cell_info *info, void *buf);

const char *nvmem_dev_name(struct nvmem_device *nvmem);
+size_t nvmem_dev_size(struct nvmem_device *nvmem);

void nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries,
size_t nentries);
--
2.35.3


2023-12-18 13:39:57

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 4/4] nvmem: layouts: add U-Boot env layout

From: Rafał Miłecki <[email protected]>

This patch moves all generic (NVMEM devices independent) code from NVMEM
device driver to NVMEM layout driver. Then it adds a simple NVMEM layout
code on top of it.

Thanks to proper layout it's possible to support U-Boot env data stored
on any kind of NVMEM device.

For backward compatibility with old DT bindings we need to keep old
NVMEM device driver functional. To avoid code duplication a parsing
function is exported and reused in it.

Signed-off-by: Rafał Miłecki <[email protected]>
---
MAINTAINERS | 1 +
drivers/nvmem/Kconfig | 3 +-
drivers/nvmem/layouts/Kconfig | 11 ++
drivers/nvmem/layouts/Makefile | 1 +
drivers/nvmem/layouts/u-boot-env.c | 212 +++++++++++++++++++++++++++++
drivers/nvmem/layouts/u-boot-env.h | 15 ++
drivers/nvmem/u-boot-env.c | 155 +--------------------
7 files changed, 243 insertions(+), 155 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 b589218605b4..1f7e6d74cd51 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22282,6 +22282,7 @@ U-BOOT ENVIRONMENT VARIABLES
M: Rafał Miłecki <[email protected]>
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..1787cfaf45f8
--- /dev/null
+++ b/drivers/nvmem/layouts/u-boot-env.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 - 2023 Rafał Miłecki <[email protected]>
+ */
+
+#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 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 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;
+ 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 = kcalloc(1, dev_size, GFP_KERNEL);
+ if (!buf) {
+ err = -ENOMEM;
+ goto err_out;
+ }
+
+ bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
+ if (bytes < 0)
+ return bytes;
+ else if (bytes != dev_size)
+ return -EIO;
+
+ 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 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
+ 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);
+ if (err)
+ dev_err(dev, "Failed to add cells: %d\n", err);
+
+err_kfree:
+ kfree(buf);
+err_out:
+ return err;
+}
+EXPORT_SYMBOL_GPL(u_boot_env_parse);
+
+static int u_boot_env_add_cells(struct device *dev, struct nvmem_device *nvmem)
+{
+ const struct of_device_id *match;
+ struct device_node *layout_np;
+ enum u_boot_env_format format;
+
+ layout_np = of_nvmem_layout_get_container(nvmem);
+ if (!layout_np)
+ return -ENOENT;
+
+ match = of_match_node(u_boot_env_of_match_table, layout_np);
+ if (!match)
+ return -ENOENT;
+
+ format = (uintptr_t)match->data;
+
+ of_node_put(layout_np);
+
+ return u_boot_env_parse(dev, 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 struct nvmem_layout_driver u_boot_env_layout = {
+ .driver = {
+ .owner = THIS_MODULE,
+ .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 ab8c9bf63d99..386b2b255c30 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 <[email protected]>
*/

-#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,131 +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;
- 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 = kcalloc(1, dev_size, GFP_KERNEL);
- if (!buf) {
- err = -ENOMEM;
- goto err_out;
- }
-
- bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
- if (bytes < 0)
- return bytes;
- else if (bytes != dev_size)
- return -EIO;
-
- 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 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
- 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);
- if (err)
- dev_err(dev, "Failed to add cells: %d\n", err);
-
-err_kfree:
- kfree(buf);
-err_out:
- return err;
-}
-
static int u_boot_env_probe(struct platform_device *pdev)
{
struct nvmem_config config = {
@@ -225,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


2023-12-18 13:45:29

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 3/4] nvmem: u-boot-env: use more nvmem subsystem helpers

From: Rafał Miłecki <[email protected]>

1. Use nvmem_dev_size() and nvmem_device_read() to make this driver less
mtd dependent
2. Use nvmem_add_one_cell() to simplify adding NVMEM cells

Signed-off-by: Rafał Miłecki <[email protected]>
---
drivers/nvmem/u-boot-env.c | 79 +++++++++++++++++---------------------
1 file changed, 35 insertions(+), 44 deletions(-)

diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
index c4ae94af4af7..ab8c9bf63d99 100644
--- a/drivers/nvmem/u-boot-env.c
+++ b/drivers/nvmem/u-boot-env.c
@@ -23,13 +23,10 @@ enum u_boot_env_format {

struct u_boot_env {
struct device *dev;
+ struct nvmem_device *nvmem;
enum u_boot_env_format format;

struct mtd_info *mtd;
-
- /* Cells */
- struct nvmem_cell_info *cells;
- int ncells;
};

struct u_boot_env_image_single {
@@ -94,72 +91,69 @@ static int u_boot_env_read_post_process_ethaddr(void *context, const char *id, i
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;
- int idx;
-
- priv->ncells = 0;
- for (var = data; var < data + data_len && *var; var += strlen(var) + 1)
- priv->ncells++;

- priv->cells = devm_kcalloc(dev, priv->ncells, sizeof(*priv->cells), GFP_KERNEL);
- if (!priv->cells)
- return -ENOMEM;
-
- for (var = data, idx = 0;
+ for (var = data;
var < data + data_len && *var;
- var = value + strlen(value) + 1, idx++) {
+ var = value + strlen(value) + 1) {
+ struct nvmem_cell_info info = {};
+
eq = strchr(var, '=');
if (!eq)
break;
*eq = '\0';
value = eq + 1;

- priv->cells[idx].name = devm_kstrdup(dev, var, GFP_KERNEL);
- if (!priv->cells[idx].name)
+ info.name = devm_kstrdup(dev, var, GFP_KERNEL);
+ if (!info.name)
return -ENOMEM;
- priv->cells[idx].offset = data_offset + value - data;
- priv->cells[idx].bytes = strlen(value);
- priv->cells[idx].np = of_get_child_by_name(dev->of_node, priv->cells[idx].name);
+ 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")) {
- priv->cells[idx].raw_len = strlen(value);
- priv->cells[idx].bytes = ETH_ALEN;
- priv->cells[idx].read_post_process = u_boot_env_read_post_process_ethaddr;
+ info.raw_len = strlen(value);
+ info.bytes = ETH_ALEN;
+ info.read_post_process = u_boot_env_read_post_process_ethaddr;
}
- }

- if (WARN_ON(idx != priv->ncells))
- priv->ncells = idx;
+ 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;
size_t data_offset;
size_t data_len;
+ size_t dev_size;
uint32_t crc32;
uint32_t calc;
- size_t bytes;
uint8_t *buf;
+ int bytes;
int err;

- buf = kcalloc(1, priv->mtd->size, GFP_KERNEL);
+ dev_size = nvmem_dev_size(nvmem);
+
+ buf = kcalloc(1, dev_size, GFP_KERNEL);
if (!buf) {
err = -ENOMEM;
goto err_out;
}

- err = mtd_read(priv->mtd, 0, priv->mtd->size, &bytes, buf);
- if ((err && !mtd_is_bitflip(err)) || bytes != priv->mtd->size) {
- dev_err(dev, "Failed to read from mtd: %d\n", err);
- goto err_kfree;
- }
+ bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
+ if (bytes < 0)
+ return bytes;
+ else if (bytes != dev_size)
+ return -EIO;

switch (priv->format) {
case U_BOOT_FORMAT_SINGLE:
@@ -179,8 +173,8 @@ static int u_boot_env_parse(struct u_boot_env *priv)
break;
}
crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
- crc32_data_len = priv->mtd->size - crc32_data_offset;
- data_len = priv->mtd->size - data_offset;
+ 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) {
@@ -189,7 +183,7 @@ static int u_boot_env_parse(struct u_boot_env *priv)
goto err_kfree;
}

- buf[priv->mtd->size - 1] = '\0';
+ buf[dev_size - 1] = '\0';
err = u_boot_env_add_cells(priv, buf, data_offset, data_len);
if (err)
dev_err(dev, "Failed to add cells: %d\n", err);
@@ -209,7 +203,6 @@ static int u_boot_env_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
struct u_boot_env *priv;
- int err;

priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
@@ -224,17 +217,15 @@ static int u_boot_env_probe(struct platform_device *pdev)
return PTR_ERR(priv->mtd);
}

- err = u_boot_env_parse(priv);
- if (err)
- return err;
-
config.dev = dev;
- config.cells = priv->cells;
- config.ncells = priv->ncells;
config.priv = priv;
config.size = priv->mtd->size;

- return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
+ priv->nvmem = devm_nvmem_register(dev, &config);
+ if (IS_ERR(priv->nvmem))
+ return PTR_ERR(priv->nvmem);
+
+ return u_boot_env_parse(priv);
}

static const struct of_device_id u_boot_env_of_match_table[] = {
--
2.35.3


2023-12-18 14:06:07

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH 3/4] nvmem: u-boot-env: use more nvmem subsystem helpers

Hi Rafał,

[email protected] wrote on Mon, 18 Dec 2023 14:37:21 +0100:

> From: Rafał Miłecki <[email protected]>
>
> 1. Use nvmem_dev_size() and nvmem_device_read() to make this driver less
> mtd dependent
> 2. Use nvmem_add_one_cell() to simplify adding NVMEM cells
>
> Signed-off-by: Rafał Miłecki <[email protected]>

Reviewed-by: Miquel Raynal <[email protected]>

Thanks,
Miquèl

2023-12-18 14:09:29

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH 2/4] nvmem: core: add nvmem_dev_size() helper

Hi Rafał,

[email protected] wrote on Mon, 18 Dec 2023 14:37:20 +0100:

> From: Rafał Miłecki <[email protected]>
>
> This is required by layouts that need to read whole NVMEM content. It's
> especially useful for NVMEM devices without hardcoded layout (like
> U-Boot environment data block).

Reviewed-by: Miquel Raynal <[email protected]>

>
> Signed-off-by: Rafał Miłecki <[email protected]>

Thanks,
Miquèl

2023-12-18 14:21:59

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout

Hi Rafał,

[email protected] wrote on Mon, 18 Dec 2023 14:37:22 +0100:

> From: Rafał Miłecki <[email protected]>
>
> This patch moves all generic (NVMEM devices independent) code from NVMEM
> device driver to NVMEM layout driver. Then it adds a simple NVMEM layout
> code on top of it.
>
> Thanks to proper layout it's possible to support U-Boot env data stored
> on any kind of NVMEM device.
>
> For backward compatibility with old DT bindings we need to keep old
> NVMEM device driver functional. To avoid code duplication a parsing
> function is exported and reused in it.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---

I have a couple of comments about the original driver which gets
copy-pasted in the new layout driver, maybe you could clean these
(the memory leak should be fixed before the migration so it can be
backported easily, the others are just style so it can be done after, I
don't mind).

...

> +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;
> + 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 = kcalloc(1, dev_size, GFP_KERNEL);

Out of curiosity, why kcalloc(1,...) rather than kzalloc() ?

> + if (!buf) {
> + err = -ENOMEM;
> + goto err_out;

We could directly return ENOMEM here I guess.

> + }
> +
> + bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
> + if (bytes < 0)
> + return bytes;
> + else if (bytes != dev_size)
> + return -EIO;

Don't we need to free buf in the above cases?

> + 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 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));

Looks a bit convoluted, any chances we can use intermediate variables
to help decipher this?

> + 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);
> + if (err)
> + dev_err(dev, "Failed to add cells: %d\n", err);

Please drop this error message, the only reason for which the function
call would fail is apparently an ENOMEM case.

> +
> +err_kfree:
> + kfree(buf);
> +err_out:
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(u_boot_env_parse);
> +
> +static int u_boot_env_add_cells(struct device *dev, struct nvmem_device *nvmem)
> +{
> + const struct of_device_id *match;
> + struct device_node *layout_np;
> + enum u_boot_env_format format;
> +
> + layout_np = of_nvmem_layout_get_container(nvmem);
> + if (!layout_np)
> + return -ENOENT;
> +
> + match = of_match_node(u_boot_env_of_match_table, layout_np);
> + if (!match)
> + return -ENOENT;
> +
> + format = (uintptr_t)match->data;

In the core there is currently an unused helper called
nvmem_layout_get_match_data() which does that. I think the original
intent of this function was to be used in this driver, so depending on
your preference, can you please either use it or remove it?

> +
> + of_node_put(layout_np);
> +
> + return u_boot_env_parse(dev, nvmem, format);
> +}

Thanks,
Miquèl

2023-12-18 14:48:44

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 1/4] dt-bindings: nvmem: layouts: add U-Boot environment variables layout


On Mon, 18 Dec 2023 14:37:19 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> U-Boot env data is a way of storing firmware variables. It's a format
> that can be used of top of various storage devices. Its binding should
> be an NVMEM layout instead of a standalone device.
>
> This patch adds layout binding which allows using it on top of MTD NVMEM
> device as well as any other. At the same time it deprecates the old
> combined binding.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> .../bindings/nvmem/layouts/u-boot,env.yaml | 55 +++++++++++++++++++
> .../devicetree/bindings/nvmem/u-boot,env.yaml | 6 ++
> 2 files changed, 61 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/u-boot,env.yaml
>

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/nvmem/u-boot,env.example.dtb: partition@40000: 'ethaddr', 'reg' do not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/nvmem/layouts/u-boot,env.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/nvmem/u-boot,env.example.dtb: partition-u-boot-env: 'ethaddr' does not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/nvmem/layouts/u-boot,env.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


2023-12-18 22:10:34

by Rafał Miłecki

[permalink] [raw]
Subject: Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout

On 18.12.2023 15:21, Miquel Raynal wrote:
> Hi Rafał,
>
> [email protected] wrote on Mon, 18 Dec 2023 14:37:22 +0100:
>
>> From: Rafał Miłecki <[email protected]>
>>
>> This patch moves all generic (NVMEM devices independent) code from NVMEM
>> device driver to NVMEM layout driver. Then it adds a simple NVMEM layout
>> code on top of it.
>>
>> Thanks to proper layout it's possible to support U-Boot env data stored
>> on any kind of NVMEM device.
>>
>> For backward compatibility with old DT bindings we need to keep old
>> NVMEM device driver functional. To avoid code duplication a parsing
>> function is exported and reused in it.
>>
>> Signed-off-by: Rafał Miłecki <[email protected]>
>> ---
>
> I have a couple of comments about the original driver which gets
> copy-pasted in the new layout driver, maybe you could clean these
> (the memory leak should be fixed before the migration so it can be
> backported easily, the others are just style so it can be done after, I
> don't mind).
>
> ...
>
>> +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;
>> + 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 = kcalloc(1, dev_size, GFP_KERNEL);
>
> Out of curiosity, why kcalloc(1,...) rather than kzalloc() ?

I used kcalloc() initially as I didn't need buffer to be zeroed.

I see that memory-allocation.rst however says:
> And, to be on the safe side it's best to use routines that set memory to zero, like kzalloc().

It's probably close to zero cost to zero that buffer so it could be kzalloc().


>> + if (!buf) {
>> + err = -ENOMEM;
>> + goto err_out;
>
> We could directly return ENOMEM here I guess.
>
>> + }
>> +
>> + bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
>> + if (bytes < 0)
>> + return bytes;
>> + else if (bytes != dev_size)
>> + return -EIO;
>
> Don't we need to free buf in the above cases?
>
>> + 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 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
>
> Looks a bit convoluted, any chances we can use intermediate variables
> to help decipher this?
>
>> + 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);
>> + if (err)
>> + dev_err(dev, "Failed to add cells: %d\n", err);
>
> Please drop this error message, the only reason for which the function
> call would fail is apparently an ENOMEM case.
>
>> +
>> +err_kfree:
>> + kfree(buf);
>> +err_out:
>> + return err;
>> +}
>> +EXPORT_SYMBOL_GPL(u_boot_env_parse);
>> +
>> +static int u_boot_env_add_cells(struct device *dev, struct nvmem_device *nvmem)
>> +{
>> + const struct of_device_id *match;
>> + struct device_node *layout_np;
>> + enum u_boot_env_format format;
>> +
>> + layout_np = of_nvmem_layout_get_container(nvmem);
>> + if (!layout_np)
>> + return -ENOENT;
>> +
>> + match = of_match_node(u_boot_env_of_match_table, layout_np);
>> + if (!match)
>> + return -ENOENT;
>> +
>> + format = (uintptr_t)match->data;
>
> In the core there is currently an unused helper called
> nvmem_layout_get_match_data() which does that. I think the original
> intent of this function was to be used in this driver, so depending on
> your preference, can you please either use it or remove it?

The problem is that nvmem_layout_get_match_data() uses:
layout->dev.driver

It doesn't work with layouts driver (since refactoring?) as driver is
NULL. That results in NULL pointer dereference when trying to reach
of_match_table.

That is why I used u_boot_env_of_match_table directly.

If you know how to fix nvmem_layout_get_match_data() that would be
great. Do we need driver_register() somewhere in NVMEM core?


>> +
>> + of_node_put(layout_np);
>> +
>> + return u_boot_env_parse(dev, nvmem, format);
>> +}
>
> Thanks,
> Miquèl


2023-12-18 22:14:22

by Rafał Miłecki

[permalink] [raw]
Subject: Re: [PATCH 1/4] dt-bindings: nvmem: layouts: add U-Boot environment variables layout

On 18.12.2023 15:48, Rob Herring wrote:
>
> On Mon, 18 Dec 2023 14:37:19 +0100, Rafał Miłecki wrote:
>> From: Rafał Miłecki <[email protected]>
>>
>> U-Boot env data is a way of storing firmware variables. It's a format
>> that can be used of top of various storage devices. Its binding should
>> be an NVMEM layout instead of a standalone device.
>>
>> This patch adds layout binding which allows using it on top of MTD NVMEM
>> device as well as any other. At the same time it deprecates the old
>> combined binding.
>>
>> Signed-off-by: Rafał Miłecki <[email protected]>
>> ---
>> .../bindings/nvmem/layouts/u-boot,env.yaml | 55 +++++++++++++++++++
>> .../devicetree/bindings/nvmem/u-boot,env.yaml | 6 ++
>> 2 files changed, 61 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/u-boot,env.yaml
>>
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/nvmem/u-boot,env.example.dtb: partition@40000: 'ethaddr', 'reg' do not match any of the regexes: 'pinctrl-[0-9]+'
> from schema $id: http://devicetree.org/schemas/nvmem/layouts/u-boot,env.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/nvmem/u-boot,env.example.dtb: partition-u-boot-env: 'ethaddr' does not match any of the regexes: 'pinctrl-[0-9]+'
> from schema $id: http://devicetree.org/schemas/nvmem/layouts/u-boot,env.yaml#

I checked my binding independently using using dt_binding_check and
missed that. I'm not aware of any way of limiting possibility of
applying binding to specific cases (like "nvmem-layout" node) so I
guess I'll just have to avoid duplicated "u-boot,env" compatible
string.


> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>


2023-12-19 07:55:37

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout

Hi Rafał,

[email protected] wrote on Mon, 18 Dec 2023 23:10:20 +0100:

> On 18.12.2023 15:21, Miquel Raynal wrote:
> > Hi Rafał,
> >
> > [email protected] wrote on Mon, 18 Dec 2023 14:37:22 +0100:
> >
> >> From: Rafał Miłecki <[email protected]>
> >>
> >> This patch moves all generic (NVMEM devices independent) code from NVMEM
> >> device driver to NVMEM layout driver. Then it adds a simple NVMEM layout
> >> code on top of it.
> >>
> >> Thanks to proper layout it's possible to support U-Boot env data stored
> >> on any kind of NVMEM device.
> >>
> >> For backward compatibility with old DT bindings we need to keep old
> >> NVMEM device driver functional. To avoid code duplication a parsing
> >> function is exported and reused in it.
> >>
> >> Signed-off-by: Rafał Miłecki <[email protected]>
> >> ---
> >
> > I have a couple of comments about the original driver which gets
> > copy-pasted in the new layout driver, maybe you could clean these
> > (the memory leak should be fixed before the migration so it can be
> > backported easily, the others are just style so it can be done after, I
> > don't mind).
> >
> > ...
> >
> >> +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;
> >> + 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 = kcalloc(1, dev_size, GFP_KERNEL);
> >
> > Out of curiosity, why kcalloc(1,...) rather than kzalloc() ?
>
> I used kcalloc() initially as I didn't need buffer to be zeroed.

I think kcalloc() initializes the memory to zero.
https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L659

If you don't need it you can switch to kmalloc() instead, I don't mind,
but kcalloc() is meant to be used with arrays, I don't see the point of
using kcalloc() in this case.

>
> I see that memory-allocation.rst however says:
> > And, to be on the safe side it's best to use routines that set memory to zero, like kzalloc().
>
> It's probably close to zero cost to zero that buffer so it could be kzalloc().
>
>
> >> + if (!buf) {
> >> + err = -ENOMEM;
> >> + goto err_out;
> >
> > We could directly return ENOMEM here I guess.
> >
> >> + }
> >> +
> >> + bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
> >> + if (bytes < 0)
> >> + return bytes;
> >> + else if (bytes != dev_size)
> >> + return -EIO;
> >
> > Don't we need to free buf in the above cases?
> >
> >> + 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 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
> >
> > Looks a bit convoluted, any chances we can use intermediate variables
> > to help decipher this?
> >
> >> + 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);
> >> + if (err)
> >> + dev_err(dev, "Failed to add cells: %d\n", err);
> >
> > Please drop this error message, the only reason for which the function
> > call would fail is apparently an ENOMEM case.
> >
> >> +
> >> +err_kfree:
> >> + kfree(buf);
> >> +err_out:
> >> + return err;
> >> +}
> >> +EXPORT_SYMBOL_GPL(u_boot_env_parse);
> >> +
> >> +static int u_boot_env_add_cells(struct device *dev, struct nvmem_device *nvmem)
> >> +{
> >> + const struct of_device_id *match;
> >> + struct device_node *layout_np;
> >> + enum u_boot_env_format format;
> >> +
> >> + layout_np = of_nvmem_layout_get_container(nvmem);
> >> + if (!layout_np)
> >> + return -ENOENT;
> >> +
> >> + match = of_match_node(u_boot_env_of_match_table, layout_np);
> >> + if (!match)
> >> + return -ENOENT;
> >> +
> >> + format = (uintptr_t)match->data;
> >
> > In the core there is currently an unused helper called
> > nvmem_layout_get_match_data() which does that. I think the original
> > intent of this function was to be used in this driver, so depending on
> > your preference, can you please either use it or remove it?
>
> The problem is that nvmem_layout_get_match_data() uses:
> layout->dev.driver

I'm surprised .driver is unset. Well anyway, please either fix the core
helper and use it or drop the core helper, because we have no user for
it otherwise?

> It doesn't work with layouts driver (since refactoring?) as driver is
> NULL. That results in NULL pointer dereference when trying to reach
> of_match_table.
>
> That is why I used u_boot_env_of_match_table directly.
>
> If you know how to fix nvmem_layout_get_match_data() that would be
> great. Do we need driver_register() somewhere in NVMEM core?
>


Thanks,
Miquèl

2023-12-19 09:55:18

by Rafał Miłecki

[permalink] [raw]
Subject: Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout

On 19.12.2023 08:55, Miquel Raynal wrote:
> Hi Rafał,
>
> [email protected] wrote on Mon, 18 Dec 2023 23:10:20 +0100:
>
>> On 18.12.2023 15:21, Miquel Raynal wrote:
>>> Hi Rafał,
>>>
>>> [email protected] wrote on Mon, 18 Dec 2023 14:37:22 +0100:
>>>
>>>> From: Rafał Miłecki <[email protected]>
>>>>
>>>> This patch moves all generic (NVMEM devices independent) code from NVMEM
>>>> device driver to NVMEM layout driver. Then it adds a simple NVMEM layout
>>>> code on top of it.
>>>>
>>>> Thanks to proper layout it's possible to support U-Boot env data stored
>>>> on any kind of NVMEM device.
>>>>
>>>> For backward compatibility with old DT bindings we need to keep old
>>>> NVMEM device driver functional. To avoid code duplication a parsing
>>>> function is exported and reused in it.
>>>>
>>>> Signed-off-by: Rafał Miłecki <[email protected]>
>>>> ---
>>>
>>> I have a couple of comments about the original driver which gets
>>> copy-pasted in the new layout driver, maybe you could clean these
>>> (the memory leak should be fixed before the migration so it can be
>>> backported easily, the others are just style so it can be done after, I
>>> don't mind).
>>>
>>> ...
>>>
>>>> +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;
>>>> + 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 = kcalloc(1, dev_size, GFP_KERNEL);
>>>
>>> Out of curiosity, why kcalloc(1,...) rather than kzalloc() ?
>>
>> I used kcalloc() initially as I didn't need buffer to be zeroed.
>
> I think kcalloc() initializes the memory to zero.
> https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L659
>
> If you don't need it you can switch to kmalloc() instead, I don't mind,
> but kcalloc() is meant to be used with arrays, I don't see the point of
> using kcalloc() in this case.
>
>>
>> I see that memory-allocation.rst however says:
>> > And, to be on the safe side it's best to use routines that set memory to zero, like kzalloc().
>>
>> It's probably close to zero cost to zero that buffer so it could be kzalloc().
>>
>>
>>>> + if (!buf) {
>>>> + err = -ENOMEM;
>>>> + goto err_out;
>>>
>>> We could directly return ENOMEM here I guess.
>>>
>>>> + }
>>>> +
>>>> + bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
>>>> + if (bytes < 0)
>>>> + return bytes;
>>>> + else if (bytes != dev_size)
>>>> + return -EIO;
>>>
>>> Don't we need to free buf in the above cases?
>>>
>>>> + 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 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
>>>
>>> Looks a bit convoluted, any chances we can use intermediate variables
>>> to help decipher this?
>>>
>>>> + 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);
>>>> + if (err)
>>>> + dev_err(dev, "Failed to add cells: %d\n", err);
>>>
>>> Please drop this error message, the only reason for which the function
>>> call would fail is apparently an ENOMEM case.
>>>
>>>> +
>>>> +err_kfree:
>>>> + kfree(buf);
>>>> +err_out:
>>>> + return err;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(u_boot_env_parse);
>>>> +
>>>> +static int u_boot_env_add_cells(struct device *dev, struct nvmem_device *nvmem)
>>>> +{
>>>> + const struct of_device_id *match;
>>>> + struct device_node *layout_np;
>>>> + enum u_boot_env_format format;
>>>> +
>>>> + layout_np = of_nvmem_layout_get_container(nvmem);
>>>> + if (!layout_np)
>>>> + return -ENOENT;
>>>> +
>>>> + match = of_match_node(u_boot_env_of_match_table, layout_np);
>>>> + if (!match)
>>>> + return -ENOENT;
>>>> +
>>>> + format = (uintptr_t)match->data;
>>>
>>> In the core there is currently an unused helper called
>>> nvmem_layout_get_match_data() which does that. I think the original
>>> intent of this function was to be used in this driver, so depending on
>>> your preference, can you please either use it or remove it?
>>
>> The problem is that nvmem_layout_get_match_data() uses:
>> layout->dev.driver
>
> I'm surprised .driver is unset. Well anyway, please either fix the core
> helper and use it or drop the core helper, because we have no user for
> it otherwise?

I believe it's because of a very minimalistic "nvmem_bus_type" bus
implementation.

From a quick look it seems that default expected FORWARD-trace is:
driver_register()
bus_add_driver()
driver_attach()
__driver_attach()
driver_probe_device()
__driver_probe_device()
really_probe()

It's really_probe() that seems to set dev->driver pointer.


>> It doesn't work with layouts driver (since refactoring?) as driver is
>> NULL. That results in NULL pointer dereference when trying to reach
>> of_match_table.
>>
>> That is why I used u_boot_env_of_match_table directly.
>>
>> If you know how to fix nvmem_layout_get_match_data() that would be
>> great. Do we need driver_register() somewhere in NVMEM core?


2023-12-19 10:10:36

by Rafał Miłecki

[permalink] [raw]
Subject: Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout

On 19.12.2023 10:55, Rafał Miłecki wrote:
> On 19.12.2023 08:55, Miquel Raynal wrote:
>> Hi Rafał,
>>
>> [email protected] wrote on Mon, 18 Dec 2023 23:10:20 +0100:
>>
>>> On 18.12.2023 15:21, Miquel Raynal wrote:
>>>> Hi Rafał,
>>>>
>>>> [email protected] wrote on Mon, 18 Dec 2023 14:37:22 +0100:
>>>>> From: Rafał Miłecki <[email protected]>
>>>>>
>>>>> This patch moves all generic (NVMEM devices independent) code from NVMEM
>>>>> device driver to NVMEM layout driver. Then it adds a simple NVMEM layout
>>>>> code on top of it.
>>>>>
>>>>> Thanks to proper layout it's possible to support U-Boot env data stored
>>>>> on any kind of NVMEM device.
>>>>>
>>>>> For backward compatibility with old DT bindings we need to keep old
>>>>> NVMEM device driver functional. To avoid code duplication a parsing
>>>>> function is exported and reused in it.
>>>>>
>>>>> Signed-off-by: Rafał Miłecki <[email protected]>
>>>>> ---
>>>>
>>>> I have a couple of comments about the original driver which gets
>>>> copy-pasted in the new layout driver, maybe you could clean these
>>>> (the memory leak should be fixed before the migration so it can be
>>>> backported easily, the others are just style so it can be done after, I
>>>> don't mind).
>>>>
>>>> ...
>>>>> +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;
>>>>> +    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 = kcalloc(1, dev_size, GFP_KERNEL);
>>>>
>>>> Out of curiosity, why kcalloc(1,...) rather than kzalloc() ?
>>>
>>> I used kcalloc() initially as I didn't need buffer to be zeroed.
>>
>> I think kcalloc() initializes the memory to zero.
>> https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L659
>>
>> If you don't need it you can switch to kmalloc() instead, I don't mind,
>> but kcalloc() is meant to be used with arrays, I don't see the point of
>> using kcalloc() in this case.
>>
>>>
>>> I see that memory-allocation.rst however says:
>>>   > And, to be on the safe side it's best to use routines that set memory to zero, like kzalloc().
>>>
>>> It's probably close to zero cost to zero that buffer so it could be kzalloc().
>>>
>>>
>>>>> +    if (!buf) {
>>>>> +        err = -ENOMEM;
>>>>> +        goto err_out;
>>>>
>>>> We could directly return ENOMEM here I guess.
>>>>> +    }
>>>>> +
>>>>> +    bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
>>>>> +    if (bytes < 0)
>>>>> +        return bytes;
>>>>> +    else if (bytes != dev_size)
>>>>> +        return -EIO;
>>>>
>>>> Don't we need to free buf in the above cases?
>>>>> +    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 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
>>>>
>>>> Looks a bit convoluted, any chances we can use intermediate variables
>>>> to help decipher this?
>>>>> +    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);
>>>>> +    if (err)
>>>>> +        dev_err(dev, "Failed to add cells: %d\n", err);
>>>>
>>>> Please drop this error message, the only reason for which the function
>>>> call would fail is apparently an ENOMEM case.
>>>>> +
>>>>> +err_kfree:
>>>>> +    kfree(buf);
>>>>> +err_out:
>>>>> +    return err;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(u_boot_env_parse);
>>>>> +
>>>>> +static int u_boot_env_add_cells(struct device *dev, struct nvmem_device *nvmem)
>>>>> +{
>>>>> +    const struct of_device_id *match;
>>>>> +    struct device_node *layout_np;
>>>>> +    enum u_boot_env_format format;
>>>>> +
>>>>> +    layout_np = of_nvmem_layout_get_container(nvmem);
>>>>> +    if (!layout_np)
>>>>> +        return -ENOENT;
>>>>> +
>>>>> +    match = of_match_node(u_boot_env_of_match_table, layout_np);
>>>>> +    if (!match)
>>>>> +        return -ENOENT;
>>>>> +
>>>>> +    format = (uintptr_t)match->data;
>>>>
>>>> In the core there is currently an unused helper called
>>>> nvmem_layout_get_match_data() which does that. I think the original
>>>> intent of this function was to be used in this driver, so depending on
>>>> your preference, can you please either use it or remove it?
>>>
>>> The problem is that nvmem_layout_get_match_data() uses:
>>> layout->dev.driver
>>
>> I'm surprised .driver is unset. Well anyway, please either fix the core
>> helper and use it or drop the core helper, because we have no user for
>> it otherwise?
>
> I believe it's because of a very minimalistic "nvmem_bus_type" bus
> implementation.

Scratch that, I was looking at "nvmem_bus_type" instead of
"nvmem_layout_bus_type". I'll see if I can debug that.


> From a quick look it seems that default expected FORWARD-trace is:
> driver_register()
> bus_add_driver()
> driver_attach()
> __driver_attach()
> driver_probe_device()
> __driver_probe_device()
> really_probe()
>
> It's really_probe() that seems to set dev->driver pointer.
>
>
>>> It doesn't work with layouts driver (since refactoring?) as driver is
>>> NULL. That results in NULL pointer dereference when trying to reach
>>> of_match_table.
>>>
>>> That is why I used u_boot_env_of_match_table directly.
>>>
>>> If you know how to fix nvmem_layout_get_match_data() that would be
>>> great. Do we need driver_register() somewhere in NVMEM core?
>