From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Thu, 21 Dec 2017 05:39:37 +0000 Subject: [U-Boot] [PATCH v4 2/2] common: Generic firmware loader for file system In-Reply-To: <20171219122155.2960c7b4@karo-electronics.de> References: <1513573856-28074-1-git-send-email-tien.fong.chee@intel.com> <1513573856-28074-3-git-send-email-tien.fong.chee@intel.com> <20171218083951.370d2373@karo-electronics.de> <1513679469.2370.6.camel@intel.com> <20171219122155.2960c7b4@karo-electronics.de> Message-ID: <1513834776.4001.9.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 at intel.com > > > wrote: > > > > > > > > > > > > From: Tien Fong Chee > > > > > > > > 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 > > > > --- > > > >  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