[U-Boot] [PATCH v5 07/20] common: Generic firmware loader for file system
Lothar Waßmann
LW at KARO-electronics.de
Fri Dec 8 08:22:09 UTC 2017
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 at intel.c
> > > > > > > > om
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > From: Tien Fong Chee <tien.fong.chee at 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 at 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
More information about the U-Boot
mailing list