[PATCH v4 01/11] FWU: Add FWU metadata structure and driver for accessing metadata

[Sughosh Ganu]

hi Masami,

On Tue, 8 Feb 2022 at 15:04, Masami Hiramatsu
<masami.hiramatsu at linaro.org> wrote:
>
> Hi Sughosh,
>
> Thanks for updating the series. I have some comment on this patch.
>
> 2022年2月8日(火) 3:21 Sughosh Ganu <sughosh.ganu at linaro.org>:
> [snip]
> > +
> > +/**
> > + * fwu_get_active_index() - Get active_index from the FWU metadata
> > + * @active_idx: active_index value to be read
> > + *
> > + * Read the active_index field from the FWU metadata and place it in
> > + * the variable pointed to be the function argument.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_active_index(u32 *active_idx)
> > +{
> > +       int ret;
> > +       struct fwu_mdata *mdata = NULL;
> > +
> > +       ret = fwu_get_mdata(&mdata);
> > +       if (ret < 0) {
> > +               log_err("Unable to get valid FWU metadata\n");
> > +               goto out;
>
> Again, as we discussed previous series, please don't return unused
> allocated memory if the function returns an error.
> That is something like putting a burden on the callers. They always
> needs to initialize the pointer before call and free it even if the
> function is failed.

I would like to keep the convention consistent. The function that
declares the pointer will also be responsible for free'ing it up. I
find the alternative to be more confusing, where in some instances the
callee frees up the memory, while in some cases it is the caller that
frees it up. If there is no stated convention in u-boot which forbids
this style of memory handling, I would like to keep this as is. I
would say that this makes the implementation easier for the callee,
since it is the responsibility of the caller to free up the memory.

>
> > +       }
> > +
> > +       /*
> > +        * Found the FWU metadata partition, now read the active_index
> > +        * value
> > +        */
> > +       *active_idx = mdata->active_index;
> > +       if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) {
> > +               log_err("Active index value read is incorrect\n");
> > +               ret = -EINVAL;
> > +       }
> > +
> > +out:
> > +       free(mdata);
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * fwu_update_active_index() - Update active_index from the FWU metadata
> > + * @active_idx: active_index value to be updated
> > + *
> > + * Update the active_index field in the FWU metadata
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_update_active_index(u32 active_idx)
> > +{
> > +       int ret;
> > +       void *buf;
> > +       struct fwu_mdata *mdata = NULL;
> > +
> > +       if (active_idx > CONFIG_FWU_NUM_BANKS - 1) {
> > +               log_err("Active index value to be updated is incorrect\n");
> > +               return -1;
> > +       }
> > +
> > +       ret = fwu_get_mdata(&mdata);
> > +       if (ret < 0) {
> > +               log_err("Unable to get valid FWU metadata\n");
> > +               goto out;
> > +       }
> > +
> > +       /*
> > +        * Update the active index and previous_active_index fields
> > +        * in the FWU metadata
> > +        */
> > +       mdata->previous_active_index = mdata->active_index;
> > +       mdata->active_index = active_idx;
> > +
> > +       /*
> > +        * Calculate the crc32 for the updated FWU metadata
> > +        * and put the updated value in the FWU metadata crc32
> > +        * field
> > +        */
> > +       buf = &mdata->version;
> > +       mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
>
> I would like to suggest moving this crc32 calculation in the fwu_update_mdata().
> If the crc32 is for detecting a broken metadata on the "storage
> media", I think it should be calculated in the fwu_update_mdata() to
> simplify the code and avoid unexpected bugs from mistakes. If the
> crc32 is calculated right before updating metadata, we can ensure that
> the crc32 is always sane except for someone breaking it on the storage
> media.

Makes sense. I will make the change as you are suggesting. Thanks.

-sughosh

>
> Thank you,
>
>
>
> --
> Masami Hiramatsu