Messages in this thread | | | Date | Tue, 19 Dec 2023 10:55:04 +0100 | Subject | Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout | From | Rafał Miłecki <> |
| |
On 19.12.2023 08:55, Miquel Raynal wrote: > Hi Rafał, > > zajec5@gmail.com wrote on Mon, 18 Dec 2023 23:10:20 +0100: > >> 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 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?
| |