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

[Chee, Tien Fong]

On Fri, 2018-06-29 at 14:13 +0200, Anatolij Gustschin wrote:
> 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.
> 
Okay, i will add the description into doc. I can create a function to
release memory allocation and setting both *firmware_p and fw_priv to
null for user.
> 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.
> 
Okay, this should work in this way by checking both *firmware_p and
fw_priv, if they are not null, memory allocation would be skipped for
better performance. However, *firmware_p always need to get updated
with function arguments, because it could be chunk by chunk
transferring if image is larger than available buffer.
> > 
> > +
> > +	fw_priv->name = name;
> > +	fw_priv->offset = offset;
> > +	firmware->data = dbuf;
> > +	firmware->size = size;
> > +	firmware->priv = fw_priv;
> > +	*firmware_p = firmware;
> > +
> > +	return 0;
> > +}
> --
> Anatolij
>