[PATCH v2 0/6] nvmem: rmem: cleanup & add checksumming support for Mobileye EyeQ5

[PATCH v2 0/6] nvmem: rmem: cleanup & add checksumming support for Mobileye EyeQ5

[Théo Lebrun]

Hi,

This series is two-fold.

 - First some cleanup to nvmem/rmem.

   [PATCH 2/6] nvmem: specify ->reg_read/reg_write() expected return values
   [PATCH 3/6] nvmem: rmem: make ->reg_read() straight forward code
   [PATCH 4/6] nvmem: rmem: remove unused struct rmem::size field

   Those patches were sent on the 2024-07-24 [0] and saw no feedback.
   There are small improvements to the commit messages but the commit
   bodies stayed the same. I did not make this a follow-up as the EyeQ5
   compatible (see below) wasn't part of V1, and I wouldn't want people
   to think it has been through a first round of lkml review.

 - Second, add a new compatible to rmem for the EyeQ5-specific usecase;
   it parses a header and does checksumming at probe.

   [PATCH 1/6] dt-bindings: nvmem: rmem: Add mobileye,eyeq5-bootloader-config
   [PATCH 5/6] nvmem: rmem: add CRC validation for Mobileye EyeQ5 NVMEM
   [PATCH 6/6] MIPS: mobileye: eyeq5: add bootloader config reserved memory

