From: Jassi Brar <jassisinghbrar@gmail.com>
To: Sughosh Ganu <sughosh.ganu@linaro.org>
Cc: u-boot@lists.denx.de, Heinrich Schuchardt <xypron.glpk@gmx.de>,
Ilias Apalodimas <ilias.apalodimas@linaro.org>,
Takahiro Akashi <takahiro.akashi@linaro.org>,
Patrick Delaunay <patrick.delaunay@foss.st.com>,
Patrice Chotard <patrice.chotard@foss.st.com>,
Simon Glass <sjg@chromium.org>, Bin Meng <bmeng.cn@gmail.com>,
Tom Rini <trini@konsulko.com>,
Etienne Carriere <etienne.carriere@linaro.org>,
Michal Simek <monstr@monstr.eu>,
Jassi Brar <jaswinder.singh@linaro.org>
Subject: Re: [PATCH v10 02/15] FWU: Add FWU metadata structure and driver for accessing metadata
Date: Tue, 27 Sep 2022 11:25:15 -0500 [thread overview]
Message-ID: <CABb+yY19hafGL0R3O346XDTV2BW5enGqFop1GgCun6Gdbgeo9A@mail.gmail.com> (raw)
In-Reply-To: <CADg8p96wdpDgJdq6w-Wdrrh9tPBJEAAxmbc8+_kDnC0zV4UoBA@mail.gmail.com>
On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > >
> >
> > > >
> > > > .....
> > > > > +/**
> > > > > + * fwu_revert_boot_index() - Revert the active index in the FWU metadata
> > > > > + *
> > > > > + * Revert the active_index value in the FWU metadata, by swapping the values
> > > > > + * of active_index and previous_active_index in both copies of the
> > > > > + * FWU metadata.
> > > > > + *
> > > > > + * Return: 0 if OK, -ve on error
> > > > > + *
> > > > > + */
> > > > > +int fwu_revert_boot_index(void)
> > > > > +{
> > > > > + int ret;
> > > > > + u32 cur_active_index;
> > > > > + struct udevice *dev;
> > > > > + struct fwu_mdata mdata = { 0 };
> > > > > +
> > > > > + ret = fwu_get_dev_mdata(&dev, &mdata);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + /*
> > > > > + * Swap the active index and previous_active_index fields
> > > > > + * in the FWU metadata
> > > > > + */
> > > > > + cur_active_index = mdata.active_index;
> > > > > + mdata.active_index = mdata.previous_active_index;
> > > > > + mdata.previous_active_index = cur_active_index;
> > > > >
> > > > This may cause problems.
> > > > We are reverting because active_index does not work, and here we set
> > > > it to previous_active_index which is supposed to mean "last good
> > > > index".
> > > > Also this logic assumes a 2-banks setup, and is obviously incorrect
> > > > for >2 banks where the previous_active_index should point to
> > > > "boot_index minus 2" bank (but of course there is no guarantee that
> > > > that bank is preserved still).
> > > > So either previous_active_index be left changed OR we also copy the
> > > > previous bank to active bank before the swap.
> > >
> > > Sorry, but I don't understand the review comment here. Even in the
> > > case of num_banks > 2, this function is simply using the
> > > previous_active_index value. It does not care what the
> > > previous_active_index value is. If you remember, the setting of the
> > > update bank is really a platform
> > > function(fwu_plat_get_update_index()). A platform can set any bank
> > > number as the update bank. So we cannot tell what the value of the
> > > previous_active_index will be.
> > >
> > Do you remember you pick update_bank in a circular-buffer manner in
> > fwu_plat_get_update_index() ? But don't even bother the >2 banks.
> >
> > Consider the simple 2-banks platform....
> > Initially:
> > active_index = 1
> > previous_active_index = 0
> >
> > After update and before reboot
> > active_index = 0 <<<< updated bank 0
> > previous_active_index = 1
> >
> > After reboot, for some reason update fails (reject bank0) and we call
> > fwu_revert_boot_index()
> > active_index = 1 <<< good
> > previous_active_index = 0 <<<< points to unbootable bank
> >
> > Which may be seen as inconsistency if we assume previous_bank to
> > always contain a bootable set of images.
> > So we also need to copy bank1 into bank0 as part of the revert (at
> > least as a backup for reasons other than a/b update failure).
>
> If the platform owner wants to restore a particular bank with good
> images, the procedure to update that bank needs to be followed just
> like it was any other update.
>
The banks are under FWU and the platform has (should have) no control
over which bank the image goes in.
> If an updated bank fails the image
> acceptance test, the following boot would be from the
> previous_active_bank. In that case, the other bank needs to be updated
> by explicitly putting capsules in the ESP and initiating the update.
>
Which capsule - the one that just failed or the previous one (which
may not be available/provided)?
Doesn't simply copying over the bank make more sense?
>
> > > All that this function does is use the
> > > previous_active_index as the partition/bank to boot from in the
> > > subsequent boot cycle.
> > >
> > That is, you assume the previous_active_index bank contains working images.
>
> It is the responsibility of the platform owner to ensure that all
> partitions have valid images.
>
I differ. The platform should not be modifying banks and meta-data
beneath the fwu framework.
The specification says "A Client can only read from or write to images
in the update bank"
-j
next prev parent reply other threads:[~2022-09-27 16:25 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-15 8:14 [PATCH v10 00/15] FWU: Add FWU Multi Bank Update feature support Sughosh Ganu
2022-09-15 8:14 ` [PATCH v10 01/15] dt/bindings: Add bindings for GPT based FWU Metadata storage device Sughosh Ganu
2022-09-15 8:14 ` [PATCH v10 02/15] FWU: Add FWU metadata structure and driver for accessing metadata Sughosh Ganu
2022-09-19 0:33 ` Jassi Brar
2022-09-19 12:39 ` Sughosh Ganu
2022-09-26 2:57 ` Jassi Brar
2022-09-26 10:00 ` Sughosh Ganu
2022-09-26 14:42 ` Jassi Brar
2022-09-27 7:14 ` Sughosh Ganu
2022-09-27 16:25 ` Jassi Brar [this message]
2022-09-28 6:00 ` Sughosh Ganu
2022-09-28 19:29 ` Jassi Brar
2022-09-29 6:01 ` Sughosh Ganu
2022-09-15 8:14 ` [PATCH v10 03/15] FWU: Add FWU metadata access driver for GPT partitioned block devices Sughosh Ganu
2022-09-22 8:46 ` Ilias Apalodimas
2022-09-26 8:46 ` Sughosh Ganu
2022-09-27 11:35 ` Etienne Carriere
2022-09-27 11:57 ` Ilias Apalodimas
2022-09-26 2:52 ` Jassi Brar
2022-09-26 8:48 ` Sughosh Ganu
2022-09-26 15:00 ` Jassi Brar
2022-09-15 8:14 ` [PATCH v10 04/15] stm32mp1: dk2: Add a node for the FWU metadata device Sughosh Ganu
2022-09-15 8:14 ` [PATCH v10 05/15] stm32mp1: dk2: Add image information for capsule updates Sughosh Ganu
2022-09-15 8:14 ` [PATCH v10 06/15] FWU: Add helper functions for accessing FWU metadata Sughosh Ganu
2022-09-22 8:59 ` Ilias Apalodimas
2022-09-22 9:35 ` Sughosh Ganu
2022-09-23 6:16 ` Ilias Apalodimas
2022-09-15 8:14 ` [PATCH v10 07/15] FWU: STM32MP1: Add support to read boot index from backup register Sughosh Ganu
2022-09-27 11:35 ` Etienne Carriere
2022-09-15 8:14 ` [PATCH v10 08/15] event: Add an event for main_loop Sughosh Ganu
2022-09-20 7:30 ` Ilias Apalodimas
2022-09-15 8:14 ` [PATCH v10 09/15] FWU: Add boot time checks as highlighted by the FWU specification Sughosh Ganu
2022-09-26 2:59 ` Jassi Brar
2022-09-26 10:08 ` Sughosh Ganu
2022-09-26 14:07 ` Jassi Brar
2022-09-27 7:00 ` Sughosh Ganu
2022-09-15 8:14 ` [PATCH v10 10/15] FWU: Add support for the FWU Multi Bank Update feature Sughosh Ganu
2022-09-16 1:47 ` Takahiro Akashi
2022-09-16 5:22 ` Sughosh Ganu
2022-09-16 6:50 ` Takahiro Akashi
2022-09-16 10:54 ` Sughosh Ganu
2022-09-20 8:16 ` Takahiro Akashi
2022-09-20 13:04 ` Sughosh Ganu
2022-09-21 5:28 ` Takahiro Akashi
2022-09-21 11:26 ` Sughosh Ganu
2022-09-22 5:21 ` Takahiro Akashi
2022-09-26 2:55 ` Jassi Brar
2022-09-26 9:01 ` Sughosh Ganu
2022-09-26 14:53 ` Jassi Brar
2022-09-27 7:22 ` Sughosh Ganu
2022-09-27 16:48 ` Jassi Brar
2022-09-28 6:22 ` Sughosh Ganu
2022-09-28 7:30 ` Etienne Carriere
2022-09-28 15:16 ` Jassi Brar
2022-10-03 11:54 ` Etienne Carriere
2022-10-03 12:21 ` Ilias Apalodimas
2022-10-03 13:29 ` Jassi Brar
2022-09-15 8:14 ` [PATCH v10 11/15] FWU: cmd: Add a command to read FWU metadata Sughosh Ganu
2022-09-15 8:14 ` [PATCH v10 12/15] test: dm: Add test cases for FWU Metadata uclass Sughosh Ganu
2022-09-15 8:14 ` [PATCH v10 13/15] mkeficapsule: Add support for generating empty capsules Sughosh Ganu
2022-09-15 8:14 ` [PATCH v10 14/15] mkeficapsule: Add support for setting OEM flags in capsule header Sughosh Ganu
2022-09-15 8:14 ` [PATCH v10 15/15] FWU: doc: Add documentation for the FWU feature Sughosh Ganu
2022-09-19 21:37 ` Jassi Brar
2022-09-27 12:01 ` Etienne Carriere
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CABb+yY19hafGL0R3O346XDTV2BW5enGqFop1GgCun6Gdbgeo9A@mail.gmail.com \
--to=jassisinghbrar@gmail.com \
--cc=bmeng.cn@gmail.com \
--cc=etienne.carriere@linaro.org \
--cc=ilias.apalodimas@linaro.org \
--cc=jaswinder.singh@linaro.org \
--cc=monstr@monstr.eu \
--cc=patrice.chotard@foss.st.com \
--cc=patrick.delaunay@foss.st.com \
--cc=sjg@chromium.org \
--cc=sughosh.ganu@linaro.org \
--cc=takahiro.akashi@linaro.org \
--cc=trini@konsulko.com \
--cc=u-boot@lists.denx.de \
--cc=xypron.glpk@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).