Message-ID: <bad448ab-15dc-46de-983c-56fa6dfb758f@redhat.com>
Date: Wed, 13 Aug 2025 14:43:57 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, Jonathan Corbet <corbet@....net>,
 Prathosh Satish <Prathosh.Satish@...rochip.com>, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, Michal Schmidt <mschmidt@...hat.com>,
 Petr Oros <poros@...hat.com>
Subject: Re: [PATCH net-next v2 3/5] dpll: zl3073x: Add firmware loading
 functionality



On 12. 08. 25 1:11 dop., Przemek Kitszel wrote:
> On 8/11/25 16:40, Ivan Vecera wrote:
>> Add functionality for loading firmware files provided by the vendor
>> to be flashed into the device's internal flash memory. The firmware
>> consists of several components, such as the firmware executable itself,
>> chip-specific customizations, and configuration files.
>>
>> The firmware file contains at least a flash utility, which is executed
>> on the device side, and one or more flashable components. Each component
>> has its own specific properties, such as the address where it should be
>> loaded during flashing, one or more destination flash pages, and
>> the flashing method that should be used.
>>
>> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
>> ---
>> v2:
>> * added additional includes
>> * removed empty line
>> * '*(dst+len)' -> '*(dst + len)'
>> * 'Santity' -> 'Sanity'
>> * fixed smatch warning about uninitialized 'rc'
>> ---
>>   drivers/dpll/zl3073x/Makefile |   2 +-
>>   drivers/dpll/zl3073x/fw.c     | 498 ++++++++++++++++++++++++++++++++++
>>   drivers/dpll/zl3073x/fw.h     |  52 ++++
>>   3 files changed, 551 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/dpll/zl3073x/fw.c
>>   create mode 100644 drivers/dpll/zl3073x/fw.h
>>
> 
> overview:
> I don't like zl3073x_fw_readline() and it's usage - sscanf will do IMO
> 
> please find other feedback inline
> 
>> +/* Sanity check */
>> +static_assert(ARRAY_SIZE(component_info) == ZL_FW_NUM_COMPONENTS);
>> +
>> +/**
>> + * zl3073x_fw_readline - Read next line from firmware
>> + * @dst: destination buffer
>> + * @dst_sz: destination buffer size
>> + * @psrc: source buffer
>> + * @psrc_sz: source buffer size
>> + *
>> + * The function read next line from the firmware buffer specified by 
>> @psrc
>> + * and @psrc_sz and stores it into buffer specified by @dst and @dst_sz.
>> + * The pointer @psrc and remaining bytes in @psrc_sz are updated 
>> accordingly.
>> + *
>> + * Return: number of characters read on success, -EINVAL on error
>> + */
>> +static ssize_t
>> +zl3073x_fw_readline(char *dst, size_t dst_sz, const char **psrc,
>> +            size_t *psrc_sz)
>> +{
>> +    const char *ptr = *psrc;
>> +    size_t len;
>> +
>> +    /* Skip any existing new-lines at the beginning */
>> +    ptr = memchr_inv(*psrc, '\n', *psrc_sz);
>> +    if (ptr) {
>> +        *psrc_sz -= ptr - *psrc;
>> +        *psrc = ptr;
>> +    }
>> +
>> +    /* Now look for the next new-line in the source */
>> +    ptr = memscan((void *)*psrc, '\n', *psrc_sz);
>> +    len = ptr - *psrc;
>> +
>> +    /* Return error if the source line is too long for destination */
>> +    if (len >= dst_sz)
>> +        return -EINVAL;
>> +
>> +    /* Copy the line from source and append NUL char  */
>> +    memcpy(dst, *psrc, len);
>> +    *(dst + len) = '\0';
>> +
>> +    *psrc = ptr;
>> +    *psrc_sz -= len;
>> +
>> +    /* Return number of read chars */
>> +    return len;
>> +}
>> +
>> +/**
>> + * zl3073x_fw_component_alloc - Alloc structure to hold firmware 
>> component
>> + * @size: size of buffer to store data
>> + *
>> + * Return: pointer to allocated component structure or NULL on error.
>> + */
>> +static struct zl3073x_fw_component *
>> +zl3073x_fw_component_alloc(size_t size)
>> +{
>> +    struct zl3073x_fw_component *comp;
>> +
>> +    comp = kzalloc(sizeof(*comp), GFP_KERNEL);
>> +    if (!comp)
>> +        return NULL;
>> +
>> +    comp->size = size;
>> +    comp->data = kzalloc(size, GFP_KERNEL);
>> +    if (!comp->data) {
>> +        kfree(comp);
>> +        return NULL;
>> +    }
>> +
>> +    return comp;
>> +}
>> +
>> +/**
>> + * zl3073x_fw_component_free - Free allocated component structure
>> + * @comp: pointer to allocated component
>> + */
>> +static void
>> +zl3073x_fw_component_free(struct zl3073x_fw_component *comp)
>> +{
>> +    if (comp)
>> +        kfree(comp->data);
>> +
>> +    kfree(comp);
>> +}
>> +
>> +/**
>> + * zl3073x_fw_component_id_get - Get ID for firmware component name
>> + * @name: input firmware component name
>> + *
>> + * Return:
>> + * - ZL3073X_FW_COMPONENT_* ID for known component name
>> + * - ZL3073X_FW_COMPONENT_INVALID if the given name is unknown
>> + */
>> +static enum zl3073x_fw_component_id
>> +zl3073x_fw_component_id_get(const char *name)
>> +{
>> +    enum zl3073x_fw_component_id id;
>> +
>> +    for (id = ZL_FW_COMPONENT_UTIL; id < ZL_FW_NUM_COMPONENTS; id++)
> 
> I would type the start as "id = 0"
> (as you did in other functions, eg zl3073x_fw_free())

Will change.

>> +        if (!strcasecmp(name, component_info[id].name))
>> +            return id;
>> +
>> +    return ZL_FW_COMPONENT_INVALID;
>> +}
>> +
>> +/**
>> + * zl3073x_fw_component_load - Load component from firmware source
>> + * @zldev: zl3073x device structure
>> + * @pcomp: pointer to loaded component
>> + * @psrc: data pointer to load component from
>> + * @psize: remaining bytes in buffer
>> + * @extack: netlink extack pointer to report errors
>> + *
>> + * The function allocates single firmware component and loads the 
>> data from
>> + * the buffer specified by @psrc and @psize. Pointer to allocated 
>> component
>> + * is stored in output @pcomp. Source data pointer @psrc and 
>> remaining bytes
>> + * @psize are updated accordingly.
>> + *
>> + * Return: 0 on success, <0 on error
> 
> document return of 1

Will fix.

>> + */
>> +static ssize_t
>> +zl3073x_fw_component_load(struct zl3073x_dev *zldev,
>> +              struct zl3073x_fw_component **pcomp,
>> +              const char **psrc, size_t *psize,
>> +              struct netlink_ext_ack *extack)
>> +{
>> +    const struct zl3073x_fw_component_info *info;
>> +    struct zl3073x_fw_component *comp = NULL;
>> +    struct device *dev = zldev->dev;
>> +    enum zl3073x_fw_component_id id;
>> +    ssize_t len, count;
>> +    u32 comp_size;
>> +    char line[32];
>> +    int rc;
>> +
>> +    /* Fetch image name from input */
>> +    len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
>> +    if (len < 0) {
>> +        rc = len;
>> +        goto err_unexpected;
>> +    } else if (!len) {
>> +        /* No more data */
>> +        return 0;
>> +    }
>> +
>> +    dev_dbg(dev, "Firmware component '%s' found\n", line);
>> +
>> +    id = zl3073x_fw_component_id_get(line);
>> +    if (id == ZL_FW_COMPONENT_INVALID) {
>> +        ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] unknown component type",
>> +                   line);
>> +        return -EINVAL;
>> +    }
>> +
>> +    info = &component_info[id];
>> +
>> +    /* Fetch image size from input */
>> +    len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
>> +    if (len < 0) {
>> +        rc = len;
>> +        goto err_unexpected;
>> +    } else if (!len) {
>> +        ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] missing size",
>> +                   info->name);
>> +        return -ENODATA;
>> +    }
>> +
>> +    rc = kstrtou32(line, 10, &comp_size);
>> +    if (rc) {
>> +        ZL3073X_FW_ERR_MSG(zldev, extack,
>> +                   "[%s] invalid size value '%s'", info->name,
>> +                   line);
>> +        return rc;
>> +    }
> 
> why not sscanf()? it would greatly simplify the above, and likely you
> could entriely remove zl3073x_fw_readline() too

