Re: [PATCH v3 07/15] devicetree: Add DeviceTree live tree representation

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

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

On Tue, Nov 03, 2026 at 08:29:06PM -0800, Doug Anderson wrote:
> Hi David,
> 
> On Mon, Nov 2, 2026 at 1:31 AM David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Sun, Nov 01, 2026 at 02:01:14PM -0800, Douglas Anderson wrote:
> > > diff --git a/devicetree.py b/devicetree.py
> > > new file mode 100644
> > > index 000000000000..198ab73eda88
> > > --- /dev/null
> > > +++ b/devicetree.py
> >
> > This should probably go in a subdir, since it's not part of dtc
> > proper.  I think putting it in pylibfdt is ok, even though it's not
> > technically part of pylibfdt either.
> 
> Sounds good. For v4 I'll move `devicetree.py` to `pylibfdt/devicetree.py`
> and update `pylibfdt/meson.build` and `tests/meson.build` accordingly.

Great.

> > > +        node = cls()
> > > +        for prop in fdt.properties(offset):
> > > +            if prop.name in node.props:
> > > +                raise ValueError(f'Duplicate property {prop.name!r}')
> > > +            node.props[prop.name] = prop
> >
> > This is, technically aliasing the Property returned directly from the
> > flattened tree, which seems a little risky.  Or at least it does if
> > Property is mutable, which it appears to be as a subclass of
> > bytearray.
> >
> > On consideration, that kind of seems like a pre-existing mistake - the
> > way we use it, I think it makes more sense for Property to be
> > immutable (a subclass of 'bytes' instead).  I wonder if making that
> > change would break things.
> 
> Note that `fdt.properties(offset)` (via `get_property_by_offset()`)
> already allocates a fresh `Property` object with copied bytes on each
> call rather than aliasing the underlying `Fdt` buffer. Even so, having
> `Property` be mutable is definitely footgun-prone when `Property`
> instances are stored in `DeviceTree` or shared across nodes, and making
> `Property` subclass `bytes` instead of `bytearray` has some nice extra
> benefits:
> - `Property` becomes hashable (can be used in sets/dict keys) and its
>   byte value is immutable.
> - `fdt.setprop()` and `sw.property()` C SWIG typemaps require
>   `PyBytes_Check()`, so previously passing a `Property` directly to
>   `fdt.setprop()` raised a `TypeError` unless wrapped in `bytes(prop)`.
>   With `Property(bytes)`, a `Property` can be passed directly.

Ah, nice.

> Looking into compatibility for external `pylibfdt` callers if we switch
> `Property` from `bytearray` to `bytes`:
> - All existing tests in `dtc` pass without modification, and U-Boot's
>   `dtoc` (`tools/dtoc/fdt.py`) immediately converts the `Property` from
>   `get_property_by_offset()` to `bytes(p)` anyway.
> - Indexing, slicing, `len()`, iteration, `struct.unpack()`, the buffer
>   protocol, and all read-only bytes/string methods work identically.
> - Equality comparisons (`==`) against `bytearray` still work, since
>   `b'abc' == bytearray(b'abc')` is `True` in Python.
> - One subtle difference is that `bytearray` has a `.copy()` method
>   (`prop.copy()`) whereas `bytes` does not (`b''.copy()` raises
>   `AttributeError`), though we could easily keep `prop.copy()` working
>   by aliasing `copy = __copy__` on `Property` if needed.
> - The only things that would break in external callers (and can't be
>   shimmed on a `bytes` subclass in CPython) are:
>   1. Mutating a `Property` in place (`prop[0] = x`, `prop.extend(...)`),
>      which seems unlikely since `Property` is a disconnected snapshot and
>      mutating it never updated the `Fdt` blob anyway.
>   2. Explicit `isinstance(prop, bytearray)` checks.

Thanks for looking into this.  That looks like an acceptable danger of
breakage to me.

