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

[Doug Anderson]

Hi David,

On Mon, Nov 2, 2026 at 1:31 AM David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> 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.


> > +        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.

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.

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)`.


> > +        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.


> > +        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()`.


> > +    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`).


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


> > +        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. ;-)

-Doug