[U-Boot] [PATCH v5 07/20] common: Generic firmware loader for file system

[Chee, Tien Fong <tien.fong.chee@intel.com>]

On Rab, 2017-12-06 at 12:00 +0100, Lothar Waßmann wrote:
> Hi,
> 
> On Wed, 6 Dec 2017 10:06:21 +0000 Chee, Tien Fong wrote:
> > 
> > On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote:
> > > 
> > > Hi,
> > > 
> > > On Tue,  5 Dec 2017 15:57:57 +0800 tien.fong.chee at intel.com
> > > wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee@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@intel.com>
> > > > ---
> > > >  common/Makefile     |   1 +
> > > >  common/fs_loader.c  | 304
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/fs_loader.h |  30 ++++++
> > > >  3 files changed, 335 insertions(+)
> > > >  create mode 100644 common/fs_loader.c
> > > >  create mode 100644 include/fs_loader.h
> > > > 
> > > > diff --git a/common/Makefile b/common/Makefile
> > > > index 801ea31..419e915 100644
> > > > --- a/common/Makefile
> > > > +++ b/common/Makefile
> > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
> > > >  obj-y += command.o
> > > >  obj-y += s_record.o
> > > >  obj-y += xyzModem.o
> > > > +obj-y += fs_loader.o
> > > > diff --git a/common/fs_loader.c b/common/fs_loader.c
> > > > new file mode 100644
> > > > index 0000000..04f682b
> > > > --- /dev/null
> > > > +++ b/common/fs_loader.c
> > > > @@ -0,0 +1,304 @@
> > > > +/*
> > > > + * Copyright (C) 2017 Intel Corporation <www.intel.com>
> > > > + *
> > > > + * SPDX-License-Identifier:    GPL-2.0
> > > > + */
> > > > +
> > > > +#include <common.h>
> > > > +#include <errno.h>
> > > > +#include <fs.h>
> > > > +#include <fs_loader.h>
> > > > +#include <nand.h>
> > > > +#include <sata.h>
> > > > +#include <spi.h>
> > > > +#include <spi_flash.h>
> > > > +#include <spl.h>
> > > > +#include <linux/string.h>
> > > > +#include <usb.h>
> > > > +
> > > > +static struct device_location default_locations[] = {
> > > > +	{
> > > > +		.name = "mmc",
> > > > +		.devpart = "0:1",
> > > > +	},
> > > > +	{
> > > > +		.name = "usb",
> > > > +		.devpart = "0:1",
> > > > +	},
> > > > +	{
> > > > +		.name = "sata",
> > > > +		.devpart = "0:1",
> > > > +	},
> > > > +};
> > > > +
> > > > +/* USB build is not supported yet in SPL */
> > > > +#ifndef CONFIG_SPL_BUILD
> > > > +#ifdef CONFIG_USB_STORAGE
> > > > +static int init_usb(void)
> > > > +{
> > > > +	int err = 0;
> > > > 
> > > Useless initialization.
> > > 
> > noted.
> > > 
> > > > 
> > > > 
> > > > +	err = usb_init();
> > > > +
> > > > +	if (err)
> > > > 
> > > Unnecessary blank line.
> > > 
> > Sorry, i'm not catching you because there is no blank line between
> > "if"
> > and "return"
> > 
> I meant the blank line between 'err = ...' and 'if (err)'
> 
Okay, i can change that.
> > 
> > > 
> > > > 
> > > > 
> > > > +		if (!strcmp(default_locations[i].name, name))
> > > > +			default_locations[i].devpart =
> > > > devpart;
> > > > +	}
> > > > +
> > > > +	return;
> > > > +}
> > > > +
> > > > +/*
> > > > + * 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;
> > > > +	int ret = 0;
> > > > +
> > > > +	if (!name || name[0] == '\0')
> > > > +		ret = -EINVAL;
> > > > +
> > > Is it really useful to continue here initializing the 'firmware'
> > > struct and returning an error at the end?
> > > 
> > I try to keep it very close to Linux firmware loader. When more API
> > ported in from Linux in future, this can be helper function.
> > Anyway, i
> > have no strong opinion, i can move to caller if you guys think that
> > is
> > better.
> The Linux firmware loader has this:
> > 
> > 	if (!name || name[0] == '\0') {
> > 		ret = -EINVAL;
> > 		goto out;
> > 	}
> Note the 'goto out' which is missing in your code.
> If following the Linux code closely, you would have to set
> *firmware_p
> to NULL and exit in this case.
> 
I can set the *firmware_p to NUll in failing case. But, i checked the
static int _request_firmware_prepare in Linux, there is no goto out
error handling method in the function. Fyi, there is no allocated
memory release in U-Boot.
https://elixir.free-electrons.com/linux/v4.13.15/source/drivers/base/fi
rmware_class.c#L1191
> 
> Lothar Waßmann