Generic firmware loader

PATCH Superseded tien.fong.chee at intel.com 3 participants 15 messages Dec 21, 2017 – Dec 22, 2017

lore

Share & tools

lore

tien.fong.chee at intel.com

author

2017-12-21 07:25 UTC #0 lore raw

v5 cover Generic firmware loader

From: Tien Fong Chee <[email protected]>

This patchset contains generic firmware loader which is very close to Linux firmware loader but for U-Boot framework. Generic firmware loader can be used load whatever into target location, and then consumer driver would use it to program whatever, ie. the FPGA. This version mainly resolved comments from Lothar WaÃŸmann, and Lukasz Majewski in [v4].

This series is working on top of u-boot.git -
http://git.denx.de/u-boot.git .

[v4]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272432.html

v4 -> v5 changes:
-----------------
- Changed the env variable name to lower case.
- Added memory freeing function in failure case.
- Assign valid pointer to *firmware_p only when everything is OK.
- Removed the casting on void pointer when assigning to any type pointer.
- Fixed with the right return code.

Patchset history
----------------
[v1]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271905.html [v2]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271979.html [v3]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272039.html

Tien Fong Chee (2):
spl: Remove static declaration on spl_mmc_find_device function
common: Generic firmware loader for file system

 common/Makefile            |   1 +
 common/fs_loader.c         | 311 +++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c       |   2 +-
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +
 6 files changed, 419 insertions(+), 1 deletion(-)
 create mode 100644 common/fs_loader.c
 create mode 100644 doc/README.firmware_loader
 create mode 100644 include/fs_loader.h

Signature · 1 line

2.2.0

tien.fong.chee at intel.com

author

2017-12-21 07:25 UTC #1 lore raw

v5 cover Generic firmware loader

From: Tien Fong Chee <[email protected]>

This series is working on top of u-boot.git -
http://git.denx.de/u-boot.git .

[v4]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272432.html

Tien Fong Chee (2):
spl: Remove static declaration on spl_mmc_find_device function
common: Generic firmware loader for file system

 common/Makefile            |   1 +
 common/fs_loader.c         | 311 +++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c       |   2 +-
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +
 6 files changed, 419 insertions(+), 1 deletion(-)
 create mode 100644 common/fs_loader.c
 create mode 100644 doc/README.firmware_loader
 create mode 100644 include/fs_loader.h

Signature · 1 line

2.2.0

tien.fong.chee at intel.com

author

2017-12-21 07:25 UTC #2 lore raw

PATCH v5 1/2 spl: Remove static declaration on spl_mmc_find_device function

In reply to tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

This patch removes the static declation on spl_mmc_find_device_function so this function is accessible by the caller from other file. This patch is required for later patch.

Signed-off-by Tien Fong Chee <[email protected]>

---
 common/spl/spl_mmc.c | 2 +-
 include/spl.h        | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

common/spl/spl_mmc.c +1 -1

index b57e0b0..183d05a 100644
@@ -114,7 +114,7 @@ static int spl_mmc_get_device_index(u32 boot_device)
 	return -ENODEV;
 }
 
-static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
+int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
 {
 #if CONFIG_IS_ENABLED(DM_MMC)
 	struct udevice *dev;

include/spl.h +2 -0

index 308ce7b..912983a 100644
@@ -10,6 +10,7 @@
 /* Platform-specific defines */
 #include <linux/compiler.h>
 #include <asm/spl.h>
+#include <mmc.h>
 
 /* Value in r0 indicates we booted from U-Boot */
 #define UBOOT_NOT_LOADED_FROM_SPL	0x13578642

@@ -72,6 +73,7 @@ void preloader_console_init(void);
 u32 spl_boot_device(void);
 u32 spl_boot_mode(const u32 boot_device);
 void spl_set_bd(void);
+int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device);
 
 /**
  * spl_set_header_raw_uboot() - Set up a standard SPL image structure

Signature · 1 line

2.2.0

tien.fong.chee at intel.com

author

2017-12-21 07:25 UTC #3 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

In reply to tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

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 <[email protected]>

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

common/Makefile +1 -0

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

common/fs_loader.c new file +311 -0

311 lines not on this page · raw message

doc/README.firmware_loader new file +76 -0

76 lines not on this page · raw message

include/fs_loader.h new file +28 -0

new file mode 100644
index 0000000..83a8b80
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+#ifndef _FS_LOADER_H_
+#define _FS_LOADER_H_
+
+#include <linux/types.h>
+
+struct firmware {
+	size_t size;		/* Size of a file */
+	const u8 *data;		/* Buffer for file */
+	void *priv;		/* Firmware loader private fields */
+};
+
+struct device_location {
+	char *name;	/* Such as mmc, usb,and sata. */
+	char *devpart;  /* Use the load command dev:part conventions */
+	char *mtdpart;	/* MTD partition for ubi part */
+	char *ubivol;	/* UBI volume-name for ubifsmount */
+};
+
+int request_firmware_into_buf(struct firmware **firmware_p,
+			      const char *name,
+			      struct device_location *location,
+			      void *buf, size_t size, u32 offset);
+#endif

Signature · 1 line

2.2.0

Lothar Waßmann

2017-12-21 08:48 UTC #4 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

Hi,

Quoted 280 lines · On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at intel.com wrote:

From: Tien Fong Chee <[email protected]>

Signed-off-by Tien Fong Chee <[email protected]>

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

common/Makefile +1 -0

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

common/fs_loader.c new file +246 -0

new file mode 100644
index 0000000..ddfce58
@@ -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;

common/fs_loader.c line 256

Quoted 10 lines

common/fs_loader.c new file +10 -0

+
+	dev_part = env_get("fw_dev_part");
+	if (dev_part)
+		set_storage_devpart(location->name, dev_part);
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);

'ret = ' is missing.

common/fs_loader.c line 268

Quoted 12 lines

common/fs_loader.c new file +12 -0

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

common/fs_loader.c line 271

Quoted 4 lines

common/fs_loader.c new file +3 -0
+	} 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.

common/fs_loader.c line 281

Quoted 10 lines

common/fs_loader.c new file +10 -0

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

Lothar WaÃŸmann

Chee

2017-12-21 09:36 UTC #5 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

Quoted 299 lines · Lothar Waßmann

Hi,

Quoted 296 lines · On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at intel.com wrote:

From: Tien Fong Chee <[email protected]>

Signed-off-by Tien Fong Chee <[email protected]>

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

