From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 827BBECAAD1 for ; Thu, 1 Sep 2022 06:45:40 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id AFF7284223; Thu, 1 Sep 2022 08:45:37 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=gmx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; secure) header.d=gmx.net header.i=@gmx.net header.b="M6TJsA1n"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 705ED8489B; Thu, 1 Sep 2022 08:45:35 +0200 (CEST) Received: from mout.gmx.net (mout.gmx.net [212.227.15.18]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 34610841F9 for ; Thu, 1 Sep 2022 08:45:32 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=xypron.glpk@gmx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1662014725; bh=khz7VWRW3lVKrfcjgExngl8ZTUGj2WK1SZGRm2/ro9g=; h=X-UI-Sender-Class:Date:Subject:To:Cc:References:From:In-Reply-To; b=M6TJsA1ntvHOHwr31ZbsHKGPPNKH0Ah2NZc1aBQVg2JvmcCy0Tq/KJ4M7AE09t+Bu eLMzIPPLqvC71HMYcNvrPTew7AV7Go5OOn9NM8jXx3zShqFCuqt7SgWwTV8MJh49QN 8pG0m2dVH/EmdIMZOOxlJ5vRArRC8TrgB0odXw6k= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from [192.168.123.94] ([84.118.157.2]) by mail.gmx.net (mrgmx005 [212.227.17.190]) with ESMTPSA (Nemesis) id 1ML9yS-1olj4J0k2E-00IEOP; Thu, 01 Sep 2022 08:45:25 +0200 Message-ID: Date: Thu, 1 Sep 2022 08:45:19 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Subject: Re: [PATCH 2/5] FWU: Add FWU metadata access driver for MTD storage regions To: jassisinghbrar@gmail.com, Sughosh Ganu Cc: ilias.apalodimas@linaro.org, takahiro.akashi@linaro.org, patrick.delaunay@foss.st.com, patrice.chotard@foss.st.com, sjg@chromium.org, bmeng.cn@gmail.com, trini@konsulko.com, etienne.carriere@linaro.org, monstr@monstr.eu, Jassi Brar , u-boot@lists.denx.de References: <20220722174240.63935-1-jassisinghbrar@gmail.com> <20220722174319.64006-1-jassisinghbrar@gmail.com> <20220722174319.64006-3-jassisinghbrar@gmail.com> Content-Language: en-US From: Heinrich Schuchardt In-Reply-To: <20220722174319.64006-3-jassisinghbrar@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-Provags-ID: V03:K1:3CBZsYq0dBW+lia2mJN+8uhzA/El8fbx6qsic/5zrL7I/BhaZTy n7W7DMZYm6EJwVf7S0fOqxK/guCpipbBzLUHGtIyiDKob/6T4L2iAZVEHkEA4v964mftbr2 0nlNIVPyJs2ar36r1MUUeAkx2w40D5ZADN8SK6/QrGJGJJtS76ULOEtw04dV2ls6Z14eS5j 2GeauDffUL6hCdy3jN6vA== X-UI-Out-Filterresults: notjunk:1;V03:K0:cTcbThkIVs0=:+teYDVcoYm9ssYNwIIlndK MMNpLjR/bjQlfGK9DWFRK5ILR877J2TrkLAY9YuvUR+2rELAvycwhlYBcP19nUVkssMiC1aEe ULmTRQXRSSu7xjbR1sp3wcaUuk6vOHmyvEqIWXHtiCvI27DrepN76oJIShVzxOi3t8K7uYgew mPEAT4XMFuSxIcNmx2nmim+4Rb//xS/sWrf1c++PBxFsLvaHNe4lOQ47ooibSLGsYCzFv8qTx ivLf25z3z1C+AkaGWEQ6Kdz9LgluQ4GOkUObTR+RqDIo7yR2AgwtXaCjfLoXrx3e/b7SH2MPf U4a1OzIn/101rY3EDK91shnJ1u8hA73vM0GtVBTGaJby9vMFVLs7OCZagAV79GqfsNDzTnr7T KuGgkb4i8jfULYHVzHMJICsmRMCrIoSv/s9hc90w+RTNeKrYqTEULeVLfw0bvzfBfRQjk1ZYc o8nneslA8gK7eN7LOiyHHyHxoZclD47A+OdPn79k8pRV3QUxjv9TPp/JBLk6PVrD3ABfbb6+a T9FTka83Ep9iQWg2S2bik9q6rQTtiT+IDrLmwt9lGTXx8cF8q+M5YGNkWN0jPDtLnBhXDOklh YLiO7je+a/i7kd/sOaLsbDtI4LV1Qe64qmpWU/rv0OAdiMFPaBeRrzOivEsrfPU3SQsKcrw6t JfaTXNrl2Wgf1AwgB1DwA5bAUZNwKgYnEbptpEREb6yWOPKxwU4MbOIq8HL8KjBhuI868dhqF MY+05eA/L112ilYN/Qw4y0eXWNNDSbs44M7/TcdQOO2Kb+zM655pBZ9ZPOiHwwNr2jKE1V/hQ u0HDpeOiQ1i7JPbX7znByHbqkSrZgRqYsRCXaiTCkQsM7GxLziADUr5o5K3HuPXBKyNw4K1YA SgOtJS6vbIVhIkKmW0GgqOB9BBbsktJr41fYxMSL/GFz2dty4oW2nxZoC8AQswBMJQ0EENAOS vSeJkPX2mAl41shiWSIiTdl/9s8K1cnntkYl//0peYmFOm7fflQyYfFaKpIawRJYwvwLgx/W7 pXCzTmfsiaadHro6xqaCTgc2VOyHw6MKayve5WV0dTFIMMsvQLaKbOqbNaxAF6zQIppNWh+8M f/jIYW676L2M/4PEv/46RO2/ECZNJcdkNomzcxzSVW8WmNqtXwiLjHubqk4PKncZjrJTU/lO6 3rkCgJW9/Xx9okn9kAqY9ggDfd X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On 7/22/22 19:43, jassisinghbrar@gmail.com wrote: > From: Sughosh Ganu > > 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 > Signed-off-by: Jassi Brar > --- > drivers/fwu-mdata/Kconfig | 8 + > drivers/fwu-mdata/Makefile | 1 + > drivers/fwu-mdata/fwu_mdata_mtd.c | 308 ++++++++++++++++++++++++++++++ > 3 files changed, 317 insertions(+) > create mode 100644 drivers/fwu-mdata/fwu_mdata_mtd.c > > diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig > index d5edef19d6..a8fa9ad783 100644 > --- a/drivers/fwu-mdata/Kconfig > +++ b/drivers/fwu-mdata/Kconfig > @@ -14,3 +14,11 @@ config FWU_MDATA_GPT_BLK > help > Enable support for accessing FWU Metadata on GPT partitioned > block devices. > + > +config FWU_MDATA_MTD > + bool "FWU Metadata access for non-GPT MTD devices" > + depends on DM_FWU_MDATA && MTD > + help > + Enable support for accessing FWU Metadata on non-partitioned > + (or non-GPT partitioned, e.g. partition nodes in devicetree) > + MTD devices. > diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile > index 313049f67a..58f8023f16 100644 > --- a/drivers/fwu-mdata/Makefile > +++ b/drivers/fwu-mdata/Makefile > @@ -5,3 +5,4 @@ > > obj-$(CONFIG_DM_FWU_MDATA) +=3D fwu-mdata-uclass.o > obj-$(CONFIG_FWU_MDATA_GPT_BLK) +=3D fwu_mdata_gpt_blk.o > +obj-$(CONFIG_FWU_MDATA_MTD) +=3D fwu_mdata_mtd.o > diff --git a/drivers/fwu-mdata/fwu_mdata_mtd.c b/drivers/fwu-mdata/fwu_m= data_mtd.c > new file mode 100644 > index 0000000000..d543a419fd > --- /dev/null > +++ b/drivers/fwu-mdata/fwu_mdata_mtd.c > @@ -0,0 +1,308 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2022, Linaro Limited > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + How should a reader know what pri_offset and sec_offset refer to? Please, provide Sphix style comments describing the structure. > +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, > +}; > + Please, document all functions. > +static bool /(struct mtd_info *mtd, u64 size) > +{ > + return !do_div(size, mtd->erasesize); > +} > + > +static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *= data, > + enum fwu_mtd_op op) Most of the functionality of this function should be in the mtd uclass. It is not FWU specific. > +{ > + struct mtd_oob_ops io_op =3D{}; > + u64 lock_offs, lock_len; > + size_t len; > + void *buf; > + int ret; > + > + if (!mtd_is_aligned_with_block_size(mtd, offs)) > + return -EINVAL; I think this is the only place where you could write an error message indicating that misplacement is the reason for the error. > + lock_offs =3D offs; > + lock_len =3D round_up(size, mtd->erasesize); > + > + ret =3D mtd_unlock(mtd, lock_offs, lock_len); > + if (ret && ret !=3D -EOPNOTSUPP) > + return ret; > + > + if (op =3D=3D FWU_MTD_WRITE) { > + struct erase_info erase_op =3D {}; > + > + /* This will expand erase size to align with the block size */ This comment is misplaced. It should be above the round_up() call. > + erase_op.mtd =3D mtd; > + erase_op.addr =3D lock_offs; > + erase_op.len =3D lock_len; > + erase_op.scrub =3D 0; > + > + ret =3D mtd_erase(mtd, &erase_op); > + if (ret) > + goto lock_out; > + } > + > + /* Also, expand the write size to align with the write size */ > + len =3D round_up(size, mtd->writesize); > + > + buf =3D memalign(ARCH_DMA_MINALIGN, len); > + if (!buf) { > + ret =3D -ENOMEM; > + goto lock_out; > + } > + io_op.mode =3D MTD_OPS_AUTO_OOB; > + io_op.len =3D len; > + io_op.ooblen =3D 0; > + io_op.datbuf =3D buf; > + io_op.oobbuf =3D NULL; > + > + if (op =3D=3D FWU_MTD_WRITE) { > + memcpy(buf, data, size); Don't copy random bytes to the flash. You have to zero out the padding bytes. > + ret =3D mtd_write_oob(mtd, offs, &io_op); > + } else { > + ret =3D mtd_read_oob(mtd, offs, &io_op); > + if (!ret) > + memcpy(data, buf, size); > + } > + free(buf); > + > +lock_out: This label sound like you want to lock something out. Please use lock: instead. > + 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 =3D sizeof(struct fwu_mdata); > + int ret; > + > + *mdata =3D malloc(size); > + if (!*mdata) > + return -ENOMEM; > + > + ret =3D mtd_io_data(mtd, offs, size, (void *)*mdata, FWU_MTD_READ); The conversion to (void *) is superfluous as the expected parameter is of type void *. > + if (ret >=3D 0) { > + ret =3D fwu_verify_mdata(*mdata, primary); > + if (ret < 0) { > + free(*mdata); > + *mdata =3D NULL; > + } > + } > + > + return ret; > +} > + > +static int fwu_mtd_load_primary_mdata(struct fwu_mdata_mtd_priv *mtd_pr= iv, > + struct fwu_mdata **mdata) > +{ > + return fwu_mtd_load_mdata(mtd_priv->mtd, mdata, mtd_priv->pri_offset, = true); Instead of true and false use an enum like enum fwu_sec_prim { FWU_META_PRIMARY, FWU_META_SECONDARY, } and get rid of these functions. That will give you the same readability with less of complexity. > +} > + > +static int fwu_mtd_load_secondary_mdata(struct fwu_mdata_mtd_priv *mtd_= priv, > + struct fwu_mdata **mdata) > +{ > + return fwu_mtd_load_mdata(mtd_priv->mtd, mdata, mtd_priv->sec_offset, = false); > +} > + > +static int fwu_mtd_save_primary_mdata(struct fwu_mdata_mtd_priv *mtd_pr= iv, > + 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) > +{ > + if (fwu_mtd_load_primary_mdata(mtd_priv, mdata) =3D=3D 0) Please, don't use =3D=3D 0. But "if (!". > + return 0; > + > + log_err("Failed to load/verify primary mdata. Try secondary.\n"); > + > + if (fwu_mtd_load_secondary_mdata(mtd_priv, mdata) =3D=3D 0) ditto > + return 0; > + > + log_err("Failed to load/verify secondary mdata.\n"); > + > + return -1; > +} > + > +static int fwu_mtd_update_mdata(struct udevice *dev, struct fwu_mdata *= mdata) > +{ > + struct fwu_mdata_mtd_priv *mtd_priv =3D dev_get_priv(dev); > + int ret; > + > + /* Update mdata crc32 field */ > + mdata->crc32 =3D crc32(0, (void *)&mdata->version, Avoid superfluous conversions. There is already a conversion in the defintion of the crc32 macro. > + sizeof(*mdata) - sizeof(u32)); > + > + /* First write the primary mdata */ > + ret =3D fwu_mtd_save_primary_mdata(mtd_priv, mdata); > + if (ret < 0) { > + log_err("Failed to update the primary mdata.\n"); > + return ret; > + } > + > + /* And now the replica */ > + ret =3D fwu_mtd_save_secondary_mdata(mtd_priv, mdata); > + if (ret < 0) { > + log_err("Failed to update the secondary mdata.\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int fwu_mtd_mdata_check(struct udevice *dev) > +{ > + struct fwu_mdata *primary =3D NULL, *secondary =3D NULL; > + struct fwu_mdata_mtd_priv *mtd_priv =3D dev_get_priv(dev); > + int ret; > + > + ret =3D fwu_mtd_load_primary_mdata(mtd_priv, &primary); > + if (ret < 0) > + log_err("Failed to read the primary mdata: %d\n", ret); > + > + ret =3D fwu_mtd_load_secondary_mdata(mtd_priv, &secondary); > + if (ret < 0) > + log_err("Failed to read the secondary mdata: %d\n", ret); > + > + if (primary && secondary) { > + if (memcmp(primary, secondary, sizeof(struct fwu_mdata))) { > + log_err("The primary and the secondary mdata are different\n"); > + ret =3D -1; > + } > + } else if (primary) { > + ret =3D fwu_mtd_save_secondary_mdata(mtd_priv, primary); > + if (ret < 0) > + log_err("Restoring secondary mdata partition failed\n"); > + } else if (secondary) { > + ret =3D fwu_mtd_save_primary_mdata(mtd_priv, secondary); > + if (ret < 0) > + log_err("Restoring primary mdata partition failed\n"); > + } If neither primary nor secondary data is available, you are happy??? > + > + free(primary); > + free(secondary); > + return ret; > +} > + > +static int fwu_mtd_get_mdata(struct udevice *dev, struct fwu_mdata **md= ata) > +{ > + struct fwu_mdata_mtd_priv *mtd_priv =3D 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 > + */ > +static int fwu_mdata_mtd_of_to_plat(struct udevice *dev) > +{ > + struct fwu_mdata_mtd_priv *mtd_priv =3D dev_get_priv(dev); > + const fdt32_t *phandle_p =3D NULL; > + struct udevice *mtd_dev; > + struct mtd_info *mtd; > + int ret, size; > + u32 phandle; > + > + /* Find the FWU mdata storage device */ > + phandle_p =3D ofnode_get_property(dev_ofnode(dev), > + "fwu-mdata-store", &size); > + if (!phandle_p) { > + log_err("fwu-mdata-store property not found\n"); A user needs to know that the problem is in the device-tree, e.g. "FWU meta data store not defined in device-tree". > + return -ENOENT; > + } > + > + phandle =3D fdt32_to_cpu(*phandle_p); > + > + ret =3D device_get_global_by_ofnode( > + ofnode_get_by_phandle(phandle), > + &mtd_dev); > + if (ret) > + return ret; No log message? > + > + mtd_probe_devices(); > + > + mtd_for_each_device(mtd) { > + if (mtd->dev =3D=3D mtd_dev) { > + mtd_priv->mtd =3D mtd; > + log_debug("Found the FWU mdata mtd device %s\n", mtd->name); > + break; > + } > + } > + if (!mtd_priv->mtd) { > + log_err("Failed to find mtd device by fwu-mdata-store\n"); > + return -ENOENT; > + } > + > + /* Get the offset of primary and seconday mdata */ > + ret =3D ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 0, > + &mtd_priv->pri_offset); > + if (ret) > + return ret; > + ret =3D ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 1, > + &mtd_priv->sec_offset); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int fwu_mdata_mtd_probe(struct udevice *dev) > +{ > + /* Ensure the metadata can be read. */ > + return fwu_mtd_mdata_check(dev); If you don't like the name of fwu_mtd_mdata_check(), change it instead of wrapping it in another function. Best regards Heinrich > +} > + > +static struct fwu_mdata_ops fwu_mtd_ops =3D { > + .mdata_check =3D fwu_mtd_mdata_check, > + .get_mdata =3D fwu_mtd_get_mdata, > + .update_mdata =3D fwu_mtd_update_mdata, > +}; > + > +static const struct udevice_id fwu_mdata_ids[] =3D { > + { .compatible =3D "u-boot,fwu-mdata-mtd" }, > + { } > +}; > + > +U_BOOT_DRIVER(fwu_mdata_mtd) =3D { > + .name =3D "fwu-mdata-mtd", > + .id =3D UCLASS_FWU_MDATA, > + .of_match =3D fwu_mdata_ids, > + .ops =3D &fwu_mtd_ops, > + .probe =3D fwu_mdata_mtd_probe, > + .of_to_plat =3D fwu_mdata_mtd_of_to_plat, > + .priv_auto =3D sizeof(struct fwu_mdata_mtd_priv), > +};