[U-Boot] [PATCH v3 3/3] common: Generic loader for file system

Anatolij Gustschin
2018-06-29T12:13:34Z

Hi,

please see some comments below.

On Mon, 25 Jun 2018 21:28:58 +0800
tien.fong.chee at intel.com tien.fong.chee at intel.com wrote:

> +/**
> + * _request_firmware_prepare - Prepare firmware struct.
> + *
> + * @firmware_p: Pointer to pointer to firmware image.
> + * @name: Name of firmware file.
> + * @dbuf: Address of buffer to load firmware into.
> + * @size: Size of buffer.
> + * @offset: Offset of a file for start reading into buffer.
> + *
> + * Return: Negative value if fail, 0 for successful.
> + */
> +static int _request_firmware_prepare(struct firmware **firmware_p,
> + const char *name, void *dbuf,
> + size_t size, u32 offset)
> +{
> + struct firmware *firmware;
> + struct firmware_priv *fw_priv;
> +
> + *firmware_p = NULL;
> +
> + if (!name || name[0] == '\0')
> + return -EINVAL;
> +
> + firmware = calloc(1, sizeof(*firmware));
> + if (!firmware)
> + return -ENOMEM;
> +
> + fw_priv = calloc(1, sizeof(*fw_priv));
> + if (!fw_priv) {
> + free(firmware);
> + return -ENOMEM;
> + }

please add a note to API description that the API user should
free() *firmware_p and *firmware_p->priv structs after usage of
request_firmware_into_buf(), otherwise it will always leak memory
while subsequent calls of request_firmware_into_buf() with the
same *firmware_p argument.

Or probably we should better allow request_firmware_into_buf() to
be called multiple times with prepared *firmware_p stuct for
reloading the same firmware image when needed.
Then request_firmware_into_buf() should check if the given
*firmware_p stuct is already initialized and must not call
_request_firmware_prepare() in this case.

> +
> + fw_priv->name = name;
> + fw_priv->offset = offset;
> + firmware->data = dbuf;
> + firmware->size = size;
> + firmware->priv = fw_priv;
> + *firmware_p = firmware;
> +
> + return 0;
> +}

--
Anatolij