common/Makefile +0 -0

index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o

obj-y += command.o
obj-y += s_record.o
obj-y += xyzModem.o

common/Makefile +1 -0

+obj-y += fs_loader.o

common/fs_loader.c new file +59 -0

new file mode 100644
index 0000000..ddfce58
@@ -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");

common/fs_loader.c new file +61 -0

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

common/fs_loader.c new file +19 -0

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

common/fs_loader.c new file +2 -0

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

FS_TYPE_ANY);

common/fs_loader.c new file +2 -0

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

FS_TYPE_ANY);

common/fs_loader.c new file +3 -0

+	} else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi", NULL,

FS_TYPE_UBIFS);

common/fs_loader.c new file +30 -0

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

common/fs_loader.c new file +35 -0

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

common/fs_loader.c new file +6 -0

+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv) failed\n",

__func__);

common/fs_loader.c new file +16 -0

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

common/fs_loader.c new file +10 -0

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

Quoted 9 lines

common/fs_loader.c line 256
Quoted 7 lines
common/fs_loader.c new file +6 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
'ret = ' is missing.

Okay.

Quoted 14 lines

Quoted 14 lines
common/fs_loader.c new file +6 -0
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
fw_priv->offset,
common/fs_loader.c new file +4 -0
+		     firmware_p->size, &actread);
+
+	if (ret) {
+		printf("Error: %d Failed to read %s from flash
%lld != %d.\n",
common/fs_loader.c new file +1 -0
+		      ret, fw_priv->name, actread, firmware_p-

Quoted 7 lines

common/fs_loader.c
Quoted 2 lines
Quoted 1 line
size);
common/fs_loader.c new file +1 -0
+		return ret;
Y
Shouldn't this be 'goto out', do do the umount_ubifs() as in all other
error cases?

okay.

Quoted 10 lines

common/fs_loader.c line 271
Quoted 5 lines
common/fs_loader.c new file +3 -0
+	} 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-

Quoted 1 line

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;

Quoted 13 lines

common/fs_loader.c line 281
Quoted 11 lines
common/fs_loader.c new file +10 -0
+
+out:
+#ifdef CONFIG_CMD_UBIFS
+	if (location->ubivol != NULL)
+		umount_ubifs();
+#endif
+
+	return ret;
+}
+
Lothar Waßmann

Chee

2017-12-21 11:08 UTC #6 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

Quoted 367 lines · Tien Fong

Quoted 307 lines · Lothar Waßmann
Hi,
Quoted 303 lines · On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at intel.com wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]>
---
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
common/Makefile +0 -0
index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
obj-y += command.o
obj-y += s_record.o
obj-y += xyzModem.o
common/Makefile +1 -0
+obj-y += fs_loader.o
common/fs_loader.c new file +21 -0
new file mode 100644
index 0000000..ddfce58
@@ -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
*/
common/fs_loader.c new file +38 -0
+};
+
+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");
common/fs_loader.c new file +61 -0
+	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);
common/fs_loader.c new file +19 -0
+		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);
common/fs_loader.c new file +2 -0
+	} else if (!strcmp("usb", location->name)) {
+		ret = fs_set_blk_dev("usb", location->devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +2 -0
+	} else if (!strcmp("sata", location->name)) {
+		ret = fs_set_blk_dev("sata", location->devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +3 -0
+	} else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi", NULL,
FS_TYPE_UBIFS);
common/fs_loader.c new file +4 -0
+		else
+			ret = -ENODEV;
+	} else {
+		printf("Error: unsupported location
storage.\n");
common/fs_loader.c new file +26 -0
+		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");
common/fs_loader.c new file +21 -0
+		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,
common/fs_loader.c new file +1 -0
+				     const char *name, void
*dbuf,
common/fs_loader.c new file +13 -0
+				     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__);
common/fs_loader.c new file +6 -0
+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv) failed\n",
__func__);
common/fs_loader.c new file +16 -0
+		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,
common/fs_loader.c new file +1 -0
+				      struct firmware
*firmware_p)
common/fs_loader.c new file +9 -0
+{
+	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);
Quoted 9 lines
common/fs_loader.c line 256
Quoted 7 lines
common/fs_loader.c new file +6 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
'ret = ' is missing.
Okay.
Quoted 17 lines
Quoted 17 lines
common/fs_loader.c new file +6 -0
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
fw_priv->offset,
common/fs_loader.c new file +4 -0
+		     firmware_p->size, &actread);
+
+	if (ret) {
+		printf("Error: %d Failed to read %s from flash
%lld != %d.\n",
common/fs_loader.c new file +1 -0
+		      ret, fw_priv->name, actread, firmware_p-
Quoted 1 line
Quoted 7 lines
common/fs_loader.c
Quoted 2 lines
Quoted 1 line
size);
common/fs_loader.c new file +1 -0
+		return ret;
Y
Shouldn't this be 'goto out', do do the umount_ubifs() as in all other
error cases?
okay.
Quoted 13 lines
common/fs_loader.c line 271
Quoted 7 lines
common/fs_loader.c new file +3 -0
+	} 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-
Quoted 1 line

Quoted 7 lines

Quoted 1 line
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;

Quoted 26 lines

		goto out;
	} else {
		ret = actread;
	}

out:

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

#endif

	return ret;

Quoted 14 lines

common/fs_loader.c line 281
Quoted 13 lines
common/fs_loader.c new file +10 -0
+
+out:
+#ifdef CONFIG_CMD_UBIFS
+	if (location->ubivol != NULL)
+		umount_ubifs();
+#endif
+
+	return ret;
+}
+
Lothar Waßmann

Lothar Waßmann

2017-12-21 11:53 UTC #7 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

In reply to Chee

Hi,

Quoted 342 lines · Tien Fong

Quoted 299 lines · Lothar Waßmann

Hi,

Quoted 296 lines · On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at intel.com wrote:

From: Tien Fong Chee <[email protected]>

Signed-off-by Tien Fong Chee <[email protected]>

 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

common/Makefile +0 -0

index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o

obj-y += command.o
obj-y += s_record.o
obj-y += xyzModem.o

common/Makefile +1 -0

+obj-y += fs_loader.o

common/fs_loader.c new file +59 -0

new file mode 100644
index 0000000..ddfce58
@@ -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");

common/fs_loader.c new file +61 -0

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

common/fs_loader.c new file +19 -0

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

common/fs_loader.c new file +2 -0

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

