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

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         | 311 +++++++++++++++++++++++++++++++++++++++++++++
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 4 files changed, 416 insertions(+)
 create mode 100644 common/fs_loader.c
 create mode 100644 doc/README.firmware_loader
 create mode 100644 include/fs_loader.h

Hi,

On Thu, 21 Dec 2017 15:25:29 +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         | 311 +++++++++++++++++++++++++++++++++++++++++++++
>  doc/README.firmware_loader |  76 +++++++++++
>  include/fs_loader.h        |  28 ++++
>  4 files changed, 416 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/Makefile b/common/Makefile
> index cec506f..2934221 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..ddfce58
> --- /dev/null
> +++ b/common/fs_loader.c
> @@ -0,0 +1,311 @@
> +/*
> + * 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>
> +
> +struct firmware_priv {
> +	const char *name;	/* Filename */
> +	u32 offset;		/* Offset of reading a file */
> +};
> +
> +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;
> +
> +	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;
> +	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 ret;
> +}
> +
> +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;
> +}
> +#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;
> +
> +	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;
> +
> +	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;
> +
> +	if (!strcmp("mmc", location->name)) {
> +		ret = init_mmc();
> +	} else if (!strcmp("sata", location->name)) {
> +		ret = init_storage_sata();
> +	} else if (location->ubivol != NULL) {
> +		ret = mount_ubifs(location);
> +#ifndef CONFIG_SPL_BUILD
> +	/* USB build is not supported yet in SPL */
> +	} else if (!strcmp("usb", location->name)) {
> +		ret = init_usb();
> +#endif
> +	} else {
> +		printf("Error: no supported storage device is available.\n");
> +		ret = -ENODEV;
> +	}
> +
> +	return ret;
> +}
> +
> +static void set_storage_devpart(char *name, char *devpart)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
> +		if (!strcmp(default_locations[i].name, name))
> +			default_locations[i].devpart = devpart;
> +	}
> +}
> +
> +/*
> + * 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;
> +	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;
> +}
> +
> +/*
> + * 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);
>
'ret = ' is missing.

> +	if (ret)
> +		goto out;
> +
> +	fw_priv = firmware_p->priv;
> +
> +	ret = fs_read(fw_priv->name, (ulong)firmware_p->data, fw_priv->offset,
> +		     firmware_p->size, &actread);
> +
> +	if (ret) {
> +		printf("Error: %d Failed to read %s from flash %lld != %d.\n",
> +		      ret, fw_priv->name, actread, firmware_p->size);
> +		return ret;
Y
Shouldn't this be 'goto out', do do the umount_ubifs() as in all other
error cases?

> +	} else {
> +		ret = actread;
> +	}
>
What if actread != firmware_p->size?
You handled it as an error before, now you happily return the number of
bytes read in case of a short read.

> +
> +out:
> +#ifdef CONFIG_CMD_UBIFS
> +	if (location->ubivol != NULL)
> +		umount_ubifs();
> +#endif
> +
> +	return ret;
> +}
> +


Lothar Waßmann

On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann wrote:
> Hi,

> 

> On Thu, 21 Dec 2017 15:25:29 +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         | 311

> > +++++++++++++++++++++++++++++++++++++++++++++

> >  doc/README.firmware_loader |  76 +++++++++++

> >  include/fs_loader.h        |  28 ++++

> >  4 files changed, 416 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/Makefile b/common/Makefile

> > index cec506f..2934221 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..ddfce58

> > --- /dev/null

> > +++ b/common/fs_loader.c

> > @@ -0,0 +1,311 @@

> > +/*

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

> > +

> > +struct firmware_priv {

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

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

> > +};

> > +

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

> > +

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

> > +	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 ret;

> > +}

> > +

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

> > +}

> > +#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;

> > +

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

> > +

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

> > +

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

> > +		ret = init_mmc();

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

> > +		ret = init_storage_sata();

> > +	} else if (location->ubivol != NULL) {

> > +		ret = mount_ubifs(location);

> > +#ifndef CONFIG_SPL_BUILD

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

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

> > +		ret = init_usb();

> > +#endif

> > +	} else {

> > +		printf("Error: no supported storage device is

> > available.\n");

> > +		ret = -ENODEV;

> > +	}

> > +

> > +	return ret;

> > +}

> > +

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

> > +{

> > +	size_t i;

> > +

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

> > +		if (!strcmp(default_locations[i].name, name))

> > +			default_locations[i].devpart = devpart;

> > +	}

> > +}

> > +

> > +/*

> > + * 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;

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

> > +}

> > +

> > +/*

> > + * 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);

> > 

> 'ret = ' is missing.

> 

Okay.
> > 

> > +	if (ret)

> > +		goto out;

> > +

> > +	fw_priv = firmware_p->priv;

> > +

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

> > fw_priv->offset,

> > +		     firmware_p->size, &actread);

> > +

> > +	if (ret) {

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

> > %lld != %d.\n",

> > +		      ret, fw_priv->name, actread, firmware_p-

> > >size);

> > +		return ret;

> Y

> Shouldn't this be 'goto out', do do the umount_ubifs() as in all

> other

> error cases?

> 

okay.
> > 

> > +	} else {

> > +		ret = actread;

> > +	}

> > 

> What if actread != firmware_p->size?

> You handled it as an error before, now you happily return the number

> of

> bytes read in case of a short read.

> 

May be i misunderstand on ur previosly comment. There is not much
description on fd_read, i just knowing that -ve is error from function
based on other example codes in U-Boot as they check the error with
"ret < 0" or "ret != 0". "actread != firmware_p->size" is the additonal
checking i added myself because when i digging to the code, i found
that such condition only with error printf inside the function without
any error code return. So, i am not sure when "actread != firmware_p-
>size", the ret would be -ve too.

How about we write in this way?

	if (ret || (actread != firmware_p->size)) {
		printf("Error: %d Failed to read %s from flash %lld !=
%d.\n",
		      ret, fw_priv->name, actread, firmware_p->size);
		goto out;
	} else {
		ret = actread;
	}

out:
#ifdef CONFIG_CMD_UBIFS
	if (location->ubivol != NULL)
		umount_ubifs();
#endif

	return ret;
> > 

> > +

> > +out:

> > +#ifdef CONFIG_CMD_UBIFS

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

> > +		umount_ubifs();

> > +#endif

> > +

> > +	return ret;

> > +}

> > +

> 

> Lothar Waßmann

On Kha, 2017-12-21 at 17:36 +0800, Chee, Tien Fong wrote:
> On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann wrote:

> > 

> > Hi,

> > 

> > On Thu, 21 Dec 2017 15:25:29 +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         | 311

> > > +++++++++++++++++++++++++++++++++++++++++++++

> > >  doc/README.firmware_loader |  76 +++++++++++

> > >  include/fs_loader.h        |  28 ++++

> > >  4 files changed, 416 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/Makefile b/common/Makefile

> > > index cec506f..2934221 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..ddfce58

> > > --- /dev/null

> > > +++ b/common/fs_loader.c

> > > @@ -0,0 +1,311 @@

> > > +/*

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

> > > +

> > > +struct firmware_priv {

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

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

> > > */

> > > +};

> > > +

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

> > > +

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

> > > +	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 ret;

> > > +}

> > > +

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

> > > +}

> > > +#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;

> > > +

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

> > > +

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

> > > +

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

> > > +		ret = init_mmc();

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

> > > +		ret = init_storage_sata();

> > > +	} else if (location->ubivol != NULL) {

> > > +		ret = mount_ubifs(location);

> > > +#ifndef CONFIG_SPL_BUILD

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

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

> > > +		ret = init_usb();

> > > +#endif

> > > +	} else {

> > > +		printf("Error: no supported storage device is

> > > available.\n");

> > > +		ret = -ENODEV;

> > > +	}

> > > +

> > > +	return ret;

> > > +}

> > > +

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

> > > +{

> > > +	size_t i;

> > > +

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

> > > +		if (!strcmp(default_locations[i].name, name))

> > > +			default_locations[i].devpart = devpart;

> > > +	}

> > > +}

> > > +

