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 880C7C43217 for ; Fri, 14 Oct 2022 07:08:05 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 3350584E53; Fri, 14 Oct 2022 09:08:03 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="Ige+bXfK"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 7369984EA8; Fri, 14 Oct 2022 09:08:01 +0200 (CEST) Received: from mail-ej1-x62c.google.com (mail-ej1-x62c.google.com [IPv6:2a00:1450:4864:20::62c]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 559A781F4E for ; Fri, 14 Oct 2022 09:07:58 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-ej1-x62c.google.com with SMTP id 13so8570435ejn.3 for ; Fri, 14 Oct 2022 00:07:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=c5gnOLt+/EyB3mpIGUo0bdn7o2pFqiwbtrXUf5eDmsI=; b=Ige+bXfK9qD09Nw9B5WCg5zxiI3puT1r87EBtUk3scEFHLy1slFI5flOaepRTOiSCp 4JdxCfXDpj59GczsD/gidbg5aImhIToXbVXhVu9EDzm0UCvMxTwU698hg+ulZ26RhYQ2 9SqN2DHI+G8I3/3M4lXUVSsjvOhu0Ca92RXqLRMHDqxXQkC/6xsRR2UGrqQgJhra1BF+ RMT+38XflhjqQdQk9R4wir3ovQ6kWcQbwI4MfGv7DNXXxB0feumYf97LYtgRqdxW2CpU drgB76R54zxIiXBa/ki04aOa+BJLRhRrD0N9uYeKVIAbtmKKBdowmuBt0+bLKixbsDhh MCpA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=c5gnOLt+/EyB3mpIGUo0bdn7o2pFqiwbtrXUf5eDmsI=; b=eU4debVpOgCa6jM98NV26hpo3ljnZv12MaGMhEcHy75IXxZRCorzQJtOm7qw38NmqD jN1B+bL6ckhmQ1sN+ypAFZaYpZGK9xx1pM+SwST/vr8ize5dQSeQNHLPFXTPyQzrHNNY ZwdU3czf6S6pGwacsGPpFcNeNnmOHODroi7kbBJt8ddIbakgY7smcpuktFB3vNBsRYL+ q1E25G3C3qbKxBbyJ4WKKNLQLtRprH2XoLukFRH6JCX/gqZdaYdGeOtYFAUfA44q8Dl3 QZ69LlqR2xePgxIxj2yWbY5jM7nlNe2LmEJd9U5AmAA3kIe2VQ2ZUAn2mfU+/Us7ltbc 5B0g== X-Gm-Message-State: ACrzQf0OvNgGXmYYALXMc82yd9N5ntTgjixowHnXLHTytnb7erp39u3b tSKvgxo0hjh2Ic/9YnW+m34OXA== X-Google-Smtp-Source: AMsMyM7iBFtRZBBjaDqBHDaRcSYuLmLUaFQl60GMpbD2WfpSjkpfo1VrkIua3a2P+WfforbHGP5oPQ== X-Received: by 2002:a17:907:7244:b0:78d:cedc:7a9e with SMTP id ds4-20020a170907724400b0078dcedc7a9emr1103188ejc.600.1665731277938; Fri, 14 Oct 2022 00:07:57 -0700 (PDT) Received: from hera (ppp046103015185.access.hol.gr. [46.103.15.185]) by smtp.gmail.com with ESMTPSA id j18-20020a508a92000000b00458b8d4f4d5sm1272580edj.57.2022.10.14.00.07.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 Oct 2022 00:07:57 -0700 (PDT) Date: Fri, 14 Oct 2022 10:07:55 +0300 From: Ilias Apalodimas To: jassisinghbrar@gmail.com Cc: u-boot@lists.denx.de, xypron.glpk@gmx.de, takahiro.akashi@linaro.org, sjg@chromium.org, trini@konsulko.com, etienne.carriere@linaro.org, monstr@monstr.eu, Sughosh Ganu , Jassi Brar Subject: Re: [PATCHv2 1/5] FWU: Add FWU metadata access driver for MTD storage regions Message-ID: References: <20221002235046.344149-1-jassisinghbrar@gmail.com> <20221002235132.344240-1-jassisinghbrar@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221002235132.344240-1-jassisinghbrar@gmail.com> 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 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 > > 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 | 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