> In my draft v4 branch I've queued up a preparatory patch ("pylibfdt:
> Make Property a subclass of bytes instead of bytearray") that changes
> `Property` to subclass `bytes` via `__new__(cls, name, value)`, updates
> the `fdt_get_property_by_offset` SWIG typemap to return
> `PyBytes_FromStringAndSize()`, and drops the redundant `bytearray()`
> wrapper in `FdtRo.getprop()`. Let me know if that sounds good to you or
> if you're worried enough about compatibility with external callers that
> we should keep `Property(bytearray)`.

Thanks!  I do wonder if we're doing this if we should update __eq_ and
__hash__ methods to take into account the property name.

> > > +        for suboff in fdt.subnodes(offset):
> >
> > So, this can be a later improvement, it doesn't need a respin, but
> > this is a pretty inefficient way of unflattening the tree.  To get
> > from one subnode to the next subnodes() needs to traverse everything
> > in between in the flattened format - which is also what the child
> > unflatten()s do.   So on at every level making this roughly O(d * n)
> > where n is the number of nodes, and d is the depth of the tree.  You
> > can unflatten in a single O(n) pass over the tree.
> 
> Since I'll be spinning v4 anyway, I've updated `DeviceTree.unflatten()`
> in my draft v4 to unflatten in a single O(n) pass using
> `fdt.next_node(offset, depth)` and a node stack.

Lovely, thanks.

> > > +        fdt = sw.as_fdt()
> > > +        fdt.set_boot_cpuid_phys(boot_cpuid_phys)
> > > +        fdt.pack()
> >
> > The pack() is harmless, but probably unnecessary - fdt_finish(),
> > called from as_fdt() leaves an already packed tree.  Unless something
> > resized after that point, which set_boot_cpuid_phys() shouldn't do, it
> > should still be packed.
> 
> Good catch. Looking at `FdtSw.as_fdt()`, while `fdt_finish()` packs the
> FDT header's `totalsize`, `FdtSw.as_fdt()` currently doesn't trim the
> backing Python `bytearray` to `fdt_totalsize()` (unlike `Fdt.pack()`,
> which runs `del self._fdt[self.totalsize():]`), leaving trailing buffer
> slack in `fdt.as_bytearray()`. For v4 I plan to add a small preparatory
> patch ("pylibfdt: Trim Fdt bytearray to totalsize() in FdtSw.as_fdt()")
> so `FdtSw.as_fdt()` trims the `bytearray` to `fdt_totalsize()`, and drop
> `fdt.pack()` from `DeviceTree.flatten()`.

Ok.

> > > +    def get_str(self, name):
> > > +        """Return property `name` decoded as a NUL-terminated UTF-8 string.
> > > +
> > > +        Args:
> > > +            name: Property name to look up in `self.props`.
> > > +
> > > +        Returns:
> > > +            Decoded string value (without the trailing NUL byte).
> > > +
> > > +        Raises:
> > > +            KeyError: If `name` is not in `self.props`.
> > > +            ValueError: If the property value is not a single NUL-terminated
> > > +                UTF-8 string.
> > > +        """
> > > +        return self.props[name].as_str()
> >
> > Rather than having wrappers around all the class Property acccessors,
> > would it be simpler (and only slightly more verbose) to just have a
> > property() method, returning a Property instance and use its accessors
> > in the caller?  Looking up properties with __getitem__ could also
> > work, although maybe the fact that doesn't get subnodes would be
> > confusing?
> >
> > I guess that would also require a bunch of new class methods on
> > Property for the set* side.
> 
> Yeah, that works out nicely. For v4 I've queued up a preparatory patch
> ("pylibfdt: Add Property.from_*() creation methods") adding
> `from_cell()`, `from_uint32()`, `from_int32()`, `from_uint64()`,
> `from_int64()`, `from_list()`, `from_uint32_list()`, `from_int32_list()`,
> `from_uint64_list()`, `from_int64_list()`, `from_str()`, and
> `from_stringlist()` classmethods to `libfdt.Property` as the
> counterparts to `Property.as_*()`. Then in `DeviceTree` I've dropped all
> the `get_*()` and `set_*()` wrapper methods and replaced them with a
> single `DeviceTree.set_prop(prop)` helper (which stores
> `self.props[prop.name] = prop` so callers don't have to repeat the
> property name when setting a `Property`).