> > > +/*

> > > + * 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;

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

> > > +}

> > > +

> > > +/*

> > > + * 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);

> > > 

> > 'ret = ' is missing.

> > 

> Okay.

> > 

> > > 

> > > 

> > > +	if (ret)

> > > +		goto out;

> > > +

> > > +	fw_priv = firmware_p->priv;

> > > +

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

> > > fw_priv->offset,

> > > +		     firmware_p->size, &actread);

> > > +

> > > +	if (ret) {

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

> > > %lld != %d.\n",

> > > +		      ret, fw_priv->name, actread, firmware_p-

> > > > 

> > > > size);

> > > +		return ret;

> > Y

> > Shouldn't this be 'goto out', do do the umount_ubifs() as in all

> > other

> > error cases?

> > 

> okay.

> > 

> > > 

> > > 

> > > +	} else {

> > > +		ret = actread;

> > > +	}

> > > 

> > What if actread != firmware_p->size?

> > You handled it as an error before, now you happily return the

> > number

> > of

> > bytes read in case of a short read.

> > 

> May be i misunderstand on ur previosly comment. There is not much

> description on fd_read, i just knowing that -ve is error from

> function

> based on other example codes in U-Boot as they check the error with

> "ret < 0" or "ret != 0". "actread != firmware_p->size" is the

> additonal

> checking i added myself because when i digging to the code, i found

> that such condition only with error printf inside the function

> without

> any error code return. So, i am not sure when "actread != firmware_p-

> > 

> > size", the ret would be -ve too.

> How about we write in this way?

> 

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

> 		printf("Error: %d Failed to read %s from flash %lld !=

> %d.\n",

> 		      ret, fw_priv->name, actread, firmware_p->size);

I suggest put  ret = -EIO;
> 		goto out;

> 	} else {

> 		ret = actread;

> 	}

> 

> out:

> #ifdef CONFIG_CMD_UBIFS

> 	if (location->ubivol != NULL)

> 		umount_ubifs();

> #endif

> 

> 	return ret;

> > 

> > > 

> > > 

> > > +

> > > +out:

> > > +#ifdef CONFIG_CMD_UBIFS

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

> > > +		umount_ubifs();

> > > +#endif

> > > +

> > > +	return ret;

> > > +}

> > > +

> > Lothar Waßmann

Hi,

On Thu, 21 Dec 2017 09:36:41 +0000 Chee, Tien Fong wrote:
> On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Thu, 21 Dec 2017 15:25:29 +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         | 311
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > >  doc/README.firmware_loader |  76 +++++++++++
> > >  include/fs_loader.h        |  28 ++++
> > >  4 files changed, 416 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/Makefile b/common/Makefile
> > > index cec506f..2934221 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..ddfce58
> > > --- /dev/null
> > > +++ b/common/fs_loader.c
> > > @@ -0,0 +1,311 @@
> > > +/*
> > > + * 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>
> > > +
> > > +struct firmware_priv {
> > > +	const char *name;	/* Filename */
> > > +	u32 offset;		/* Offset of reading a file */
> > > +};
> > > +
> > > +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;
> > > +
> > > +	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;
> > > +	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 ret;
> > > +}
> > > +
> > > +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;
> > > +}
> > > +#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;
> > > +
> > > +	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;
> > > +
> > > +	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;
> > > +
> > > +	if (!strcmp("mmc", location->name)) {
> > > +		ret = init_mmc();
> > > +	} else if (!strcmp("sata", location->name)) {
> > > +		ret = init_storage_sata();
> > > +	} else if (location->ubivol != NULL) {
> > > +		ret = mount_ubifs(location);
> > > +#ifndef CONFIG_SPL_BUILD
> > > +	/* USB build is not supported yet in SPL */
> > > +	} else if (!strcmp("usb", location->name)) {
> > > +		ret = init_usb();
> > > +#endif
> > > +	} else {
> > > +		printf("Error: no supported storage device is
> > > available.\n");
> > > +		ret = -ENODEV;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void set_storage_devpart(char *name, char *devpart)
> > > +{
> > > +	size_t i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
> > > +		if (!strcmp(default_locations[i].name, name))
> > > +			default_locations[i].devpart = devpart;
> > > +	}
> > > +}
> > > +
> > > +/*
> > > + * 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;
> > > +	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;
> > > +}
> > > +
> > > +/*
> > > + * 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);
> > > 
> > 'ret = ' is missing.
> > 
> Okay.
> > > 
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	fw_priv = firmware_p->priv;
> > > +
> > > +	ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
> > > fw_priv->offset,
> > > +		     firmware_p->size, &actread);
> > > +
> > > +	if (ret) {
> > > +		printf("Error: %d Failed to read %s from flash
> > > %lld != %d.\n",
> > > +		      ret, fw_priv->name, actread, firmware_p-
> > > >size);
> > > +		return ret;
> > Y
> > Shouldn't this be 'goto out', do do the umount_ubifs() as in all
> > other
> > error cases?
> > 
> okay.
> > > 
> > > +	} else {
> > > +		ret = actread;
> > > +	}
> > > 
> > What if actread != firmware_p->size?
> > You handled it as an error before, now you happily return the number
> > of
> > bytes read in case of a short read.
> > Operation not permitted
> May be i misunderstand on ur previosly comment. There is not much
>
My comment was about returning -EPERM in this case.
First of all you discarded the possible error code returned from
fs_read() and secondly -EIO would be more appropriate than EPERM in
case of reading less data than expected. 

> description on fd_read, i just knowing that -ve is error from function
> based on other example codes in U-Boot as they check the error with
> "ret < 0" or "ret != 0". "actread != firmware_p->size" is the additonal
> checking i added myself because when i digging to the code, i found
> that such condition only with error printf inside the function without
> any error code return. So, i am not sure when "actread != firmware_p-
> >size", the ret would be -ve too.
>
If 'size' is the actual size of the caller provided buffer and not the
exact size of the firmware to be loaded, the condition actread != size
would not be an error at all.
If 'size' designates the exact size of the firmware, that the caller
may know by other means, it is obviously an error if actread != size.
Since you treated actread != size as an error in your original patch I
assumed the latter case.

AFAICT Linux uses the first semantics ('size' specifies the size of the
caller provided buffer as an upper limit for the read() call and not
the exact amount of data to be read).
Unfortunately the fs_read() semantics is fundamentally broken as it
interprets 'size == 0' as 'read as much data as you can get' which may
easily lead to buffer overflows and prints a very unhelpful error
message "<filename> shorter than offset + len" in case 'size' does not
match the actual amount of data read. 

You should also consider what happens, if the firmware file you read is
larger than the buffer you provided, as in that case fs_read() will
stop reading at the provided buffer size and you won't be able to
notice, that the file was not completely loaded!


Lothar Waßmann

On Kha, 2017-12-21 at 12:53 +0100, Lothar Waßmann wrote:
> Hi,

> 

> On Thu, 21 Dec 2017 09:36:41 +0000 Chee, Tien Fong wrote:

> > 

> > On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann wrote:

> > > 

> > > Hi,

> > > 

> > > On Thu, 21 Dec 2017 15:25:29 +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         | 311

> > > > +++++++++++++++++++++++++++++++++++++++++++++

> > > >  doc/README.firmware_loader |  76 +++++++++++

> > > >  include/fs_loader.h        |  28 ++++

> > > >  4 files changed, 416 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/Makefile b/common/Makefile

> > > > index cec506f..2934221 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..ddfce58

> > > > --- /dev/null

> > > > +++ b/common/fs_loader.c

> > > > @@ -0,0 +1,311 @@

> > > > +/*

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

> > > > +

> > > > +struct firmware_priv {

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

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

> > > > */

> > > > +};

> > > > +

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

> > > > +

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

> > > > +	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 ret;

> > > > +}

> > > > +

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

> > > > +}

> > > > +#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;

> > > > +

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

> > > > +

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

> > > > +

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

> > > > +		ret = init_mmc();

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

> > > > +		ret = init_storage_sata();

> > > > +	} else if (location->ubivol != NULL) {

> > > > +		ret = mount_ubifs(location);

> > > > +#ifndef CONFIG_SPL_BUILD

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

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

> > > > +		ret = init_usb();

> > > > +#endif

> > > > +	} else {

> > > > +		printf("Error: no supported storage device is

> > > > available.\n");

> > > > +		ret = -ENODEV;

> > > > +	}

> > > > +

> > > > +	return ret;

> > > > +}

> > > > +

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

> > > > +{

> > > > +	size_t i;

> > > > +

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

> > > > +		if (!strcmp(default_locations[i].name, name))

> > > > +			default_locations[i].devpart =

> > > > devpart;

> > > > +	}

> > > > +}

> > > > +

