On Fri, Feb 19, 2021 at 09:28:04AM +0000, Paul Barker wrote:
> On Wed, 17 Feb 2021 at 05:25, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Sat, Dec 19, 2020 at 05:28:08PM +0000, Paul Barker wrote:
> > > When applying an overlay fragment, we should take care not to overwrite
> > > an existing phandle property of the target node as this could break
> > > references to the target node elsewhere in the base dtb.
> >
> > It could.. but my first inclination is to say "don't do that".
>
> I entirely agree. However, folks are already doing this [1] and I
> think fdtoverlay should be more defensive here. My idea was to handle
> this safely in fdtoverlay if we can assume that it is never legitimate
> to modify the phandle property of a node from an overlay.
The trouble is that both applying the change and ignoring the change
will break something. Applying it will break things that attempt to
bind to the node with its "old" phandle, but ignoring it will break
something that attempts to bind to the new handle. e.g.
fragment@1 {
target = < &somenode >;
anothername: __overlay__ {
...
};
}
fragment@2 {
target = < &differentnode >;
__overlay__ {
some-prop = < &anothername >;
};
}
> A couple of
> alternatives would be raising an error in fdtoverlay instead of trying
> to silently ignore the phandle, or raising an error in dtc when the
> phandle is introduced.
Right, I think those are better options.
In the short term, I think raising an error in dtc is probably the
right choice - we can remove it when/if we implement this properly
("merging" the old and new phandles).
> > > In addition to potentially breaking references within the resulting fdt,
> > > if the overlay is built with symbols enabled (`-@` option to dtc) then
> > > fdtoverlay will be unable to merge the overlay with a base dtb file.
> >
> > If we can manage, I really think the better fix is to *not* have the
> > overlay provide conflicting phandles, rather than having the merge
> > process ignore them.
> >
> > I think we need to pin down the cirucmstances in which overlays with
> > phandles are being generated and address those, if possible.
>
> Am I right in understanding this to mean we should raise an error in
> dtc when a phandle is generated in an __overlay__ node?
As long as we're unable to generate stuff to have that phandle
automatically resolved to the same value as the old phandle, then yes.
> > > A new test case is added to check how fdtoverlay handles this case.
> > > Attempting to apply this test overlay without the fix in this patch
> > > results in the following output:
> > >
> > > input = tests/overlay_base_ref.test.dtb
> > > output = tests/overlay_overlay_ref.fdtoverlay.dtb
> > > overlay[0] = tests/overlay_overlay_ref.test.dtb
> > >
> > > Failed to apply 'tests/overlay_overlay_ref.test.dtb':
> > > FDT_ERR_NOTFOUND
> >
> > [snip]
> > > diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
> > > new file mode 100644
> > > index 0000000..1fc02a2
> > > --- /dev/null
> > > +++ b/tests/overlay_base_ref.dts
> > > @@ -0,0 +1,19 @@
> > > +/*
> > > + * Copyright (c) 2016 NextThing Co
> > > + * Copyright (c) 2016 Free Electrons
> > > + * Copyright (c) 2016 Konsulko Inc.
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0+
> > > + */
> > > +
> > > +/dts-v1/;
> > > +
> > > +/ {
> > > + test: test-node {
> > > + test-int-property = <42>;
> > > + };
> > > +
> > > + test-refs {
> > > + refs = <&test>;
> > > + };
> > > +};
> > > diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
> > > new file mode 100644
> > > index 0000000..a45c95d
> > > --- /dev/null
> > > +++ b/tests/overlay_overlay_ref.dts
> > > @@ -0,0 +1,24 @@
> > > +/*
> > > + * Copyright (c) 2016 NextThing Co
> > > + * Copyright (c) 2016 Free Electrons
> > > + * Copyright (c) 2016 Konsulko Inc.
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0+
> > > + */
> > > +
> > > +/dts-v1/;
> > > +/plugin/;
> >
> > Given that you're using the /plugin/ tag, it doesn't make sense to use
> > the "manual" way of constructing the overlay, rather than dtc's
> > built-in syntax:
> >
> > &test {
> > ...
> > }
> >
> > > +
> > > +/ {
> > > + fragment@0 {
> > > + target = <&test>;
> > > +
> > > + frag0: __overlay__ {
> > > + test-int-property = <43>;
> > > + };
> > > + };
> > > +
> > > + test-ref {
> > > + ref = <&frag0>;
> > > + };
> > > +};
> >
> > Things get weird in this example, because the point is we're equating
> > the __overlay__ node with the target node. Just ignoring the phandle
> > overwrite is *wrong* in this case, because it will break test-ref
> > (except that test-ref isn't in a fragment, and therefore discarded,
> > but that could be changed). Instead to handle this case correctly
> > we need to identify that we're asking the __overlay__ node to be the
> > same thing as &test and so &frag0 needs to be rewritten as &test.
>
> This isn't intended to be an example. It's a minimal test case to
> reproduce the bug.
>
> The actual example where this issue was first seen is [1]. The entries
> in __overrides__ are interpreted as different options which can be
> selected at runtime by a config option. I've looked for the code which
> actually implements this but I can't find it, my understanding is that
> the proprietary first stage bootloader on the Raspberry Pi applies
> these overrides and the overlays together.
Oh... right. Well, if you do rely on random extra nodes that aren't
described anywhere in the overlay spec...
Ugh, this really looks like yet more bending dts to do things it was
never really meant to handle.
> In Automotive Grade Linux we want to be able to network boot a
> Raspberry Pi and load a device tree with appropriate overlays applied.
> The files are fetched over the network by u-boot after the first stage
> bootloader has exited so we can't rely on the proprietary Raspberry Pi
> firmware to handle this for us. Instead the overlay needs to be
> applied either manually at compile time or in u-boot at runtime. This
> resulted in the FDT_ERR_NOTFOUND error described in the commit message
> for this patch. Discussion and investigation is captured in [2].
>
> [1]: https://github.com/raspberrypi/linux/blob/rpi-5.4.y/arch/arm/boot/dts/overlays/cma-overlay.dts
> [2]: https://jira.automotivelinux.org/browse/SPEC-3702
>
> Thanks,
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