Great.

> > > diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
> > > index ae46b29a8f5c..cfdb8f5749b6 100644
> > > --- a/tests/pylibfdt_tests.py
> > > +++ b/tests/pylibfdt_tests.py
> > > @@ -11,6 +11,8 @@ import types
> > >  import unittest
> > >
> > >  sys.path.insert(0, '../pylibfdt')
> > > +sys.path.insert(0, '..')
> >
> > See above about location of devicetree.py.  But.. for that matter,
> > with the code in a separate module, it probably makes sense to have
> > its tests in a separate file, too.
> 
> Makes sense. For v4 I'll move the `DeviceTree` unit tests into a new
> `tests/devicetree_tests.py` file and run it alongside `pylibfdt_tests.py`
> in `tests/run_tests.sh`.
> 
> 
> > > +    def testDeviceTreeRoundTrip(self):
> > > +        """Test unflattening and flattening a tree via DeviceTree"""
> > > +        tree = copy.deepcopy(devicetree.DeviceTree.unflatten(self.fdt))
> >
> > What's the deepcopy for?  You're not retaining the original
> > unflattened copy.
> 
> I'll drop that in v4. (It was a leftover from testing
> `Property.__deepcopy__()` before that was split out into its own test in
> `pylibfdt_tests.py`.)
> 
> 
> > > +        self.assertIsInstance(tree.props['compatible'], libfdt.Property)
> >
> > I don't know if directly accessing the tree's internal dictionary from
> > outside is a good idea.
> 
> My intent was for `props` and `children` to be the primary public `dict`
> attributes of `DeviceTree` (rather than internal/private state), which
> keeps property and child-node lookups, iteration, and deletion
> (e.g. `node.props['compatible'].as_stringlist()`, `node.props.get(...)`,
> `del node.props[...]`) idiomatic in Python without needing a parallel
> set of dict-wrapper methods. Dropping the competing `get_*()`/`set_*()`
> wrappers on `DeviceTree` and adding `node.set_prop(prop)` makes that
> model much more consistent; for v4 I'll also update the `DeviceTree`
> class docstring to explicitly document `props` and `children` as public
> attributes for callers.

Ok, makes sense.

> > > +        tree2 = devicetree.DeviceTree.unflatten(fdt2)
> > > +        nodes1 = [(path, n.props, list(n.children.keys()))
> > > +                  for path, n in tree.walk()]
> > > +        nodes2 = [(path, n.props, list(n.children.keys()))
> > > +                  for path, n in tree2.walk()]
> > > +        self.assertEqual(nodes1, nodes2)
> >
> > I don't think it's guaranteed that walk() will get the nodes in the
> > same order each time, so you probably want sets instead of lists here.
> 
> Will fix in v4 by comparing
> `{path: (n.props, set(n.children.keys())) for path, n in tree.walk()}`
> (and using sets of paths in the other `walk()` test assertions).
> 
> 
> For now I'll hold these draft v4 changes in my local tree. If you have
> any feedback on the responses above (or any other patches in the
> series) I can make further changes before actually sending out a v4.
> If you'd prefer me to send out what I have now so you can review the
> latest-and-greatest, I'm happy to do that too. If nothing else, some
> of the changes we've made to how DeviceTree works here bleed into the
> other patches a little, so sending a v4 before you review further
> could be nice. ...but I also don't want to churn the series faster
> than you're ready for it. ;-)

Thanks.  And, yes, it's probably best to wait until I've reviewed the
rest of the series.  Which might take a while, sorry.

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