> > > > +/*

> > > > + * 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;

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

> > > > +}

> > > > +

> > > > +/*

> > > > + * 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);

> > > > 

> > > 'ret = ' is missing.

> > > 

> > Okay.

> > > 

> > > > 

> > > > 

> > > > +	if (ret)

> > > > +		goto out;

> > > > +

> > > > +	fw_priv = firmware_p->priv;

> > > > +

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

> > > > fw_priv->offset,

> > > > +		     firmware_p->size, &actread);

> > > > +

> > > > +	if (ret) {

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

> > > > %lld != %d.\n",

> > > > +		      ret, fw_priv->name, actread, firmware_p-

> > > > > 

> > > > > size);

> > > > +		return ret;

> > > Y

> > > Shouldn't this be 'goto out', do do the umount_ubifs() as in all

> > > other

> > > error cases?

> > > 

> > okay.

> > > 

> > > > 

> > > > 

> > > > +	} else {

> > > > +		ret = actread;

> > > > +	}

> > > > 

> > > What if actread != firmware_p->size?

> > > You handled it as an error before, now you happily return the

> > > number

> > > of

> > > bytes read in case of a short read.

> > > Operation not permitted

> > May be i misunderstand on ur previosly comment. There is not much

> > 

> My comment was about returning -EPERM in this case.

> First of all you discarded the possible error code returned from

> fs_read() and secondly -EIO would be more appropriate than EPERM in

> case of reading less data than expected. 

> 

One of the reason I prefer to use -EIO instead of return from fs_read,
because i'm not sure when "actread != firmware_p->size", the return
from the fs_read is still -ve or not.

> > 

> > description on fd_read, i just knowing that -ve is error from

> > function

> > based on other example codes in U-Boot as they check the error with

> > "ret < 0" or "ret != 0". "actread != firmware_p->size" is the

> > additonal

> > checking i added myself because when i digging to the code, i found

> > that such condition only with error printf inside the function

> > without

> > any error code return. So, i am not sure when "actread !=

> > firmware_p-

> > > 

> > > size", the ret would be -ve too.

> If 'size' is the actual size of the caller provided buffer and not

> the

> exact size of the firmware to be loaded, the condition actread !=

> size

> would not be an error at all.

> If 'size' designates the exact size of the firmware, that the caller

> may know by other means, it is obviously an error if actread != size.

> Since you treated actread != size as an error in your original patch

> I

> assumed the latter case.

> 

The size here refer to the size of firmware or chunk of firmware to
read when the buffer is smaller than firmware.

> AFAICT Linux uses the first semantics ('size' specifies the size of

> the

> caller provided buffer as an upper limit for the read() call and not

> the exact amount of data to be read).

> Unfortunately the fs_read() semantics is fundamentally broken as it

> interprets 'size == 0' as 'read as much data as you can get' which

> may

> easily lead to buffer overflows and prints a very unhelpful error

> message "<filename> shorter than offset + len" in case 'size' does

> not

> match the actual amount of data read. 

> 

> You should also consider what happens, if the firmware file you read

> is

> larger than the buffer you provided, as in that case fs_read() will

> stop reading at the provided buffer size and you won't be able to

> notice, that the file was not completely loaded!

> 

I prefer to let the caller handles this, because this function can be
used to read the firmware chunk by chunk when the allocated buffer is
smaller than firmware.
> 

> Lothar Waßmann

Hi,

On Thu, 21 Dec 2017 12:48:53 +0000 Chee, Tien Fong wrote:
> On Kha, 2017-12-21 at 12:53 +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Thu, 21 Dec 2017 09:36:41 +0000 Chee, Tien Fong wrote:
> > > 
> > > On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On Thu, 21 Dec 2017 15:25:29 +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         | 311
> > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > >  doc/README.firmware_loader |  76 +++++++++++
> > > > >  include/fs_loader.h        |  28 ++++
> > > > >  4 files changed, 416 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/Makefile b/common/Makefile
> > > > > index cec506f..2934221 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..ddfce58
> > > > > --- /dev/null
> > > > > +++ b/common/fs_loader.c
> > > > > @@ -0,0 +1,311 @@
> > > > > +/*
> > > > > + * 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>
> > > > > +
> > > > > +struct firmware_priv {
> > > > > +	const char *name;	/* Filename */
> > > > > +	u32 offset;		/* Offset of reading a file
> > > > > */
> > > > > +};
> > > > > +
> > > > > +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;
> > > > > +
> > > > > +	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;
> > > > > +	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 ret;
> > > > > +}
> > > > > +
> > > > > +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;
> > > > > +}
> > > > > +#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;
> > > > > +
> > > > > +	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;
> > > > > +
> > > > > +	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;
> > > > > +
> > > > > +	if (!strcmp("mmc", location->name)) {
> > > > > +		ret = init_mmc();
> > > > > +	} else if (!strcmp("sata", location->name)) {
> > > > > +		ret = init_storage_sata();
> > > > > +	} else if (location->ubivol != NULL) {
> > > > > +		ret = mount_ubifs(location);
> > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > +	/* USB build is not supported yet in SPL */
> > > > > +	} else if (!strcmp("usb", location->name)) {
> > > > > +		ret = init_usb();
> > > > > +#endif
> > > > > +	} else {
> > > > > +		printf("Error: no supported storage device is
> > > > > available.\n");
> > > > > +		ret = -ENODEV;
> > > > > +	}
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +static void set_storage_devpart(char *name, char *devpart)
> > > > > +{
> > > > > +	size_t i;
> > > > > +
> > > > > +	for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
> > > > > +		if (!strcmp(default_locations[i].name, name))
> > > > > +			default_locations[i].devpart =
> > > > > devpart;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * 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;
> > > > > +	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;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * 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);
> > > > > 
> > > > 'ret = ' is missing.
> > > > 
> > > Okay.
> > > > 
> > > > > 
> > > > > 
> > > > > +	if (ret)
> > > > > +		goto out;
> > > > > +
> > > > > +	fw_priv = firmware_p->priv;
> > > > > +
> > > > > +	ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
> > > > > fw_priv->offset,
> > > > > +		     firmware_p->size, &actread);
> > > > > +
> > > > > +	if (ret) {
> > > > > +		printf("Error: %d Failed to read %s from flash
> > > > > %lld != %d.\n",
> > > > > +		      ret, fw_priv->name, actread, firmware_p-
> > > > > > 
> > > > > > size);
> > > > > +		return ret;
> > > > Y
> > > > Shouldn't this be 'goto out', do do the umount_ubifs() as in all
> > > > other
> > > > error cases?
> > > > 
> > > okay.
> > > > 
> > > > > 
> > > > > 
> > > > > +	} else {
> > > > > +		ret = actread;
> > > > > +	}
> > > > > 
> > > > What if actread != firmware_p->size?
> > > > You handled it as an error before, now you happily return the
> > > > number
> > > > of
> > > > bytes read in case of a short read.
> > > > Operation not permitted
> > > May be i misunderstand on ur previosly comment. There is not much
> > > 
> > My comment was about returning -EPERM in this case.
> > First of all you discarded the possible error code returned from
> > fs_read() and secondly -EIO would be more appropriate than EPERM in
> > case of reading less data than expected. 
> > 
> One of the reason I prefer to use -EIO instead of return from fs_read,
> because i'm not sure when "actread != firmware_p->size", the return
> from the fs_read is still -ve or not.
> 
The return code of fs_read() could be for example -ENOENT, if the file
you try to load does not exist. Replacing it with -EIO will mislead
users about the cause of the error.
Thus, if fs_read()'s return code is < 0, you should return that!
If it is zero and you decide to treat actread =! firmware_p->size as an
error, you should return -EIO only in that case!

> > > 
> > > description on fd_read, i just knowing that -ve is error from
> > > function
> > > based on other example codes in U-Boot as they check the error with
> > > "ret < 0" or "ret != 0". "actread != firmware_p->size" is the
> > > additonal
> > > checking i added myself because when i digging to the code, i found
> > > that such condition only with error printf inside the function
> > > without
> > > any error code return. So, i am not sure when "actread !=
> > > firmware_p-
> > > > 
> > > > size", the ret would be -ve too.
> > If 'size' is the actual size of the caller provided buffer and not
> > the
> > exact size of the firmware to be loaded, the condition actread !=
> > size
> > would not be an error at all.
> > If 'size' designates the exact size of the firmware, that the caller
> > may know by other means, it is obviously an error if actread != size.
> > Since you treated actread != size as an error in your original patch
> > I
> > assumed the latter case.
> > 
> The size here refer to the size of firmware or chunk of firmware to
> read when the buffer is smaller than firmware.
> 
OK. In that case, you should not print a message when actread !=
firmware_p->size, since fs_read will have already printed a message due
to this.

> > AFAICT Linux uses the first semantics ('size' specifies the size of
> > the
> > caller provided buffer as an upper limit for the read() call and not
> > the exact amount of data to be read).
> > Unfortunately the fs_read() semantics is fundamentally broken as it
> > interprets 'size == 0' as 'read as much data as you can get' which
> > may
> > easily lead to buffer overflows and prints a very unhelpful error
> > message "<filename> shorter than offset + len" in case 'size' does
> > not
> > match the actual amount of data read. 
> > 
> > You should also consider what happens, if the firmware file you read
> > is
> > larger than the buffer you provided, as in that case fs_read() will
> > stop reading at the provided buffer size and you won't be able to
> > notice, that the file was not completely loaded!
> > 
> I prefer to let the caller handles this, because this function can be
> used to read the firmware chunk by chunk when the allocated buffer is
> smaller than firmware.
>
So, the caller should also check, whether the return value of the
function matches the requested size and you should omit the special
handling of actread != firmware_p->size in fw_get_filesystem_firmware()
(as you did in the current version of your patch).



Lothar Waßmann

On Kha, 2017-12-21 at 16:08 +0100, Lothar Waßmann wrote:
> Hi,

> 

> On Thu, 21 Dec 2017 12:48:53 +0000 Chee, Tien Fong wrote:

> > 

> > On Kha, 2017-12-21 at 12:53 +0100, Lothar Waßmann wrote:

> > > 

> > > Hi,

> > > 

> > > On Thu, 21 Dec 2017 09:36:41 +0000 Chee, Tien Fong wrote:

> > > > 

> > > > 

> > > > On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann wrote:

> > > > > 

> > > > > 

> > > > > Hi,

> > > > > 

> > > > > On Thu, 21 Dec 2017 15:25:29 +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         | 311

> > > > > > +++++++++++++++++++++++++++++++++++++++++++++

> > > > > >  doc/README.firmware_loader |  76 +++++++++++

> > > > > >  include/fs_loader.h        |  28 ++++

> > > > > >  4 files changed, 416 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/Makefile b/common/Makefile

> > > > > > index cec506f..2934221 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..ddfce58

> > > > > > --- /dev/null

> > > > > > +++ b/common/fs_loader.c

> > > > > > @@ -0,0 +1,311 @@

> > > > > > +/*

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

> > > > > > +

> > > > > > +struct firmware_priv {

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

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

> > > > > > file

> > > > > > */

> > > > > > +};

> > > > > > +

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

> > > > > > +

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

> > > > > > +	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 ret;

> > > > > > +}

> > > > > > +

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

> > > > > > +}

> > > > > > +#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;

> > > > > > +

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

> > > > > > +

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

> > > > > > +

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

> > > > > > +		ret = init_mmc();

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

> > > > > > +		ret = init_storage_sata();

> > > > > > +	} else if (location->ubivol != NULL) {

> > > > > > +		ret = mount_ubifs(location);

> > > > > > +#ifndef CONFIG_SPL_BUILD

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

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

> > > > > > +		ret = init_usb();

> > > > > > +#endif

> > > > > > +	} else {

> > > > > > +		printf("Error: no supported storage device

> > > > > > is

> > > > > > available.\n");

> > > > > > +		ret = -ENODEV;

> > > > > > +	}

> > > > > > +

> > > > > > +	return ret;

> > > > > > +}

> > > > > > +

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

> > > > > > +{

> > > > > > +	size_t i;

> > > > > > +

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

> > > > > > i++) {

> > > > > > +		if (!strcmp(default_locations[i].name,

> > > > > > name))

> > > > > > +			default_locations[i].devpart =

> > > > > > devpart;

> > > > > > +	}

> > > > > > +}

> > > > > > +

