Generic firmware loader

PATCH Superseded tien.fong.chee at intel.com 4 participants 8 messages Dec 18, 2017 – Dec 21, 2017

lore

Share & tools

lore

tien.fong.chee at intel.com

author

2017-12-18 05:10 UTC #0 lore raw

v4 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, Lukasz Majewski and Prabhakar Kushwaha in [v3].

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

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

v3 -> v4 changes:
-----------------
- Moved the firmware loader private structure declaration to source file.
- Added README.firmware_loader to doc directory.

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

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 |  77 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +
 6 files changed, 420 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-18 05:10 UTC #1 lore raw

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

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-18 05:10 UTC #2 lore raw

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

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 +77 -0

77 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-18 07:39 UTC #3 lore raw

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

Hi,

Quoted 18 lines · On Mon, 18 Dec 2017 13:10:56 +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 |  77 +++++++++++
 include/fs_loader.h        |  28 ++++
 4 files changed, 417 insertions(+)
 create mode 100644 common/fs_loader.c
 create mode 100644 doc/README.firmware_loader
 create mode 100644 include/fs_loader.h

[...]

Quoted 5 lines

common/fs_loader.c new file +0 -0
new file mode 100644
index 0000000..81cf5d6

[...]

Quoted 18 lines

common/fs_loader.c new file +18 -0

+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware **firmware_p,
+				     const char *name, void *dbuf,
+				     size_t size, u32 offset)
+{
+	struct firmware *firmware = NULL;
+	struct firmware_priv *fw_priv = NULL;
+
+	*firmware_p = NULL;
+
+	if (!name || name[0] == '\0')
+		return -EINVAL;
+
+	*firmware_p = firmware = calloc(1, sizeof(*firmware));
+

common/fs_loader.c line 220

Quoted 10 lines

common/fs_loader.c new file +10 -0

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

What about freeing 'firmware' and NULLing *firmware_p here? Or better, do the assignment of *firmware_p at the end.

Quoted 31 lines

common/fs_loader.c new file +31 -0

+	}
+
+	fw_priv->name = name;
+	fw_priv->offset = offset;
+	firmware->data = dbuf;
+	firmware->size = size;
+	firmware->priv = fw_priv;
+
+	return 0;
+}
+
+/*
+ * fw_get_filesystem_firmware - load firmware into an allocated buffer
+ * @location: An array of supported firmware location
+ * @firmware_p: pointer to firmware image
+ *
+ * @return: size of total read
+ *	    -ve when error
+ */
+static int fw_get_filesystem_firmware(struct device_location *location,
+				      struct firmware *firmware_p)
+{
+	struct firmware_priv *fw_priv = NULL;
+	loff_t actread;
+	char *dev_part;
+	int ret;
+
+	dev_part = env_get("FW_DEV_PART");
+	if (dev_part)
+		set_storage_devpart(location->name, dev_part);
+

common/fs_loader.c line 261

Quoted 10 lines

common/fs_loader.c new file +10 -0

+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
+	if (ret)
+		goto out;
+
+	fw_priv = (struct firmware_priv *)firmware_p->priv;
+

useless type cast.

common/fs_loader.c line 268

Quoted 7 lines

common/fs_loader.c new file +7 -0

+	ret = fs_read(fw_priv->name, (ulong)firmware_p->data, fw_priv->offset,
+		     firmware_p->size, &actread);
+
+	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);
+		return -EPERM;

Quoting myself from an earlier mail
(20171212091442.2f6826fe at karo-electronics.de):

|That's definitely not the right return code in this situation.
|If 'ret' is != 0 you should return 'ret', otherwise EIO is more
|appropriate here.

Lothar WaÃŸmann

Chee

2017-12-19 10:31 UTC #4 lore raw

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

Quoted 55 lines · Lothar Waßmann

Hi,
Quoted 20 lines · On Mon, 18 Dec 2017 13:10:56 +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 |  77 +++++++++++
include/fs_loader.h        |  28 ++++
 4 files changed, 417 insertions(+)
create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h
[...]
Quoted 6 lines
common/fs_loader.c new file +0 -0
new file mode 100644
index 0000000..81cf5d6
[...]
Quoted 24 lines
common/fs_loader.c new file +20 -0
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware **firmware_p,
+				     const char *name, void *dbuf,
+				     size_t size, u32 offset)
+{
+	struct firmware *firmware = NULL;
+	struct firmware_priv *fw_priv = NULL;
+
+	*firmware_p = NULL;
+
+	if (!name || name[0] == '\0')
+		return -EINVAL;
+
+	*firmware_p = firmware = calloc(1, sizeof(*firmware));
+
+	if (!firmware) {
+		printf("%s: calloc(struct firmware) failed\n",
__func__);
common/fs_loader.c new file +2 -0
+		return -ENOMEM;
+	}

Quoted 8 lines

common/fs_loader.c
Quoted 7 lines
common/fs_loader.c new file +5 -0
+
+	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 +1 -0
+		return -ENOMEM;
What about freeing 'firmware' and NULLing *firmware_p here?

There is no "freeing" support in U-Boot. I can assign NULL to *firmware_p.

Quoted 1 line

Or better, do the assignment of *firmware_p at the end.

Are you means switch the location between *firmware_p and fw_priv in calloc?

Quoted 38 lines

Quoted 38 lines
common/fs_loader.c new file +13 -0
+	}
+
+	fw_priv->name = name;
+	fw_priv->offset = offset;
+	firmware->data = dbuf;
+	firmware->size = size;
+	firmware->priv = fw_priv;
+
+	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 +14 -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);
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;