FS_TYPE_ANY);

common/fs_loader.c new file +2 -0

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

FS_TYPE_ANY);

common/fs_loader.c new file +3 -0

+	} else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi", NULL,

FS_TYPE_UBIFS);

common/fs_loader.c new file +30 -0

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

common/fs_loader.c new file +35 -0

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

common/fs_loader.c new file +6 -0

+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv) failed\n",

__func__);

common/fs_loader.c new file +16 -0

+		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

common/fs_loader.c new file +10 -0

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

Quoted 9 lines

common/fs_loader.c line 256
Quoted 7 lines
common/fs_loader.c new file +6 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
'ret = ' is missing.

Okay.

Quoted 14 lines

Quoted 14 lines
common/fs_loader.c new file +6 -0
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
fw_priv->offset,
common/fs_loader.c new file +4 -0
+		     firmware_p->size, &actread);
+
+	if (ret) {
+		printf("Error: %d Failed to read %s from flash
%lld != %d.\n",
common/fs_loader.c new file +1 -0
+		      ret, fw_priv->name, actread, firmware_p-

Quoted 7 lines

common/fs_loader.c
Quoted 2 lines
Quoted 1 line
size);
common/fs_loader.c new file +1 -0
+		return ret;
Y
Shouldn't this be 'goto out', do do the umount_ubifs() as in all other
error cases?

okay.

Quoted 10 lines

common/fs_loader.c line 271
Quoted 5 lines
common/fs_loader.c new file +3 -0
+	} 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

Quoted 2 lines

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.

Quoted 8 lines

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-
Quoted 1 line
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

Chee

2017-12-21 12:48 UTC #8 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

Quoted 368 lines · Lothar Waßmann

Hi,

Quoted 365 lines · Tien Fong

Quoted 315 lines · Lothar Waßmann

Hi,

Quoted 310 lines · On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at intel.com wrote:

From: Tien Fong Chee <[email protected]>

Signed-off-by Tien Fong Chee <[email protected]>

 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

common/Makefile +0 -0

index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o

obj-y += command.o
obj-y += s_record.o
obj-y += xyzModem.o

common/Makefile +1 -0

+obj-y += fs_loader.o

common/fs_loader.c new file +21 -0

new file mode 100644
index 0000000..ddfce58
@@ -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

common/fs_loader.c new file +38 -0

+};
+
+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");

common/fs_loader.c new file +45 -0

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

common/fs_loader.c new file +5 -0

+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) &&

defined(CONFIG_SPL_BUILD)

common/fs_loader.c new file +11 -0

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

common/fs_loader.c new file +19 -0

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

common/fs_loader.c new file +2 -0

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

FS_TYPE_ANY);

common/fs_loader.c new file +2 -0

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

Quoted 1 line

devpart,

FS_TYPE_ANY);

common/fs_loader.c new file +3 -0

+	} else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi", NULL,

FS_TYPE_UBIFS);

common/fs_loader.c new file +4 -0

+		else
+			ret = -ENODEV;
+	} else {
+		printf("Error: unsupported location

storage.\n");

common/fs_loader.c new file +10 -0

+		return -ENODEV;
+	}
+
+	if (ret)
+		printf("Error: could not access storage.\n");
+
+	return ret;
+}
+
+static int init_storage_device(struct device_location

*location)

common/fs_loader.c new file +16 -0

+{
+	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");

common/fs_loader.c new file +13 -0

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

common/fs_loader.c new file +8 -0

+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware

**firmware_p,

common/fs_loader.c new file +1 -0

+				     const char *name, void

*dbuf,

common/fs_loader.c new file +13 -0

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

common/fs_loader.c new file +6 -0

+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv) failed\n",

__func__);

common/fs_loader.c new file +16 -0

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

common/fs_loader.c new file +1 -0

+				      struct firmware

*firmware_p)

common/fs_loader.c new file +9 -0

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

Quoted 9 lines

common/fs_loader.c line 256
Quoted 7 lines
common/fs_loader.c new file +6 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
'ret = ' is missing.

Okay.

Quoted 17 lines

Quoted 17 lines
common/fs_loader.c new file +6 -0
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
fw_priv->offset,
common/fs_loader.c new file +4 -0
+		     firmware_p->size, &actread);
+
+	if (ret) {
+		printf("Error: %d Failed to read %s from flash
%lld != %d.\n",
common/fs_loader.c new file +1 -0
+		      ret, fw_priv->name, actread, firmware_p-
Quoted 1 line

Quoted 7 lines

common/fs_loader.c
Quoted 2 lines
Quoted 1 line
size);
common/fs_loader.c new file +1 -0
+		return ret;
Y
Shouldn't this be 'goto out', do do the umount_ubifs() as in all other
error cases?

okay.

Quoted 13 lines

common/fs_loader.c line 271
Quoted 7 lines
common/fs_loader.c new file +3 -0
+	} 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

Quoted 7 lines

Quoted 2 lines
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.

Quoted 13 lines

Quoted 13 lines
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-
Quoted 2 lines
size", the ret would be -ve too.

Quoted 11 lines

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.

Quoted 12 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

Lothar Waßmann

Lothar Waßmann

2017-12-21 15:08 UTC #9 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

Hi,

Quoted 371 lines · Tien Fong

Quoted 370 lines · Lothar Waßmann

Hi,

Quoted 367 lines · Tien Fong

Quoted 315 lines · Lothar Waßmann

Hi,

Quoted 310 lines · On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at intel.com wrote:

From: Tien Fong Chee <[email protected]>

Signed-off-by Tien Fong Chee <[email protected]>

 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

common/Makefile +0 -0

index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o

obj-y += command.o
obj-y += s_record.o
obj-y += xyzModem.o

common/Makefile +1 -0

+obj-y += fs_loader.o

common/fs_loader.c new file +21 -0

new file mode 100644
index 0000000..ddfce58
@@ -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

common/fs_loader.c new file +38 -0

+};
+
+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");

common/fs_loader.c new file +45 -0

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

common/fs_loader.c new file +5 -0

+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) &&

defined(CONFIG_SPL_BUILD)

common/fs_loader.c new file +11 -0

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

common/fs_loader.c new file +19 -0

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

common/fs_loader.c new file +2 -0

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

FS_TYPE_ANY);

common/fs_loader.c new file +2 -0

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

Quoted 1 line

devpart,

FS_TYPE_ANY);

common/fs_loader.c new file +3 -0