> > > > > > +/*

> > > > > > + * 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;

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

> > > > > > +}

> > > > > > +

> > > > > > +/*

> > > > > > + * 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);

> > > > > > 

> > > > > 'ret = ' is missing.

> > > > > 

> > > > Okay.

> > > > > 

> > > > > 

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > +	if (ret)

> > > > > > +		goto out;

> > > > > > +

> > > > > > +	fw_priv = firmware_p->priv;

> > > > > > +

> > > > > > +	ret = fs_read(fw_priv->name, (ulong)firmware_p-

> > > > > > >data,

> > > > > > fw_priv->offset,

> > > > > > +		     firmware_p->size, &actread);

> > > > > > +

> > > > > > +	if (ret) {

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

> > > > > > flash

> > > > > > %lld != %d.\n",

> > > > > > +		      ret, fw_priv->name, actread,

> > > > > > firmware_p-

> > > > > > > 

> > > > > > > 

> > > > > > > size);

> > > > > > +		return ret;

> > > > > Y

> > > > > Shouldn't this be 'goto out', do do the umount_ubifs() as in

> > > > > all

> > > > > other

> > > > > error cases?

> > > > > 

> > > > okay.

> > > > > 

> > > > > 

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > +	} else {

> > > > > > +		ret = actread;

> > > > > > +	}

> > > > > > 

> > > > > What if actread != firmware_p->size?

> > > > > You handled it as an error before, now you happily return the

> > > > > number

> > > > > of

> > > > > bytes read in case of a short read.

> > > > > Operation not permitted

> > > > May be i misunderstand on ur previosly comment. There is not

> > > > much

> > > > 

> > > My comment was about returning -EPERM in this case.

> > > First of all you discarded the possible error code returned from

> > > fs_read() and secondly -EIO would be more appropriate than EPERM

> > > in

> > > case of reading less data than expected. 

> > > 

> > One of the reason I prefer to use -EIO instead of return from

> > fs_read,

> > because i'm not sure when "actread != firmware_p->size", the return

> > from the fs_read is still -ve or not.

> > 

> The return code of fs_read() could be for example -ENOENT, if the

> file

> you try to load does not exist. Replacing it with -EIO will mislead

> users about the cause of the error.

> Thus, if fs_read()'s return code is < 0, you should return that!

> If it is zero and you decide to treat actread =! firmware_p->size as

> an

> error, you should return -EIO only in that case!

> 

Okay.
> > 

> > > 

> > > > 

> > > > 

> > > > description on fd_read, i just knowing that -ve is error from

> > > > function

> > > > based on other example codes in U-Boot as they check the error

> > > > with

> > > > "ret < 0" or "ret != 0". "actread != firmware_p->size" is the

> > > > additonal

> > > > checking i added myself because when i digging to the code, i

> > > > found

> > > > that such condition only with error printf inside the function

> > > > without

> > > > any error code return. So, i am not sure when "actread !=

> > > > firmware_p-

> > > > > 

> > > > > 

> > > > > size", the ret would be -ve too.

> > > If 'size' is the actual size of the caller provided buffer and

> > > not

> > > the

> > > exact size of the firmware to be loaded, the condition actread !=

> > > size

> > > would not be an error at all.

> > > If 'size' designates the exact size of the firmware, that the

> > > caller

> > > may know by other means, it is obviously an error if actread !=

> > > size.

> > > Since you treated actread != size as an error in your original

> > > patch

> > > I

> > > assumed the latter case.

> > > 

> > The size here refer to the size of firmware or chunk of firmware to

> > read when the buffer is smaller than firmware.

> > 

> OK. In that case, you should not print a message when actread !=

> firmware_p->size, since fs_read will have already printed a message

> due

> to this.

> 

The printed message with additional info such as ret, size and actread.
> > 

> > > 

> > > AFAICT Linux uses the first semantics ('size' specifies the size

> > > of

> > > the

> > > caller provided buffer as an upper limit for the read() call and

> > > not

> > > the exact amount of data to be read).

> > > Unfortunately the fs_read() semantics is fundamentally broken as

> > > it

> > > interprets 'size == 0' as 'read as much data as you can get'

> > > which

> > > may

> > > easily lead to buffer overflows and prints a very unhelpful error

> > > message "<filename> shorter than offset + len" in case 'size'

> > > does

> > > not

> > > match the actual amount of data read. 

> > > 

> > > You should also consider what happens, if the firmware file you

> > > read

> > > is

> > > larger than the buffer you provided, as in that case fs_read()

> > > will

> > > stop reading at the provided buffer size and you won't be able to

> > > notice, that the file was not completely loaded!

> > > 

> > I prefer to let the caller handles this, because this function can

> > be

> > used to read the firmware chunk by chunk when the allocated buffer

> > is

> > smaller than firmware.

> > 

> So, the caller should also check, whether the return value of the

> function matches the requested size and you should omit the special

> handling of actread != firmware_p->size in

> fw_get_filesystem_firmware()

> (as you did in the current version of your patch).

> 

Just to confirm we are in same page, the caller i means is function
which call the request_firmware_into_buf. firmware_p->size is the size
gonna be read from firmware, could be part of firmware with different
offset.
Would you mind to explain more why the special handling should be
omitted? This special handling would return error code if fail,or 
actread if pass to the request_firmware_into_buf.
request_firmware_into_buf just need to know status of file reading is
performed successfully or not.
> 

> 

> Lothar Waßmann

Hi,

On Fri, 22 Dec 2017 01:43:38 +0000 Chee, Tien Fong wrote:
> On Kha, 2017-12-21 at 16:08 +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Thu, 21 Dec 2017 12:48:53 +0000 Chee, Tien Fong wrote:
> > > 
> > > On Kha, 2017-12-21 at 12:53 +0100, Lothar Waßmann wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On Thu, 21 Dec 2017 09:36:41 +0000 Chee, Tien Fong wrote:
> > > > > 
> > > > > 
> > > > > On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann wrote:
> > > > > > 
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On Thu, 21 Dec 2017 15:25:29 +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         | 311
> > > > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  doc/README.firmware_loader |  76 +++++++++++
> > > > > > >  include/fs_loader.h        |  28 ++++
> > > > > > >  4 files changed, 416 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/Makefile b/common/Makefile
> > > > > > > index cec506f..2934221 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..ddfce58
> > > > > > > --- /dev/null
> > > > > > > +++ b/common/fs_loader.c
> > > > > > > @@ -0,0 +1,311 @@
> > > > > > > +/*
> > > > > > > + * 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>
> > > > > > > +
> > > > > > > +struct firmware_priv {
> > > > > > > +	const char *name;	/* Filename */
> > > > > > > +	u32 offset;		/* Offset of reading a
> > > > > > > file
> > > > > > > */
> > > > > > > +};
> > > > > > > +
> > > > > > > +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;
> > > > > > > +
> > > > > > > +	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;
> > > > > > > +	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 ret;
> > > > > > > +}
> > > > > > > +
> > > > > > > +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;
> > > > > > > +}
> > > > > > > +#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;
> > > > > > > +
> > > > > > > +	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;
> > > > > > > +
> > > > > > > +	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;
> > > > > > > +
> > > > > > > +	if (!strcmp("mmc", location->name)) {
> > > > > > > +		ret = init_mmc();
> > > > > > > +	} else if (!strcmp("sata", location->name)) {
> > > > > > > +		ret = init_storage_sata();
> > > > > > > +	} else if (location->ubivol != NULL) {
> > > > > > > +		ret = mount_ubifs(location);
> > > > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > > > +	/* USB build is not supported yet in SPL */
> > > > > > > +	} else if (!strcmp("usb", location->name)) {
> > > > > > > +		ret = init_usb();
> > > > > > > +#endif
> > > > > > > +	} else {
> > > > > > > +		printf("Error: no supported storage device
> > > > > > > is
> > > > > > > available.\n");
> > > > > > > +		ret = -ENODEV;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return ret;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void set_storage_devpart(char *name, char *devpart)
> > > > > > > +{
> > > > > > > +	size_t i;
> > > > > > > +
> > > > > > > +	for (i = 0; i < ARRAY_SIZE(default_locations);
> > > > > > > i++) {
> > > > > > > +		if (!strcmp(default_locations[i].name,
> > > > > > > name))
> > > > > > > +			default_locations[i].devpart =
> > > > > > > devpart;
> > > > > > > +	}
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * 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;
> > > > > > > +	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;
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * 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);
> > > > > > > 
> > > > > > 'ret = ' is missing.
> > > > > > 
> > > > > Okay.
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > +	if (ret)
> > > > > > > +		goto out;
> > > > > > > +
> > > > > > > +	fw_priv = firmware_p->priv;
> > > > > > > +
> > > > > > > +	ret = fs_read(fw_priv->name, (ulong)firmware_p-
> > > > > > > >data,
> > > > > > > fw_priv->offset,
> > > > > > > +		     firmware_p->size, &actread);
> > > > > > > +
> > > > > > > +	if (ret) {
> > > > > > > +		printf("Error: %d Failed to read %s from
> > > > > > > flash
> > > > > > > %lld != %d.\n",
> > > > > > > +		      ret, fw_priv->name, actread,
> > > > > > > firmware_p-
> > > > > > > > 
> > > > > > > > 
> > > > > > > > size);
> > > > > > > +		return ret;
> > > > > > Y
> > > > > > Shouldn't this be 'goto out', do do the umount_ubifs() as in
> > > > > > all
> > > > > > other
> > > > > > error cases?
> > > > > > 
> > > > > okay.
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > +	} else {
> > > > > > > +		ret = actread;
> > > > > > > +	}
> > > > > > > 
> > > > > > What if actread != firmware_p->size?
> > > > > > You handled it as an error before, now you happily return the
> > > > > > number
> > > > > > of
> > > > > > bytes read in case of a short read.
> > > > > > Operation not permitted
> > > > > May be i misunderstand on ur previosly comment. There is not
> > > > > much
> > > > > 
> > > > My comment was about returning -EPERM in this case.
> > > > First of all you discarded the possible error code returned from
> > > > fs_read() and secondly -EIO would be more appropriate than EPERM
> > > > in
> > > > case of reading less data than expected. 
> > > > 
> > > One of the reason I prefer to use -EIO instead of return from
> > > fs_read,
> > > because i'm not sure when "actread != firmware_p->size", the return
> > > from the fs_read is still -ve or not.
> > > 
> > The return code of fs_read() could be for example -ENOENT, if the
> > file
> > you try to load does not exist. Replacing it with -EIO will mislead
> > users about the cause of the error.
> > Thus, if fs_read()'s return code is < 0, you should return that!
> > If it is zero and you decide to treat actread =! firmware_p->size as
> > an
> > error, you should return -EIO only in that case!
> > 
> Okay.
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > description on fd_read, i just knowing that -ve is error from
> > > > > function
> > > > > based on other example codes in U-Boot as they check the error
> > > > > with
> > > > > "ret < 0" or "ret != 0". "actread != firmware_p->size" is the
> > > > > additonal
> > > > > checking i added myself because when i digging to the code, i
> > > > > found
> > > > > that such condition only with error printf inside the function
> > > > > without
> > > > > any error code return. So, i am not sure when "actread !=
> > > > > firmware_p-
> > > > > > 
> > > > > > 
> > > > > > size", the ret would be -ve too.
> > > > If 'size' is the actual size of the caller provided buffer and
> > > > not
> > > > the
> > > > exact size of the firmware to be loaded, the condition actread !=
> > > > size
> > > > would not be an error at all.
> > > > If 'size' designates the exact size of the firmware, that the
> > > > caller
> > > > may know by other means, it is obviously an error if actread !=
> > > > size.
> > > > Since you treated actread != size as an error in your original
> > > > patch
> > > > I
> > > > assumed the latter case.
> > > > 
> > > The size here refer to the size of firmware or chunk of firmware to
> > > read when the buffer is smaller than firmware.
> > > 
> > OK. In that case, you should not print a message when actread !=
> > firmware_p->size, since fs_read will have already printed a message
> > due
> > to this.
> > 
> The printed message with additional info such as ret, size and actread.
> > > 
> > > > 
> > > > AFAICT Linux uses the first semantics ('size' specifies the size
> > > > of
> > > > the
> > > > caller provided buffer as an upper limit for the read() call and
> > > > not
> > > > the exact amount of data to be read).
> > > > Unfortunately the fs_read() semantics is fundamentally broken as
> > > > it
> > > > interprets 'size == 0' as 'read as much data as you can get'
> > > > which
> > > > may
> > > > easily lead to buffer overflows and prints a very unhelpful error
> > > > message "<filename> shorter than offset + len" in case 'size'
> > > > does
> > > > not
> > > > match the actual amount of data read. 
> > > > 
> > > > You should also consider what happens, if the firmware file you
> > > > read
> > > > is
> > > > larger than the buffer you provided, as in that case fs_read()
> > > > will
> > > > stop reading at the provided buffer size and you won't be able to
> > > > notice, that the file was not completely loaded!
> > > > 
> > > I prefer to let the caller handles this, because this function can
> > > be
> > > used to read the firmware chunk by chunk when the allocated buffer
> > > is
> > > smaller than firmware.
> > > 
> > So, the caller should also check, whether the return value of the
> > function matches the requested size and you should omit the special
> > handling of actread != firmware_p->size in
> > fw_get_filesystem_firmware()
> > (as you did in the current version of your patch).
> > 
> Just to confirm we are in same page, the caller i means is function
> which call the request_firmware_into_buf. firmware_p->size is the size
> gonna be read from firmware, could be part of firmware with different
> offset.
> Would you mind to explain more why the special handling should be
> omitted? This special handling would return error code if fail,or 
> actread if pass to the request_firmware_into_buf.
> request_firmware_into_buf just need to know status of file reading is
> performed successfully or not.
>
The point is whether to define a short read as failure and return an
error code in this case or just let the caller know the actual amount
of data read. If you always return the actual number of bytes read (or
a negative error code), no matter whether it matches the requested size
or not, the caller has all information to handle the situation
correctly. It could just request a new chunk for the missing bytes in
case of a short read, or handle a short read as an error condition at
its own discretion. So there is no need to check the actual read size
in your function.

But probably this discussion is completely vain, since if you want to
load a piece of firmware, you would probably check for the existence
of the file and its size beforehand, allocate a sufficiently large
buffer and then call request_firmware_into_buf() with the correct size.
Thus the situation actread != size should never occur anyway in
practice.


Lothar Waßmann

On Jum, 2017-12-22 at 08:44 +0100, Lothar Waßmann wrote:
> Hi,

> 

> On Fri, 22 Dec 2017 01:43:38 +0000 Chee, Tien Fong wrote:

> > 

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

> > > 

> > > Hi,

> > > 

> > > On Thu, 21 Dec 2017 12:48:53 +0000 Chee, Tien Fong wrote:

> > > > 

> > > > 

> > > > On Kha, 2017-12-21 at 12:53 +0100, Lothar Waßmann wrote:

> > > > > 

> > > > > 

> > > > > Hi,

> > > > > 

> > > > > On Thu, 21 Dec 2017 09:36:41 +0000 Chee, Tien Fong wrote:

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann wrote:

> > > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > Hi,

> > > > > > > 

> > > > > > > On Thu, 21 Dec 2017 15:25:29 +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         | 311

> > > > > > > > +++++++++++++++++++++++++++++++++++++++++++++

> > > > > > > >  doc/README.firmware_loader |  76 +++++++++++

> > > > > > > >  include/fs_loader.h        |  28 ++++

> > > > > > > >  4 files changed, 416 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/Makefile b/common/Makefile

> > > > > > > > index cec506f..2934221 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..ddfce58

> > > > > > > > --- /dev/null

> > > > > > > > +++ b/common/fs_loader.c

> > > > > > > > @@ -0,0 +1,311 @@

> > > > > > > > +/*

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

> > > > > > > > +

> > > > > > > > +struct firmware_priv {

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

> > > > > > > > +	u32 offset;		/* Offset of

> > > > > > > > reading a

> > > > > > > > file

> > > > > > > > */

> > > > > > > > +};

> > > > > > > > +

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

> > > > > > > > +

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

> > > > > > > > +	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 ret;

> > > > > > > > +}

