Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout
From: Rafał Miłecki
Date: Tue Dec 19 2023 - 05:10:36 EST
On 19.12.2023 10:55, Rafał Miłecki wrote:
On 19.12.2023 08:55, Miquel Raynal wrote:
Hi Rafał,
zajec5@xxxxxxxxx wrote on Mon, 18 Dec 2023 23:10:20 +0100:
On 18.12.2023 15:21, Miquel Raynal wrote:
Hi Rafał,
zajec5@xxxxxxxxx wrote on Mon, 18 Dec 2023 14:37:22 +0100:
From: Rafał Miłecki <rafal@xxxxxxxxxx>
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 <rafal@xxxxxxxxxx>
---
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?