On Mon, Feb 22, 2021 at 08:39:48AM +0000, Paul Barker wrote:
> On Mon, 22 Feb 2021 at 06:47, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > 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.
>
> Agreed. I'd be happy to see this turned into an error in dtc, that
> would prompt folks to stop doing such crazy things.
Looking a little deeper at what's going on here, there are basically
three different semantic levels here. They're all being encoded into
the flattened tree format which is causing some confusion.
1) There's the actual device tree information itself. This uses
phandles as a way of referencing nodes, no problem so far.
2) There's the overlay fragments. This refers to things in the layer
(1) data via those phandles, again no problem so far.
3) There's the automotive linux extension stuff that's trying to
update the overlay fragments before they're assembled into the
final DT.
Layer (3) is trying to refer to layer (2) pieces using phandles, and
this is where it really breaks down. phandles refer to layer (1)
constructs, which might get modified or rearranged by layer (2). The
overlay resoluion is assumed to resolve all those into phandles that
reference layer (1).
In your specific case where a label is being attached to an
__overlay__ node itself, the ambiguity becomes more obvious. Are you
trying to refer to (a) the final DT node where that fragment will be
eventually placed, or (b) to a piece of the overlay *before* it is
resolved?
dtc (or the overlay applicator) really has no way to resolve that
ambiguity, except to say that phandles are really only good for
referring to final DT nodes, not to locations in an overlay.
> I'll have a look at the dtc code when I get chance and see if I can
> figure out where we can detect this and raise an error.
--
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