> > > > > > > > +

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

> > > > > > > > +}

> > > > > > > > +#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;

> > > > > > > > +

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

> > > > > > > > +

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

> > > > > > > > +

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

> > > > > > > > +		ret = init_mmc();

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

> > > > > > > > +		ret = init_storage_sata();

> > > > > > > > +	} else if (location->ubivol != NULL) {

> > > > > > > > +		ret = mount_ubifs(location);

> > > > > > > > +#ifndef CONFIG_SPL_BUILD

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

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

> > > > > > > > +		ret = init_usb();

> > > > > > > > +#endif

> > > > > > > > +	} else {

> > > > > > > > +		printf("Error: no supported storage

> > > > > > > > device

> > > > > > > > is

> > > > > > > > available.\n");

> > > > > > > > +		ret = -ENODEV;

> > > > > > > > +	}

> > > > > > > > +

> > > > > > > > +	return ret;

> > > > > > > > +}

> > > > > > > > +

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

> > > > > > > > *devpart)

> > > > > > > > +{

> > > > > > > > +	size_t i;

> > > > > > > > +

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

> > > > > > > > i++) {

> > > > > > > > +		if (!strcmp(default_locations[i].name,

> > > > > > > > name))

> > > > > > > > +			default_locations[i].devpart =

> > > > > > > > devpart;

> > > > > > > > +	}

> > > > > > > > +}

> > > > > > > > +

