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:
signature.asc
Description: PGP signature