I cannot use sscanf here because the input buffer is passed by devlink
as 'struct firmware' buffer and this buffer is not null-terminated.
In this case the driver would have to make a copy of the input buffer
and place null char at the end. The firmware can have up to 256kB and
IMO it is better to process original firmware buffer instead of extra
copy.

>> +
>> +    comp_size *= sizeof(u32); /* convert num of dwords to bytes */
>> +
>> +    /* Check image size validity */
>> +    if (comp_size > component_info[id].max_size) {
>> +        ZL3073X_FW_ERR_MSG(zldev, extack,
>> +                   "[%s] component is too big (%u bytes)\n",
>> +                   info->name, comp_size);
>> +        return -EINVAL;
>> +    }
>> +
>> +    dev_dbg(dev, "Indicated component image size: %u bytes\n", 
>> comp_size);
>> +
>> +    /* Alloc component */
>> +    comp = zl3073x_fw_component_alloc(comp_size);
>> +    if (!comp) {
>> +        ZL3073X_FW_ERR_MSG(zldev, extack, "failed to alloc memory");
>> +        return -ENOMEM;
>> +    }
>> +    comp->id = id;
>> +
>> +    /* Load component data from firmware source */
>> +    for (count = 0; count < comp_size; count += 4) {
>> +        len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
>> +        if (len < 0) {
>> +            rc = len;
>> +            goto err_unexpected;
>> +        } else if (!len) {
>> +            ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] missing data",
>> +                       info->name);
>> +            rc = -ENODATA;
>> +            goto err;
>> +        }
>> +
>> +        rc = kstrtou32(line, 16, comp->data + count);
>> +        if (rc) {
>> +            ZL3073X_FW_ERR_MSG(zldev, extack,
>> +                       "[%s] invalid data: '%s'",
>> +                       info->name, line);
>> +            goto err;
>> +        }
>> +    }
>> +
>> +    *pcomp = comp;
>> +
>> +    return 1;> +
>> +err_unexpected:
>> +    ZL3073X_FW_ERR_MSG(zldev, extack, "unexpected input");
>> +err:
>> +    zl3073x_fw_component_free(comp);
>> +
>> +    return rc;
>> +}
>> +
>> +/**
>> + * zl3073x_fw_free - Free allocated firmware
>> + * @fw: firmware pointer
>> + *
>> + * The function frees existing firmware allocated by @zl3073x_fw_load.
>> + */
>> +void zl3073x_fw_free(struct zl3073x_fw *fw)
>> +{
>> +    size_t i;
>> +
>> +    if (!fw)
>> +        return;
>> +
>> +    for (i = 0; i < ZL_FW_NUM_COMPONENTS; i++)
>> +        zl3073x_fw_component_free(fw->component[i]);
>> +
>> +    kfree(fw);
>> +}
>> +
>> +/**
>> + * zl3073x_fw_load - Load all components from source
>> + * @zldev: zl3073x device structure
>> + * @data: source buffer pointer
>> + * @size: size of source buffer
>> + * @extack: netlink extack pointer to report errors
>> + *
>> + * The functions allocate firmware structure and loads all components 
>> from
>> + * the given buffer specified by @data and @size.
>> + *
>> + * Return: pointer to firmware on success, error pointer on error
>> + */
>> +struct zl3073x_fw *zl3073x_fw_load(struct zl3073x_dev *zldev, const 
>> char *data,
>> +                   size_t size, struct netlink_ext_ack *extack)
>> +{
>> +    struct zl3073x_fw_component *comp;
>> +    enum zl3073x_fw_component_id id;
>> +    struct zl3073x_fw *fw;
>> +    ssize_t rc;
>> +
>> +    /* Allocate firmware structure */
>> +    fw = kzalloc(sizeof(*fw), GFP_KERNEL);
>> +    if (!fw)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    do {
>> +        /* Load single component */
>> +        rc = zl3073x_fw_component_load(zldev, &comp, &data, &size,
>> +                           extack);
>> +        if (rc <= 0)
>> +            /* Everything was read or error occurred */
>> +            break;
>> +
>> +        id = comp->id;
>> +
>> +        /* Report error if the given component is present twice
>> +         * or more.
>> +         */
>> +        if (fw->component[id]) {
>> +            ZL3073X_FW_ERR_MSG(zldev, extack,
>> +                       "duplicate component '%s' detected",
>> +                       component_info[id].name);
>> +            zl3073x_fw_component_free(comp);
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +
>> +        fw->component[id] = comp;
>> +    } while (1);
> 
> s/1/true/

Will fix.

>> +
>> +    if (rc) {
>> +        /* Free allocated firmware in case of error */
>> +        zl3073x_fw_free(fw);
> 
> I found no call to it on success.

The caller will deallocate it using zl3073x_fw_free()

> 
>> +        return ERR_PTR(rc);
>> +    }
>> +
>> +    return fw;
>> +}
> 
> 
>> +++ b/drivers/dpll/zl3073x/fw.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef _ZL3073X_FW_H
>> +#define _ZL3073X_FW_H
>> +
>> +/*
>> + * enum zl3073x_fw_component_id - Identifiers for possible flash 
>> components
>> + */
>> +enum zl3073x_fw_component_id {
>> +    ZL_FW_COMPONENT_INVALID = -1,
>> +    ZL_FW_COMPONENT_UTIL = 0,
>> +    ZL_FW_COMPONENT_FW1,
>> +    ZL_FW_COMPONENT_FW2,
>> +    ZL_FW_COMPONENT_FW3,
>> +    ZL_FW_COMPONENT_CFG0,
>> +    ZL_FW_COMPONENT_CFG1,
>> +    ZL_FW_COMPONENT_CFG2,
>> +    ZL_FW_COMPONENT_CFG3,
>> +    ZL_FW_COMPONENT_CFG4,
>> +    ZL_FW_COMPONENT_CFG5,
>> +    ZL_FW_COMPONENT_CFG6,
>> +    ZL_FW_NUM_COMPONENTS,
> 
> no comma after enum that will be last forever (guard/size/max/num/cnt)

Will fix.

>> +};
> 

Thanks,
Ivan

Powered by blists - more mailing lists