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