> > > > > > > > +/*

> > > > > > > > + * 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;

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

> > > > > > > > +}

> > > > > > > > +

> > > > > > > > +/*

> > > > > > > > + * 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);

> > > > > > > > 

> > > > > > > 'ret = ' is missing.

> > > > > > > 

> > > > > > Okay.

> > > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > +	if (ret)

> > > > > > > > +		goto out;

> > > > > > > > +

> > > > > > > > +	fw_priv = firmware_p->priv;

> > > > > > > > +

> > > > > > > > +	ret = fs_read(fw_priv->name,

> > > > > > > > (ulong)firmware_p-

> > > > > > > > > 

> > > > > > > > > data,

> > > > > > > > fw_priv->offset,

> > > > > > > > +		     firmware_p->size, &actread);

> > > > > > > > +

> > > > > > > > +	if (ret) {

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

> > > > > > > > from

> > > > > > > > flash

> > > > > > > > %lld != %d.\n",

> > > > > > > > +		      ret, fw_priv->name, actread,

> > > > > > > > firmware_p-

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > size);

> > > > > > > > +		return ret;

> > > > > > > Y

> > > > > > > Shouldn't this be 'goto out', do do the umount_ubifs() as

> > > > > > > in

> > > > > > > all

> > > > > > > other

> > > > > > > error cases?

> > > > > > > 

> > > > > > okay.

> > > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > +	} else {

> > > > > > > > +		ret = actread;

> > > > > > > > +	}

> > > > > > > > 

> > > > > > > What if actread != firmware_p->size?

> > > > > > > You handled it as an error before, now you happily return

> > > > > > > the

> > > > > > > number

> > > > > > > of

> > > > > > > bytes read in case of a short read.

> > > > > > > Operation not permitted

> > > > > > May be i misunderstand on ur previosly comment. There is

> > > > > > not

> > > > > > much

> > > > > > 

> > > > > My comment was about returning -EPERM in this case.

> > > > > First of all you discarded the possible error code returned

> > > > > from

> > > > > fs_read() and secondly -EIO would be more appropriate than

> > > > > EPERM

> > > > > in

> > > > > case of reading less data than expected. 

> > > > > 

> > > > One of the reason I prefer to use -EIO instead of return from

> > > > fs_read,

> > > > because i'm not sure when "actread != firmware_p->size", the

> > > > return

> > > > from the fs_read is still -ve or not.

> > > > 

> > > The return code of fs_read() could be for example -ENOENT, if the

> > > file

> > > you try to load does not exist. Replacing it with -EIO will

> > > mislead

> > > users about the cause of the error.

> > > Thus, if fs_read()'s return code is < 0, you should return that!

> > > If it is zero and you decide to treat actread =! firmware_p->size 

> > > as

> > > an

> > > error, you should return -EIO only in that case!

> > > 

> > Okay.

> > > 

> > > > 

> > > > 

> > > > > 

> > > > > 

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > description on fd_read, i just knowing that -ve is error

> > > > > > from

> > > > > > function

> > > > > > based on other example codes in U-Boot as they check the

> > > > > > error

> > > > > > with

> > > > > > "ret < 0" or "ret != 0". "actread != firmware_p->size" is

> > > > > > the

> > > > > > additonal

> > > > > > checking i added myself because when i digging to the code,

> > > > > > i

> > > > > > found

> > > > > > that such condition only with error printf inside the

> > > > > > function

> > > > > > without

> > > > > > any error code return. So, i am not sure when "actread !=

> > > > > > firmware_p-

> > > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > size", the ret would be -ve too.

> > > > > If 'size' is the actual size of the caller provided buffer

> > > > > and

> > > > > not

> > > > > the

> > > > > exact size of the firmware to be loaded, the condition

> > > > > actread !=

> > > > > size

> > > > > would not be an error at all.

> > > > > If 'size' designates the exact size of the firmware, that the

> > > > > caller

> > > > > may know by other means, it is obviously an error if actread

> > > > > !=

> > > > > size.

> > > > > Since you treated actread != size as an error in your

> > > > > original

> > > > > patch

> > > > > I

> > > > > assumed the latter case.

> > > > > 

> > > > The size here refer to the size of firmware or chunk of

> > > > firmware to

> > > > read when the buffer is smaller than firmware.

> > > > 

> > > OK. In that case, you should not print a message when actread !=

> > > firmware_p->size, since fs_read will have already printed a

> > > message

> > > due

> > > to this.

> > > 

> > The printed message with additional info such as ret, size and

> > actread.

> > > 

> > > > 

> > > > 

> > > > > 

> > > > > 

> > > > > AFAICT Linux uses the first semantics ('size' specifies the

> > > > > size

> > > > > of

> > > > > the

> > > > > caller provided buffer as an upper limit for the read() call

> > > > > and

> > > > > not

> > > > > the exact amount of data to be read).

> > > > > Unfortunately the fs_read() semantics is fundamentally broken

> > > > > as

> > > > > it

> > > > > interprets 'size == 0' as 'read as much data as you can get'

> > > > > which

> > > > > may

> > > > > easily lead to buffer overflows and prints a very unhelpful

> > > > > error

> > > > > message "<filename> shorter than offset + len" in case 'size'

> > > > > does

> > > > > not

> > > > > match the actual amount of data read. 

> > > > > 

> > > > > You should also consider what happens, if the firmware file

> > > > > you

> > > > > read

> > > > > is

> > > > > larger than the buffer you provided, as in that case

> > > > > fs_read()

> > > > > will

> > > > > stop reading at the provided buffer size and you won't be

> > > > > able to

> > > > > notice, that the file was not completely loaded!

> > > > > 

> > > > I prefer to let the caller handles this, because this function

> > > > can

> > > > be

> > > > used to read the firmware chunk by chunk when the allocated

> > > > buffer

> > > > is

> > > > smaller than firmware.

> > > > 

> > > So, the caller should also check, whether the return value of the

> > > function matches the requested size and you should omit the

> > > special

> > > handling of actread != firmware_p->size in

> > > fw_get_filesystem_firmware()

> > > (as you did in the current version of your patch).

> > > 

> > Just to confirm we are in same page, the caller i means is function

> > which call the request_firmware_into_buf. firmware_p->size is the

> > size

> > gonna be read from firmware, could be part of firmware with

> > different

> > offset.

> > Would you mind to explain more why the special handling should be

> > omitted? This special handling would return error code if fail,or 

> > actread if pass to the request_firmware_into_buf.

> > request_firmware_into_buf just need to know status of file reading

> > is

> > performed successfully or not.

> > 

> The point is whether to define a short read as failure and return an

> error code in this case or just let the caller know the actual amount

> of data read. If you always return the actual number of bytes read

> (or

> a negative error code), no matter whether it matches the requested

> size

> or not, the caller has all information to handle the situation

> correctly. It could just request a new chunk for the missing bytes in

> case of a short read, or handle a short read as an error condition at

> its own discretion. So there is no need to check the actual read size

> in your function.

> 

> But probably this discussion is completely vain, since if you want to

> load a piece of firmware, you would probably check for the existence

> of the file and its size beforehand, allocate a sufficiently large

> buffer and then call request_firmware_into_buf() with the correct

> size.

> Thus the situation actread != size should never occur anyway in

> practice.

> 

Okay, so you means that regardless fs_read  is success or fail, just
return the actread to the caller, and let caller to decide the next
handling action, right?
> 

> Lothar Waßmann

Hi,

On Fri, 22 Dec 2017 08:04:32 +0000 Chee, Tien Fong wrote:
> On Jum, 2017-12-22 at 08:44 +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Fri, 22 Dec 2017 01:43:38 +0000 Chee, Tien Fong wrote:
> > > 
> > > On Kha, 2017-12-21 at 16:08 +0100, Lothar Waßmann wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On Thu, 21 Dec 2017 12:48:53 +0000 Chee, Tien Fong wrote:
> > > > > 
> > > > > 
> > > > > On Kha, 2017-12-21 at 12:53 +0100, Lothar Waßmann wrote:
> > > > > > 
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On Thu, 21 Dec 2017 09:36:41 +0000 Chee, Tien Fong wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On Thu, 21 Dec 2017 15:25:29 +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         | 311
> > > > > > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > >  doc/README.firmware_loader |  76 +++++++++++
> > > > > > > > >  include/fs_loader.h        |  28 ++++
> > > > > > > > >  4 files changed, 416 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/Makefile b/common/Makefile
> > > > > > > > > index cec506f..2934221 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..ddfce58
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/common/fs_loader.c
> > > > > > > > > @@ -0,0 +1,311 @@
> > > > > > > > > +/*
> > > > > > > > > + * 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>
> > > > > > > > > +
> > > > > > > > > +struct firmware_priv {
> > > > > > > > > +	const char *name;	/* Filename */
> > > > > > > > > +	u32 offset;		/* Offset of
> > > > > > > > > reading a
> > > > > > > > > file
> > > > > > > > > */
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > +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;
> > > > > > > > > +
> > > > > > > > > +	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;
> > > > > > > > > +	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 ret;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +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;
> > > > > > > > > +}
> > > > > > > > > +#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;
> > > > > > > > > +
> > > > > > > > > +	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;
> > > > > > > > > +
> > > > > > > > > +	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;
> > > > > > > > > +
> > > > > > > > > +	if (!strcmp("mmc", location->name)) {
> > > > > > > > > +		ret = init_mmc();
> > > > > > > > > +	} else if (!strcmp("sata", location->name)) {
> > > > > > > > > +		ret = init_storage_sata();
> > > > > > > > > +	} else if (location->ubivol != NULL) {
> > > > > > > > > +		ret = mount_ubifs(location);
> > > > > > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > > > > > +	/* USB build is not supported yet in SPL */
> > > > > > > > > +	} else if (!strcmp("usb", location->name)) {
> > > > > > > > > +		ret = init_usb();
> > > > > > > > > +#endif
> > > > > > > > > +	} else {
> > > > > > > > > +		printf("Error: no supported storage
> > > > > > > > > device
> > > > > > > > > is
> > > > > > > > > available.\n");
> > > > > > > > > +		ret = -ENODEV;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	return ret;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static void set_storage_devpart(char *name, char
> > > > > > > > > *devpart)
> > > > > > > > > +{
> > > > > > > > > +	size_t i;
> > > > > > > > > +
> > > > > > > > > +	for (i = 0; i < ARRAY_SIZE(default_locations);
> > > > > > > > > i++) {
> > > > > > > > > +		if (!strcmp(default_locations[i].name,
> > > > > > > > > name))
> > > > > > > > > +			default_locations[i].devpart =
> > > > > > > > > devpart;
> > > > > > > > > +	}
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +/*
> > > > > > > > > + * 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;
> > > > > > > > > +	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;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +/*
> > > > > > > > > + * 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);
> > > > > > > > > 
> > > > > > > > 'ret = ' is missing.
> > > > > > > > 
> > > > > > > Okay.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +	if (ret)
> > > > > > > > > +		goto out;
> > > > > > > > > +
> > > > > > > > > +	fw_priv = firmware_p->priv;
> > > > > > > > > +
> > > > > > > > > +	ret = fs_read(fw_priv->name,
> > > > > > > > > (ulong)firmware_p-
> > > > > > > > > > 
> > > > > > > > > > data,
> > > > > > > > > fw_priv->offset,
> > > > > > > > > +		     firmware_p->size, &actread);
> > > > > > > > > +
> > > > > > > > > +	if (ret) {
> > > > > > > > > +		printf("Error: %d Failed to read %s
> > > > > > > > > from
> > > > > > > > > flash
> > > > > > > > > %lld != %d.\n",
> > > > > > > > > +		      ret, fw_priv->name, actread,
> > > > > > > > > firmware_p-
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > size);
> > > > > > > > > +		return ret;
> > > > > > > > Y
> > > > > > > > Shouldn't this be 'goto out', do do the umount_ubifs() as
> > > > > > > > in
> > > > > > > > all
> > > > > > > > other
> > > > > > > > error cases?
> > > > > > > > 
> > > > > > > okay.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +	} else {
> > > > > > > > > +		ret = actread;
> > > > > > > > > +	}
> > > > > > > > > 
> > > > > > > > What if actread != firmware_p->size?
> > > > > > > > You handled it as an error before, now you happily return
> > > > > > > > the
> > > > > > > > number
> > > > > > > > of
> > > > > > > > bytes read in case of a short read.
> > > > > > > > Operation not permitted
> > > > > > > May be i misunderstand on ur previosly comment. There is
> > > > > > > not
> > > > > > > much
> > > > > > > 
> > > > > > My comment was about returning -EPERM in this case.
> > > > > > First of all you discarded the possible error code returned
> > > > > > from
> > > > > > fs_read() and secondly -EIO would be more appropriate than
> > > > > > EPERM
> > > > > > in
> > > > > > case of reading less data than expected. 
> > > > > > 
> > > > > One of the reason I prefer to use -EIO instead of return from
> > > > > fs_read,
> > > > > because i'm not sure when "actread != firmware_p->size", the
> > > > > return
> > > > > from the fs_read is still -ve or not.
> > > > > 
> > > > The return code of fs_read() could be for example -ENOENT, if the
> > > > file
> > > > you try to load does not exist. Replacing it with -EIO will
> > > > mislead
> > > > users about the cause of the error.
> > > > Thus, if fs_read()'s return code is < 0, you should return that!
> > > > If it is zero and you decide to treat actread =! firmware_p->size 
> > > > as
> > > > an
> > > > error, you should return -EIO only in that case!
> > > > 
> > > Okay.
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > description on fd_read, i just knowing that -ve is error
> > > > > > > from
> > > > > > > function
> > > > > > > based on other example codes in U-Boot as they check the
> > > > > > > error
> > > > > > > with
> > > > > > > "ret < 0" or "ret != 0". "actread != firmware_p->size" is
> > > > > > > the
> > > > > > > additonal
> > > > > > > checking i added myself because when i digging to the code,
> > > > > > > i
> > > > > > > found
> > > > > > > that such condition only with error printf inside the
> > > > > > > function
> > > > > > > without
> > > > > > > any error code return. So, i am not sure when "actread !=
> > > > > > > firmware_p-
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > size", the ret would be -ve too.
> > > > > > If 'size' is the actual size of the caller provided buffer
> > > > > > and
> > > > > > not
> > > > > > the
> > > > > > exact size of the firmware to be loaded, the condition
> > > > > > actread !=
> > > > > > size
> > > > > > would not be an error at all.
> > > > > > If 'size' designates the exact size of the firmware, that the
> > > > > > caller
> > > > > > may know by other means, it is obviously an error if actread
> > > > > > !=
> > > > > > size.
> > > > > > Since you treated actread != size as an error in your
> > > > > > original
> > > > > > patch
> > > > > > I
> > > > > > assumed the latter case.
> > > > > > 
> > > > > The size here refer to the size of firmware or chunk of
> > > > > firmware to
> > > > > read when the buffer is smaller than firmware.
> > > > > 
> > > > OK. In that case, you should not print a message when actread !=
> > > > firmware_p->size, since fs_read will have already printed a
> > > > message
> > > > due
> > > > to this.
> > > > 
> > > The printed message with additional info such as ret, size and
> > > actread.
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > AFAICT Linux uses the first semantics ('size' specifies the
> > > > > > size
> > > > > > of
> > > > > > the
> > > > > > caller provided buffer as an upper limit for the read() call
> > > > > > and
> > > > > > not
> > > > > > the exact amount of data to be read).
> > > > > > Unfortunately the fs_read() semantics is fundamentally broken
> > > > > > as
> > > > > > it
> > > > > > interprets 'size == 0' as 'read as much data as you can get'
> > > > > > which
> > > > > > may
> > > > > > easily lead to buffer overflows and prints a very unhelpful
> > > > > > error
> > > > > > message "<filename> shorter than offset + len" in case 'size'
> > > > > > does
> > > > > > not
> > > > > > match the actual amount of data read. 
> > > > > > 
> > > > > > You should also consider what happens, if the firmware file
> > > > > > you
> > > > > > read
> > > > > > is
> > > > > > larger than the buffer you provided, as in that case
> > > > > > fs_read()
> > > > > > will
> > > > > > stop reading at the provided buffer size and you won't be
> > > > > > able to
> > > > > > notice, that the file was not completely loaded!
> > > > > > 
> > > > > I prefer to let the caller handles this, because this function
> > > > > can
> > > > > be
> > > > > used to read the firmware chunk by chunk when the allocated
> > > > > buffer
> > > > > is
> > > > > smaller than firmware.
> > > > > 
> > > > So, the caller should also check, whether the return value of the
> > > > function matches the requested size and you should omit the
> > > > special
> > > > handling of actread != firmware_p->size in
> > > > fw_get_filesystem_firmware()
> > > > (as you did in the current version of your patch).
> > > > 
> > > Just to confirm we are in same page, the caller i means is function
> > > which call the request_firmware_into_buf. firmware_p->size is the
> > > size
> > > gonna be read from firmware, could be part of firmware with
> > > different
> > > offset.
> > > Would you mind to explain more why the special handling should be
> > > omitted? This special handling would return error code if fail,or 
> > > actread if pass to the request_firmware_into_buf.
> > > request_firmware_into_buf just need to know status of file reading
> > > is
> > > performed successfully or not.
> > > 
> > The point is whether to define a short read as failure and return an
> > error code in this case or just let the caller know the actual amount
> > of data read. If you always return the actual number of bytes read
> > (or
> > a negative error code), no matter whether it matches the requested
> > size
> > or not, the caller has all information to handle the situation
> > correctly. It could just request a new chunk for the missing bytes in
> > case of a short read, or handle a short read as an error condition at
> > its own discretion. So there is no need to check the actual read size
> > in your function.
> > 
> > But probably this discussion is completely vain, since if you want to
> > load a piece of firmware, you would probably check for the existence
> > of the file and its size beforehand, allocate a sufficiently large
> > buffer and then call request_firmware_into_buf() with the correct
> > size.
> > Thus the situation actread != size should never occur anyway in
> > practice.
> > 
> Okay, so you means that regardless fs_read  is success or fail, just
> return the actread to the caller, and let caller to decide the next
> handling action, right?
> > 
NOOOOO!
If fs_read() returns an error code, you should return that of course!

If the return code from fs_read() is zero, you should just return
actread without spewing any message and let the caller handle the
situation appropriately.


Lothar Waßmann

On Jum, 2017-12-22 at 09:47 +0100, Lothar Waßmann wrote:
> Hi,

> 

> On Fri, 22 Dec 2017 08:04:32 +0000 Chee, Tien Fong wrote:

> > 

> > On Jum, 2017-12-22 at 08:44 +0100, Lothar Waßmann wrote:

> > > 

> > > Hi,

> > > 

> > > On Fri, 22 Dec 2017 01:43:38 +0000 Chee, Tien Fong wrote:

> > > > 

> > > > 

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

> > > > > 

> > > > > 

> > > > > Hi,

> > > > > 

> > > > > On Thu, 21 Dec 2017 12:48:53 +0000 Chee, Tien Fong wrote:

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > On Kha, 2017-12-21 at 12:53 +0100, Lothar Waßmann wrote:

> > > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > Hi,

> > > > > > > 

> > > > > > > On Thu, 21 Dec 2017 09:36:41 +0000 Chee, Tien Fong wrote:

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann

> > > > > > > > wrote:

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > Hi,

> > > > > > > > > 

> > > > > > > > > On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee@int

> > > > > > > > > el.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         | 311

> > > > > > > > > > +++++++++++++++++++++++++++++++++++++++++++++

> > > > > > > > > >  doc/README.firmware_loader |  76 +++++++++++

> > > > > > > > > >  include/fs_loader.h        |  28 ++++

> > > > > > > > > >  4 files changed, 416 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/Makefile b/common/Makefile

> > > > > > > > > > index cec506f..2934221 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..ddfce58

> > > > > > > > > > --- /dev/null

> > > > > > > > > > +++ b/common/fs_loader.c

> > > > > > > > > > @@ -0,0 +1,311 @@

> > > > > > > > > > +/*

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

> > > > > > > > > > +

> > > > > > > > > > +struct firmware_priv {

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

> > > > > > > > > > +	u32 offset;		/* Offset of

> > > > > > > > > > reading a

> > > > > > > > > > file

> > > > > > > > > > */

> > > > > > > > > > +};

> > > > > > > > > > +

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

> > > > > > > > > > +

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

> > > > > > > > > > +	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 ret;

> > > > > > > > > > +}

> > > > > > > > > > +

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

> > > > > > > > > > +}

