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

Message ID 1512460690-3454-8-git-send-email-tien.fong.chee@intel.com
State New
Delegated to: Marek Vasut
Headers show
Series
  • Add FPGA driver, SDRAM driver, generic firmware loader and booting U-Boot.
Related show

Commit Message

Chee, Tien Fong Dec. 5, 2017, 7:57 a.m.
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

Comments

Lothar Waßmann Dec. 5, 2017, 8:53 a.m. | #1
Hi,

On Tue,  5 Dec 2017 15:57:57 +0800 tien.fong.chee@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.

> +	err = usb_init();
> +
> +	if (err)
>
Unnecessary blank line.

> +		return err;
> +
> +#ifndef CONFIG_DM_USB
> +	err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
> +#endif
> +
> +	return err;
> +}
> +#else
> +static int init_usb(void)
> +{
> +	printf("Error: Cannot load flash image: no USB support\n");
> +	return -ENOSYS;
> +}
> +#endif
> +#endif
> +
> +#ifdef CONFIG_SATA
> +static int init_storage_sata(void)
> +{
> +	return sata_probe(0);
> +}
> +#else
> +static int init_storage_sata(void)
> +{
> +	printf("Error: Cannot load image: no SATA support\n");
> +	return -ENOSYS;
> +}
> +#endif
> +
> +#ifdef CONFIG_CMD_UBIFS
> +static int mount_ubifs(struct device_location *location)
> +{
> +	int ret = 0;
>
Useless initialization.

> +	char cmd[32];
> +
> +	sprintf(cmd, "ubi part %s", location->mtdpart);
> +
> +	ret = run_command(cmd, 0);
> +
> +	if (ret)
Unnecessary blank line.

> +		return ret;
> +
> +	sprintf(cmd, "ubifsmount %s", location->ubivol);
> +
> +	ret = run_command(cmd, 0);
> +
> +	return res;
>
s/res/ret/
You obviously never compiled this code with CONFIG_CMD_UBIFS enabled.

> +}
> +
> +static int umount_ubifs(void)
> +{
> +	return run_command("ubifsumount", 0);
> +}
> +#else
> +static int mount_ubifs(struct device_location *location)
> +{
> +	printf("Error: Cannot load image: no UBIFS support\n");
> +	return -ENOSYS;
> +}
> +
> +static int umount_ubifs(void)
> +{
> +	printf("Error: Cannot unmount UBIFS: no UBIFS support\n");
>
This error message is useless, since umount_ubifs() would
probably never be called when mount_ubifs() failed.

> +}
> +#endif
> +
> +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> +static int init_mmc(void)
> +{
> +	/* Just for the case MMC is not yet initialized */
> +	struct mmc *mmc = NULL;
> +	int err = 0;
>
Useless initialization.

> +
> +	spl_mmc_find_device(&mmc, spl_boot_device());
> +
> +	err = mmc_init(mmc);
> +
> +	if (err) {
> +		printf("spl: mmc init failed with error: %d\n", err);
> +		return err;
> +	}
> +
> +	return err;
> +}
> +#else
> +static int init_mmc(void)
> +{
> +	/* Expect somewhere already initialize MMC */
> +	return 0;
> +}
> +#endif
> +
> +static int select_fs_dev(struct device_location *location)
> +{
> +	int ret = 0;
>
Useless initialization. Omitting the initialization here makes sure,
that 'ret' is properly set in each of the below if/else clauses.
Otherwise the compiler will complain about an unitialized use of 'ret'.

> +	if (!strcmp("mmc", location->name))
> +		ret = fs_set_blk_dev("mmc", location->devpart, FS_TYPE_ANY);
> +	else if (!strcmp("usb", location->name))
> +		ret = fs_set_blk_dev("usb", location->devpart, FS_TYPE_ANY);
> +	else if (!strcmp("sata", location->name))
> +		ret = fs_set_blk_dev("sata", location->devpart, FS_TYPE_ANY);
> +	else if (!strcmp("ubi", location->name))
> +		if (location->ubivol != NULL)
> +			ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
> +		else
> +			ret = -ENODEV;
> +	else {
> +		printf("Error: unsupported location storage.\n");
> +		return -ENODEV;
> +	}
> +
All if/else clauses should be enclosed in {}.
Refer to Documentation/CodingStyle in the Linux source which also
applies to U-Boot.

> +	if (ret)
> +		printf("Error: could not access storage.\n");
> +
> +	return ret;
> +}
> +
> +static int init_storage_device(struct device_location *location)
> +{
> +	int ret = 0;
> +#ifndef CONFIG_SPL_BUILD
> +	/* USB build is not supported yet in SPL */
> +	if (!strcmp("usb", location->name))
> +		ret = init_usb();
> +#endif
> +
> +	if (!strcmp("mmc", location->name))
> +		ret = init_mmc();
> +
> +	if (!strcmp("sata", location->name))
> +		ret = init_storage_sata();
> +
> +	if (location->ubivol != NULL)
> +		ret = mount_ubifs(location);
> +
> +	return ret;
> +}
> +
> +static void set_storage_devpart(char *name, char *devpart)
> +{
> +	int i;
> +	u32 size;
> +
> +	size = ARRAY_SIZE(default_locations);
> +
> +	for (i = 0; i < size; i++) {
>
The compiler should complain about a comparison of signed and unsigned
integers.
I would do:
	size_t i;

	for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
[...]

> +		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?

> +	*firmware_p = firmware = calloc(1, sizeof(*firmware));
> +
> +	if (!firmware) {
> +		printf("%s: calloc(struct firmware) failed\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	firmware->name = name;
> +	firmware->data = dbuf;
> +	firmware->size = size;
> +	firmware->offset = offset;
> +
> +	return ret;
> +}
> +
> +/*
> + * fw_get_filesystem_firmware - load firmware into a allocated buffer
> + * @firmware_p: pointer to firmware image
> + * @location: An array of supported firmware location
> + *
> + * @return: size of total read
> + *	    -ve when error
> + */
> +static int fw_get_filesystem_firmware(struct device_location *location,
> +				      struct firmware *firmware_p)
> +{
> +	loff_t actread;
> +	char *dev_part;
> +	int ret = 0;
> +
Useless initialization.

> +	dev_part = env_get("FW_DEV_PART");
> +
> +	if (dev_part)
>
Unnecessary blank line.

> +		set_storage_devpart(location->name, dev_part);
> +
> +	ret = init_storage_device(location);
> +
> +	if (ret)
Unnecessary blank line.

> +		goto out;
> +
> +	select_fs_dev(location);
> +
> +	ret = fs_read(firmware_p->name, (u32)firmware_p->data,
>
The second parameter of fs_read is of type 'ulong' NOT 'u32'!
This will blow up when compiling for 64bit machines.

> +		      firmware_p->offset, firmware_p->size, &actread);
> +
> +	if (ret || (actread != firmware_p->size)) {
> +		printf("Error: %d Failed to read %s from flash %lld ",
> +		      ret,
> +		      firmware_p->name,
> +		      actread);
> +		printf("!= %d.\n", firmware_p->size);
> +		return -EPERM;
> +		} else
> +			ret = actread;
> +
broken indentation.
All clauses of the if/else statement should be enclosed in {}.

> +out:
> +	if (location->ubivol != NULL)
> +		umount_ubifs();
> +
> +	return ret;
> +}
> +/*
> + * request_firmware_into_buf - load firmware into a previously allocated buffer
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @buf: address of buffer to load firmware into
> + * @size: size of buffer
> + * @offset: offset of a file for start reading
> + *
> + * This firmware is loaded directly into the buffer pointed to by @buf and
> + * the @firmware_p data member is pointed at @buf.
> + *
> + * @return: size of total read
> + *	    -ve when error
> + */
> +int request_firmware_into_buf(struct firmware **firmware_p,
> +			      const char *name,
> +			      struct device_location *location,
> +			      void *buf, size_t size, u32 offset)
> +{
> +	int ret = 0;
>
Useless initialization.

> +	ret = _request_firmware_prepare(firmware_p, name, buf, size, offset);
> +
> +	if (ret < 0) /* error */
>
Unnecessary blank line.

> +		return ret;
> +
> +	ret = fw_get_filesystem_firmware(location, *firmware_p);
> +
> +	return ret;
> +}
> diff --git a/include/fs_loader.h b/include/fs_loader.h
> new file mode 100644
> index 0000000..dacc1f3
> --- /dev/null
> +++ b/include/fs_loader.h
> @@ -0,0 +1,30 @@
> +/*
> + * Copyright (C) 2017 Intel Corporation <www.intel.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0
> + */
> +#ifndef _FS_LOADER_H_
> +#define _FS_LOADER_H_
> +
> +#include <linux/types.h>
> +
> +struct firmware {
> +	size_t size;		/* Size of a file */
> +	const u8 *data;		/* Buffer for file */
>
const void *data?

> +	const char *name;	/* Filename */
> +	u32 offset;		/* Offset of reading a file */
> +	void *priv;		/* Firmware loader private fields */
> +};
> +
> +struct device_location {
> +	char *name;	/* Such as mmc, usb,and sata. */
> +	char *devpart;  /* Use the load command dev:part conventions */
> +	char *mtdpart;	/* MTD partition for ubi part */
> +	char *ubivol;	/* UBI volume-name for ubifsmount */
> +};
> +
> +int request_firmware_into_buf(struct firmware **firmware_p,
> +			      const char *name,
> +			      struct device_location *location,
> +			      void *buf, size_t size, u32 offset);
> +#endif


Lothar Waßmann
Chee, Tien Fong Dec. 6, 2017, 10:06 a.m. | #2
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@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"
> > 

> > +		return err;

> > +

> > +#ifndef CONFIG_DM_USB

> > +	err = usb_stor_scan(1) < 0 ? -ENODEV : 0;

> > +#endif

> > +

> > +	return err;

> > +}

> > +#else

> > +static int init_usb(void)

> > +{

> > +	printf("Error: Cannot load flash image: no USB

> > support\n");

> > +	return -ENOSYS;

> > +}

> > +#endif

> > +#endif

> > +

> > +#ifdef CONFIG_SATA

> > +static int init_storage_sata(void)

> > +{

> > +	return sata_probe(0);

> > +}

> > +#else

> > +static int init_storage_sata(void)

> > +{

> > +	printf("Error: Cannot load image: no SATA support\n");

> > +	return -ENOSYS;

> > +}

> > +#endif

> > +

> > +#ifdef CONFIG_CMD_UBIFS

> > +static int mount_ubifs(struct device_location *location)

> > +{

> > +	int ret = 0;

> > 

> Useless initialization.

> 

noted.
> > 

> > +	char cmd[32];

> > +

> > +	sprintf(cmd, "ubi part %s", location->mtdpart);

> > +

> > +	ret = run_command(cmd, 0);

> > +

> > +	if (ret)

> Unnecessary blank line.

> 

> > 

> > +		return ret;

> > +

> > +	sprintf(cmd, "ubifsmount %s", location->ubivol);

> > +

> > +	ret = run_command(cmd, 0);

> > +

> > +	return res;

> > 

> s/res/ret/

> You obviously never compiled this code with CONFIG_CMD_UBIFS enabled.

> 

Good catch!! I will rectify.
> > 

> > +}

> > +

> > +static int umount_ubifs(void)

> > +{

> > +	return run_command("ubifsumount", 0);

> > +}

> > +#else

> > +static int mount_ubifs(struct device_location *location)

> > +{

> > +	printf("Error: Cannot load image: no UBIFS support\n");

> > +	return -ENOSYS;

> > +}

> > +

> > +static int umount_ubifs(void)

> > +{

> > +	printf("Error: Cannot unmount UBIFS: no UBIFS support\n");

> > 

> This error message is useless, since umount_ubifs() would

> probably never be called when mount_ubifs() failed.

> 

Yeah, you are right, i will remove it.
> > 

> > +}

> > +#endif

> > +

> > +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)

> > +static int init_mmc(void)

> > +{

> > +	/* Just for the case MMC is not yet initialized */

> > +	struct mmc *mmc = NULL;

> > +	int err = 0;

> > 

> Useless initialization.

> 

noted.
> > 

> > +

> > +	spl_mmc_find_device(&mmc, spl_boot_device());

> > +

> > +	err = mmc_init(mmc);

> > +

> > +	if (err) {

> > +		printf("spl: mmc init failed with error: %d\n",

> > err);

> > +		return err;

> > +	}

> > +

> > +	return err;

> > +}

> > +#else

> > +static int init_mmc(void)

> > +{

> > +	/* Expect somewhere already initialize MMC */

> > +	return 0;

> > +}

> > +#endif

> > +

> > +static int select_fs_dev(struct device_location *location)

> > +{

> > +	int ret = 0;

> > 

> Useless initialization. Omitting the initialization here makes sure,

> that 'ret' is properly set in each of the below if/else clauses.

> Otherwise the compiler will complain about an unitialized use of

> 'ret'.

> 

Okay. sure.
> > 

> > +	if (!strcmp("mmc", location->name))

> > +		ret = fs_set_blk_dev("mmc", location->devpart,

> > FS_TYPE_ANY);

> > +	else if (!strcmp("usb", location->name))

> > +		ret = fs_set_blk_dev("usb", location->devpart,

> > FS_TYPE_ANY);

> > +	else if (!strcmp("sata", location->name))

> > +		ret = fs_set_blk_dev("sata", location->devpart,

> > FS_TYPE_ANY);

> > +	else if (!strcmp("ubi", location->name))

> > +		if (location->ubivol != NULL)

> > +			ret = fs_set_blk_dev("ubi", NULL,

> > FS_TYPE_UBIFS);

> > +		else

> > +			ret = -ENODEV;

> > +	else {

> > +		printf("Error: unsupported location storage.\n");

> > +		return -ENODEV;

> > +	}

> > +

> All if/else clauses should be enclosed in {}.

> Refer to Documentation/CodingStyle in the Linux source which also

> applies to U-Boot.

> 

Okay.
> > 

> > +	if (ret)

> > +		printf("Error: could not access storage.\n");

> > +

> > +	return ret;

> > +}

> > +

> > +static int init_storage_device(struct device_location *location)

> > +{

> > +	int ret = 0;

> > +#ifndef CONFIG_SPL_BUILD

> > +	/* USB build is not supported yet in SPL */

> > +	if (!strcmp("usb", location->name))

> > +		ret = init_usb();

> > +#endif

> > +

> > +	if (!strcmp("mmc", location->name))

> > +		ret = init_mmc();

> > +

> > +	if (!strcmp("sata", location->name))

> > +		ret = init_storage_sata();

> > +

> > +	if (location->ubivol != NULL)

> > +		ret = mount_ubifs(location);

> > +

> > +	return ret;

> > +}

> > +

> > +static void set_storage_devpart(char *name, char *devpart)

> > +{

> > +	int i;

> > +	u32 size;

> > +

> > +	size = ARRAY_SIZE(default_locations);

> > +

> > +	for (i = 0; i < size; i++) {

> > 

> The compiler should complain about a comparison of signed and

> unsigned

> integers.

> I would do:

> 	size_t i;

> 

> 	for (i = 0; i < ARRAY_SIZE(default_locations); i++) {

> [...]

> 

Okay. noted.
> > 

> > +		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.
> > 

> > +	*firmware_p = firmware = calloc(1, sizeof(*firmware));

> > +

> > +	if (!firmware) {

> > +		printf("%s: calloc(struct firmware) failed\n",

> > __func__);

> > +		return -ENOMEM;

> > +	}

> > +

> > +	firmware->name = name;

> > +	firmware->data = dbuf;

> > +	firmware->size = size;

> > +	firmware->offset = offset;

> > +

> > +	return ret;

> > +}

> > +

> > +/*

> > + * fw_get_filesystem_firmware - load firmware into a allocated

> > buffer

> > + * @firmware_p: pointer to firmware image

> > + * @location: An array of supported firmware location

> > + *

> > + * @return: size of total read

> > + *	    -ve when error

> > + */

> > +static int fw_get_filesystem_firmware(struct device_location

> > *location,

> > +				      struct firmware *firmware_p)

> > +{

> > +	loff_t actread;

> > +	char *dev_part;

> > +	int ret = 0;

> > +

> Useless initialization.

> 

noted.
> > 

> > +	dev_part = env_get("FW_DEV_PART");

> > +

> > +	if (dev_part)

> > 

> Unnecessary blank line.

> 

> > 

> > +		set_storage_devpart(location->name, dev_part);

> > +

> > +	ret = init_storage_device(location);

> > +

> > +	if (ret)

> Unnecessary blank line.

> 

> > 

> > +		goto out;

> > +

> > +	select_fs_dev(location);

> > +

> > +	ret = fs_read(firmware_p->name, (u32)firmware_p->data,

> > 

> The second parameter of fs_read is of type 'ulong' NOT 'u32'!

> This will blow up when compiling for 64bit machines.

> 

Okay.
> > 

> > +		      firmware_p->offset, firmware_p->size,

> > &actread);

> > +

> > +	if (ret || (actread != firmware_p->size)) {

> > +		printf("Error: %d Failed to read %s from flash

> > %lld ",

> > +		      ret,

> > +		      firmware_p->name,

> > +		      actread);

> > +		printf("!= %d.\n", firmware_p->size);

> > +		return -EPERM;

> > +		} else

> > +			ret = actread;

> > +

> broken indentation.

> All clauses of the if/else statement should be enclosed in {}.

> 

Okay.
> > 

> > +out:

> > +	if (location->ubivol != NULL)

> > +		umount_ubifs();

> > +

> > +	return ret;

> > +}

> > +/*

> > + * request_firmware_into_buf - load firmware into a previously

> > allocated buffer

> > + * @firmware_p: pointer to firmware image

> > + * @name: name of firmware file

> > + * @buf: address of buffer to load firmware into

> > + * @size: size of buffer

> > + * @offset: offset of a file for start reading

> > + *

> > + * This firmware is loaded directly into the buffer pointed to by

> > @buf and

> > + * the @firmware_p data member is pointed at @buf.

> > + *

> > + * @return: size of total read

> > + *	    -ve when error

> > + */

> > +int request_firmware_into_buf(struct firmware **firmware_p,

> > +			      const char *name,

> > +			      struct device_location *location,

> > +			      void *buf, size_t size, u32 offset)

> > +{

> > +	int ret = 0;

> > 

> Useless initialization.

> 

noted.
> > 

> > +	ret = _request_firmware_prepare(firmware_p, name, buf,

> > size, offset);

> > +

> > +	if (ret < 0) /* error */

> > 

> Unnecessary blank line.

> 

> > 

> > +		return ret;

> > +

> > +	ret = fw_get_filesystem_firmware(location, *firmware_p);

> > +

> > +	return ret;

> > +}

> > diff --git a/include/fs_loader.h b/include/fs_loader.h

> > new file mode 100644

> > index 0000000..dacc1f3

> > --- /dev/null

> > +++ b/include/fs_loader.h

> > @@ -0,0 +1,30 @@

> > +/*

> > + * Copyright (C) 2017 Intel Corporation <www.intel.com>

> > + *

> > + * SPDX-License-Identifier:    GPL-2.0

> > + */

> > +#ifndef _FS_LOADER_H_

> > +#define _FS_LOADER_H_

> > +

> > +#include <linux/types.h>

> > +

> > +struct firmware {

> > +	size_t size;		/* Size of a file */

> > +	const u8 *data;		/* Buffer for file */

> > 

> const void *data?

> 

okay.
> > 

> > +	const char *name;	/* Filename */

> > +	u32 offset;		/* Offset of reading a file */

> > +	void *priv;		/* Firmware loader private

> > fields */

> > +};

> > +

> > +struct device_location {

> > +	char *name;	/* Such as mmc, usb,and sata. */

> > +	char *devpart;  /* Use the load command dev:part

> > conventions */

> > +	char *mtdpart;	/* MTD partition for ubi part */

> > +	char *ubivol;	/* UBI volume-name for ubifsmount */

> > +};

> > +

> > +int request_firmware_into_buf(struct firmware **firmware_p,

> > +			      const char *name,

> > +			      struct device_location *location,

> > +			      void *buf, size_t size, u32 offset);

> > +#endif

> 

> Lothar Waßmann
Lothar Waßmann Dec. 6, 2017, 11 a.m. | #3
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@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)'

> > > 
> > > +		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.


Lothar Waßmann
Chee, Tien Fong Dec. 7, 2017, 5:29 a.m. | #4
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@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
Lothar Waßmann Dec. 7, 2017, 7:49 a.m. | #5
Hi,

On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote:
> 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@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
> > 
I was referring to _request_firmware() which calls
request_firmware_prepare() and does the checking of 'name' as your
code does.


Lothar Waßmann
Chee, Tien Fong Dec. 7, 2017, 8:10 a.m. | #6
On Kha, 2017-12-07 at 08:49 +0100, Lothar Waßmann wrote:
> Hi,

> 

> On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote:

> > 

> > 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@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/bas

> > e/fi

> > rmware_class.c#L1191

> > > 

> > > 

> I was referring to _request_firmware() which calls

> request_firmware_prepare() and does the checking of 'name' as your

> code does.

> 

Ohh.....i skipped _request_firmware() because there is no error
handling required in U-Boot such as memory release. For the checking at
both firmware_p and name, i would rather to merge them into
request_firware_prepare() because they are related to each other
instead of creating one function just for checking purpose. What do you
think?
> 

> Lothar Waßmann
Lothar Waßmann Dec. 7, 2017, 9 a.m. | #7
Hi,

On Thu, 7 Dec 2017 08:10:06 +0000 Chee, Tien Fong wrote:
> On Kha, 2017-12-07 at 08:49 +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote:
> > > 
> > > 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@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/bas
> > > e/fi
> > > rmware_class.c#L1191
> > > > 
> > > > 
> > I was referring to _request_firmware() which calls
> > request_firmware_prepare() and does the checking of 'name' as your
> > code does.
> > 
> Ohh.....i skipped _request_firmware() because there is no error
> handling required in U-Boot such as memory release. For the checking at
> both firmware_p and name, i would rather to merge them into
> request_firware_prepare() because they are related to each other
> instead of creating one function just for checking purpose. What do you
> think?
> 
I don't mind whether you combine those functions or not as long as you
reproduce the same functionality which your patch currently does not do.


Lothar Waßmann
Chee, Tien Fong Dec. 8, 2017, 5:25 a.m. | #8
On Kha, 2017-12-07 at 10:00 +0100, Lothar Waßmann wrote:
> Hi,

> 

> On Thu, 7 Dec 2017 08:10:06 +0000 Chee, Tien Fong wrote:

> > 

> > On Kha, 2017-12-07 at 08:49 +0100, Lothar Waßmann wrote:

> > > 

> > > Hi,

> > > 

> > > On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote:

> > > > 

> > > > 

> > > > 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@intel.c

> > > > > > > om

> > > > > > > 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

> > > > /bas

> > > > e/fi

> > > > rmware_class.c#L1191

> > > > > 

> > > > > 

> > > > > 

> > > I was referring to _request_firmware() which calls

> > > request_firmware_prepare() and does the checking of 'name' as

> > > your

> > > code does.

> > > 

> > Ohh.....i skipped _request_firmware() because there is no error

> > handling required in U-Boot such as memory release. For the

> > checking at

> > both firmware_p and name, i would rather to merge them into

> > request_firware_prepare() because they are related to each other

> > instead of creating one function just for checking purpose. What do

> > you

> > think?

> > 

> I don't mind whether you combine those functions or not as long as

> you

> reproduce the same functionality which your patch currently does not

> do.

> 

Okay. SO i will add the checking on firmware_p whether is NULL, and
assign NULL to *firmware_P if calloc is failed. There is no goout since
there is no error handling required.
>  

> Lothar Waßmann
Lothar Waßmann Dec. 8, 2017, 8:22 a.m. | #9
Hi,

On Fri, 8 Dec 2017 05:25:58 +0000 Chee, Tien Fong wrote:
> On Kha, 2017-12-07 at 10:00 +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Thu, 7 Dec 2017 08:10:06 +0000 Chee, Tien Fong wrote:
> > > 
> > > On Kha, 2017-12-07 at 08:49 +0100, Lothar Waßmann wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote:
> > > > > 
> > > > > 
> > > > > 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@intel.c
> > > > > > > > om
> > > > > > > > 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
> > > > > /bas
> > > > > e/fi
> > > > > rmware_class.c#L1191
> > > > > > 
> > > > > > 
> > > > > > 
> > > > I was referring to _request_firmware() which calls
> > > > request_firmware_prepare() and does the checking of 'name' as
> > > > your
> > > > code does.
> > > > 
> > > Ohh.....i skipped _request_firmware() because there is no error
> > > handling required in U-Boot such as memory release. For the
> > > checking at
> > > both firmware_p and name, i would rather to merge them into
> > > request_firware_prepare() because they are related to each other
> > > instead of creating one function just for checking purpose. What do
> > > you
> > > think?
> > > 
> > I don't mind whether you combine those functions or not as long as
> > you
> > reproduce the same functionality which your patch currently does not
> > do.
> > 
> Okay. SO i will add the checking on firmware_p whether is NULL, and
> assign NULL to *firmware_P if calloc is failed. There is no goout since
> there is no error handling required.
> >  
You should also exit early setting *firmware_p to NULL when the 'name'
check fails, like the Linux code does!
I.e.:
+	struct firmware *firmware = 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;
+	}
+
+	firmware->name = name;
+	firmware->data = dbuf;
+	firmware->size = size;
+	firmware->offset = offset;
+
+	return 0;


 Lothar Waßmann

Patch

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;
+
+	err = usb_init();
+
+	if (err)
+		return err;
+
+#ifndef CONFIG_DM_USB
+	err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
+#endif
+
+	return err;
+}
+#else
+static int init_usb(void)
+{
+	printf("Error: Cannot load flash image: no USB support\n");
+	return -ENOSYS;
+}
+#endif
+#endif
+
+#ifdef CONFIG_SATA
+static int init_storage_sata(void)
+{
+	return sata_probe(0);
+}
+#else
+static int init_storage_sata(void)
+{
+	printf("Error: Cannot load image: no SATA support\n");
+	return -ENOSYS;
+}
+#endif
+
+#ifdef CONFIG_CMD_UBIFS
+static int mount_ubifs(struct device_location *location)
+{
+	int ret = 0;
+	char cmd[32];
+
+	sprintf(cmd, "ubi part %s", location->mtdpart);
+
+	ret = run_command(cmd, 0);
+
+	if (ret)
+		return ret;
+
+	sprintf(cmd, "ubifsmount %s", location->ubivol);
+
+	ret = run_command(cmd, 0);
+
+	return res;
+}
+
+static int umount_ubifs(void)
+{
+	return run_command("ubifsumount", 0);
+}
+#else
+static int mount_ubifs(struct device_location *location)
+{
+	printf("Error: Cannot load image: no UBIFS support\n");
+	return -ENOSYS;
+}
+
+static int umount_ubifs(void)
+{
+	printf("Error: Cannot unmount UBIFS: no UBIFS support\n");
+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
+static int init_mmc(void)
+{
+	/* Just for the case MMC is not yet initialized */
+	struct mmc *mmc = NULL;
+	int err = 0;
+
+	spl_mmc_find_device(&mmc, spl_boot_device());
+
+	err = mmc_init(mmc);
+
+	if (err) {
+		printf("spl: mmc init failed with error: %d\n", err);
+		return err;
+	}
+
+	return err;
+}
+#else
+static int init_mmc(void)
+{
+	/* Expect somewhere already initialize MMC */
+	return 0;
+}
+#endif
+
+static int select_fs_dev(struct device_location *location)
+{
+	int ret = 0;
+
+	if (!strcmp("mmc", location->name))
+		ret = fs_set_blk_dev("mmc", location->devpart, FS_TYPE_ANY);
+	else if (!strcmp("usb", location->name))
+		ret = fs_set_blk_dev("usb", location->devpart, FS_TYPE_ANY);
+	else if (!strcmp("sata", location->name))
+		ret = fs_set_blk_dev("sata", location->devpart, FS_TYPE_ANY);
+	else if (!strcmp("ubi", location->name))
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
+		else
+			ret = -ENODEV;
+	else {
+		printf("Error: unsupported location storage.\n");
+		return -ENODEV;
+	}
+
+	if (ret)
+		printf("Error: could not access storage.\n");
+
+	return ret;
+}
+
+static int init_storage_device(struct device_location *location)
+{
+	int ret = 0;
+#ifndef CONFIG_SPL_BUILD
+	/* USB build is not supported yet in SPL */
+	if (!strcmp("usb", location->name))
+		ret = init_usb();
+#endif
+
+	if (!strcmp("mmc", location->name))
+		ret = init_mmc();
+
+	if (!strcmp("sata", location->name))
+		ret = init_storage_sata();
+
+	if (location->ubivol != NULL)
+		ret = mount_ubifs(location);
+
+	return ret;
+}
+
+static void set_storage_devpart(char *name, char *devpart)
+{
+	int i;
+	u32 size;
+
+	size = ARRAY_SIZE(default_locations);
+
+	for (i = 0; i < size; i++) {
+		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;
+
+	*firmware_p = firmware = calloc(1, sizeof(*firmware));
+
+	if (!firmware) {
+		printf("%s: calloc(struct firmware) failed\n", __func__);
+		return -ENOMEM;
+	}
+
+	firmware->name = name;
+	firmware->data = dbuf;
+	firmware->size = size;
+	firmware->offset = offset;
+
+	return ret;
+}
+
+/*
+ * fw_get_filesystem_firmware - load firmware into a allocated buffer
+ * @firmware_p: pointer to firmware image
+ * @location: An array of supported firmware location
+ *
+ * @return: size of total read
+ *	    -ve when error
+ */
+static int fw_get_filesystem_firmware(struct device_location *location,
+				      struct firmware *firmware_p)
+{
+	loff_t actread;
+	char *dev_part;
+	int ret = 0;
+
+	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);
+
+	ret = fs_read(firmware_p->name, (u32)firmware_p->data,
+		      firmware_p->offset, firmware_p->size, &actread);
+
+	if (ret || (actread != firmware_p->size)) {
+		printf("Error: %d Failed to read %s from flash %lld ",
+		      ret,
+		      firmware_p->name,
+		      actread);
+		printf("!= %d.\n", firmware_p->size);
+		return -EPERM;
+		} else
+			ret = actread;
+
+out:
+	if (location->ubivol != NULL)
+		umount_ubifs();
+
+	return ret;
+}
+/*
+ * request_firmware_into_buf - load firmware into a previously allocated buffer
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @buf: address of buffer to load firmware into
+ * @size: size of buffer
+ * @offset: offset of a file for start reading
+ *
+ * This firmware is loaded directly into the buffer pointed to by @buf and
+ * the @firmware_p data member is pointed at @buf.
+ *
+ * @return: size of total read
+ *	    -ve when error
+ */
+int request_firmware_into_buf(struct firmware **firmware_p,
+			      const char *name,
+			      struct device_location *location,
+			      void *buf, size_t size, u32 offset)
+{
+	int ret = 0;
+
+	ret = _request_firmware_prepare(firmware_p, name, buf, size, offset);
+
+	if (ret < 0) /* error */
+		return ret;
+
+	ret = fw_get_filesystem_firmware(location, *firmware_p);
+
+	return ret;
+}
diff --git a/include/fs_loader.h b/include/fs_loader.h
new file mode 100644
index 0000000..dacc1f3
--- /dev/null
+++ b/include/fs_loader.h
@@ -0,0 +1,30 @@ 
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+#ifndef _FS_LOADER_H_
+#define _FS_LOADER_H_
+
+#include <linux/types.h>
+
+struct firmware {
+	size_t size;		/* Size of a file */
+	const u8 *data;		/* Buffer for file */
+	const char *name;	/* Filename */
+	u32 offset;		/* Offset of reading a file */
+	void *priv;		/* Firmware loader private fields */
+};
+
+struct device_location {
+	char *name;	/* Such as mmc, usb,and sata. */
+	char *devpart;  /* Use the load command dev:part conventions */
+	char *mtdpart;	/* MTD partition for ubi part */
+	char *ubivol;	/* UBI volume-name for ubifsmount */
+};
+
+int request_firmware_into_buf(struct firmware **firmware_p,
+			      const char *name,
+			      struct device_location *location,
+			      void *buf, size_t size, u32 offset);
+#endif