Quoted 9 lines

common/fs_loader.c line 261
Quoted 7 lines
common/fs_loader.c new file +7 -0
+
+	select_fs_dev(location);
+	if (ret)
+		goto out;
+
+	fw_priv = (struct firmware_priv *)firmware_p->priv;
+
useless type cast.

I assume you are saying autocast, right? Let me check is there any warning from compiler after removing the cast.

Quoted 9 lines

Quoted 9 lines
common/fs_loader.c new file +1 -0
+	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 || (actread != firmware_p->size)) {
+		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 9 lines

common/fs_loader.c line 268
Quoted 2 lines
Quoted 1 line
size);
common/fs_loader.c new file +1 -0
+		return -EPERM;
Quoting myself from an earlier mail
(20171212091442.2f6826fe at karo-electronics.de):
Quoted 4 lines
That's definitely not the right return code in this situation. If 'ret' is != 0 you should return 'ret', otherwise EIO is more appropriate here.

Sorry for mising out this part. I would change that.

Quoted 2 lines

Lothar Waßmann

Marek Vasut

2017-12-19 11:15 UTC #5 lore raw

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

Quoted 64 lines · Tien Fong

Quoted 55 lines · Lothar Waßmann
Hi,
Quoted 20 lines · On Mon, 18 Dec 2017 13:10:56 +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 |  77 +++++++++++
include/fs_loader.h        |  28 ++++
 4 files changed, 417 insertions(+)
