Hi, On Fri, 8 Dec 2017 05:25:58 +0000 Chee, Tien Fong wrote: > On Kha, 2017-12-07 at 10:00 +0100, Lothar Waßmann wrote: > > Hi, > > > > On Thu, 7 Dec 2017 08:10:06 +0000 Chee, Tien Fong wrote: > > > > > > On Kha, 2017-12-07 at 08:49 +0100, Lothar Waßmann wrote: > > > > > > > > Hi, > > > > > > > > On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote: > > > > > > > > > > > > > > > On Rab, 2017-12-06 at 12:00 +0100, Lothar Waßmann wrote: > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Wed, 6 Dec 2017 10:06:21 +0000 Chee, Tien Fong wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.c > > > > > > > > om > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.c...@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.c...@intel.com > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > common/Makefile | 1 + > > > > > > > > > common/fs_loader.c | 304 > > > > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > > > include/fs_loader.h | 30 ++++++ > > > > > > > > > 3 files changed, 335 insertions(+) > > > > > > > > > create mode 100644 common/fs_loader.c > > > > > > > > > create mode 100644 include/fs_loader.h > > > > > > > > > > > > > > > > > > diff --git a/common/Makefile b/common/Makefile > > > > > > > > > index 801ea31..419e915 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..04f682b > > > > > > > > > --- /dev/null > > > > > > > > > +++ b/common/fs_loader.c > > > > > > > > > @@ -0,0 +1,304 @@ > > > > > > > > > +/* > > > > > > > > > + * 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> > > > > > > > > > + > > > > > > > > > +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 = 0; > > > > > > > > > > > > > > > > > Useless initialization. > > > > > > > > > > > > > > > noted. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + err = usb_init(); > > > > > > > > > + > > > > > > > > > + if (err) > > > > > > > > > > > > > > > > > Unnecessary blank line. > > > > > > > > > > > > > > > Sorry, i'm not catching you because there is no blank line > > > > > > > between > > > > > > > "if" > > > > > > > and "return" > > > > > > > > > > > > > I meant the blank line between 'err = ...' and 'if (err)' > > > > > > > > > > > Okay, i can change that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + if (!strcmp(default_locations[i].name, > > > > > > > > > name)) > > > > > > > > > + default_locations[i].devpart = > > > > > > > > > devpart; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + return; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +/* > > > > > > > > > + * 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; > > > > > > > > > + int ret = 0; > > > > > > > > > + > > > > > > > > > + if (!name || name[0] == '\0') > > > > > > > > > + ret = -EINVAL; > > > > > > > > > + > > > > > > > > Is it really useful to continue here initializing the > > > > > > > > 'firmware' > > > > > > > > struct and returning an error at the end? > > > > > > > > > > > > > > > I try to keep it very close to Linux firmware loader. When > > > > > > > more > > > > > > > API > > > > > > > ported in from Linux in future, this can be helper > > > > > > > function. > > > > > > > Anyway, i > > > > > > > have no strong opinion, i can move to caller if you guys > > > > > > > think > > > > > > > that > > > > > > > is > > > > > > > better. > > > > > > The Linux firmware loader has this: > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (!name || name[0] == '\0') { > > > > > > > ret = -EINVAL; > > > > > > > goto out; > > > > > > > } > > > > > > Note the 'goto out' which is missing in your code. > > > > > > If following the Linux code closely, you would have to set > > > > > > *firmware_p > > > > > > to NULL and exit in this case. > > > > > > > > > > > I can set the *firmware_p to NUll in failing case. But, i > > > > > checked > > > > > the > > > > > static int _request_firmware_prepare in Linux, there is no goto > > > > > out > > > > > error handling method in the function. Fyi, there is no > > > > > allocated > > > > > memory release in U-Boot. > > > > > https://elixir.free-electrons.com/linux/v4.13.15/source/drivers > > > > > /bas > > > > > e/fi > > > > > rmware_class.c#L1191 > > > > > > > > > > > > > > > > > > > > > > I was referring to _request_firmware() which calls > > > > request_firmware_prepare() and does the checking of 'name' as > > > > your > > > > code does. > > > > > > > Ohh.....i skipped _request_firmware() because there is no error > > > handling required in U-Boot such as memory release. For the > > > checking at > > > both firmware_p and name, i would rather to merge them into > > > request_firware_prepare() because they are related to each other > > > instead of creating one function just for checking purpose. What do > > > you > > > think? > > > > > I don't mind whether you combine those functions or not as long as > > you > > reproduce the same functionality which your patch currently does not > > do. > > > Okay. SO i will add the checking on firmware_p whether is NULL, and > assign NULL to *firmware_P if calloc is failed. There is no goout since > there is no error handling required. > > You should also exit early setting *firmware_p to NULL when the 'name' check fails, like the Linux code does! I.e.: + struct firmware *firmware = 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; + } + + firmware->name = name; + firmware->data = dbuf; + firmware->size = size; + firmware->offset = offset; + + return 0;
Lothar Waßmann _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot