[RFC PATCH v3 2/9] FWU: Add FWU metadata access functions for GPT partitioned block devices

[Sughosh Ganu]

hi Masami,

On Thu, 20 Jan 2022 at 14:13, Masami Hiramatsu
<masami.hiramatsu at linaro.org> wrote:
>
> Hi Sughosh,
>
> 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu at 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