+	} else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi", NULL,

FS_TYPE_UBIFS);

common/fs_loader.c new file +4 -0

+		else
+			ret = -ENODEV;
+	} else {
+		printf("Error: unsupported location

storage.\n");

common/fs_loader.c new file +10 -0

+		return -ENODEV;
+	}
+
+	if (ret)
+		printf("Error: could not access storage.\n");
+
+	return ret;
+}
+
+static int init_storage_device(struct device_location

*location)

common/fs_loader.c new file +16 -0

+{
+	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");

common/fs_loader.c new file +13 -0

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

common/fs_loader.c new file +8 -0

+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware

**firmware_p,

common/fs_loader.c new file +1 -0

+				     const char *name, void

*dbuf,

common/fs_loader.c new file +13 -0

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

common/fs_loader.c new file +6 -0

+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv) failed\n",

__func__);

common/fs_loader.c new file +16 -0

+		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

common/fs_loader.c new file +1 -0

+				      struct firmware

*firmware_p)

common/fs_loader.c new file +9 -0

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

Quoted 9 lines

common/fs_loader.c line 256
Quoted 7 lines
common/fs_loader.c new file +6 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
'ret = ' is missing.

Okay.

Quoted 17 lines

Quoted 17 lines
common/fs_loader.c new file +6 -0
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
fw_priv->offset,
common/fs_loader.c new file +4 -0
+		     firmware_p->size, &actread);
+
+	if (ret) {
+		printf("Error: %d Failed to read %s from flash
%lld != %d.\n",
common/fs_loader.c new file +1 -0
+		      ret, fw_priv->name, actread, firmware_p-
Quoted 1 line

Quoted 7 lines

common/fs_loader.c
Quoted 2 lines
Quoted 1 line
size);
common/fs_loader.c new file +1 -0
+		return ret;
Y
Shouldn't this be 'goto out', do do the umount_ubifs() as in all other
error cases?

okay.

Quoted 13 lines

common/fs_loader.c line 271
Quoted 7 lines
common/fs_loader.c new file +3 -0
+	} 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

Quoted 9 lines

Quoted 5 lines
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!

Quoted 24 lines

Quoted 13 lines
Quoted 13 lines
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-
Quoted 2 lines
size", the ret would be -ve too.
Quoted 11 lines
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.

Quoted 3 lines

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.

Quoted 18 lines

Quoted 12 lines
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.
Quoted 6 lines
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!

Quoted 4 lines

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

Chee

2017-12-22 01:43 UTC #10 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

Quoted 416 lines · Lothar Waßmann

Hi,
Quoted 408 lines · Tien Fong
Quoted 397 lines · Lothar Waßmann
Hi,
Quoted 393 lines · Tien Fong
Quoted 333 lines · Lothar Waßmann
Hi,
Quoted 327 lines · On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at intel.com wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]>
---
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
common/Makefile +0 -0
index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
obj-y += command.o
obj-y += s_record.o
obj-y += xyzModem.o
common/Makefile +1 -0
+obj-y += fs_loader.o
common/fs_loader.c new file +21 -0
new file mode 100644
index 0000000..ddfce58
@@ -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
*/
common/fs_loader.c new file +38 -0
+};
+
+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");
common/fs_loader.c new file +14 -0
+	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");
common/fs_loader.c new file +31 -0
+	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");
common/fs_loader.c new file +5 -0
+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) &&
defined(CONFIG_SPL_BUILD)
common/fs_loader.c new file +11 -0
+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);
common/fs_loader.c new file +19 -0
+		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-
Quoted 1 line
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +2 -0
+	} else if (!strcmp("usb", location->name)) {
+		ret = fs_set_blk_dev("usb", location-
Quoted 1 line
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +2 -0
+	} else if (!strcmp("sata", location->name)) {
+		ret = fs_set_blk_dev("sata", location-
Quoted 2 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +3 -0
+	} else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi", NULL,
FS_TYPE_UBIFS);
common/fs_loader.c new file +4 -0
+		else
+			ret = -ENODEV;
+	} else {
+		printf("Error: unsupported location
storage.\n");
common/fs_loader.c new file +5 -0
+		return -ENODEV;
+	}
+
+	if (ret)
+		printf("Error: could not access
storage.\n");
common/fs_loader.c new file +5 -0
+
+	return ret;
+}
+
+static int init_storage_device(struct device_location
*location)
common/fs_loader.c new file +16 -0
+{
+	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");
common/fs_loader.c new file +11 -0
+		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++) {
common/fs_loader.c new file +1 -0
+		if (!strcmp(default_locations[i].name,
name))
common/fs_loader.c new file +1 -0
+			default_locations[i].devpart =
devpart;
common/fs_loader.c new file +8 -0
+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware
**firmware_p,
common/fs_loader.c new file +1 -0
+				     const char *name,
void
*dbuf,
common/fs_loader.c new file +1 -0
+				     size_t size, u32
offset)
common/fs_loader.c new file +12 -0
+{
+	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__);
common/fs_loader.c new file +6 -0
+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv)
failed\n",
__func__);
common/fs_loader.c new file +16 -0
+		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,
common/fs_loader.c new file +1 -0
+				      struct firmware
*firmware_p)
common/fs_loader.c new file +9 -0
+{
+	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);
Quoted 9 lines
common/fs_loader.c line 256
Quoted 7 lines
common/fs_loader.c new file +6 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
'ret = ' is missing.
Okay.
Quoted 23 lines
Quoted 23 lines
common/fs_loader.c new file +6 -0
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name, (ulong)firmware_p-
Quoted 1 line
data,
fw_priv->offset,
common/fs_loader.c new file +4 -0
+		     firmware_p->size, &actread);
+
+	if (ret) {
+		printf("Error: %d Failed to read %s from
flash
%lld != %d.\n",
common/fs_loader.c new file +1 -0
+		      ret, fw_priv->name, actread,
firmware_p-
Quoted 2 lines
Quoted 8 lines
common/fs_loader.c
Quoted 2 lines
Quoted 1 line
size);
common/fs_loader.c new file +1 -0
+		return ret;
Y
Shouldn't this be 'goto out', do do the umount_ubifs() as in all
other
error cases?
okay.
Quoted 15 lines
common/fs_loader.c line 271
Quoted 9 lines
common/fs_loader.c new file +3 -0
+	} 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
Quoted 9 lines
Quoted 3 lines
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.
Quoted 5 lines
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.

