[U-Boot] [PATCH v4 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Sel, 2017-12-19 at 12:21 +0100, Lothar Waßmann wrote:
> Hi,
> 
> On Tue, 19 Dec 2017 10:31:13 +0000 Chee, Tien Fong wrote:
> > 
> > On Isn, 2017-12-18 at 08:39 +0100, Lothar Waßmann wrote:
> > > 
> > > Hi,
> > > 
> > > On Mon, 18 Dec 2017 13:10:56 +0800 tien.fong.chee at intel.com
> > > wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee at intel.com>
> > > > 
> > > > This is file system generic loader which can be used to load
> > > > the file image from the storage into target such as memory.
> > > > The consumer driver would then use this loader to program
> > > > whatever,
> > > > ie. the FPGA device.
> > > > 
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
> > > > ---
> > > >  common/Makefile            |   1 +
> > > >  common/fs_loader.c         | 311
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > >  doc/README.firmware_loader |  77 +++++++++++
> > > >  include/fs_loader.h        |  28 ++++
> > > >  4 files changed, 417 insertions(+)
> > > >  create mode 100644 common/fs_loader.c
> > > >  create mode 100644 doc/README.firmware_loader
> > > >  create mode 100644 include/fs_loader.h
> > > > 
> > > [...]
> > > > 
> > > > 
> > > > diff --git a/common/fs_loader.c b/common/fs_loader.c
> > > > new file mode 100644
> > > > index 0000000..81cf5d6
> > > > --- /dev/null
> > > > +++ b/common/fs_loader.c
> > > [...]
> > > > 
> > > > 
> > > > +/*
> > > > + * Prepare firmware struct;
> > > > + * return -ve if fail.
> > > > + */
> > > > +static int _request_firmware_prepare(struct firmware
> > > > **firmware_p,
> > > > +				     const char *name, void
> > > > *dbuf,
> > > > +				     size_t size, u32 offset)
> > > > +{
> > > > +	struct firmware *firmware = NULL;
> > > > +	struct firmware_priv *fw_priv = NULL;
> > > > +
> > > > +	*firmware_p = NULL;
> > > > +
> > > > +	if (!name || name[0] == '\0')
> > > > +		return -EINVAL;
> > > > +
> > > > +	*firmware_p = firmware = calloc(1, sizeof(*firmware));
> > > > +
> > > > +	if (!firmware) {
> > > > +		printf("%s: calloc(struct firmware) failed\n",
> > > > __func__);
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	fw_priv = calloc(1, sizeof(*fw_priv));
> > > > +
> > > > +	if (!fw_priv) {
> > > > +		printf("%s: calloc(struct fw_priv) failed\n",
> > > > __func__);
> > > > +		return -ENOMEM;
> > > What about freeing 'firmware' and NULLing *firmware_p here?
> > There is no "freeing" support in U-Boot. I can assign NULL
> > 
> How do you come to that conclusion?
> 
Sorry for misleading because i was tracking the free function until i
saw the function direct return when the bit GD_FLG_FULL_MALLOC_INIT was
found in gd->flags in beginning of the function long time ago. So, i
always assume that memory will always be freed in relocation on most
cases.
I will put the free to the firmware in next release.
> > 
> > to *firmware_p.
> > > 
> > > Or better, do the assignment of *firmware_p at the end.
> > Are you means switch the location between *firmware_p and fw_priv
> > in
> > calloc?
> > 
> No. I would assign *firmware_p only when everything else was OK, so
> that there won't be a valid pointer in *firmware_p when the struct
> firmware it is pointing to has not been set up completely.
> 
> I would do like this:
> 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) {
> 		printf("%s: calloc(struct firmware) failed\n",
> __func__);
> 		return -ENOMEM;
> 	}
> 
> 	fw_priv = calloc(1, sizeof(*fw_priv));
> 	if (!fw_priv) {
> 		printf("%s: calloc(struct fw_priv) failed\n",
> __func__);
> 		free(firmware);
> 		return -ENOMEM;
> 	}
> 
> 	fw_priv->name = name;
> 	fw_priv->offset = offset;
> 	firmware->data = dbuf;
> 	firmware->size = size;
> 	firmware->priv = fw_priv;
> 	*firmware_p = firmware;
> 
> 	return 0;
> }
> so that *firmware_p is only assigned to when everything was OK.
> (Note, that there is no need to initialize firmware and fw_priv to
> NULL)
> 
Okay.
> > 
> > > 
> > > > 
> > > > + * fw_get_filesystem_firmware - load firmware into an
> > > > allocated
> > > > buffer
> > > > + * @location: An array of supported firmware location
> > > > + * @firmware_p: pointer to firmware image
> > > > + *
> > > > + * @return: size of total read
> > > > + *	    -ve when error
> > > > + */
> > > > +static int fw_get_filesystem_firmware(struct device_location
> > > > *location,
> > > > +				      struct firmware
> > > > *firmware_p)
> > > > +{
> > > > +	struct firmware_priv *fw_priv = NULL;
> > > > +	loff_t actread;
> > > > +	char *dev_part;
> > > > +	int ret;
> > > > +
> > > > +	dev_part = env_get("FW_DEV_PART");
> > > > +	if (dev_part)
> > > > +		set_storage_devpart(location->name, dev_part);
> > > > +
> > > > +	ret = init_storage_device(location);
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > > > +	select_fs_dev(location);
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > > > +	fw_priv = (struct firmware_priv *)firmware_p->priv;
> > > > +
> > > useless type cast.
> > > 
> > I assume you are saying autocast, right? Let me check is there any
> > warning from compiler after removing the cast.
> > 
> In C void pointers can be assigned to any type pointers without need
> for
> a cast.
> 
Oo....initially i worry the compiler would trigger warnings when the
fw_priv was pass as parameter to fs_read function without casting.
However, i have ran the check, and there is no warning from compiler
without casting. So, i will remove the cast.
> 
> Lothar Waßmann