Hi,
On Thu, 21 Dec 2017 09:36:41 +0000 Chee, Tien Fong wrote:
On Kha, 2017-12-21 at 09:48 +0100, Lothar Waßmann wrote:
Hi,
On Thu, 21 Dec 2017 15:25:29 +0800 tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 311 +++++++++++++++++++++++++++++++++++++++++++++ doc/README.firmware_loader | 76 +++++++++++ include/fs_loader.h | 28 ++++ 4 files changed, 416 insertions(+) create mode 100644 common/fs_loader.c create mode 100644 doc/README.firmware_loader create mode 100644 include/fs_loader.h
diff --git a/common/Makefile b/common/Makefile index cec506f..2934221 100644 --- a/common/Makefile +++ b/common/Makefile @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o obj-y += command.o obj-y += s_record.o obj-y += xyzModem.o +obj-y += fs_loader.o diff --git a/common/fs_loader.c b/common/fs_loader.c new file mode 100644 index 0000000..ddfce58 --- /dev/null +++ b/common/fs_loader.c @@ -0,0 +1,311 @@ +/*
- Copyright (C) 2017 Intel Corporation <www.intel.com>
- SPDX-License-Identifier: GPL-2.0
- */
+#include <common.h> +#include <errno.h> +#include <fs.h> +#include <fs_loader.h> +#include <nand.h> +#include <sata.h> +#include <spi.h> +#include <spi_flash.h> +#include <spl.h> +#include <linux/string.h> +#include <usb.h>
+struct firmware_priv {
- const char *name; /* Filename */
- u32 offset; /* Offset of reading a file */
+};
+static struct device_location default_locations[] = {
- {
.name = "mmc",.devpart = "0:1",- },
- {
.name = "usb",.devpart = "0:1",- },
- {
.name = "sata",.devpart = "0:1",- },
+};
+/* USB build is not supported yet in SPL */ +#ifndef CONFIG_SPL_BUILD +#ifdef CONFIG_USB_STORAGE +static int init_usb(void) +{
- int err;
- err = usb_init();
- if (err)
return err;+#ifndef CONFIG_DM_USB
- err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
+#endif
- return err;
+} +#else +static int init_usb(void) +{
- printf("Error: Cannot load flash image: no USB
support\n");
- return -ENOSYS;
+} +#endif +#endif
+#ifdef CONFIG_SATA +static int init_storage_sata(void) +{
- return sata_probe(0);
+} +#else +static int init_storage_sata(void) +{
- printf("Error: Cannot load image: no SATA support\n");
- return -ENOSYS;
+} +#endif
+#ifdef CONFIG_CMD_UBIFS +static int mount_ubifs(struct device_location *location) +{
- int ret;
- char cmd[32];
- sprintf(cmd, "ubi part %s", location->mtdpart);
- ret = run_command(cmd, 0);
- if (ret)
return ret;- sprintf(cmd, "ubifsmount %s", location->ubivol);
- ret = run_command(cmd, 0);
- return ret;
+}
+static int umount_ubifs(void) +{
- return run_command("ubifsumount", 0);
+} +#else +static int mount_ubifs(struct device_location *location) +{
- printf("Error: Cannot load image: no UBIFS support\n");
- return -ENOSYS;
+} +#endif
+#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD) +static int init_mmc(void) +{
- /* Just for the case MMC is not yet initialized */
- struct mmc *mmc = NULL;
- int err;
- spl_mmc_find_device(&mmc, spl_boot_device());
- err = mmc_init(mmc);
- if (err) {
printf("spl: mmc init failed with error: %d\n",err);
return err;- }
- return err;
+} +#else +static int init_mmc(void) +{
- /* Expect somewhere already initialize MMC */
- return 0;
+} +#endif
+static int select_fs_dev(struct device_location *location) +{
- int ret;
- if (!strcmp("mmc", location->name)) {
ret = fs_set_blk_dev("mmc", location->devpart,FS_TYPE_ANY);
- } else if (!strcmp("usb", location->name)) {
ret = fs_set_blk_dev("usb", location->devpart,FS_TYPE_ANY);
- } else if (!strcmp("sata", location->name)) {
ret = fs_set_blk_dev("sata", location->devpart,FS_TYPE_ANY);
- } else if (!strcmp("ubi", location->name)) {
if (location->ubivol != NULL)ret = fs_set_blk_dev("ubi", NULL,FS_TYPE_UBIFS);
elseret = -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 isavailable.\n");
ret = -ENODEV;- }
- return ret;
+}
+static void set_storage_devpart(char *name, char *devpart) +{
- size_t i;
- for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
if (!strcmp(default_locations[i].name, name))default_locations[i].devpart = devpart;- }
+}
+/*
- Prepare firmware struct;
- return -ve if fail.
- */
+static int _request_firmware_prepare(struct firmware **firmware_p,
const char *name, void *dbuf,size_t size, u32 offset)+{
- struct firmware *firmware;
- struct firmware_priv *fw_priv;
- *firmware_p = NULL;
- if (!name || name[0] == '\0')
return -EINVAL;- firmware = calloc(1, sizeof(*firmware));
- if (!firmware) {
printf("%s: calloc(struct firmware) failed\n",__func__);
return -ENOMEM;- }
- fw_priv = calloc(1, sizeof(*fw_priv));
- if (!fw_priv) {
printf("%s: calloc(struct fw_priv) failed\n",__func__);
free(firmware);return -ENOMEM;- }
- fw_priv->name = name;
- fw_priv->offset = offset;
- firmware->data = dbuf;
- firmware->size = size;
- firmware->priv = fw_priv;
- *firmware_p = firmware;
- return 0;
+}
+/*
- fw_get_filesystem_firmware - load firmware into an allocated
buffer
- @location: An array of supported firmware location
- @firmware_p: pointer to firmware image
- @return: size of total read
- -ve when error
- */
+static int fw_get_filesystem_firmware(struct device_location *location,
struct firmware *firmware_p)+{
- struct firmware_priv *fw_priv = NULL;
- loff_t actread;
- char *dev_part;
- int ret;
- dev_part = env_get("fw_dev_part");
- if (dev_part)
set_storage_devpart(location->name, dev_part);- ret = init_storage_device(location);
- if (ret)
goto out;- select_fs_dev(location);
'ret = ' is missing.
Okay.
- if (ret)
goto out;- fw_priv = firmware_p->priv;
- ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
fw_priv->offset,
firmware_p->size, &actread);- if (ret) {
printf("Error: %d Failed to read %s from flash%lld != %d.\n",
ret, fw_priv->name, actread, firmware_p-size);
return ret;Y Shouldn't this be 'goto out', do do the umount_ubifs() as in all other error cases?
okay.
- } else {
ret = actread;- }
What if actread != firmware_p->size? You handled it as an error before, now you happily return the number of bytes read in case of a short read. Operation not permitted
May be i misunderstand on ur previosly comment. There is not much
My comment was about returning -EPERM in this case. First of all you discarded the possible error code returned from fs_read() and secondly -EIO would be more appropriate than EPERM in case of reading less data than expected.
description on fd_read, i just knowing that -ve is error from function based on other example codes in U-Boot as they check the error with "ret < 0" or "ret != 0". "actread != firmware_p->size" is the additonal checking i added myself because when i digging to the code, i found that such condition only with error printf inside the function without any error code return. So, i am not sure when "actread != firmware_p-
size", the ret would be -ve too.
If 'size' is the actual size of the caller provided buffer and not the exact size of the firmware to be loaded, the condition actread != size would not be an error at all. If 'size' designates the exact size of the firmware, that the caller may know by other means, it is obviously an error if actread != size. Since you treated actread != size as an error in your original patch I assumed the latter case.
AFAICT Linux uses the first semantics ('size' specifies the size of the caller provided buffer as an upper limit for the read() call and not the exact amount of data to be read). Unfortunately the fs_read() semantics is fundamentally broken as it interprets 'size == 0' as 'read as much data as you can get' which may easily lead to buffer overflows and prints a very unhelpful error message "<filename> shorter than offset + len" in case 'size' does not match the actual amount of data read.
You should also consider what happens, if the firmware file you read is larger than the buffer you provided, as in that case fs_read() will stop reading at the provided buffer size and you won't be able to notice, that the file was not completely loaded!
Lothar Waßmann