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

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

Hi Jassi,

Thanks for the patches and apologies for the late reply

On Sun, Oct 02, 2022 at 06:51:32PM -0500, jassisinghbrar@gmail.com wrote:
> From: Sughosh Ganu <sughosh.ganu@linaro.org>
> 
> In the FWU Multi Bank Update feature, the information about the
> updatable images is stored as part of the metadata, on a separate
> region. Add a driver for reading from and writing to the metadata
> when the updatable images and the metadata are stored on a raw
> MTD region.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  drivers/fwu-mdata/Kconfig   |  17 +-
>  drivers/fwu-mdata/Makefile  |   1 +

[...]

> +
> +/* Internal helper structure to move data around */
> +struct fwu_mdata_mtd_priv {
> +	struct mtd_info *mtd;
> +	u32 pri_offset;
> +	u32 sec_offset;
> +};
> +
> +enum fwu_mtd_op {
> +	FWU_MTD_READ,
> +	FWU_MTD_WRITE,
> +};
> +
> +#define FWU_MDATA_PRIMARY	true
> +#define FWU_MDATA_SECONDARY	false
> +
> +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 ?

> +static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *data,
> +		       enum fwu_mtd_op op)
> +{
> +	struct mtd_oob_ops io_op ={};
> +	u64 lock_offs, lock_len;
> +	size_t len;
> +	void *buf;
> +	int ret;
> +
> +	if (!mtd_is_aligned_with_block_size(mtd, offs)) {
> +		log_err("Offset unaligned with a block (0x%x)\n", mtd->erasesize);
> +		return -EINVAL;
> +	}
> +
> +	lock_offs = offs;
> +	/* This will expand erase size to align with the block size */
> +	lock_len = round_up(size, mtd->erasesize);
> +
> +	ret = mtd_unlock(mtd, lock_offs, lock_len);
> +	if (ret && ret != -EOPNOTSUPP)
> +		return ret;
> +
> +	if (op == FWU_MTD_WRITE) {
> +		struct erase_info erase_op = {};
> +
> +		erase_op.mtd = mtd;
> +		erase_op.addr = lock_offs;
> +		erase_op.len = lock_len;
> +		erase_op.scrub = 0;
> +
> +		ret = mtd_erase(mtd, &erase_op);
> +		if (ret)
> +			goto lock;
> +	}
> +
> +	/* Also, expand the write size to align with the write size */
> +	len = round_up(size, mtd->writesize);
> +
> +	buf = memalign(ARCH_DMA_MINALIGN, len);
> +	if (!buf) {
> +		ret = -ENOMEM;
> +		goto lock;
> +	}
> +	memset(buf, 0xff, len);
> +
> +	io_op.mode = MTD_OPS_AUTO_OOB;
> +	io_op.len = len;
> +	io_op.ooblen = 0;
> +	io_op.datbuf = buf;
> +	io_op.oobbuf = NULL;
> +
> +	if (op == FWU_MTD_WRITE) {
> +		memcpy(buf, data, size);
> +		ret = mtd_write_oob(mtd, offs, &io_op);
> +	} else {
> +		ret = mtd_read_oob(mtd, offs, &io_op);
> +		if (!ret)
> +			memcpy(data, buf, size);
> +	}
> +	free(buf);
> +
> +lock:
> +	mtd_lock(mtd, lock_offs, lock_len);
> +
> +	return ret;
> +}
> +
> +static int fwu_mtd_load_mdata(struct mtd_info *mtd, struct fwu_mdata *mdata,
> +			      u32 offs, bool primary)
> +{
> +	size_t size = sizeof(struct fwu_mdata);
> +	int ret;
> +
> +	ret = mtd_io_data(mtd, offs, size, mdata, FWU_MTD_READ);
> +	if (ret >= 0)
> +		ret = fwu_verify_mdata(mdata, primary);
> +
> +	return ret;
> +}
> +
> +static int fwu_mtd_save_primary_mdata(struct fwu_mdata_mtd_priv *mtd_priv,
> +				     struct fwu_mdata *mdata)
> +{
> +	return mtd_io_data(mtd_priv->mtd, mtd_priv->pri_offset,
> +			   sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
> +}
> +
> +static int fwu_mtd_save_secondary_mdata(struct fwu_mdata_mtd_priv *mtd_priv,
> +				       struct fwu_mdata *mdata)
> +{
> +	return mtd_io_data(mtd_priv->mtd, mtd_priv->sec_offset,
> +			   sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
> +}
> +
> +static int fwu_mtd_get_valid_mdata(struct fwu_mdata_mtd_priv *mtd_priv,
> +				  struct fwu_mdata *mdata)
> +{
> +	int ret;
> +
> +	ret = fwu_mtd_load_mdata(mtd_priv->mtd, mdata,
> +							 mtd_priv->pri_offset, FWU_MDATA_PRIMARY);
> +	if (!ret)
> +		return 0;
> +
> +	log_debug("Failed to load primary mdata. Trying secondary...\n");
> +
> +	ret = fwu_mtd_load_mdata(mtd_priv->mtd, mdata,
> +							 mtd_priv->sec_offset, FWU_MDATA_SECONDARY);
> +	if (ret) {
> +		log_debug("Failed to load secondary mdata.\n");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fwu_mtd_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> +{
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +	int ret;
> +
> +	/* First write the primary mdata */
> +	ret = fwu_mtd_save_primary_mdata(mtd_priv, mdata);
> +	if (ret < 0) {
> +		log_debug("Failed to update the primary mdata.\n");
> +		return ret;
> +	}
> +
> +	/* And now the replica */
> +	ret = fwu_mtd_save_secondary_mdata(mtd_priv, mdata);
> +	if (ret < 0) {
> +		log_debug("Failed to update the secondary mdata.\n");
> +		return ret;
> +	}

Looking at Sughosh patches as well, we can probably abstract this even more with
either ptrs or just including the device type in the function arguments,
since all update mdata functions are calling xxx_save_primary_mdata() ->
xxx_ave_secondary_mdata().  But I am fine as-is for now.  We can clean that
up once the patches get in 

> +
> +	return 0;
> +}
> +
> +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 ?

> +
> +	return ret;
> +}
> +
> +static int fwu_mtd_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> +{
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +
> +	return fwu_mtd_get_valid_mdata(mtd_priv, mdata);
> +}
> +
> +/**
> + * fwu_mdata_mtd_of_to_plat() - Translate from DT to fwu mdata device
> + */
 

Thanks
/Ilias