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

Rafał Miłecki
2023-12-18T22:10:23Z

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