From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chee, Tien Fong <tien.fong.chee@intel.com>
Date: Mon, 2 Jul 2018 08:12:12 +0000
Subject: [U-Boot] [PATCH v3 3/3] common: Generic loader for file system
In-Reply-To: <20180629141334.09b2000f@crub>
References: <1529933338-11010-1-git-send-email-tien.fong.chee@intel.com>
 <1529933338-11010-4-git-send-email-tien.fong.chee@intel.com>
 <20180629141334.09b2000f@crub>
Message-ID: <1530519132.9877.8.camel@intel.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
To: u-boot@lists.denx.de

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
>