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

["Rafał Miłecki" <zajec5@gmail.com>]

On 19.12.2023 10:55, Rafał Miłecki wrote:
> 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.

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?
>