Quoted 9 lines

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.

Quoted 34 lines

Quoted 34 lines
Quoted 34 lines
Quoted 4 lines
Quoted 15 lines
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-
Quoted 3 lines
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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 33 lines

Quoted 27 lines
Quoted 19 lines
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.
Quoted 8 lines
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!
Quoted 6 lines
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.

Quoted 6 lines

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.

Quoted 3 lines

Lothar Waßmann

Lothar Waßmann

2017-12-22 07:44 UTC #11 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

Hi,

Quoted 509 lines · Tien Fong

Quoted 416 lines · Lothar Waßmann
Hi,
Quoted 408 lines · Tien Fong
Quoted 397 lines · Lothar Waßmann
Hi,
Quoted 393 lines · Tien Fong
Quoted 333 lines · Lothar Waßmann
Hi,
Quoted 327 lines · On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at intel.com wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]>
---
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
common/Makefile +0 -0
index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
obj-y += command.o
obj-y += s_record.o
obj-y += xyzModem.o
common/Makefile +1 -0
+obj-y += fs_loader.o
common/fs_loader.c new file +21 -0
new file mode 100644
index 0000000..ddfce58
@@ -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
*/
common/fs_loader.c new file +38 -0
+};
+
+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");
common/fs_loader.c new file +14 -0
+	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");
common/fs_loader.c new file +31 -0
+	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");
common/fs_loader.c new file +5 -0
+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) &&
defined(CONFIG_SPL_BUILD)
common/fs_loader.c new file +11 -0
+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);
common/fs_loader.c new file +19 -0
+		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-
Quoted 1 line
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +2 -0
+	} else if (!strcmp("usb", location->name)) {
+		ret = fs_set_blk_dev("usb", location-
Quoted 1 line
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +2 -0
+	} else if (!strcmp("sata", location->name)) {
+		ret = fs_set_blk_dev("sata", location-
Quoted 2 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +3 -0
+	} else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi", NULL,
FS_TYPE_UBIFS);
common/fs_loader.c new file +4 -0
+		else
+			ret = -ENODEV;
+	} else {
+		printf("Error: unsupported location
storage.\n");
common/fs_loader.c new file +5 -0
+		return -ENODEV;
+	}
+
+	if (ret)
+		printf("Error: could not access
storage.\n");
common/fs_loader.c new file +5 -0
+
+	return ret;
+}
+
+static int init_storage_device(struct device_location
*location)
common/fs_loader.c new file +16 -0
+{
+	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");
common/fs_loader.c new file +11 -0
+		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++) {
common/fs_loader.c new file +1 -0
+		if (!strcmp(default_locations[i].name,
name))
common/fs_loader.c new file +1 -0
+			default_locations[i].devpart =
devpart;
common/fs_loader.c new file +8 -0
+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware
**firmware_p,
common/fs_loader.c new file +1 -0
+				     const char *name,
void
*dbuf,
common/fs_loader.c new file +1 -0
+				     size_t size, u32
offset)
common/fs_loader.c new file +12 -0
+{
+	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__);
common/fs_loader.c new file +6 -0
+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv)
failed\n",
__func__);
common/fs_loader.c new file +16 -0
+		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,
common/fs_loader.c new file +1 -0
+				      struct firmware
*firmware_p)
common/fs_loader.c new file +9 -0
+{
+	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);
Quoted 9 lines
common/fs_loader.c
Quoted 7 lines
common/fs_loader.c new file +6 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
'ret = ' is missing.
Okay.
Quoted 23 lines
Quoted 23 lines
common/fs_loader.c new file +6 -0
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name, (ulong)firmware_p-
Quoted 1 line
data,
fw_priv->offset,
common/fs_loader.c new file +4 -0
+		     firmware_p->size, &actread);
+
+	if (ret) {
+		printf("Error: %d Failed to read %s from
flash
%lld != %d.\n",
common/fs_loader.c new file +1 -0
+		      ret, fw_priv->name, actread,
firmware_p-
Quoted 2 lines
Quoted 8 lines
common/fs_loader.c
Quoted 2 lines
Quoted 1 line
size);
common/fs_loader.c new file +1 -0
+		return ret;
Y
Shouldn't this be 'goto out', do do the umount_ubifs() as in all
other
error cases?
okay.
Quoted 15 lines
common/fs_loader.c
Quoted 9 lines
common/fs_loader.c new file +3 -0
+	} 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
Quoted 9 lines
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 9 lines
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.
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
Quoted 4 lines
Quoted 15 lines
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-
Quoted 3 lines
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 33 lines
Quoted 27 lines
Quoted 19 lines
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.
Quoted 8 lines
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!
Quoted 6 lines
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.
Quoted 6 lines
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).

Quoted 10 lines

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

Chee

2017-12-22 08:04 UTC #12 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

Quoted 609 lines · Lothar Waßmann

