Re: [PATCHv2 1/5] FWU: Add FWU metadata access driver for MTD storage regions

Jassi Brar
2022-10-31T23:38:07Z

On Fri, Oct 14, 2022 at 2:07 AM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Sun, Oct 02, 2022 at 06:51:32PM -0500, jassisinghbrar@gmail.com wrote:
> > +
> > +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> > +{
> > + return !do_div(size, mtd->erasesize);
> > +}
> > +
>
> Can we please add some sphinx style documentation overall ?
>
I can but I thought that is for public api and not static helper functions?

> > +
> > +static int fwu_mtd_check_mdata(struct udevice *dev)
> > +{
> > + struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> > + struct fwu_mdata primary, secondary;
> > + bool pri = false, sec = false;
> > + int ret;
> > +
> > + ret = fwu_mtd_load_mdata(mtd_priv->mtd, &primary,
> > + mtd_priv->pri_offset, FWU_MDATA_PRIMARY);
> > + if (ret < 0)
> > + log_debug("Failed to read the primary mdata: %d\n", ret);
> > + else
> > + pri = true;
> > +
> > + ret = fwu_mtd_load_mdata(mtd_priv->mtd, &secondary,
> > + mtd_priv->sec_offset, FWU_MDATA_SECONDARY);
> > + if (ret < 0)
> > + log_debug("Failed to read the secondary mdata: %d\n", ret);
> > + else
> > + sec = true;
> > +
> > + if (pri && sec) {
> > + if (memcmp(&primary, &secondary, sizeof(struct fwu_mdata))) {
> > + log_debug("The primary and the secondary mdata are different\n");
> > + ret = -1;
> > + }
> > + } else if (pri) {
> > + ret = fwu_mtd_save_secondary_mdata(mtd_priv, &primary);
> > + if (ret < 0)
> > + log_debug("Restoring secondary mdata partition failed\n");
> > + } else if (sec) {
> > + ret = fwu_mtd_save_primary_mdata(mtd_priv, &secondary);
> > + if (ret < 0)
> > + log_debug("Restoring primary mdata partition failed\n");
> > + }
>
> Same on this one. The requirements here are
> - Read our metadata
> - Compare the 2 partitions
>
> The only thing that's 'hardware' specific here is reading the metadata. We
> should at least unify the comparing part and restoration in case of
> failures, no ?
>
Yes. Since redundant copy of meta-data is a requirement of the spec,
we should maintain that in the fwu core code.
Maybe something like adding 'bool primary' argument to get_mdata() and
update_mdata()

thanks