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

[Masami Hiramatsu <masami.hiramatsu@linaro.org>]

Hi Sughosh,

Thanks for updating the series. I have some comment on this patch.

2022年2月8日(火) 3:21 Sughosh Ganu <sughosh.ganu@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.

> +       }
> +
> +       /*
> +        * 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.

Thank you,



-- 
Masami Hiramatsu