Hi,
Quoted 581 lines · Tien Fong
Quoted 455 lines · Lothar Waßmann
Hi,
Quoted 445 lines · Tien Fong
Quoted 434 lines · Lothar Waßmann
Hi,
Quoted 425 lines · Tien Fong
Quoted 354 lines · Lothar Waßmann
Hi,
Quoted 346 lines · On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at intel.c om wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]
Quoted 1 line
---
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
common/Makefile +0 -0
index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
obj-y += command.o
obj-y += s_record.o
obj-y += xyzModem.o
common/Makefile +1 -0
+obj-y += fs_loader.o
common/fs_loader.c new file +2 -0
new file mode 100644
index 0000000..ddfce58
@@ -0,0 +1,311 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com
Quoted 1 line
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
common/fs_loader.c new file +16 -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
*/
common/fs_loader.c new file +38 -0
+};
+
+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");
common/fs_loader.c new file +14 -0
+	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");
common/fs_loader.c new file +6 -0
+	return -ENOSYS;
+}
+#endif
+
+#ifdef CONFIG_CMD_UBIFS
+static int mount_ubifs(struct device_location
*location)
common/fs_loader.c new file +5 -0
+{
+	int ret;
+	char cmd[32];
+
+	sprintf(cmd, "ubi part %s", location-
Quoted 1 line
mtdpart);
common/fs_loader.c new file +6 -0
+
+	ret = run_command(cmd, 0);
+	if (ret)
+		return ret;
+
+	sprintf(cmd, "ubifsmount %s", location-
Quoted 1 line
ubivol);
common/fs_loader.c new file +12 -0
+
+	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)
common/fs_loader.c new file +2 -0
+{
+	printf("Error: Cannot load image: no UBIFS
support\n");
common/fs_loader.c new file +5 -0
+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) &&
defined(CONFIG_SPL_BUILD)
common/fs_loader.c new file +3 -0
+static int init_mmc(void)
+{
+	/* Just for the case MMC is not yet
initialized */
common/fs_loader.c new file +8 -0
+	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);
common/fs_loader.c new file +14 -0
+		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)
common/fs_loader.c new file +5 -0
+{
+	int ret;
+
+	if (!strcmp("mmc", location->name)) {
+		ret = fs_set_blk_dev("mmc", location-
Quoted 2 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +2 -0
+	} else if (!strcmp("usb", location->name)) {
+		ret = fs_set_blk_dev("usb", location-
Quoted 2 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +2 -0
+	} else if (!strcmp("sata", location->name)) {
+		ret = fs_set_blk_dev("sata", location-
Quoted 3 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +3 -0
+	} else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi",
NULL,
FS_TYPE_UBIFS);
common/fs_loader.c new file +4 -0
+		else
+			ret = -ENODEV;
+	} else {
+		printf("Error: unsupported location
storage.\n");
common/fs_loader.c new file +5 -0
+		return -ENODEV;
+	}
+
+	if (ret)
+		printf("Error: could not access
storage.\n");
common/fs_loader.c new file +5 -0
+
+	return ret;
+}
+
+static int init_storage_device(struct device_location
*location)
common/fs_loader.c new file +16 -0
+{
+	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");
common/fs_loader.c new file +7 -0
+		ret = -ENODEV;
+	}
+
+	return ret;
+}
+
+static void set_storage_devpart(char *name, char
*devpart)
common/fs_loader.c new file +4 -0
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(default_locations);
i++) {
common/fs_loader.c new file +1 -0
+		if (!strcmp(default_locations[i].name,
name))
common/fs_loader.c new file +1 -0
+			default_locations[i].devpart =
devpart;
common/fs_loader.c new file +8 -0
+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware
**firmware_p,
common/fs_loader.c new file +1 -0
+				     const char *name,
void
*dbuf,
common/fs_loader.c new file +1 -0
+				     size_t size, u32
offset)
common/fs_loader.c new file +12 -0
+{
+	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__);
common/fs_loader.c new file +6 -0
+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv)
failed\n",
__func__);
common/fs_loader.c new file +16 -0
+		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,
common/fs_loader.c new file +1 -0
+				      struct firmware
*firmware_p)
common/fs_loader.c new file +9 -0
+{
+	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);
Quoted 9 lines
common/fs_loader.c
Quoted 7 lines
common/fs_loader.c new file +6 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
'ret = ' is missing.
Okay.
Quoted 29 lines
Quoted 29 lines
common/fs_loader.c new file +6 -0
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name,
(ulong)firmware_p-
Quoted 2 lines
data,
fw_priv->offset,
common/fs_loader.c new file +4 -0
+		     firmware_p->size, &actread);
+
+	if (ret) {
+		printf("Error: %d Failed to read %s
from
flash
%lld != %d.\n",
common/fs_loader.c new file +1 -0
+		      ret, fw_priv->name, actread,
firmware_p-
Quoted 3 lines
Quoted 9 lines
common/fs_loader.c
Quoted 2 lines
Quoted 1 line
size);
common/fs_loader.c new file +1 -0
+		return ret;
Y
Shouldn't this be 'goto out', do do the umount_ubifs() as in
all
other
error cases?
okay.
Quoted 7 lines
Quoted 7 lines
Quoted 11 lines
common/fs_loader.c
Quoted 4 lines
common/fs_loader.c new file +3 -0
+	} 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
Quoted 4 lines
May be i misunderstand on ur previosly comment. There is not
much
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 11 lines
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.
Quoted 52 lines
Quoted 48 lines
Quoted 39 lines
Quoted 17 lines
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
Quoted 12 lines
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-
Quoted 4 lines
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
Quoted 9 lines
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.
Quoted 4 lines
The size here refer to the size of firmware or chunk of firmware to
read when the buffer is smaller than firmware.
Quoted 6 lines
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.
Quoted 45 lines
Quoted 37 lines
Quoted 26 lines
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.
Quoted 11 lines
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!
Quoted 8 lines
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.
Quoted 7 lines
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).
Quoted 13 lines
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.

Quoted 8 lines

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?

Quoted 2 lines

Lothar Waßmann

Lothar Waßmann

2017-12-22 08:47 UTC #13 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

Hi,

Quoted 618 lines · Tien Fong