create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h
[...]
Quoted 6 lines
common/fs_loader.c new file +0 -0
new file mode 100644
index 0000000..81cf5d6
[...]
Quoted 24 lines
common/fs_loader.c new file +20 -0
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware **firmware_p,
+				     const char *name, void *dbuf,
+				     size_t size, u32 offset)
+{
+	struct firmware *firmware = NULL;
+	struct firmware_priv *fw_priv = NULL;
+
+	*firmware_p = NULL;
+
+	if (!name || name[0] == '\0')
+		return -EINVAL;
+
+	*firmware_p = firmware = calloc(1, sizeof(*firmware));
+
+	if (!firmware) {
+		printf("%s: calloc(struct firmware) failed\n",
__func__);
common/fs_loader.c new file +2 -0
+		return -ENOMEM;
+	}
Quoted 8 lines
common/fs_loader.c
Quoted 7 lines
common/fs_loader.c new file +5 -0
+
+	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 +1 -0
+		return -ENOMEM;
What about freeing 'firmware' and NULLing *firmware_p here?

Quoted 2 lines

There is no "freeing" support in U-Boot. I can assign NULL to *firmware_p.

Try git grep free maybe ?

Quoted 3 lines

Quoted 1 line
Or better, do the assignment of *firmware_p at the end.
Are you means switch the location between *firmware_p and fw_priv in calloc?

[...

Signature · 2 lines

Best regards,
Marek Vasut

Lothar Waßmann

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

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

In reply to Chee

Hi,

Quoted 64 lines · Tien Fong

Quoted 55 lines · Lothar Waßmann
Hi,
Quoted 20 lines · On Mon, 18 Dec 2017 13:10:56 +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 |  77 +++++++++++
include/fs_loader.h        |  28 ++++
 4 files changed, 417 insertions(+)
create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h
[...]
Quoted 6 lines
common/fs_loader.c new file +0 -0
new file mode 100644
index 0000000..81cf5d6
[...]
Quoted 24 lines
common/fs_loader.c new file +20 -0
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware **firmware_p,
+				     const char *name, void *dbuf,
+				     size_t size, u32 offset)
+{
+	struct firmware *firmware = NULL;
+	struct firmware_priv *fw_priv = NULL;
+
+	*firmware_p = NULL;
+
+	if (!name || name[0] == '\0')
+		return -EINVAL;
+
+	*firmware_p = firmware = calloc(1, sizeof(*firmware));
+
+	if (!firmware) {
+		printf("%s: calloc(struct firmware) failed\n",
__func__);
common/fs_loader.c new file +2 -0
+		return -ENOMEM;
+	}
Quoted 8 lines
common/fs_loader.c
Quoted 7 lines
common/fs_loader.c new file +5 -0
+
+	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 +1 -0
+		return -ENOMEM;
What about freeing 'firmware' and NULLing *firmware_p here?

Quoted 2 lines

There is no "freeing" support in U-Boot. I can assign NULL

How do you come to that conclusion?

Quoted 5 lines

to *firmware_p.
Quoted 1 line
Or better, do the assignment of *firmware_p at the end.
Are you means switch the location between *firmware_p and fw_priv in calloc?

No. I would assign *firmware_p only when everything else was OK, so that there won't be a valid pointer in *firmware_p when the struct firmware it is pointing to has not been set up completely.

I would do like this:
static int _request_firmware_prepare(struct firmware **firmware_p,
const char *name, void *dbuf, size_t size, u32 offset)

{
	struct firmware *firmware;
	struct firmware_priv *fw_priv;

	*firmware_p = NULL;

	if (!name || name[0] == '\0')
		return -EINVAL;

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

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

	fw_priv->name = name;
	fw_priv->offset = offset;
	firmware->data = dbuf;
	firmware->size = size;
	firmware->priv = fw_priv;
	*firmware_p = firmware;

	return 0;
}

so that *firmware_p is only assigned to when everything was OK. (Note, that there is no need to initialize firmware and fw_priv to NULL)

Quoted 33 lines

Quoted 24 lines
Quoted 24 lines
+ * 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 +14 -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);
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
Quoted 9 lines
common/fs_loader.c line 261
Quoted 7 lines
common/fs_loader.c new file +7 -0
+
+	select_fs_dev(location);
+	if (ret)
+		goto out;
+
+	fw_priv = (struct firmware_priv *)firmware_p->priv;
+
useless type cast.

Quoted 3 lines

I assume you are saying autocast, right? Let me check is there any warning from compiler after removing the cast.

In C void pointers can be assigned to any type pointers without need for a cast.

Lothar Waßmann

Chee

2017-12-21 05:39 UTC #7 lore raw

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

Quoted 76 lines · Lothar Waßmann

Hi,
Quoted 73 lines · Tien Fong
Quoted 71 lines · Lothar Waßmann
Hi,
Quoted 22 lines · On Mon, 18 Dec 2017 13:10:56 +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 |  77 +++++++++++
include/fs_loader.h        |  28 ++++
 4 files changed, 417 insertions(+)
create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h
[...]
Quoted 7 lines
common/fs_loader.c new file +0 -0
new file mode 100644
index 0000000..81cf5d6
[...]
Quoted 33 lines
common/fs_loader.c new file +5 -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 +14 -0
+				     size_t size, u32 offset)
+{
+	struct firmware *firmware = NULL;
+	struct firmware_priv *fw_priv = NULL;
+
+	*firmware_p = NULL;
+
+	if (!name || name[0] == '\0')
+		return -EINVAL;
+
+	*firmware_p = firmware = calloc(1, sizeof(*firmware));
+
+	if (!firmware) {
+		printf("%s: calloc(struct firmware) failed\n",
__func__);
common/fs_loader.c new file +7 -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
Quoted 1 line
common/fs_loader.c new file +1 -0
+		return -ENOMEM;
What about freeing 'firmware' and NULLing *firmware_p here?

Quoted 4 lines

Quoted 2 lines
There is no "freeing" support in U-Boot. I can assign NULL
How do you come to that conclusion?

Sorry for misleading because i was tracking the free function until i saw the function direct return when the bit GD_FLG_FULL_MALLOC_INIT was found in gd->flags in beginning of the function long time ago. So, i always assume that memory will always be freed in relocation on most cases.
I will put the free to the firmware in next release.

Quoted 49 lines

Quoted 8 lines
to *firmware_p.
Quoted 2 lines
Or better, do the assignment of *firmware_p at the end.
Are you means switch the location between *firmware_p and fw_priv in
calloc?
No. I would assign *firmware_p only when everything else was OK, so that there won't be a valid pointer in *firmware_p when the struct firmware it is pointing to has not been set up completely.
I would do like this:
static int _request_firmware_prepare(struct firmware **firmware_p,
const char *name, void *dbuf, size_t size, u32 offset)
{
	struct firmware *firmware;
	struct firmware_priv *fw_priv;

	*firmware_p = NULL;

	if (!name || name[0] == '\0')
		return -EINVAL;

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

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

	fw_priv->name = name;
	fw_priv->offset = offset;
	firmware->data = dbuf;
	firmware->size = size;
	firmware->priv = fw_priv;
	*firmware_p = firmware;

	return 0;
}

Quoted 4 lines

so that *firmware_p is only assigned to when everything was OK. (Note, that there is no need to initialize firmware and fw_priv to NULL)

Okay.

Quoted 38 lines

Quoted 38 lines
Quoted 38 lines
Quoted 26 lines
+ * 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 +10 -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);
+
common/fs_loader.c line 261
Quoted 10 lines
common/fs_loader.c new file +10 -0
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
+	if (ret)
+		goto out;
+
+	fw_priv = (struct firmware_priv *)firmware_p->priv;
+
useless type cast.

Quoted 7 lines

Quoted 3 lines
I assume you are saying autocast, right? Let me check is there any warning from compiler after removing the cast.
In C void pointers can be assigned to any type pointers without need for
a cast.

Oo....initially i worry the compiler would trigger warnings when the fw_priv was pass as parameter to fs_read function without casting. However, i have ran the check, and there is no warning from compiler without casting. So, i will remove the cast.

Quoted 2 lines

Lothar Waßmann