On Sel, 2017-12-19 at 12:21 +0100, Lothar Waßmann wrote:
Hi,
On Tue, 19 Dec 2017 10:31:13 +0000 Chee, Tien Fong wrote:
On Isn, 2017-12-18 at 08:39 +0100, Lothar Waßmann wrote:
Hi,
On Mon, 18 Dec 2017 13:10:56 +0800 tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 311 +++++++++++++++++++++++++++++++++++++++++++++ doc/README.firmware_loader | 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
[...]
diff --git a/common/fs_loader.c b/common/fs_loader.c new file mode 100644 index 0000000..81cf5d6 --- /dev/null +++ b/common/fs_loader.c
[...]
+/*
- 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__);
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?
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.
to *firmware_p.
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)
Okay.
- fw_get_filesystem_firmware - load firmware into an
allocated buffer
- @location: An array of supported firmware location
- @firmware_p: pointer to firmware image
- @return: size of total read
- -ve when error
- */
+static int fw_get_filesystem_firmware(struct device_location *location,
struct firmware*firmware_p) +{
- struct firmware_priv *fw_priv = NULL;
- loff_t actread;
- char *dev_part;
- int ret;
- dev_part = env_get("FW_DEV_PART");
- if (dev_part)
set_storage_devpart(location->name, dev_part);- ret = init_storage_device(location);
- if (ret)
goto out;- select_fs_dev(location);
- 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.
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.
Lothar Waßmann