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 AAEA3C74A5B for ; Mon, 9 Jan 2023 12:54:39 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 2E5A385369; Mon, 9 Jan 2023 13:54:37 +0100 (CET) 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="Jf4fLHHS"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 3ECF28526A; Mon, 9 Jan 2023 13:54:34 +0100 (CET) Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) (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 076B08512E for ; Mon, 9 Jan 2023 13:54:31 +0100 (CET) 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-x635.google.com with SMTP id az20so755346ejc.1 for ; Mon, 09 Jan 2023 04:54:30 -0800 (PST) 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=aOcTxYLcHcIAMvO9JVhM8tAfLRdcE3ndHQZGFxIoU4A=; b=Jf4fLHHS/saGj14s+d+r5kJvUbLaNnstCE/IFtimItopxhgcJtY4rq0UqBRhA3Wnc7 eS4q3podSDf5pcIRc5kopTa5qyF2+genE9z7JhlI6EE6PaeNavxtrNJDacHOcSUsilrR QLmCGXeUgBl321ICR/1tl+cSrL4ZUHReUzE7e8YOjl3ix7Be7JnAjoq6M1+9V61sN3iJ 0yP+iRQ3F6fOyYuk7MG+7DKkA+/0gvCqmBPMNVKqzM00jD0DHGw9x7L5wz5wHvOOJXGS Sd6stKbVg4OAX62F9GYUBPPO8lLdszULQgXYYL0Tq5HabSfw5WPuIYmy/o2mta1foLoB C+lQ== 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=aOcTxYLcHcIAMvO9JVhM8tAfLRdcE3ndHQZGFxIoU4A=; b=fFfmcxSY10OM921wwW0s0+1bb6JI+RZ+QcZw5vBaSy16o0geiepykLcpQ0J1AnTSc5 U62w/g3ZnrCMmPCfgo+ki7PKlJup/SvmYwAvFQzFOzSG7tlN05lTkcQRiK0OVabH3onF 4c7RI0aippW/Nzw7hl9RYyjg7EOXMjKZddVZzmuGx/HtTEQfgtVAmTCi/L2ZLV6iLE6E zUH3Jmmjpk6luufZ2FDjZT39XL1I8dJ0o2NEt4HH7xNprCngF22orEmluBcaJbtpQ2oN 72sMre/+cFUcklBTtT/GVMSgvTDMFuZyQ58YO+Pe7VxrQnewclHz7DOZcAsDOWVM9xGK cYbw== X-Gm-Message-State: AFqh2krt3cYB2prGLa0vNT2uStdNpR+d7iX/hwKI2jbTZSH580iijAjZ OYi5S7z9pLpVXi7Ri2RZwis8Zw== X-Google-Smtp-Source: AMrXdXvYfe86zUixC20pEvpCYxK30VLMjnkDct7CR/imFmVoQclt6y2h5AOL5oWQnKHj9pkgG67U/A== X-Received: by 2002:a17:907:8b93:b0:7c1:1dc7:8837 with SMTP id tb19-20020a1709078b9300b007c11dc78837mr56436207ejc.66.1673268870509; Mon, 09 Jan 2023 04:54:30 -0800 (PST) Received: from hades (ppp079167090036.access.hol.gr. [79.167.90.36]) by smtp.gmail.com with ESMTPSA id y16-20020a1709063a9000b0084ce5d5d21bsm3685795ejd.22.2023.01.09.04.54.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 09 Jan 2023 04:54:30 -0800 (PST) Date: Mon, 9 Jan 2023 14:54:27 +0200 From: Ilias Apalodimas To: Jassi Brar Cc: u-boot@lists.denx.de, etienne.carriere@linaro.org, trini@konsulko.com, sjg@chromium.org, sughosh.ganu@linaro.org, xypron.glpk@gmx.de, patrick.delaunay@foss.st.com, patrice.chotard@foss.st.com, Jassi Brar Subject: Re: [PATCHv3 2/5] fwu: move meta-data management in core Message-ID: References: <20230102182532.2411125-1-jaswinder.singh@linaro.org> <20230102182640.2411224-1-jaswinder.singh@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230102182640.2411224-1-jaswinder.singh@linaro.org> 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, 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 > --- > 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 > #include [...] > + * 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