> > > > > > > > > > +#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;

> > > > > > > > > > +

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

> > > > > > > > > > +

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

> > > > > > > > > > +

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

> > > > > > > > > > +		ret = init_mmc();

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

> > > > > > > > > > >name)) {

> > > > > > > > > > +		ret = init_storage_sata();

> > > > > > > > > > +	} else if (location->ubivol != NULL) {

> > > > > > > > > > +		ret = mount_ubifs(location);

> > > > > > > > > > +#ifndef CONFIG_SPL_BUILD

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

> > > > > > > > > > */

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

> > > > > > > > > > {

> > > > > > > > > > +		ret = init_usb();

> > > > > > > > > > +#endif

> > > > > > > > > > +	} else {

> > > > > > > > > > +		printf("Error: no supported

> > > > > > > > > > storage

> > > > > > > > > > device

> > > > > > > > > > is

> > > > > > > > > > available.\n");

> > > > > > > > > > +		ret = -ENODEV;

> > > > > > > > > > +	}

> > > > > > > > > > +

> > > > > > > > > > +	return ret;

> > > > > > > > > > +}

> > > > > > > > > > +

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

> > > > > > > > > > *devpart)

> > > > > > > > > > +{

> > > > > > > > > > +	size_t i;

> > > > > > > > > > +

> > > > > > > > > > +	for (i = 0; i <

> > > > > > > > > > ARRAY_SIZE(default_locations);

> > > > > > > > > > i++) {

> > > > > > > > > > +		if

> > > > > > > > > > (!strcmp(default_locations[i].name,

> > > > > > > > > > name))

> > > > > > > > > > +			default_locations[i].devpa

> > > > > > > > > > rt =

> > > > > > > > > > devpart;

> > > > > > > > > > +	}

> > > > > > > > > > +}

