Re: Array manipulation revisit

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

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

On Mon, Aug 05, 2024 at 04:59:35PM -0600, Rob Herring wrote:
> On Mon, Aug 05, 2024 at 04:35:14PM +0000, Hoover, Erich (Orion) wrote:
> > > From: David Gibson
> > > Sent: Saturday, August 3, 2024 12:37 AM
> > > ...
> 
> Please wrap quoted lines...
> 
> > > dtb overlays are kind of a hack layered on top of dtb, and don't 
> > > have a way to represent things at a granularity less than a whole 
> > > property (except for special handling of phandles).
> > 
> > I'm going to say that they seem to be a _very_ useful hack.
> > 
> > > Introducing new syntax to dts always requires care to not break 
> > > compatibility, but that's really the least of the problems here.  
> > > To implement this you'd need to invent a way of encoding partial 
> > > property updates into dtb, which would be an incompatible 
> > > extension.  It would need to cover ways of describing the part of 
> > > the property to be replaced more complex than fixed byte offsets to 
> > > handle the string list case here.
> > 
> > Maybe I'm being naive, but I think that this syntax could be very 
> > simple.  It looks to me like the current implementation disallows 
> > properties with brackets in the name, so I would propose taking 
> > advantage of that to create an invalid property name:
> >     PROPERTY[S*INDEX] = /bits/ BITS VALUE;
> > where S (single byte element size) is one of 1,2,4,8 or s (string 
> > array).  I could see "S*" being filled in automatically from the RHS 
> > data type (but override-able if you need to do something special).  
> > So, in our string array case that means:
> >     PROPERTY[INDEX] = "string";
> > becomes a property named "PROPERTY[s*INDEX]" with the value "string" 
> > when stored in the dtbo.
> 
> The above doesn't work because existing software can't parse it if it 
> is invalid.
> 
> This just continues the hack that is overlays. The hack is putting 
> information which should be in the format itself (DTB) into the contents 
> instead. The lack of type information is a major root issue. That's why 
> __fixups__ and __local_fixups__ nodes exist. They store phandle 
> locations with properties.
> 
> I think the major mistake we made with overlays was trying to preserve 
> DTB format. That's ended up being pretty pointless because we have to 
> update software applying overlays anyways. So why not just make the 
> updated s/w understand a new DTB format. We could have required applying 
> an overlay required a DTB in the new format (you need a rebuilt DTB 
> anyways to add the symbols to it). Backwards compatibility could have 
> been maintained by outputing the old format before passing to the OS. 

I tend to agree.

> The other issue is when a new DTB format is proposed, it becomes a 
> laundry list[1] of changes that goes nowhere.

Right.  Note there are also two layers even here: there's the abstract
format of the underlying device tree itself (a tree of nodes with
bytestring properties, as defined by IEEE1275), then there's the
linearized dtb format.

> What I think we should do is first rev the DTB format to allow for 
> unknown tags so we can add additional, optional data which software can 
> safely ignore. That would be an incompatible change. After that, we can 
> add new tags in a backwards compatible way. Roughly what's needed is the 
> same information stored in dtc's "marker" data: types and offsets.

You could do this, but it has its own difficulties.  It's adding
metadata in the dtb that's not present in the underlying device tree.
That's already a bit risky in terms of things relying on it at stages
they shouldn't - people already too often fail to realize that "a\0b"
and <0x61006200> are indistinguishable once you're in the kernel.
Making it so you _can_ distinguish them right up until the very last
stage seems likely to make that worse.

You could consider replacing the whole - admittendly pretty legacy -
OF1275 style information with something more modern and
self-describing.  But of course that's an even more widespread
overhaul.  And it's not obvious what you'd use: json (or one of the
various isomorphic formats) seems the obvious choice, except that json
can't naturally and safely encode 64-bit integers, which is a pretty
fatal flaw for this use case.

> > > It would need to cover ways of describing the part of the property 
> > > to be replaced more complex than fixed byte offsets to handle the 
> > > string list case here.
> > 
> > It seems like string arrays are the only complicated case, and by 
> > knowing that it's a string array (like with the above "s" size byte) 
> > the implementation can count NULL bytes to find the correct element to replace.
> 
> prop = "foo\0bar", "baz";
> 
> How many strings in "prop"? :)
> 
> Strings are actually the easy case IME as the heuristics to determine 
> if a property is a string works pretty well (minus the above which is a 
> test case in dtc, but not something I've seen in practice). It's 
> determining the size of integer arrays I have problems with (in context 
> of schema validation).
> 
> > > Finally you'd need to update libfdt (and any other overlay 
> > > implementation out there) to process this new fixup information.
> > > 
> > > So, yeah, it's not going to happen.
> 
> Why not? Yeah, updating stuff is painful, but so is not fixing some of 
> these problems. Are we just going to live with them forever? I think the 
> reality is most things use libfdt (probably everything that cares about 
> overlays) and they update libfdt at some frequency. And how old of a 
> component do we really need to worry about compatibility with?

I was meaning in the context of retaining a compatible dtb format.
Going outside that opens new possibilities.  But, that's a big job and
not one I'm going to take on.

> This reminds me of the saying about planting trees. When is the best 
> time to break compatibility? 15 years ago. When is the 2nd best time? 
> Now. 
> 
> Rob
> 
> [1] https://elinux.org/New_FDT_format
> 

-- 
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 --]