Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 18 Dec 2023 23:10:20 +0100 | Subject | Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout | From | Rafał Miłecki <> |
| |
On 18.12.2023 15:21, Miquel Raynal wrote: > Hi Rafał, > > zajec5@gmail.com wrote on Mon, 18 Dec 2023 14:37:22 +0100: > >> From: Rafał Miłecki <rafal@milecki.pl> >> >> 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@milecki.pl> >> --- > > 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
| ![\](/images/icornerr.gif) |