Code is tested on real hardware, an EyeQ5 evaluation board.
With the patch series:

   # rmem=/sys/bus/nvmem/devices/rmem0
   # for i in $rmem/cells/*; do basename $i; hexdump -C $i | head -n1; done
   mac@7c,0
   00000000  00 28 f8 6b 87 1b                                 |.(.k..|
   mac@82,0
   00000000  00 28 f8 6c 88 1c                                 |.(.l..|

Have a nice day,
Thanks,
Théo

[0]: https://lore.kernel.org/lkml/20240724-nvmem-rmem-v1-0-d2e3a97349a0@bootlin.com/

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Changes in v2:
- [PATCH 1/6] dt-bindings: take "Acked-by: Rob Herring".
- [PATCH 5/6]: add "#include <linux/slab.h>" for kfree(). It caused
  compile errors on some architectures, thanks kernel test robot.
- Link to v1: https://lore.kernel.org/r/20241203-rmem-v1-0-24f4970cf14e@bootlin.com

---
Théo Lebrun (6):
      dt-bindings: nvmem: rmem: Add mobileye,eyeq5-bootloader-config
      nvmem: specify ->reg_read/reg_write() expected return values
      nvmem: rmem: make ->reg_read() straight forward code
      nvmem: rmem: remove unused struct rmem::size field
      nvmem: rmem: add CRC validation for Mobileye EyeQ5 NVMEM
      MIPS: mobileye: eyeq5: add bootloader config reserved memory

 Documentation/devicetree/bindings/nvmem/rmem.yaml |  1 +
 arch/mips/boot/dts/mobileye/eyeq5.dtsi            | 22 ++++++
 drivers/nvmem/rmem.c                              | 96 ++++++++++++++++++++---
 include/linux/nvmem-provider.h                    |  4 +-
 4 files changed, 111 insertions(+), 12 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241203-rmem-15df9301cf0b

Best regards,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH v2 1/6] dt-bindings: nvmem: rmem: Add mobileye,eyeq5-bootloader-config

[Théo Lebrun]

On Mobileye EyeQ5, the bootloader will put MAC addresses into memory.
Declare that as reserved memory to be used by the kernel, exposing
nvmem cells. That region has a 12-byte header and a 4-byte trailing CRC.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/nvmem/rmem.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/nvmem/rmem.yaml b/Documentation/devicetree/bindings/nvmem/rmem.yaml
index 1ec0d09bcafa857c05ae722834c9e13bf63ee851..85f9f5de3906d2f1afce74ce001cf5db06e50806 100644
--- a/Documentation/devicetree/bindings/nvmem/rmem.yaml
+++ b/Documentation/devicetree/bindings/nvmem/rmem.yaml
@@ -16,6 +16,7 @@ properties:
   compatible:
     items:
       - enum:
+          - mobileye,eyeq5-bootloader-config
           - raspberrypi,bootloader-config
           - raspberrypi,bootloader-public-key
       - const: nvmem-rmem

-- 
2.47.1

[PATCH v2 2/6] nvmem: specify ->reg_read/reg_write() expected return values

[Théo Lebrun]

Both ->reg_read() and ->reg_write() return values are not easy to
deduce. Explicit that they should return zero on success (and negative
values otherwise).

Such callbacks, in some alternative world, could return the number of
bytes in the success case. That would be translated to errors in the
nvmem core because of checks like:

	ret = nvmem->reg_write(nvmem->priv, offset, val, bytes);
	if (ret) {
		// error case
	}

This mistake is not just theoretical, see commit
28b008751aa2 ("nvmem: rmem: Fix return value of rmem_read()").

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 include/linux/nvmem-provider.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 3ebeaa0ded00c8d12230e5be7eed045a89a82669..515676ebe59875fa0ab8a4e772542bdc90ead8e3 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -92,8 +92,8 @@ struct nvmem_cell_info {
  * @read_only:	Device is read-only.
  * @root_only:	Device is accessibly to root only.
  * @of_node:	If given, this will be used instead of the parent's of_node.
- * @reg_read:	Callback to read data.
- * @reg_write:	Callback to write data.
+ * @reg_read:	Callback to read data; return zero if successful.
+ * @reg_write:	Callback to write data; return zero if successful.
  * @size:	Device size.
  * @word_size:	Minimum read/write access granularity.
  * @stride:	Minimum read/write access stride.

-- 
2.47.1

[PATCH v2 3/6] nvmem: rmem: make ->reg_read() straight forward code

[Théo Lebrun]

memory_read_from_buffer() is a weird choice; it:
 - is made for iteration with ppos a pointer.
 - does futile error checking in our case.
 - does NOT ensure we read exactly N bytes.

Replace it by:
1. A check that (offset + bytes) lands inside the region and,
2. a plain memcpy().

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/nvmem/rmem.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
index 7f907c5a445e7865c8626e00362df0040fe52241..0dc5c8237c7538efe4597c182d7bdb709b945851 100644
--- a/drivers/nvmem/rmem.c
+++ b/drivers/nvmem/rmem.c
@@ -21,10 +21,10 @@ static int rmem_read(void *context, unsigned int offset,
 		     void *val, size_t bytes)
 {
 	struct rmem *priv = context;
-	size_t available = priv->mem->size;
-	loff_t off = offset;
 	void *addr;
-	int count;
+
+	if ((phys_addr_t)offset + bytes > priv->mem->size)
+		return -EIO;
 
 	/*
 	 * Only map the reserved memory at this point to avoid potential rogue
@@ -36,20 +36,17 @@ static int rmem_read(void *context, unsigned int offset,
 	 * An alternative would be setting the memory as RO, set_memory_ro(),
 	 * but as of Dec 2020 this isn't possible on arm64.
 	 */
-	addr = memremap(priv->mem->base, available, MEMREMAP_WB);
+	addr = memremap(priv->mem->base, priv->mem->size, MEMREMAP_WB);
 	if (!addr) {
 		dev_err(priv->dev, "Failed to remap memory region\n");
 		return -ENOMEM;
 	}
 
-	count = memory_read_from_buffer(val, bytes, &off, addr, available);
+	memcpy(val, addr + offset, bytes);
 
 	memunmap(addr);
 
-	if (count < 0)
-		return count;
-
-	return count == bytes ? 0 : -EIO;
+	return 0;
 }
 
 static int rmem_probe(struct platform_device *pdev)

-- 
2.47.1

[PATCH v2 4/6] nvmem: rmem: remove unused struct rmem::size field

[Théo Lebrun]

The private structure used by the rmem driver contains
a `size` field that is unused. Remove it.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/nvmem/rmem.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
index 0dc5c8237c7538efe4597c182d7bdb709b945851..ca89c2689031534ff316a48e03360aeec823b025 100644
--- a/drivers/nvmem/rmem.c
+++ b/drivers/nvmem/rmem.c
@@ -13,8 +13,6 @@ struct rmem {
 	struct device *dev;
 	struct nvmem_device *nvmem;
 	struct reserved_mem *mem;
-
-	phys_addr_t size;
 };
 
 static int rmem_read(void *context, unsigned int offset,

-- 
2.47.1

[PATCH v2 5/6] nvmem: rmem: add CRC validation for Mobileye EyeQ5 NVMEM

[Théo Lebrun]

Mobileye EyeQ5 has a non-volatile memory region which
gets used to store MAC addresses. Its format includes
a prefix 12-byte header and a suffix 4-byte CRC.

Add an optional ->checksum() callback inside match data;
it runs CRC32 onto the content.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/nvmem/rmem.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
index ca89c2689031534ff316a48e03360aeec823b025..1f0caf1d2dc1b3d42e5ac79b55222beb59fdd30d 100644
--- a/drivers/nvmem/rmem.c
+++ b/drivers/nvmem/rmem.c
@@ -3,11 +3,13 @@
  * Copyright (C) 2020 Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
  */
 
+#include <linux/crc32.h>
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/nvmem-provider.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 
 struct rmem {
 	struct device *dev;
@@ -15,6 +17,18 @@ struct rmem {
 	struct reserved_mem *mem;
 };
 
+struct rmem_match_data {
+	int (*checksum)(struct rmem *priv);
+};
+
+struct __packed rmem_eyeq5_header {
+	u32 magic;
+	u32 version;
+	u32 size;
+};
+
+#define RMEM_EYEQ5_MAGIC	((u32)0xDABBAD00)
+
 static int rmem_read(void *context, unsigned int offset,
 		     void *val, size_t bytes)
 {
@@ -47,10 +61,66 @@ static int rmem_read(void *context, unsigned int offset,
 	return 0;
 }
 
+static int rmem_eyeq5_checksum(struct rmem *priv)
+{
+	struct rmem_eyeq5_header header;
+	void *buf __free(kfree) = NULL;
+	u32 computed_crc, *target_crc;
+	size_t data_size;
+	int ret;
+
+	ret = rmem_read(priv, 0, &header, sizeof(header));
+	if (ret)
+		return ret;
+
+	if (header.magic != RMEM_EYEQ5_MAGIC)
+		return -EINVAL;
+
+	/*
+	 * Avoid massive kmalloc() if header read is invalid;
+	 * the check would be done by the next rmem_read() anyway.
+	 */
+	if (header.size > priv->mem->size)
+		return -EINVAL;
+
+	/*
+	 *           0 +-------------------+
+	 *             | Header (12 bytes) | \
+	 *             +-------------------+ |
+	 *             |                   | | data to be CRCed
+	 *             |        ...        | |
+	 *             |                   | /
+	 *   data_size +-------------------+
+	 *             |   CRC (4 bytes)   |
+	 * header.size +-------------------+
+	 */
+
+	buf = kmalloc(header.size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = rmem_read(priv, 0, buf, header.size);
+	if (ret)
+		return ret;
+
+	data_size = header.size - sizeof(*target_crc);
+	target_crc = buf + data_size;
+	computed_crc = crc32(U32_MAX, buf, data_size) ^ U32_MAX;
+
+	if (computed_crc == *target_crc)
+		return 0;
+
+	dev_err(priv->dev,
+		"checksum failed: computed %#x, expected %#x, header (%#x, %#x, %#x)\n",
+		computed_crc, *target_crc, header.magic, header.version, header.size);
+	return -EINVAL;
+}
+
 static int rmem_probe(struct platform_device *pdev)
 {
 	struct nvmem_config config = { };
 	struct device *dev = &pdev->dev;
+	const struct rmem_match_data *match_data = device_get_match_data(dev);
 	struct reserved_mem *mem;
 	struct rmem *priv;
 
@@ -73,10 +143,21 @@ static int rmem_probe(struct platform_device *pdev)
 	config.size = mem->size;
 	config.reg_read = rmem_read;
 
+	if (match_data && match_data->checksum) {
+		int ret = match_data->checksum(priv);
+		if (ret)
+			return ret;
+	}
+
 	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
 }
 
+static const struct rmem_match_data rmem_eyeq5_match_data = {
+	.checksum = rmem_eyeq5_checksum,
+};
+
 static const struct of_device_id rmem_match[] = {
+	{ .compatible = "mobileye,eyeq5-bootloader-config", .data = &rmem_eyeq5_match_data },
 	{ .compatible = "nvmem-rmem", },
 	{ /* sentinel */ },
 };

-- 
2.47.1

[PATCH v2 6/6] MIPS: mobileye: eyeq5: add bootloader config reserved memory

[Théo Lebrun]

Add a new reserved-memory node, containing bootloader config with MAC
addresses for both ethernet instances of the SoC.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 5d73e8320b8efc1b4f68923482bf188c4345f1cb..a84e6e720619ef99e1405ae6296d8bad1aa3fa23 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -49,6 +49,28 @@ mini_coredump0_reserved: mini-coredump0@806200000 {
 		mhm_reserved_0: the-mhm-reserved-0@0 {
 			reg = <0x8 0x00000000 0x0 0x0000800>;
 		};
+
+		nvram@461fe00 {
+			compatible = "mobileye,eyeq5-bootloader-config", "nvmem-rmem";
+			reg = <0x0 0x0461fe00 0x0 0x200>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			no-map;
+
+			nvmem-layout {
+				compatible = "fixed-layout";
+				#address-cells = <1>;
+				#size-cells = <1>;
+
+				eth0_mac: mac@7c {
+					reg = <0x7c 0x6>;
+				};
+
+				eth1_mac: mac@82 {
+					reg = <0x82 0x6>;
+				};
+			};
+		};
 	};
 
 	aliases {

-- 
2.47.1

Re: [PATCH v2 0/6] nvmem: rmem: cleanup & add checksumming support for Mobileye EyeQ5

[Srinivas Kandagatla]

On Mon, 09 Dec 2024 16:59:34 +0100, Théo Lebrun wrote:
> This series is two-fold.
> 
>  - First some cleanup to nvmem/rmem.
> 
>    [PATCH 2/6] nvmem: specify ->reg_read/reg_write() expected return values
>    [PATCH 3/6] nvmem: rmem: make ->reg_read() straight forward code
>    [PATCH 4/6] nvmem: rmem: remove unused struct rmem::size field
> 
> [...]

Applied, thanks!

[1/6] dt-bindings: nvmem: rmem: Add mobileye,eyeq5-bootloader-config
      commit: 21c97c8e6c3a71be6bbc65c1208179aa660fe09c
[2/6] nvmem: specify ->reg_read/reg_write() expected return values
      commit: c66eab50332f47a12fca4839e64beceb125dacd7
[3/6] nvmem: rmem: make ->reg_read() straight forward code
      commit: 231465538ec46e7cfccc76cbf9f62525acba7fc0
[4/6] nvmem: rmem: remove unused struct rmem::size field
      commit: f6565dd8f1f38ef7eecba78848bea20255177a0d
[5/6] nvmem: rmem: add CRC validation for Mobileye EyeQ5 NVMEM
      commit: 1b2cc9a90cf4542a3267dedcd5db69fc75da0ef6
[6/6] MIPS: mobileye: eyeq5: add bootloader config reserved memory
      commit: b5edb40c25c38b0c6cc7330dea75efc4bc7b0c2e

Best regards,
-- 
Srinivas Kandagatla <srinivas.kandagatla@linaro.org>