> > > > > > > > > > +

> > > > > > > > > > +/*

> > > > > > > > > > + * 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;

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

> > > > > > > > > > +}

> > > > > > > > > > +

> > > > > > > > > > +/*

> > > > > > > > > > + * 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);

> > > > > > > > > > 

> > > > > > > > > 'ret = ' is missing.

> > > > > > > > > 

> > > > > > > > Okay.

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > +	if (ret)

> > > > > > > > > > +		goto out;

> > > > > > > > > > +

> > > > > > > > > > +	fw_priv = firmware_p->priv;

> > > > > > > > > > +

> > > > > > > > > > +	ret = fs_read(fw_priv->name,

> > > > > > > > > > (ulong)firmware_p-

> > > > > > > > > > > 

> > > > > > > > > > > 

> > > > > > > > > > > data,

> > > > > > > > > > fw_priv->offset,

> > > > > > > > > > +		     firmware_p->size, &actread);

> > > > > > > > > > +

> > > > > > > > > > +	if (ret) {

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

> > > > > > > > > > %s

> > > > > > > > > > from

> > > > > > > > > > flash

> > > > > > > > > > %lld != %d.\n",

> > > > > > > > > > +		      ret, fw_priv->name, actread,

> > > > > > > > > > firmware_p-

> > > > > > > > > > > 

> > > > > > > > > > > 

> > > > > > > > > > > 

> > > > > > > > > > > 

> > > > > > > > > > > size);

> > > > > > > > > > +		return ret;

> > > > > > > > > Y

> > > > > > > > > Shouldn't this be 'goto out', do do the

> > > > > > > > > umount_ubifs() as

> > > > > > > > > in

> > > > > > > > > all

> > > > > > > > > other

> > > > > > > > > error cases?

> > > > > > > > > 

> > > > > > > > okay.

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > 

> > > > > > > > > > +	} else {

> > > > > > > > > > +		ret = actread;

> > > > > > > > > > +	}

> > > > > > > > > > 

> > > > > > > > > What if actread != firmware_p->size?

> > > > > > > > > You handled it as an error before, now you happily

> > > > > > > > > return

> > > > > > > > > the

> > > > > > > > > number

> > > > > > > > > of

> > > > > > > > > bytes read in case of a short read.

> > > > > > > > > Operation not permitted

> > > > > > > > May be i misunderstand on ur previosly comment. There

> > > > > > > > is

> > > > > > > > not

> > > > > > > > much

> > > > > > > > 

> > > > > > > My comment was about returning -EPERM in this case.

> > > > > > > First of all you discarded the possible error code

> > > > > > > returned

> > > > > > > from

> > > > > > > fs_read() and secondly -EIO would be more appropriate

> > > > > > > than

> > > > > > > EPERM

> > > > > > > in

> > > > > > > case of reading less data than expected. 

> > > > > > > 

> > > > > > One of the reason I prefer to use -EIO instead of return

> > > > > > from

> > > > > > fs_read,

> > > > > > because i'm not sure when "actread != firmware_p->size",

> > > > > > the

> > > > > > return

> > > > > > from the fs_read is still -ve or not.

> > > > > > 

> > > > > The return code of fs_read() could be for example -ENOENT, if

> > > > > the

> > > > > file

> > > > > you try to load does not exist. Replacing it with -EIO will

> > > > > mislead

> > > > > users about the cause of the error.

> > > > > Thus, if fs_read()'s return code is < 0, you should return

> > > > > that!

> > > > > If it is zero and you decide to treat actread =! firmware_p-

> > > > > >size 

> > > > > as

> > > > > an

> > > > > error, you should return -EIO only in that case!

> > > > > 

> > > > Okay.

> > > > > 

> > > > > 

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > 

> > > > > > > > description on fd_read, i just knowing that -ve is

> > > > > > > > error

> > > > > > > > from

> > > > > > > > function

> > > > > > > > based on other example codes in U-Boot as they check

> > > > > > > > the

> > > > > > > > error

> > > > > > > > with

> > > > > > > > "ret < 0" or "ret != 0". "actread != firmware_p->size"

> > > > > > > > is

> > > > > > > > the

> > > > > > > > additonal

> > > > > > > > checking i added myself because when i digging to the

> > > > > > > > code,

> > > > > > > > i

> > > > > > > > found

> > > > > > > > that such condition only with error printf inside the

> > > > > > > > function

> > > > > > > > without

> > > > > > > > any error code return. So, i am not sure when "actread

> > > > > > > > !=

> > > > > > > > firmware_p-

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > size", the ret would be -ve too.

> > > > > > > If 'size' is the actual size of the caller provided

> > > > > > > buffer

> > > > > > > and

> > > > > > > not

> > > > > > > the

> > > > > > > exact size of the firmware to be loaded, the condition

> > > > > > > actread !=

> > > > > > > size

> > > > > > > would not be an error at all.

> > > > > > > If 'size' designates the exact size of the firmware, that

> > > > > > > the

> > > > > > > caller

> > > > > > > may know by other means, it is obviously an error if

> > > > > > > actread

> > > > > > > !=

> > > > > > > size.

> > > > > > > Since you treated actread != size as an error in your

> > > > > > > original

> > > > > > > patch

> > > > > > > I

> > > > > > > assumed the latter case.

> > > > > > > 

> > > > > > The size here refer to the size of firmware or chunk of

> > > > > > firmware to

> > > > > > read when the buffer is smaller than firmware.

> > > > > > 

> > > > > OK. In that case, you should not print a message when actread

> > > > > !=

> > > > > firmware_p->size, since fs_read will have already printed a

> > > > > message

> > > > > due

> > > > > to this.

> > > > > 

> > > > The printed message with additional info such as ret, size and

> > > > actread.

> > > > > 

> > > > > 

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > AFAICT Linux uses the first semantics ('size' specifies

> > > > > > > the

> > > > > > > size

> > > > > > > of

> > > > > > > the

> > > > > > > caller provided buffer as an upper limit for the read()

> > > > > > > call

> > > > > > > and

> > > > > > > not

> > > > > > > the exact amount of data to be read).

> > > > > > > Unfortunately the fs_read() semantics is fundamentally

> > > > > > > broken

> > > > > > > as

> > > > > > > it

> > > > > > > interprets 'size == 0' as 'read as much data as you can

> > > > > > > get'

> > > > > > > which

> > > > > > > may

> > > > > > > easily lead to buffer overflows and prints a very

> > > > > > > unhelpful

> > > > > > > error

> > > > > > > message "<filename> shorter than offset + len" in case

> > > > > > > 'size'

> > > > > > > does

> > > > > > > not

> > > > > > > match the actual amount of data read. 

> > > > > > > 

> > > > > > > You should also consider what happens, if the firmware

> > > > > > > file

> > > > > > > you

> > > > > > > read

> > > > > > > is

> > > > > > > larger than the buffer you provided, as in that case

> > > > > > > fs_read()

> > > > > > > will

> > > > > > > stop reading at the provided buffer size and you won't be

> > > > > > > able to

> > > > > > > notice, that the file was not completely loaded!

> > > > > > > 

> > > > > > I prefer to let the caller handles this, because this

> > > > > > function

> > > > > > can

> > > > > > be

> > > > > > used to read the firmware chunk by chunk when the allocated

> > > > > > buffer

> > > > > > is

> > > > > > smaller than firmware.

> > > > > > 

> > > > > So, the caller should also check, whether the return value of

> > > > > the

> > > > > function matches the requested size and you should omit the

> > > > > special

> > > > > handling of actread != firmware_p->size in

> > > > > fw_get_filesystem_firmware()

> > > > > (as you did in the current version of your patch).

> > > > > 

> > > > Just to confirm we are in same page, the caller i means is

> > > > function

> > > > which call the request_firmware_into_buf. firmware_p->size is

> > > > the

> > > > size

> > > > gonna be read from firmware, could be part of firmware with

> > > > different

> > > > offset.

> > > > Would you mind to explain more why the special handling should

> > > > be

> > > > omitted? This special handling would return error code if

> > > > fail,or 

> > > > actread if pass to the request_firmware_into_buf.

> > > > request_firmware_into_buf just need to know status of file

> > > > reading

> > > > is

> > > > performed successfully or not.

> > > > 

> > > The point is whether to define a short read as failure and return

> > > an

> > > error code in this case or just let the caller know the actual

> > > amount

> > > of data read. If you always return the actual number of bytes

> > > read

> > > (or

> > > a negative error code), no matter whether it matches the

> > > requested

> > > size

> > > or not, the caller has all information to handle the situation

> > > correctly. It could just request a new chunk for the missing

> > > bytes in

> > > case of a short read, or handle a short read as an error

> > > condition at

> > > its own discretion. So there is no need to check the actual read

> > > size

> > > in your function.

> > > 

> > > But probably this discussion is completely vain, since if you

> > > want to

> > > load a piece of firmware, you would probably check for the

> > > existence

> > > of the file and its size beforehand, allocate a sufficiently

> > > large

> > > buffer and then call request_firmware_into_buf() with the correct

> > > size.

> > > Thus the situation actread != size should never occur anyway in

> > > practice.

> > > 

> > Okay, so you means that regardless fs_read  is success or fail,

> > just

> > return the actread to the caller, and let caller to decide the next

> > handling action, right?

> > > 

> > > 

> NOOOOO!

> If fs_read() returns an error code, you should return that of course!

> 

> If the return code from fs_read() is zero, you should just return

> actread without spewing any message and let the caller handle the

> situation appropriately.

> 

Okay, i have no problem with that.
> 

> Lothar Waßmann