hi Masami,
On Thu, 20 Jan 2022 at 14:13, Masami Hiramatsu
<masami.hirama...@linaro.org> wrote:
>
> Hi Sughosh,
>
> 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.g...@linaro.org>:
>
> > +static int fwu_gpt_update_mdata(struct fwu_mdata *mdata)
> > +{
> > + int ret;
> > + struct blk_desc *desc;
> > + u16 primary_mpart = 0, secondary_mpart = 0;
> > +
>
> I think this update_mdata() method (or fwu_update_mdata() wrapper)
> should always update mdata::crc32. calculate crc32 at each call site is
> inefficient and easy to introduce bugs.
Okay. Will do.
>
> > + ret = fwu_plat_get_blk_desc(&desc);
> > + if (ret < 0) {
> > + log_err("Block device not found\n");
> > + return -ENODEV;
> > + }
> > +
> > + ret = gpt_get_mdata_partitions(desc, &primary_mpart,
> > + &secondary_mpart);
> > +
> > + if (ret < 0) {
> > + log_err("Error getting the FWU metadata partitions\n");
> > + return -ENODEV;
> > + }
> > +
> > + /* First write the primary partition*/
> > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart);
> > + if (ret < 0) {
> > + log_err("Updating primary FWU metadata partition failed\n");
> > + return ret;
> > + }
> > +
> > + /* And now the replica */
> > + ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart);
> > + if (ret < 0) {
> > + log_err("Updating secondary FWU metadata partition
> > failed\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int gpt_get_mdata(struct fwu_mdata **mdata)
> > +{
> > + int ret;
> > + struct blk_desc *desc;
> > + u16 primary_mpart = 0, secondary_mpart = 0;
> > +
> > + ret = fwu_plat_get_blk_desc(&desc);
> > + if (ret < 0) {
> > + log_err("Block device not found\n");
> > + return -ENODEV;
> > + }
> > +
> > + ret = gpt_get_mdata_partitions(desc, &primary_mpart,
> > + &secondary_mpart);
> > +
> > + if (ret < 0) {
> > + log_err("Error getting the FWU metadata partitions\n");
> > + return -ENODEV;
> > + }
> > +
> > + *mdata = malloc(sizeof(struct fwu_mdata));
> > + if (!*mdata) {
> > + log_err("Unable to allocate memory for reading FWU
> > metadata\n");
> > + return -ENOMEM;
> > + }
> > +
> > + ret = gpt_read_mdata(desc, *mdata, primary_mpart);
> > + if (ret < 0) {
> > + log_err("Failed to read the FWU metadata from the
> > device\n");
>
> Also, please release mdata inside the gpt_get_mdata() itself.
>
> I think it is not a good design to ask caller to free mdata if get_mdata()
> operation is failed because mdata may or may not allocated in error case.
>
> In success case, user must free it because it is allocated (user accessed it),
> and in error case, user can ignore it because it should not be allocated.
> This is simpler mind model and less memory leak chance.
I think this is confusing. It would be better to be consistent and
have the caller free up the allocated/not allocated memory. If you
check, the mdata pointer is being initialised to NULL in every
instance. Calling free with a NULL pointer is a valid case, which the
free call handles. There are multiple places in u-boot where the
caller is freeing up the allocated memory. So i think this should not
be an issue.
-sughosh
>
> Thank you,
> --
> Masami Hiramatsu