Quoted 609 lines · Lothar Waßmann
Hi,
Quoted 581 lines · Tien Fong
Quoted 455 lines · Lothar Waßmann
Hi,
Quoted 445 lines · Tien Fong
Quoted 434 lines · Lothar Waßmann
Hi,
Quoted 425 lines · Tien Fong
Quoted 354 lines · Lothar Waßmann
Hi,
Quoted 346 lines · On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at intel.c om wrote:
From: Tien Fong Chee <[email protected]>
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 <[email protected]
Quoted 1 line
---
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
common/Makefile +0 -0
index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
obj-y += command.o
obj-y += s_record.o
obj-y += xyzModem.o
common/Makefile +1 -0
+obj-y += fs_loader.o
common/fs_loader.c new file +2 -0
new file mode 100644
index 0000000..ddfce58
@@ -0,0 +1,311 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com
Quoted 1 line
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
common/fs_loader.c new file +16 -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
*/
common/fs_loader.c new file +38 -0
+};
+
+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");
common/fs_loader.c new file +14 -0
+	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");
common/fs_loader.c new file +6 -0
+	return -ENOSYS;
+}
+#endif
+
+#ifdef CONFIG_CMD_UBIFS
+static int mount_ubifs(struct device_location
*location)
common/fs_loader.c new file +5 -0
+{
+	int ret;
+	char cmd[32];
+
+	sprintf(cmd, "ubi part %s", location-
Quoted 1 line
mtdpart);
common/fs_loader.c new file +6 -0
+
+	ret = run_command(cmd, 0);
+	if (ret)
+		return ret;
+
+	sprintf(cmd, "ubifsmount %s", location-
Quoted 1 line
ubivol);
common/fs_loader.c new file +12 -0
+
+	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)
common/fs_loader.c new file +2 -0
+{
+	printf("Error: Cannot load image: no UBIFS
support\n");
common/fs_loader.c new file +5 -0
+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) &&
defined(CONFIG_SPL_BUILD)
common/fs_loader.c new file +3 -0
+static int init_mmc(void)
+{
+	/* Just for the case MMC is not yet
initialized */
common/fs_loader.c new file +8 -0
+	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);
common/fs_loader.c new file +14 -0
+		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)
common/fs_loader.c new file +5 -0
+{
+	int ret;
+
+	if (!strcmp("mmc", location->name)) {
+		ret = fs_set_blk_dev("mmc", location-
Quoted 2 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +2 -0
+	} else if (!strcmp("usb", location->name)) {
+		ret = fs_set_blk_dev("usb", location-
Quoted 2 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +2 -0
+	} else if (!strcmp("sata", location->name)) {
+		ret = fs_set_blk_dev("sata", location-
Quoted 3 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +3 -0
+	} else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi",
NULL,
FS_TYPE_UBIFS);
common/fs_loader.c new file +4 -0
+		else
+			ret = -ENODEV;
+	} else {
+		printf("Error: unsupported location
storage.\n");
common/fs_loader.c new file +5 -0
+		return -ENODEV;
+	}
+
+	if (ret)
+		printf("Error: could not access
storage.\n");
common/fs_loader.c new file +5 -0
+
+	return ret;
+}
+
+static int init_storage_device(struct device_location
*location)
common/fs_loader.c new file +16 -0
+{
+	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");
common/fs_loader.c new file +7 -0
+		ret = -ENODEV;
+	}
+
+	return ret;
+}
+
+static void set_storage_devpart(char *name, char
*devpart)
common/fs_loader.c new file +4 -0
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(default_locations);
i++) {
common/fs_loader.c new file +1 -0
+		if (!strcmp(default_locations[i].name,
name))
common/fs_loader.c new file +1 -0
+			default_locations[i].devpart =
devpart;
common/fs_loader.c new file +8 -0
+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware
**firmware_p,
common/fs_loader.c new file +1 -0
+				     const char *name,
void
*dbuf,
common/fs_loader.c new file +1 -0
+				     size_t size, u32
offset)
common/fs_loader.c new file +12 -0
+{
+	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__);
common/fs_loader.c new file +6 -0
+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv)
failed\n",
__func__);
common/fs_loader.c new file +16 -0
+		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,
common/fs_loader.c new file +1 -0
+				      struct firmware
*firmware_p)
common/fs_loader.c new file +9 -0
+{
+	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);
Quoted 9 lines
common/fs_loader.c
Quoted 7 lines
common/fs_loader.c new file +6 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
'ret = ' is missing.
Okay.
Quoted 29 lines
Quoted 29 lines
common/fs_loader.c new file +6 -0
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name,
(ulong)firmware_p-
Quoted 2 lines
data,
fw_priv->offset,
common/fs_loader.c new file +4 -0
+		     firmware_p->size, &actread);
+
+	if (ret) {
+		printf("Error: %d Failed to read %s
from
flash
%lld != %d.\n",
common/fs_loader.c new file +1 -0
+		      ret, fw_priv->name, actread,
firmware_p-
Quoted 3 lines
Quoted 9 lines
common/fs_loader.c
Quoted 2 lines
Quoted 1 line
size);
common/fs_loader.c new file +1 -0
+		return ret;
Y
Shouldn't this be 'goto out', do do the umount_ubifs() as in
all
other
error cases?
okay.
Quoted 7 lines
Quoted 7 lines
Quoted 11 lines
common/fs_loader.c
Quoted 4 lines
common/fs_loader.c new file +3 -0
+	} 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
Quoted 4 lines
May be i misunderstand on ur previosly comment. There is not
much
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 11 lines
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.
Quoted 52 lines
Quoted 48 lines
Quoted 39 lines
Quoted 17 lines
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
Quoted 12 lines
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-
Quoted 4 lines
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
Quoted 9 lines
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.
Quoted 4 lines
The size here refer to the size of firmware or chunk of firmware to
read when the buffer is smaller than firmware.
Quoted 6 lines
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.
Quoted 45 lines
Quoted 37 lines
Quoted 26 lines
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.
Quoted 11 lines
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!
Quoted 8 lines
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.
Quoted 7 lines
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).
Quoted 13 lines
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.
Quoted 8 lines
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.

Quoted 4 lines

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?
Quoted 1 line

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

Chee

2017-12-22 09:21 UTC #14 lore raw

PATCH v5 2/2 common: Generic firmware loader for file system

Quoted 735 lines · Lothar Waßmann

