Re: [PATCH 5/5] tests: Add path tests for overlay

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4642 bytes --]

On Sat, Dec 14, 2024 at 10:15:49AM +0530, Ayush Singh wrote:
> On 03/12/24 10:16, David Gibson wrote:
> > On Sat, Nov 16, 2024 at 08:30:23PM +0530, Ayush Singh wrote:
> > > Add tests to verify path reference support in overlays
> > > 
> > > Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> > > ---
> > >   tests/overlay_overlay.dts               | 11 +++++++++++
> > >   tests/overlay_overlay_manual_fixups.dts | 26 +++++++++++++++++++++++++-
> > >   tests/overlay_overlay_nosugar.dts       | 19 +++++++++++++++++++
> > >   3 files changed, 55 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/overlay_overlay.dts b/tests/overlay_overlay.dts
> > > index c4ef1d47f1f159c5284c8f7282a0232d944ecfd1..18382762eb3d7c26b0f2e507acc3803f38194fb1 100644
> > > --- a/tests/overlay_overlay.dts
> > > +++ b/tests/overlay_overlay.dts
> > > @@ -50,3 +50,14 @@
> > >   		new-sub-test-property;
> > >   	};
> > >   };
> > > +
> > > +&test {
> > > +	test-patha = &test;
> > > +	test-pathb = &test;
> > > +};
> > > +
> > > +&test {
> > > +	sub-path-test-node {
> > > +		test-path = &test;
> > > +	};
> > > +};
> > 
> > You should test path references combined with other pieces too:
> > 
> > 	test-pathc = "a string", &test, "another string";
> > 
> > In fact it would even be a good idea to test path references combined
> > with phandle references.
> > 	test-pathd = "a string", <0x1 0x2 &test>, &test;
> 
> I have hit a bit of roadblock with the following case:
> 
> 	test-pathe = "a string", &test1, &test2;

Oh... good catch.  I felt like I was missing some additional wrinkle
with path substitutions at runtime... and here it is.  Ouch that
complicates things a bunch.

> Resolving &test1 will make the poffset for &test2 invalid. So do I need to
> worry about multiple path references in the same property? I do have a few
> ways to deal with this, but maybe I am missing something:

I really dislike having the arbitrary seeming constraint that you
can't put multiple path substitutions in a single property, especially
since that constraint doesn't apply for compile time substitution.
So, yeah, I think we need to worry about this.

> 1. Introduce something like `__fixups_path__` where we can use a different
> way to store information regarding path references. I am not too keen on
> going this way though. If we really need to introduce a new fixup node,
> might as well go all the way and have the node work for phandles as well.

At least in principle, I like the idea of improving the representation
to handle this case.  However, minimally extending the current format
just for this case doesn't seem like a good idea.  If we have to go
that far, seems like we should go further and clean up a bunch of the
other ugly problems with the current representation.

Note that my being comfortable with adding this feature in (roughly)
the current representation at all was based on missing this serious
complication.

> 2. Change the resolution algorithm to maybe create an intermediate tree
> first from `__fixups__` and resolve each property in reverse order in the
> tree. It seems great, until the fact that we cannot use heap comes into
> play. Playing with offsets all over the place has not been great in v2 (lots
> of bugs), and this will just make it harder.

Yeah, this is really tricky.  The basic problem is that the fixups are
grouped by the target of the reference, not the property into which
we're substituting.  That's quite diffferent from the internal
representation in dtc, where the fixups are represented as "markers"
attached to the property which the substitution is happening.  The
dtbo representation makes it awkward to get the fixups in order; and
not having a heap takes it from awkward to very, very tricky.

Note that it would not be sufficient to order just the path
substitutions we'd need to handle all substitutions - both path and
phandle - in a property in reverse order to correctly handle:
	prop = &target0, <&target1>;

...and, it gets even worse. In the un-substitute property, both of
those references are at offset 0, so the offsets alone aren't enough
information to correctly order the substitutions.  Somehow or other we
need additional information in the dtbo.

Crap.

At this stage, I don't know this feature is feasible without a rather
wider ranging revision of how dtbos are encoded.

-- 
David Gibson (he or they)	| 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]