Re: [PATCHv3 2/5] fwu: move meta-data management in core

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Jassi,

On Mon, Jan 02, 2023 at 12:26:40PM -0600, Jassi Brar wrote:
> Instead of each i/f having to implement their own meta-data verification
> and storage, move the logic in common code. This simplifies the i/f code
> much simpler and compact.
> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  drivers/fwu-mdata/fwu-mdata-uclass.c |  34 +++++++
>  include/fwu.h                        |  41 ++++++++
>  lib/fwu_updates/fwu.c                | 142 ++++++++++++++++++++++++++-
>  3 files changed, 213 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> index b477e9603f..e03773c584 100644
> --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> @@ -16,6 +16,40 @@
>  #include <linux/types.h>
>  #include <u-boot/crc.h>

[...]

> + * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided
> + * @mdata: FWU metadata structure
> + * @part: Bitmask of FWU metadata partitions to be written to
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> +{
> +	void *buf = &mdata->version;
> +	int err = 0;
> +
> +	/*
> +	 * Calculate the crc32 for the updated FWU metadata
> +	 * and put the updated value in the FWU metadata crc32
> +	 * field
> +	 */
> +	mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> +
> +	if (part & PRIMARY_PART)
> +		err = fwu_write_mdata(g_dev, mdata, true);
> +
> +	if (err) {
> +		log_err("Unable to write primary mdata\n");
> +		return err;
> +	}
> +
> +	if (part & SECONDARY_PART)
> +		err = fwu_write_mdata(g_dev, mdata, false);
> +
> +	if (err) {
> +		log_err("Unable to write secondary mdata\n");
> +		return err;
> +	}

Can we write this 
err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true: false);
if (err)
    log_err("Unable to write %s partition\n", part & PRIMARY_PART ?  "primary": "secondary" );
    ....

> +
> +	/* update the cached copy of meta-data */
> +	memcpy(&g_mdata, mdata, sizeof(struct fwu_mdata));
> +
> +	return 0;
> +}
> +
> +static inline int mdata_crc_check(struct fwu_mdata *mdata)
> +{
> +	void *buf = &mdata->version;
> +	u32 calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> +
> +	return calc_crc32 == mdata->crc32 ? 0 : -EINVAL;
> +}
> +
> +/**
> + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> + *
> + * Read both the metadata copies from the storage media, verify their checksum,
> + * and ascertain that both copies match. If one of the copies has gone bad,
> + * restore it from the good copy.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> +{
> +	int err;
> +	bool pri_ok, sec_ok;
> +	struct fwu_mdata s, *p_mdata, *s_mdata;
> +
> +	p_mdata = &g_mdata;
> +	s_mdata = &s;

Why are we defining it like this? Readability to have pointers for primary
and secondary metadata?

> +
> +	/* if mdata already read and ready */
> +	err = mdata_crc_check(p_mdata);
> +	if (!err)
> +		goto ret_mdata;

Shouldn't we check the secondary metadata ? At least that's what the old
fwu_check_mdata_validity() was doing.

> +	/* else read, verify and, if needed, fix mdata */
> +
> +	pri_ok = false;
> +	err = fwu_read_mdata(g_dev, p_mdata, true);
> +	if (!err) {
> +		err = mdata_crc_check(p_mdata);
> +		if (!err)
> +			pri_ok = true;
> +		else
> +			log_debug("primary mdata: crc32 failed\n");
> +	}
> +
> +	sec_ok = false;
> +	err = fwu_read_mdata(g_dev, s_mdata, false);
> +	if (!err) {
> +		err = mdata_crc_check(s_mdata);
> +		if (!err)
> +			sec_ok = true;
> +		else
> +			log_debug("secondary mdata: crc32 failed\n");
> +	}
> +
> +	if (pri_ok && sec_ok) {
> +		/*
> +		 * Before returning, check that both the
> +		 * FWU metadata copies are the same.
> +		 */
> +		err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> +		if (!err)
> +			goto ret_mdata;
> +
> +		/*
> +		 * If not, populate the secondary partition from the
> +		 * primary partition copy.
> +		 */
> +		log_info("Both FWU metadata copies are valid but do not match.");
> +		log_info(" Restoring the secondary partition from the primary\n");
> +		sec_ok = false;
> +	}
> +
> +	if (!pri_ok) {
> +		memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> +		err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
> +		if (err)
> +			goto ret_mdata;

The error print here is a bit misleading.  It's a failed write, not a crc32
mismatch

> +	}
> +
> +	if (!sec_ok) {
> +		memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> +		err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
> +		if (err)
> +			goto ret_mdata;
> +	}
> +
> +ret_mdata:
> +	if (err)
> +		log_debug("mdata : crc32 failed\n");
> +	else if (mdata)
> +		memcpy(mdata, p_mdata, sizeof(struct fwu_mdata));
> +
> +	return err;
> +}
> +
>  /**
>   * fwu_verify_mdata() - Verify the FWU metadata
>   * @mdata: FWU metadata structure
> -- 
> 2.34.1
> 

Regards
/Ilias