Hi,
Quoted 726 lines · Tien Fong
Quoted 713 lines · Lothar Waßmann
Hi,
Quoted 681 lines · Tien Fong
Quoted 525 lines · Lothar Waßmann
Hi,
Quoted 503 lines · Tien Fong
Quoted 489 lines · Lothar Waßmann
Hi,
Quoted 478 lines · Tien Fong
Quoted 395 lines · Lothar Waßmann
Hi,
On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee at int el.c
om
Quoted 385 lines · wrote:
From: Tien Fong Chee <[email protected]>
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
Quoted 2 lines
---
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
common/Makefile +0 -0
index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
obj-y += command.o
obj-y += s_record.o
obj-y += xyzModem.o
common/Makefile +1 -0
+obj-y += fs_loader.o
common/fs_loader.c new file +2 -0
new file mode 100644
index 0000000..ddfce58
@@ -0,0 +1,311 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel
.com
Quoted 2 lines
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
common/fs_loader.c new file +16 -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
*/
common/fs_loader.c new file +3 -0
+};
+
+static struct device_location default_locations[]
= {
common/fs_loader.c new file +35 -0
+	{
+		.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");
common/fs_loader.c new file +14 -0
+	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");
common/fs_loader.c new file +6 -0
+	return -ENOSYS;
+}
+#endif
+
+#ifdef CONFIG_CMD_UBIFS
+static int mount_ubifs(struct device_location
*location)
common/fs_loader.c new file +5 -0
+{
+	int ret;
+	char cmd[32];
+
+	sprintf(cmd, "ubi part %s", location-
Quoted 2 lines
mtdpart);
common/fs_loader.c new file +6 -0
+
+	ret = run_command(cmd, 0);
+	if (ret)
+		return ret;
+
+	sprintf(cmd, "ubifsmount %s", location-
Quoted 2 lines
ubivol);
common/fs_loader.c new file +12 -0
+
+	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)
common/fs_loader.c new file +2 -0
+{
+	printf("Error: Cannot load image: no UBIFS
support\n");
common/fs_loader.c new file +5 -0
+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) &&
defined(CONFIG_SPL_BUILD)
common/fs_loader.c new file +3 -0
+static int init_mmc(void)
+{
+	/* Just for the case MMC is not yet
initialized */
common/fs_loader.c new file +4 -0
+	struct mmc *mmc = NULL;
+	int err;
+
+	spl_mmc_find_device(&mmc,
spl_boot_device());
common/fs_loader.c new file +4 -0
+
+	err = mmc_init(mmc);
+	if (err) {
+		printf("spl: mmc init failed with
error:
%d\n",
err);
common/fs_loader.c new file +9 -0
+		return err;
+	}
+
+	return err;
+}
+#else
+static int init_mmc(void)
+{
+	/* Expect somewhere already initialize MMC
*/
common/fs_loader.c new file +5 -0
+	return 0;
+}
+#endif
+
+static int select_fs_dev(struct device_location
*location)
common/fs_loader.c new file +5 -0
+{
+	int ret;
+
+	if (!strcmp("mmc", location->name)) {
+		ret = fs_set_blk_dev("mmc",
location-
Quoted 3 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +1 -0
+	} else if (!strcmp("usb", location->name)) 
{
common/fs_loader.c new file +1 -0
+		ret = fs_set_blk_dev("usb",
location-
Quoted 3 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +1 -0
+	} else if (!strcmp("sata", location-
Quoted 1 line
name)) {
common/fs_loader.c new file +1 -0
+		ret = fs_set_blk_dev("sata",
location-
Quoted 4 lines
devpart,
FS_TYPE_ANY);
common/fs_loader.c new file +1 -0
+	} else if (!strcmp("ubi", location->name)) 
{
common/fs_loader.c new file +2 -0
+		if (location->ubivol != NULL)
+			ret =
fs_set_blk_dev("ubi",
NULL,
FS_TYPE_UBIFS);
common/fs_loader.c new file +4 -0
+		else
+			ret = -ENODEV;
+	} else {
+		printf("Error: unsupported
location
storage.\n");
common/fs_loader.c new file +5 -0
+		return -ENODEV;
+	}
+
+	if (ret)
+		printf("Error: could not access
storage.\n");
common/fs_loader.c new file +5 -0
+
+	return ret;
+}
+
+static int init_storage_device(struct
device_location
*location)
common/fs_loader.c new file +6 -0
+{
+	int ret;
+
+	if (!strcmp("mmc", location->name)) {
+		ret = init_mmc();
+	} else if (!strcmp("sata", location-
Quoted 1 line
name)) {
common/fs_loader.c new file +5 -0
+		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
*/
common/fs_loader.c new file +1 -0
+	} else if (!strcmp("usb", location->name)) 
{
common/fs_loader.c new file +4 -0
+		ret = init_usb();
+#endif
+	} else {
+		printf("Error: no supported
storage
device
is
available.\n");
common/fs_loader.c new file +7 -0
+		ret = -ENODEV;
+	}
+
+	return ret;
+}
+
+static void set_storage_devpart(char *name, char
*devpart)
common/fs_loader.c new file +4 -0
+{
+	size_t i;
+
+	for (i = 0; i <
ARRAY_SIZE(default_locations);
i++) {
common/fs_loader.c new file +1 -0
+		if
(!strcmp(default_locations[i].name,
name))
common/fs_loader.c new file +1 -0
+			default_locations[i].devpa
rt =
devpart;
common/fs_loader.c new file +8 -0
+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct
firmware
**firmware_p,
common/fs_loader.c new file +1 -0
+				     const char
*name,
void
*dbuf,
common/fs_loader.c new file +1 -0
+				     size_t size,
u32
offset)
common/fs_loader.c new file +12 -0
+{
+	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__);
common/fs_loader.c new file +6 -0
+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv)
failed\n",
__func__);
common/fs_loader.c new file +16 -0
+		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,
common/fs_loader.c new file +1 -0
+				      struct
firmware
*firmware_p)
common/fs_loader.c new file +9 -0
+{
+	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-
Quoted 1 line
name,
Quoted 10 lines
common/fs_loader.c
Quoted 8 lines
dev_part);
common/fs_loader.c new file +6 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
'ret = ' is missing.
Okay.
Quoted 35 lines
Quoted 35 lines
common/fs_loader.c new file +6 -0
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name,
(ulong)firmware_p-
Quoted 3 lines
data,
fw_priv->offset,
common/fs_loader.c new file +4 -0
+		     firmware_p->size, &actread);
+
+	if (ret) {
+		printf("Error: %d Failed to read
%s
from
flash
%lld != %d.\n",
common/fs_loader.c new file +1 -0
+		      ret, fw_priv->name, actread,
firmware_p-
Quoted 5 lines
size);
Quoted 9 lines
common/fs_loader.c
Quoted 1 line
common/fs_loader.c new file +1 -0
+		return ret;
Y
Shouldn't this be 'goto out', do do the
umount_ubifs() as
in
all
other
error cases?
okay.
Quoted 9 lines
Quoted 9 lines
Quoted 12 lines
common/fs_loader.c
Quoted 4 lines
common/fs_loader.c new file +3 -0
+	} 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
Quoted 5 lines
May be i misunderstand on ur previosly comment. There is
not
much
Quoted 10 lines
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.
Quoted 8 lines
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-
Quoted 5 lines
Quoted 1 line
size
as
an
error, you should return -EIO only in that case!
Okay.
Quoted 65 lines
Quoted 61 lines
Quoted 52 lines
Quoted 26 lines
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,
Quoted 13 lines
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-
Quoted 5 lines
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
Quoted 9 lines
actread
!=
size.
Since you treated actread != size as an error in your original
patch
I
assumed the latter case.
Quoted 4 lines
The size here refer to the size of firmware or chunk of firmware to
read when the buffer is smaller than firmware.
Quoted 7 lines
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.
Quoted 56 lines
Quoted 47 lines
Quoted 35 lines
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.
Quoted 12 lines
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!
Quoted 9 lines
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.
Quoted 8 lines
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
Quoted 9 lines
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.
Quoted 11 lines
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.
Quoted 6 lines
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?
Quoted 2 lines

Quoted 7 lines

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.

Quoted 2 lines

Lothar Waßmann