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.c...@intel.com 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 | 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?
> 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)
> > > + * 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.
Lothar Waßmann
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
