[PATCH] dt/bindings: fwu-mdata-mtd: drop changes outside FWU

[Sughosh Ganu]

hi Ilias,

On Thu, 4 May 2023 at 16:15, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Rob,
>
> On Wed, May 03, 2023 at 12:24:39PM -0500, Rob Herring wrote:
> > On Wed, May 3, 2023 at 9:37 AM Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Hi Krzysztof,
> > >
> > > On Tue, Apr 11, 2023 at 07:38:11AM +0200, Krzysztof Kozlowski wrote:
> > > > On 11/04/2023 01:21, jaswinder.singh at linaro.org wrote:
> > > > > From: Jassi Brar <jaswinder.singh at linaro.org>
> > > > >
> > > > > Any requirement of FWU should not require changes to bindings
> > > > > of other subsystems. For example, for mtd-backed storage we
> > > > > can do without requiring 'fixed-partitions' children to also
> > > > > carry 'uuid', a property which is non-standard and not in the
> > > > > bindings.
> > > > >
> > > > >  There exists no code yet, so we can change the fwu-mtd bindings
> > > > > to contain all properties within the fwu-mdata node.
> > > > >
> > > > > Signed-off-by: Jassi Brar <jaswinder.singh at linaro.org>
> > > > > ---
> > > > >
> > > > > Hi Rob, Hi Krzysztof,
> > > > >
> > > > >   I was suggested, and I agree, it would be a good idea to get your blessings
> > > > > for the location and meta-data (fwu-mdata) bindings for the FWU.
> > > > >
> > > > >   The FWU images can be located in GPT partitions or MTD backed storage.
> > > > > The basic bindings for fwu-mdata has already been merged in uboot (ideally they
> > > > > too should have had your review). Now I am trying to fully support MTD backed
> > > > > storage and hence looking for your review. The proposed bindings are totally
> > > > > self-contained and don't require changes to any other subsystem.
> > > > >
> > > > > Thanks.
> > > >
> > > > I think we do not review U-Boot bindings usually, except these put in
> > > > the Linux kernel. There were few targeting U-Boot specifically (e.g.
> > > > Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml and
> > > > Documentation/devicetree/bindings/nvmem/u-boot,env.yaml) so if you want
> > > > our blessing, the bindings should be done in Linux kernel repo.
> > > >
> > > > I am pretty sure that reviewing other project bindings would be too much
> > > > of work for me.
> > >
> > > Sure that makes sense.  But an answer here of whether the bindings make
> > > sense to the DT maintainers or not would help to move forward.
> >
> > I have lots of comments on the bindings, but I'm missing what is the
> > problem to solve. Yes, it's 'A/B firmware updates', but what
> > configuration data do you need, why and when do you need it. IOW, give
> > me enough information to understand why a binding is designed the way
> > it is and be able to propose alternatives.
>
> Thanks for taking the time on this.  I do have questions for Jassi as
> well, but let me describe what I have in mind first. What we are trying
> to do here is standardize how EFI compliant devices (and in extent SystemReady-IR)
> can reliably perform firmware updates adding capabilities like rollback, brick
> protection etc.
>
> The high level description of how this is done is described here [0].
> The really short version of that document is that you:
> - Perform a capsule update of the non active partition
> - Reboot to that partition and launch your OS
> - If the OS approves the updates (which is implementation defined), then
>   you install an empty capsule indicating the acceptance of the update
> - If the device for any reason reboots for X amounts of times and the
>   acceptance capsule is not found, it will revert to the primary partition.
> - If the acceptance capsule is found the device permanently switches over
>   to the new firmware, bumping any rollback counters it might have (that's
>   optional).
> - All these information along with UUIDs for the storage medium and the
>   image types are stored in the metadata [1], which also has a backup
>   location.
>
> What you fundamentally need to perform an A/B booting is a first stage boot
> loader (or a masked ROM) that can understand the metadata described here
> [1].
>
> Ideally we would only require the DT to describe where we can find that
> metadata -- but the reality is a bit different.  Currently we only support
> of 2 types of devices
> - metadata lives in a GPT partition
> - They are located in an specific offset on a SPI-NOR
>
> GPT based devices are straightforward.  As you noticed we only need the phandle
> of the device.  There is a defined UUID for the metadata, so we discover
> them along with the backup partition.
>
> The MTD case is a bit more complicated. The metadata format only
> defines GUIDs for
> - UUID identifying the image type (image type GUID)
> - the UUID of the storage volume where the image is located
>
> Since we now don't have a UUID of the storage volume where the image is
> located, we somehow need to map the image UUID to a device offset. I think
> this is what Jassi is trying to define here -- it's worth noting that this
> has to happen for all non-GPT devices.
>
> As for when the data is needed, since this is backed up by EFI capsule updates,
> we only need access from the bootloader (and that's where the update agent
> currently resides, it's part of the u-boot EFI capsule functionality).
>
> >
> > That's easy enough to deduce for the GPT case. It's just needing to
> > know which device has the f/w update GPT? That's easily solved with an
> > alias, a boolean property in the device, or property with the disk
> > GUID. All of those options are much more simple than a whole node and
> > compatible.
> >
>
> Sure you can do that.  But wouldn't it be easier for bootloader to 'just'
> look for a compatible node regardless of where the metadata is stored?
> Sughosh did the u-boot implementation so maybe he can chime in on the
> challenges he saw code-wise.

We can indeed have a boolean property in the device which will have
the fwu metadata stored. The reason for having the compatible property
is that the fwu metadata access functions have been implemented in a
driver, and the u-boot driver model requires having a node with a
compatible property for the driver's probe function to be called, when
the corresponding device is accessed. This was primarily the reason
for having a separate node with the compatible property.

>
> >
> > > These bindings are trying to define a standardized interface for A/B *firmware*
> > > updates [0] which is not what traditionally goes into a device tree.  OTOH we
> > > already have some U-Boot specific bindings as you already mentioned.  As we
> > > move forward we need to be very precise on what is allowed or not on the DT
> > > since it's now tested and verified on SystemReady certifications.  IOW if
> > > we add those binding in U-Boot only, we would need to strip them before
> > > handing the DT to linux, otherwise certification would fail.  If you do
> > > think that having them in the kernel repo makes sense,  it would help
> > > standardizing other boot loaders (at least it would standardize where that
> > > metadata lives) if they want to implement something similar.
> >
> > I think it is fine if u-boot has things in DT which aren't an ABI, so
> > private and bundled, but I agree those should be stripped before
> > passing. To put it another way, if it's not an ABI nor shared amongst
> > projects, I don't think we need a schema nor to care outside of
> > u-boot.
>
> Exactly, we completely agree on this.
>
> >
> > However, it doesn't seem to me that needs for A/B updates would be
> > unique to u-boot.
>
> Yes.  TBH I was reluctant in trying to send the bindings upstream, since
> they are not related to hardware at all.  But as I mentioned the only
> reason to have them standardized is if people buy in to that update
> document and want to replicate the functionality.
>
> >
> > > Just keep in mind we would need a schema per storage medium.  IOW this tries to
> > > standardize devices which keep the firmware binary in an mtd.  There's also
> > > another biding which describes firmware files on a GPT [1].
> >
> > I'm assuming it's per partition type rather than storage medium (e.g.
> > SATA, USB, SD, NAND, SPI-NOR)? GPT, 'fixed-partitions', other DT
> > partition bindings, etc. If so, then I'm really wondering why it's a
> > standalone tree rather than integrated into those existing partition
> > bindings.
>
> I think it's per medium, unless I misunderstood something.  Sughosh?

The bindings would be required as per the metadata access mechanism.
So for example, the MTD bindings would suffice for all the storage
mediums which would have MTD based partitioning schemes. Same for all
GPT partitioned devices. Again, as for the need for a separate node
with the compatible property, it is as per the driver model design.
For the other properties, we can indeed have them integrated with the
corresponding partition bindings.

-sughosh